refactor: Address codebase audit findings

[evansenter]

Summary

Addresses findings from /audit-codebase:

P1: Documentation Staleness (README.md)

• Added all 27 MCP tools organized by category
• Added all CLI commands with descriptions
• Updated examples to use correct function names

P2: Import Organization (cli.py)

• Consolidated 8 fragmented import statements into 3 clean blocks
• Grouped by module (ingest, patterns, queries)

P3: Duplicated Cutoff Calculation

• Added get_cutoff(days, hours) helper in queries.py
• Refactored 18+ occurrences across queries.py, patterns.py, ingest.py

P4: Inconsistent Aliasing (cli.py)

• Removed all do_* import aliases
• Functions now use their original names

P5: Public API Surface (init.py)

• Exported build_where_clause and get_cutoff helpers
• These are genuinely useful for extending the library

Test plan

• All 205 tests pass
• Lint and format checks pass
• No circular import issues (verified manually)

🤖 Generated with Claude Code

[claude]

Code Review: refactor: Address codebase audit findings

Overall, this is a solid cleanup PR that addresses documentation staleness and improves code consistency. Here's my detailed feedback:

✅ What's Good

1. Import Consolidation (cli.py)
The removal of do_* aliases is a good change. The previous pattern:

from session_analytics.ingest import (
    correlate_git_with_sessions as do_correlate_git,
)

was unnecessarily verbose. Using the original function names directly is cleaner and reduces cognitive overhead.

2. get_cutoff() Helper (queries.py:56-67)
Good DRY improvement. The helper is well-designed:

• Supports both days and hours parameters
• Handles fractional days correctly
• Clear docstring

3. README Updates
The documentation now accurately reflects the available tools with proper categorization. The examples showing breakdown fields (e.g., get_tool_frequency with breakdown array) are helpful.

4. Public API Exports (init.py)
Exporting build_where_clause and get_cutoff makes sense for library consumers who want to extend functionality.

---

⚠️ Suggestions

1. Missing Test Coverage for get_cutoff()

The new get_cutoff() helper has no dedicated tests. While it's used throughout the codebase (so it gets indirect coverage), a unit test would be valuable since it's now part of the public API via __init__.py.

Consider adding to tests/test_queries.py:

def test_get_cutoff_days():
    from session_analytics.queries import get_cutoff
    from datetime import datetime, timedelta
    
    cutoff = get_cutoff(days=7)
    expected = datetime.now() - timedelta(days=7)
    # Allow 1 second tolerance
    assert abs((cutoff - expected).total_seconds()) < 1

def test_get_cutoff_hours():
    from session_analytics.queries import get_cutoff
    from datetime import datetime, timedelta
    
    cutoff = get_cutoff(hours=12)
    expected = datetime.now() - timedelta(hours=12)
    assert abs((cutoff - expected).total_seconds()) < 1

def test_get_cutoff_combined():
    from session_analytics.queries import get_cutoff
    from datetime import datetime, timedelta
    
    cutoff = get_cutoff(days=1, hours=6)
    expected = datetime.now() - timedelta(hours=30)
    assert abs((cutoff - expected).total_seconds()) < 1

2. Type Annotation Consistency

In get_cutoff(), the days parameter uses int | float while hours uses just float. For consistency, consider:

def get_cutoff(days: float = 7, hours: float = 0) -> datetime:

Since int is a subtype of float in practice, this simplifies the signature while maintaining the same behavior.

3. Potential Edge Case: get_cutoff(days=0, hours=0)

If called with both defaults as 0 (or explicitly), this returns datetime.now(), which is technically correct but might be surprising. Consider documenting this behavior or adding a guard if it shouldn't be allowed.

---

📋 Observations (No Action Required)

1. Consistent Refactoring: All 18+ occurrences of datetime.now() - timedelta(days=days) have been replaced with get_cutoff(). Good systematic cleanup.

2. No Breaking Changes: The refactoring maintains the same behavior - existing callers are unaffected.

3. Import Order: The new imports in __init__.py follow the existing pattern of importing from submodules then re-exporting.

---

Summary

Approve with minor suggestions. The changes are clean, well-organized, and improve maintainability. The only notable gap is the missing test coverage for the new get_cutoff() public API function.

