Conversation
When scanning commits, LiberateService now reads existing AI-* trailers and imports them as high-confidence memories with source 'commit-trailer'. Heuristic extraction is skipped for memory types already covered by trailers, preventing duplicates. LLM enrichment still runs independently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughIntegrates trailer-based fact extraction into ExtractService by adding a new extractTrailerFacts method, wiring an optional TrailerService dependency, and extending fact merging to incorporate commit trailer-derived facts alongside heuristic and LLM enrichment results. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ExtractService
participant TrailerService
participant MemoryRepository
Client->>ExtractService: extract(commit)
activate ExtractService
ExtractService->>ExtractService: heuristic extraction
opt trailerService provided
ExtractService->>TrailerService: read trailers from commit
activate TrailerService
TrailerService-->>ExtractService: trailers data
deactivate TrailerService
ExtractService->>ExtractService: convert trailers to IUnifiedFact[]
ExtractService->>ExtractService: skip heuristic matches<br/>covered by trailers
end
opt LLM enrichment enabled
ExtractService->>ExtractService: llm enrichment
end
ExtractService->>ExtractService: merge heuristic + trailer + llm facts
ExtractService->>MemoryRepository: persist unified facts
activate MemoryRepository
MemoryRepository-->>ExtractService: persisted
deactivate MemoryRepository
ExtractService-->>Client: result
deactivate ExtractService
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements bidirectional trailer integration for git-mem, enabling the system to both write and read AI-* commit trailers. The implementation adds dual-write functionality to MemoryService.remember() (writing both git notes and commit trailers) and unified recall that merges memories from both sources. The LiberateService now reads existing AI-* trailers during commit scanning and treats them as high-confidence, authoritative facts, skipping heuristic extraction for types already covered by trailers to prevent duplicates.
Changes:
- Added
addTrailers()andbuildCommitMessage()methods toTrailerServicefor writing AI-* trailers to commit messages - Implemented dual-write in
MemoryService.remember()to persist memories in both git notes and commit trailers (opt-out viatrailers: false) - Extended
MemoryService.recall()to merge memories from both notes and trailers, with deduplication by AI-Memory-Id - Modified
LiberateService.liberate()to extract trailer facts and skip heuristic extraction for types already covered by trailers - Added
--no-trailersCLI flag andtrailersMCP parameter to allow opting out of trailer writes - Registered
trailerServicein DI container as singleton
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/infrastructure/services/TrailerService.ts | Adds addTrailers() to amend HEAD with new trailers and buildCommitMessage() to merge trailers into commit messages |
| src/domain/interfaces/ITrailerService.ts | Extends interface with addTrailers() and buildCommitMessage() method signatures |
| src/infrastructure/di/container.ts | Registers trailerService as singleton in DI container |
| src/infrastructure/di/types.ts | Adds trailerService to ICradle interface |
| src/application/services/MemoryService.ts | Implements dual-write (notes + trailers) and unified recall merging both sources |
| src/application/services/LiberateService.ts | Extracts trailer facts during commit scanning and skips heuristic extraction for trailer-covered types |
| src/domain/entities/IMemoryEntity.ts | Adds trailers?: boolean option to ICreateMemoryOptions |
| src/commands/remember.ts | Adds noTrailers option and passes trailers: !options.noTrailers to service |
| src/cli.ts | Adds --no-trailers CLI flag to remember command |
| src/mcp/tools/remember.ts | Adds trailers boolean parameter to MCP tool schema |
| tests/unit/infrastructure/services/TrailerService.test.ts | Tests for buildCommitMessage() and addTrailers() methods |
| tests/unit/infrastructure/di/container.test.ts | Adds verification that trailerService is registered and resolves correctly |
| tests/unit/application/services/MemoryService.test.ts | Tests dual-write behavior, unified recall, and deduplication scenarios |
| tests/unit/application/services/LiberateService.test.ts | Tests trailer extraction, deduplication with heuristics, and backward compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .map(t => t.value); | ||
|
|
||
| // Shared metadata from the commit's trailers | ||
| const confidence = (commit.trailers.find(t => t.key === AI_TRAILER_KEYS.CONFIDENCE)?.value || 'high') as ConfidenceLevel; |
There was a problem hiding this comment.
The confidence value from trailers is cast to ConfidenceLevel without validation. If a manually-added trailer has an invalid confidence value (e.g., "AI-Confidence: very-high"), this will silently accept it and potentially cause issues downstream. Consider using the isValidConfidence helper function from domain/types/IMemoryQuality to validate the value before casting, falling back to 'high' if invalid.
There was a problem hiding this comment.
Fixed — now uses isValidConfidence() from domain types to validate the trailer confidence value, falling back to 'high' for invalid values. See 97190a7.
| if (trailers.length === 0) return []; | ||
|
|
||
| const facts: IUnifiedFact[] = []; | ||
| const confidence = (trailers.find(t => t.key === AI_TRAILER_KEYS.CONFIDENCE)?.value || 'high') as ConfidenceLevel; |
There was a problem hiding this comment.
The confidence value from trailers is cast to ConfidenceLevel without validation. If a manually-added trailer has an invalid confidence value (e.g., "AI-Confidence: very-high"), this will silently accept it and potentially cause issues downstream. Consider using the isValidConfidence helper function from domain/types/IMemoryQuality to validate the value before casting, falling back to 'high' if invalid.
There was a problem hiding this comment.
Fixed — extractTrailerFacts() now validates the confidence value using isValidConfidence() with a fallback to 'high'. See 97190a7.
| const TRAILER_KEY_TO_MEMORY_TYPE: Record<string, MemoryType> = { | ||
| [AI_TRAILER_KEYS.DECISION]: 'decision', | ||
| [AI_TRAILER_KEYS.GOTCHA]: 'gotcha', | ||
| [AI_TRAILER_KEYS.CONVENTION]: 'convention', | ||
| [AI_TRAILER_KEYS.FACT]: 'fact', | ||
| }; |
There was a problem hiding this comment.
The TRAILER_KEY_TO_MEMORY_TYPE mapping is duplicated in both MemoryService and LiberateService. This violates the DRY principle and creates a maintenance burden - any changes to the mapping would need to be made in both places. Consider moving this constant to a shared location like domain/entities/ITrailer.ts or creating a dedicated utility module for trailer-to-memory-type mappings.
| const TRAILER_KEY_TO_MEMORY_TYPE: Record<string, MemoryType> = { | |
| [AI_TRAILER_KEYS.DECISION]: 'decision', | |
| [AI_TRAILER_KEYS.GOTCHA]: 'gotcha', | |
| [AI_TRAILER_KEYS.CONVENTION]: 'convention', | |
| [AI_TRAILER_KEYS.FACT]: 'fact', | |
| }; | |
| const TRAILER_KEY_TO_MEMORY_TYPE: Record<string, MemoryType> = Object | |
| .entries(MEMORY_TYPE_TO_TRAILER_KEY) | |
| .reduce((acc, [memoryType, trailerKey]) => { | |
| acc[trailerKey] = memoryType as MemoryType; | |
| return acc; | |
| }, {} as Record<string, MemoryType>); |
There was a problem hiding this comment.
Fixed — moved MEMORY_TYPE_TO_TRAILER_KEY and TRAILER_KEY_TO_MEMORY_TYPE to src/domain/entities/ITrailer.ts as the single source of truth. The reverse mapping is derived automatically from the forward mapping. Both MemoryService and LiberateService now import from there. See 97190a7.
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), |
There was a problem hiding this comment.
The createdAt and updatedAt timestamps are set to the current time (new Date()) instead of the commit's timestamp. This makes trailer-sourced memories appear to have been created "now" rather than when the commit was actually made. For historical accuracy, these timestamps should reflect the commit's author date. The ICommitTrailers interface could be extended to include the commit date, or the SHA could be used to fetch the timestamp separately.
There was a problem hiding this comment.
Acknowledged — added a documentation comment noting this as a known limitation. Using the commit author date would require an extra git log call per trailer commit. The commit SHA can be used to look up the actual date if needed.
| const trailers = this.trailerService.readTrailers(sha, cwd); | ||
| if (trailers.length === 0) return []; | ||
|
|
There was a problem hiding this comment.
The extractTrailerFacts method spawns a git process for each commit via readTrailers(sha). For large liberations, this could be inefficient. Consider using queryTrailers (which can batch-query multiple commits at once) to fetch all trailer data upfront, then look up by SHA during processing. This is a performance optimization opportunity, not a blocking issue, but could significantly speed up large-scale liberations.
| const trailers = this.trailerService.readTrailers(sha, cwd); | |
| if (trailers.length === 0) return []; | |
| let trailers: readonly { key: string; value: string }[] = []; | |
| // Prefer batch-capable queryTrailers when available, fall back to per-commit readTrailers. | |
| const trailerServiceAny = this.trailerService as any; | |
| const queried = typeof trailerServiceAny.queryTrailers === 'function' | |
| ? trailerServiceAny.queryTrailers([sha], cwd) | |
| : undefined; | |
| if (Array.isArray(queried)) { | |
| // Some implementations may return the trailers array directly for a single SHA. | |
| trailers = queried; | |
| } else if (queried && typeof queried === 'object') { | |
| // Others may return a map/dictionary keyed by SHA. | |
| const bySha = (queried as Record<string, readonly { key: string; value: string }[] | undefined>)[sha]; | |
| if (Array.isArray(bySha)) { | |
| trailers = bySha; | |
| } | |
| } | |
| if (trailers.length === 0) { | |
| trailers = this.trailerService.readTrailers(sha, cwd); | |
| } | |
| if (!trailers || trailers.length === 0) return []; |
There was a problem hiding this comment.
Good callout. For now, readTrailers() per commit is acceptable since liberate processes high-interest commits only (typically a small subset). If this becomes a bottleneck, we can switch to queryTrailers() for batch retrieval. Deferring as a future optimization.
| const found = result.memories.find(m => m.content === 'Use Redis'); | ||
| assert.ok(found); | ||
| assert.deepEqual(found.tags, ['cache', 'performance']); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for invalid confidence values in trailers. Consider adding a test case that verifies the system handles manually-added trailers with invalid AI-Confidence values (e.g., "AI-Confidence: very-high" or "AI-Confidence: invalid"). This would help catch the validation issue at lines 170 and 207 in MemoryService.ts and LiberateService.ts respectively.
| }); | |
| }); | |
| it('should handle invalid AI-Confidence trailer values without failing', () => { | |
| writeFileSync(join(repoDir, 'invalid-confidence.txt'), 'invalid-confidence'); | |
| git(['add', '.'], repoDir); | |
| git( | |
| [ | |
| 'commit', | |
| '-m', | |
| 'feat: invalid confidence\n\n' + | |
| 'AI-Fact: Confidence parsing should be robust\n' + | |
| 'AI-Confidence: very-high', | |
| ], | |
| repoDir, | |
| ); | |
| const result = serviceWithTrailers.recall('Confidence parsing should be robust', { cwd: repoDir }); | |
| const found = result.memories.find( | |
| m => m.content === 'Confidence parsing should be robust', | |
| ); | |
| assert.ok(found); | |
| }); |
There was a problem hiding this comment.
Fixed — added a test should handle invalid AI-Confidence trailer values gracefully that verifies invalid confidence values (e.g., 'very-high') fall back to 'high'. See 97190a7.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/remember.ts (1)
11-32:⚠️ Potential issue | 🟠 MajorCLI
--no-trailersflag is ignored.Commander.js stores
--no-trailersasoptions.trailers(defaulting totrue), notoptions.noTrailers. Since the code referencesoptions.noTrailers(which remains undefined),!options.noTrailersalways evaluates totrue, and trailers are always enabled regardless of the flag.🔧 Suggested fix
interface IRememberOptions { commit?: string; type?: string; confidence?: string; lifecycle?: string; tags?: string; - noTrailers?: boolean; + trailers?: boolean; } const memory = memoryService.remember(text, { sha: options.commit, type: (options.type || 'fact') as MemoryType, confidence: (options.confidence || 'high') as ConfidenceLevel, lifecycle: (options.lifecycle || 'project') as MemoryLifecycle, tags: options.tags, - trailers: !options.noTrailers, + trailers: options.trailers ?? true, });
🤖 Fix all issues with AI agents
In `@src/application/services/LiberateService.ts`:
- Around line 199-225: The extractTrailerFacts method currently reads global
confidence/tags once and applies them to all facts; instead iterate trailers in
order and apply metadata sequentially: walk the trailers array, update
currentConfidence when encountering AI_TRAILER_KEYS.CONFIDENCE and currentTags
when encountering AI_TRAILER_KEYS.TAGS (split/trim into string[]), and when you
hit a trailer whose key maps via TRAILER_KEY_TO_MEMORY_TYPE to a memory type,
create a fact using the currentConfidence/currentTags for that specific entry;
update or reset these current metadata values as needed so each memory-type
trailer uses the most recently-seen metadata rather than the first found values.
In `@src/application/services/MemoryService.ts`:
- Around line 75-107: The returned total currently undercounts when notes are
paged because it uses allMemories.length (which reflects only the sliced/merged
page) instead of the full-match counts; in recall(), compute a combinedTotal =
notesResult.total + trailerMemories.length (trailerMemories are already
trailer-only because recallFromTrailers was passed notesResult.memories), use
combinedTotal in the returned object and in the logger, and keep applying
pagination/limit to the merged array (allMemories.slice(0, limit)) so the
memories array is paged but total reflects the full available matches.
- Around line 157-194: In trailerCommitToEntities, synthetic IDs use
`trailer:${commit.sha}:${type}` which collides when a commit has multiple
trailers of the same type; change the synthetic `id` generation (where `id` is
set using `memoryIds[i] || ...`) to include a uniqueness factor such as the
trailer loop index (`i`) or a short hash of `typeTrailer.value` (or both) so
each generated `id` is unique per trailer; update any tests/code that assume the
old format if needed and ensure the new `id` still respects existing dedupe
logic.
In `@tests/unit/application/services/MemoryService.test.ts`:
- Around line 61-153: The tests in MemoryService.test.ts use real git and
filesystem calls (writeFileSync and git([...], repoDir)) via the
serviceWithTrailers.remember and service.remember flows and
trailerService.readTrailers, which breaks unit-test isolation; replace those
real-repo interactions with manual mocks: stub out trailerService.readTrailers
and any NotesService/git helper used by MemoryService so the tests call
serviceWithTrailers.remember/service.remember but rely on injected mock
implementations (or a mocked git helper) that return deterministic trailers/SHAs
instead of invoking git, and remove actual writeFileSync/git([...]) steps;
alternatively move these specs to tests/integration if you want real repo
behavior. Ensure you reference and mock the exact symbols:
serviceWithTrailers.remember, service.remember, trailerService.readTrailers, and
any helper that invokes git.
- Move TRAILER_KEY_TO_MEMORY_TYPE to ITrailer.ts (DRY, shared mapping) - Validate confidence with isValidConfidence(), fallback to 'high' - Fix synthetic ID collision by appending index suffix - Fix total undercount: use notesResult.total + trailerMemories.length - Normalize trailer values to single-line (strip newlines) - Skip trailer writes for non-HEAD commits - Guard against staged changes before amending in addTrailers - Fix tag filter consistency (case-sensitive, matches notes) - Add documentation for shared metadata and timestamp limitations - Add tests for invalid confidence and synthetic ID uniqueness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # tests/unit/application/services/ExtractService.test.ts
Summary
source: 'commit-trailer'Closes GIT-69
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features