fix: address code review findings for performance and quality

[zircote]

Summary

• Performance improvements (PERF-001 through PERF-007): Batch git operations, batch embeddings, model prewarming, struct caching
• Quality improvements (QUAL-001, QUAL-002): Function naming clarity, specific exception handling
• Security improvements (SEC-001, SEC-002): Input length limits for ReDoS prevention
• Test updates: All fixtures and assertions updated for new batch operations

Test plan

• All 1806 tests passing
• 89.29% code coverage maintained
• make quality passes (format, lint, typecheck, security)
• Batch operations verified in test_sync.py
• Exception handling verified in test_context_builder.py

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This pull request implements performance optimizations and quality improvements addressing code review findings. The changes introduce batch operations for git and embedding operations (PERF-001 through PERF-007), improve exception handling specificity (QUAL-001, QUAL-002), and add security safeguards against ReDoS attacks (SEC-001, SEC-002). A new ServiceRegistry class centralizes singleton management, replacing module-level global variables. All 1806 tests pass with 89.29% coverage maintained.

Key changes:

• Batch git operations via git cat-file --batch reduce subprocess overhead when fetching multiple notes
• Batch embedding generation processes multiple texts in one model call for better throughput
• ServiceRegistry provides centralized singleton management with clean reset mechanism for testing

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
src/git_notes_memory/git_ops.py	Adds show_notes_batch() for bulk note retrieval via git cat-file; adds path sanitization to error messages
src/git_notes_memory/sync.py	Updates collect_notes(), reindex(), and verify_consistency() to use batch operations; migrates to ServiceRegistry
src/git_notes_memory/recall.py	Implements batch hydration with grouped git operations; optimizes token estimation with generator expression
src/git_notes_memory/index.py	Adds struct format caching for embedding serialization to avoid repeated format parsing
src/git_notes_memory/embedding.py	Adds prewarm() method for eager model loading; migrates to ServiceRegistry
src/git_notes_memory/capture.py	Extracts validation and front matter building into separate methods; migrates to ServiceRegistry
src/git_notes_memory/registry.py	New centralized singleton registry with type-safe get/register/reset methods
src/git_notes_memory/hooks/stop_handler.py	Renames _read_input to _read_input_with_fallback for clarity
src/git_notes_memory/hooks/signal_detector.py	Adds 100KB input length limit to prevent ReDoS attacks
src/git_notes_memory/hooks/context_builder.py	Replaces bare Exception with specific MemoryIndexError and OSError catches
tests/test_sync.py	Updates all test mocks for batch operations (show_notes_batch, embed_batch)
tests/test_hook_handlers.py	Updates imports and calls for renamed _read_input_with_fallback function
tests/test_context_builder.py	Updates exception mocking to use specific exception types
tests/conftest.py	Major refactor: uses ServiceRegistry.reset() instead of direct module access; adds comprehensive fixtures for git repos, mocks, and models

Comments suppressed due to low confidence (1)

tests/test_sync.py:800

• The test mock setup may not match the actual behavior. In the implementation (sync.py:316), parser.parse_many(content) is called with a single content string, but the mock returns [record] (one record). However, parse_many can return multiple records from a single note. In the test at line 794, mock_note_parser.parse_many.return_value = [record] is set once, but parse_many is called twice (once for commit1 and once for commit2). This means the second call will also return the same single record, which is correct for this test but doesn't reflect that parse_many is called multiple times. The test expects 2 insertions (line 800), but only mocks one record being returned each time parse_many is called, which would work only if each note contains exactly one memory. This is fine for the test, but the comment should clarify this assumption.

        record = make_note_record()
        mock_note_parser.parse_many.return_value = [record]

        result = sync_service.reindex()

        assert result == 2
        assert mock_index.insert.call_count == 2

[Copilot]

@zircote I've opened a new pull request, #15, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

Pull request overview

Copilot reviewed 28 out of 33 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 32 out of 37 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

tests/test_sync.py:1

• These parameters are marked as unused with # noqa: ARG001 but the comment says 'Required by signature'. If they are truly required by the signature but not used in the function body, consider if the function signature itself needs refactoring. If this is a callback or interface requirement, add a more specific comment explaining why these parameters must match a specific signature.
  tests/test_sync.py:1
• The fixture is marked as unused with # noqa: ARG002 and noted as 'Required fixture'. If the fixture is truly required (e.g., for setup/teardown side effects), the comment should explain what side effect is needed. If it's not actually required, remove it from the function signature.