The PR summary accurately describes all changes, and the test plan confirms all 205 tests pass. Good work on the audit cleanup!

[claude]

Code Review: PR #32 - refactor: Address codebase audit findings

This is a well-organized cleanup PR that addresses several audit findings. Here's my detailed review:

✅ Strengths

1. Documentation Updates (README.md)

• Excellent reorganization of CLI commands by category (Status, Analytics, Workflow, etc.)
• MCP tools table now accurately reflects the 27 available tools with correct naming (get_tool_frequency vs old query_tool_frequency)
• Added helpful examples including the new get_session_signals output format

2. Import Cleanup (cli.py)

• Removing the do_* aliases is a good choice - the original function names are clear enough
• Consolidating fragmented imports into 3 clean blocks improves readability
• The imports are now logically grouped by module

3. DRY Improvement: get_cutoff() Helper

• The new helper in queries.py:56-67 consolidates 18+ occurrences of datetime.now() - timedelta(...)
• API design is clean: get_cutoff(days=7, hours=12) reads naturally
• Correctly handles fractional days via total_hours = (days * 24) + hours

4. Public API Export (init.py)

• Exporting build_where_clause and get_cutoff is appropriate - these are genuinely useful for extending the library

⚠️ Suggestions

1. Missing test coverage for get_cutoff()
The new get_cutoff() helper is used throughout the codebase but I don't see dedicated unit tests for it. Consider adding tests for edge cases:

def test_get_cutoff_days_only():
    result = get_cutoff(days=7)
    expected = datetime.now() - timedelta(days=7)
    assert abs((result - expected).total_seconds()) < 1

def test_get_cutoff_hours_only():
    result = get_cutoff(days=0, hours=12)
    expected = datetime.now() - timedelta(hours=12)
    assert abs((result - expected).total_seconds()) < 1

def test_get_cutoff_combined():
    result = get_cutoff(days=1, hours=6)
    expected = datetime.now() - timedelta(hours=30)  # 24 + 6
    assert abs((result - expected).total_seconds()) < 1

2. Circular Import Risk
ingest.py and patterns.py now import from queries.py. While this works currently, be mindful if queries.py ever needs to import from these modules - that would create a circular dependency. The current structure looks safe, but worth noting.

3. guide.md tip placement
The added tip in guide.md:

> **Tip:** Read this guide via the MCP resource `session-analytics://guide`...

This is a bit self-referential since someone reading via the MCP resource is already doing what the tip suggests. Consider moving this to README.md instead, where it would be more useful for discovery.

📝 Minor Nits

1. In queries.py:56, the type annotation days: int | float = 7 - using float alone would be sufficient since int is a subtype in practice for arithmetic operations

2. The commit message mentions "18+ occurrences" - the actual count I see in the diff is closer to ~15 replacements across the 3 files, but this is just a minor description discrepancy

🏁 Verdict

LGTM with minor suggestions. The refactoring is clean, the documentation is significantly improved, and the code follows the project's established patterns. The missing tests for get_cutoff() are worth addressing but not blocking.

Good work addressing the audit findings systematically!

[evansenter]

Feedback Addressed (Remote Review)

Implemented

• [Important] Added 5 unit tests for get_cutoff() public API function covering: days-only, hours-only, combined, fractional days, and defaults

Skipped

• [Suggestion] Type annotation change (int | float → float) - keeping explicit type for clarity
• [Suggestion] Move guide.md tip to README - keeping aligned with claude-event-bus pattern where tip is in the guide itself

[claude]

PR Review: refactor: Address codebase audit findings

This is a well-structured cleanup PR that addresses real technical debt. Here's my detailed feedback:

✅ What's Good

1. Import Organization (cli.py)
The consolidation of fragmented imports is clean:

# Before: 8 separate import statements with do_* aliases
from session_analytics.ingest import correlate_git_with_sessions as do_correlate_git
from session_analytics.ingest import ingest_git_history as do_ingest_git
# ...

# After: 3 clean import blocks grouped by module
from session_analytics.ingest import (
    correlate_git_with_sessions,
    ingest_git_history,
    ingest_logs,
)

This improves readability and removes the unnecessary do_* aliasing.

