Refactor codebase per RFC #18 audit findings

[evansenter]

Summary

Implements all 12 findings from the codebase audit (RFC #18):

• P1 - Encapsulation: Add execute_query() / execute_write() public API to SQLiteStorage, migrate 8 locations from _connect()
• P2 - Code Quality: Add build_where_clause() helper, formatter registry pattern in CLI, module-qualified imports, importlib.metadata for version
• P3 - Extensibility: Schema migration framework with @migration decorator, __all__ exports
• P4 - Polish: Remove dead code, document timestamp handling, CLI help epilog

Test plan

• All 84 tests pass
• make check passes (format, lint, test)
• CLI help shows new epilog with examples
• Verify CLAUDE.md and README.md document new patterns

Closes #18

🤖 Generated with Claude Code

[claude]

Code Review: PR #19 - Refactor codebase per RFC #18 audit findings

This is an excellent refactoring that systematically addresses all 12 findings from RFC #18. The implementation is clean, well-tested, and follows best practices.

Strengths

1. Excellent Encapsulation - The new public API (execute_query/execute_write) in storage.py:162-192 properly abstracts database access with clear separation and good documentation.

2. Strong Test Coverage - 1,591 lines of tests across 6 test files (84 tests total) covering storage, queries, patterns, ingest, cli, and server.

3. Elegant Formatter Registry - cli.py:17-135 implements clean decorator-based registry that's easy to extend.

4. Good Query Helper - queries.py:8-40 build_where_clause() eliminates duplication across 4 instances.

5. Migration Framework - storage.py:112-132 provides future-proof schema evolution with decorator pattern.

Issues Found

Critical: None

High Priority:

1. settings.local.json contains hardcoded absolute paths (/Users/evansenter/...) - should not be committed or should use relative paths only

Medium Priority:
2. ingest.py:134-139 silently drops events on timestamp parse failure - suggest WARNING level logging
3. queries.py:29 uses f-string for column name - add whitelist validation to prevent future SQL injection
4. Inconsistent datetime handling across storage.py and ingest.py - centralize in utility module

Low Priority:
5. cli.py formatters lack return type annotations
6. scripts/install-launchagent.sh doesn't validate template file existence

Recommendations

Documentation: Excellent README and CLAUDE.md updates. Consider adding more API docstrings.

Security: Uses parameterized queries throughout. settings.local.json should not be committed.

Performance: Proper indexes, batch operations, pre-computed patterns, auto-refresh with staleness detection.

Maintainability: Clear separation of concerns, consistent naming, migration framework, comprehensive tests.

Summary

All 12 RFC #18 findings successfully addressed:

• Encapsulation (P1): Complete - public API replaces private _connect() usage
• Code Quality (P2): Complete - query helper, formatter registry, proper imports
• Extensibility (P3): Complete - migration framework, all exports
• Polish (P4): Complete - dead code removed, timestamp docs, CLI help

Recommended Actions:

1. Before merge: Remove or gitignore .claude/settings.local.json
2. Optional: Add column validation, improve error logging, add type hints

Verdict: Approve with minor changes (fix settings.local.json before merge)

Excellent work on this comprehensive refactoring!

[claude]

Code Review: PR #19 - Refactor codebase per RFC #18 audit findings

Overview

This is an impressive refactor that systematically addresses all 12 findings from RFC #18. The PR adds ~4,700 lines of well-structured code with comprehensive test coverage (84 tests across 7 test files). The implementation follows clean architecture principles and establishes clear patterns for future development.

✅ Strengths

Architecture & Design

• Public API Encapsulation: The introduction of execute_query() and execute_write() as the public API for SQLiteStorage is excellent. This provides a clear boundary and makes the code more maintainable.
• Formatter Registry Pattern: The CLI formatter registry using decorators (@_register_formatter) is elegant and extensible. The predicate-based matching (first-match-wins) is clean and easy to understand.
• Migration Framework: The @migration(version, name) decorator pattern is well-designed for future schema evolution.
• Query Helper: build_where_clause() successfully eliminates code duplication across query functions.

Code Quality

• Type Hints: Extensive use of modern Python type hints (str | None, list[Event], etc.) improves code clarity and IDE support.
• Documentation: Good docstring coverage with clear Args/Returns sections.
• Error Handling: Appropriate use of try/except with logging in critical paths (e.g., timestamp parsing, file I/O).
• Testing: 84 tests provide strong coverage across all modules.

Developer Experience

• Makefile: Clean, well-organized targets (check, install, dev, uninstall).
• CLI Help: The epilog with examples is a nice touch (cli.py:231-240).
• LaunchAgent Setup: Automated installation scripts make deployment straightforward.

🔍 Issues & Recommendations

Security Concerns

P0: SQL Injection Risk (storage.py:427, 434)
While where_clause is built from hardcoded conditions in get_events_in_range(), using f-strings for SQL construction is a dangerous pattern. If this pattern is copied elsewhere with user input, it could lead to SQL injection.

Recommendation: Use string concatenation instead of f-strings for SQL to make the pattern safer.

P1: Path Traversal (ingest.py:43)
The project_filter parameter is used in a simple substring match without validation. If this filter comes from user input, it could be exploited.

Recommendation: Validate or sanitize project_filter to ensure it does not contain path separators.

Bugs

P1: Timezone Handling (storage.py:567-569)
This handles both strings and datetime objects, but the timezone handling could be more explicitly documented. SQLite MAX() returns strings which are converted back.

P2: Silent Failure in Formatter Registry (cli.py:130-135)
If a predicate raises an exception, it will propagate and crash the CLI. Consider wrapping predicate calls in try/except.

P2: Missing Error Handling (queries.py:68-73)
If ingest_logs() fails, the query will fail too. Consider catching exceptions and logging warnings.

Performance

P1: Missing Index (storage.py:262-265)
Missing index on command column, which is used in query_commands(). Add: CREATE INDEX IF NOT EXISTS idx_events_command ON events(command)

P2: Unbounded Query (storage.py:409)
The limit defaults to 100 but consider adding a maximum cap to prevent accidental DoS.

Code Quality

P2: Magic Number (cli.py:36, 44, 67)
The limit of 20 items appears in multiple formatters. Extract to a constant.

P2: Duplicate Code (storage.py:318-350, 354-392)
The add_event() and add_events_batch() methods have identical INSERT statements. Extract to a shared constant.

P3: Type Annotation (cli.py:23)
Use Callable[[dict], bool] instead of callable for better type checking.

Testing

P1: Test Coverage Gap
No tests for the migration framework itself. Add tests for schema version upgrades.

P2: Missing Edge Case Tests

• What happens if ~/.claude/projects/ does not exist?
• What happens if a JSONL file is malformed?
• What happens if the database is locked?

📊 Test Coverage Analysis

Strong coverage across 7 test files:

• ✅ test_storage.py: Event operations, sessions, patterns
• ✅ test_queries.py: All query functions
• ✅ test_ingest.py: JSONL parsing
• ✅ test_patterns.py: Sequence detection, permission gaps
• ✅ test_cli.py: CLI commands and formatters
• ✅ test_server.py: MCP server endpoints
• ⚠️ Missing: Migration framework, error scenarios

🎯 Recommendations

Before Merge

1. Fix SQL f-string pattern (P0)
2. Add index on command column
3. Add migration framework test

Follow-up PRs

4. Extract magic numbers to constants
5. Add error handling improvements
6. Add edge case tests

✨ Conclusion

This is excellent work that significantly improves codebase maintainability. The architecture patterns provide a solid foundation.

Recommendation: Merge after addressing the P0 security concern and adding the missing index.

Great job! 🚀