2. DRY: get_cutoff() Helper
The centralized cutoff calculation is a good abstraction:

def get_cutoff(days: int | float = 7, hours: float = 0) -> datetime:
    total_hours = (days * 24) + hours
    return datetime.now() - timedelta(hours=total_hours)

• Replaces 18+ scattered datetime.now() - timedelta(days=days) calls
• Supports fractional days and additional hours parameter
• Well-tested with 5 test cases

3. README Updates
Comprehensive documentation update with:

• All 27 MCP tools organized by category
• Corrected function names (get_tool_frequency vs old query_tool_frequency)
• Practical examples including the new get_session_signals output

4. Test Coverage
The TestGetCutoff class covers edge cases well:

• Days only, hours only, combined
• Fractional days (0.5 = 12 hours)
• Default values

---

🔶 Suggestions for Consideration

1. Public API Surface Expansion
Exporting get_cutoff in __init__.py is fine since it's genuinely useful for library consumers. However, consider whether build_where_clause should also be exported — it's more internal (depends on storage schema knowledge). The comment in __init__.py says "These are genuinely useful for extending the library" which applies more to get_cutoff than build_where_clause.

2. Minor: Guide.md Self-Reference

> **Tip:** Read this guide via the MCP resource `session-analytics://guide`...

This creates a slightly circular reference (reading the guide to learn you can read it via MCP). Consider moving this to README.md instead or rephrasing as a reminder for LLM consumers.

3. Type Consistency in get_cutoff

def get_cutoff(days: int | float = 7, hours: float = 0) -> datetime:

The days parameter accepts int | float but hours is typed as just float. For consistency, you might consider hours: int | float = 0 since integer hours are common. This is a very minor nit.

---

✅ Summary

Approve — This PR makes the codebase cleaner and more maintainable:

• Import organization ✓
• DRY principle applied well ✓
• Documentation updated ✓
• Tests added ✓
• No breaking changes ✓

The changes follow the project's conventions (formatter registry pattern, module imports in server.py, etc.) and the README updates ensure CLI/MCP parity in documentation.

---

🤖 Generated with Claude Code

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

This PR addresses codebase audit findings: updates README documentation with all 27 MCP tools, consolidates fragmented imports in cli.py, adds a get_cutoff() helper to DRY up datetime calculations, and exports public API helpers.

Issues Found

Critical

None

Important

None

Suggestions

None

Verdict

APPROVE - Code looks good, no issues found.

The changes are well-organized and address each audit finding systematically:

• Documentation: README now accurately reflects all available MCP tools and CLI commands with proper categorization
• Import cleanup: Removing do_* aliases and consolidating imports improves readability
• DRY improvement: get_cutoff() helper consolidates 18+ occurrences and is now well-tested (5 test cases added per previous feedback)
• Bug fix: The _format_signals predicate now handles empty sessions list correctly (cli.py:425)
• Test infrastructure: Shared fixtures in conftest.py will benefit future tests

Previous review suggestions have been addressed appropriately in the "Feedback Addressed" comment.

---

Automated review by Claude Code

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

This PR addresses codebase audit findings: updates README with all 27 MCP tools, consolidates fragmented imports in cli.py, adds a get_cutoff() helper to DRY up datetime calculations, exports public API helpers, and adds shared test fixtures.

Issues Found

Critical

None

Important

None

Suggestions

None

Verdict

APPROVE - Code looks good, no issues found.

The changes are well-organized and systematically address each audit finding:

• Documentation: README now accurately reflects all MCP tools and CLI commands with proper categorization
• Import cleanup: Removing do_* aliases and consolidating into 3 clean import blocks improves readability
• DRY improvement: get_cutoff() helper consolidates 18+ datetime calculations and is well-tested (5 test cases)
• Bug fix: The _format_signals predicate now correctly handles empty sessions list (cli.py:425)
• Test infrastructure: Shared fixtures in conftest.py (populated_storage, pattern_storage) will benefit future tests
• Public API: Exporting build_where_clause and get_cutoff in __init__.py makes sense for library consumers

Previous review suggestions have been appropriately addressed per the "Feedback Addressed" comment.

---

Automated review by Claude Code