Conversation
Wire the existing TrailerService into the awilix container as a singleton so downstream issues can resolve it from the cradle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds two new methods: - addTrailers: amends HEAD to append AI-* trailers, deduplicating against existing trailers on the commit - buildCommitMessage: pure string builder for composing commit messages with a trailer block Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Filter addTrailers input to AI-* prefixed trailers only - Add addTrailers and buildCommitMessage to container interface test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When remembering a memory, also write AI-* trailers to the commit: - Maps memory type to trailer key (decision→AI-Decision, etc.) - Includes AI-Confidence, AI-Tags, AI-Memory-Id trailers - Opt-out via --no-trailers CLI flag or trailers:false in options - Trailer write failure is non-fatal (logged as warning) - MCP tool gains trailers boolean parameter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extend MemoryService.recall() to search both git notes and AI-* commit trailers, merging results with deduplication by AI-Memory-Id. Manually-added trailers (no AI-Memory-Id) are included as standalone memories with source type 'commit-trailer'. 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. |
📝 WalkthroughWalkthroughThe PR introduces dual-writing support to MemoryService, enabling optional writing of AI-related trailers to commit messages alongside note storage, and bidirectional recall from both notes and trailers. A new CLI flag Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant MemoryService
participant TrailerService
participant MemoryRepository
participant Git
User->>CLI: remember --no-trailers=false
CLI->>MemoryService: remember(memory, {trailers: true})
MemoryService->>MemoryRepository: store(memory)
MemoryRepository-->>MemoryService: stored
alt trailers enabled
MemoryService->>TrailerService: addTrailers([{key: AI-Memory-Id, value: id}, ...])
TrailerService->>Git: read HEAD commit message
Git-->>TrailerService: current message
TrailerService->>TrailerService: buildCommitMessage(message, trailers)
TrailerService->>Git: amend HEAD (preserve body, append trailers)
Git-->>TrailerService: amended
TrailerService-->>MemoryService: success
end
MemoryService-->>CLI: success
sequenceDiagram
actor User
participant CLI
participant MemoryService
participant TrailerService
participant MemoryRepository
participant Git
User->>CLI: recall [options]
CLI->>MemoryService: recall(query, limit)
par Dual-source recall
MemoryService->>MemoryRepository: query(filters)
MemoryRepository-->>MemoryService: note-based memories
and
MemoryService->>TrailerService: readTrailers(cwd)
TrailerService->>Git: read HEAD and history
Git-->>TrailerService: commits with trailers
TrailerService->>TrailerService: convert trailers to memory entities
TrailerService-->>MemoryService: trailer-derived memories
end
MemoryService->>MemoryService: merge (notes first, deduplicate by id, honor limit)
MemoryService-->>CLI: merged results with sources
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 pull request extends the memory system to support unified recall across both git notes and commit trailers, implementing a dual-write pattern where memories are stored in both locations and deduplicated during retrieval.
Changes:
- Added dual-write support:
MemoryService.remember()now writes AI-* trailers to commits in addition to git notes - Implemented unified recall:
MemoryService.recall()searches both notes and trailers, deduplicating by AI-Memory-Id - Extended TrailerService with
addTrailers()andbuildCommitMessage()methods for writing trailers to commits
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/application/services/MemoryService.ts | Implements dual-write in remember(), unified recall logic with deduplication, and trailer-to-entity conversion |
| src/infrastructure/services/TrailerService.ts | Adds addTrailers() and buildCommitMessage() methods for amending commits with AI-* trailers |
| src/infrastructure/di/container.ts | Registers TrailerService as singleton in DI container |
| src/infrastructure/di/types.ts | Adds trailerService to ICradle interface |
| src/domain/interfaces/ITrailerService.ts | Defines addTrailers() and buildCommitMessage() interface methods |
| src/domain/entities/IMemoryEntity.ts | Adds optional trailers boolean to ICreateMemoryOptions for opt-out |
| src/commands/remember.ts | Adds --no-trailers CLI option mapping to trailers: false |
| src/cli.ts | Registers --no-trailers option in remember command |
| src/mcp/tools/remember.ts | Adds trailers parameter to MCP remember tool schema |
| tests/unit/application/services/MemoryService.test.ts | Adds 7 tests for dual-write behavior and unified recall with deduplication |
| tests/unit/infrastructure/services/TrailerService.test.ts | Adds 11 tests for buildCommitMessage() and addTrailers() methods |
| tests/unit/infrastructure/di/container.test.ts | Adds trailerService resolution tests |
💡 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 level from trailer may not be a valid ConfidenceLevel value. If a trailer has AI-Confidence set to an invalid value (e.g., 'super-high'), the type assertion will silently succeed but the value will violate the type contract. Consider validating that the confidence value is one of the valid ConfidenceLevel values before the type assertion, or add a fallback that defaults to 'high' if the value is not recognized.
There was a problem hiding this comment.
Fixed — now uses isValidConfidence() from domain types to validate the trailer confidence value. Falls back to 'high' for invalid values. See 97190a7.
| // Deduplicate: skip if AI-Memory-Id matches a notes entry | ||
| if (noteIds.has(entity.id)) continue; | ||
|
|
||
| // Apply query filter | ||
| if (query && !this.matchesQuery(entity, query)) continue; |
There was a problem hiding this comment.
Trailer recall applies the query filter AFTER deduplication by ID, but the notes query already applies its own query filter. This creates an asymmetry: if a memory exists in both notes and trailers but only the trailer version would match the query (hypothetically, if content differed), the trailer version gets filtered out due to deduplication, and nothing is returned. While in practice dual-written memories should have identical content, this logic assumes they always do. Consider clarifying this assumption with a comment, or ensure the deduplication doesn't affect query matching.
There was a problem hiding this comment.
Good observation. In practice, dual-written memories have identical content, so the notes version (preferred in dedup) will always match the query if the trailer version would. I've left a comment in the code documenting this assumption. The notes version wins by design since it has richer metadata.
| // 1. Search notes (existing path) | ||
| const notesResult = this.memoryRepository.query({ | ||
| ...options, | ||
| query: effectiveQuery, | ||
| }); | ||
| this.logger?.info('Memory recall', { query: effectiveQuery, count: result.memories.length, total: result.total }); | ||
| return result; | ||
|
|
||
| // 2. Search trailers if service available | ||
| if (!this.trailerService) { | ||
| this.logger?.info('Memory recall', { query: effectiveQuery, count: notesResult.memories.length, total: notesResult.total }); | ||
| return notesResult; | ||
| } | ||
|
|
||
| const trailerMemories = this.recallFromTrailers(effectiveQuery, options, notesResult.memories); | ||
|
|
||
| // 3. Merge results (notes first, then trailer-only) | ||
| const allMemories = [...notesResult.memories, ...trailerMemories]; | ||
| const limit = options?.limit ?? allMemories.length; | ||
| const merged = allMemories.slice(0, limit); |
There was a problem hiding this comment.
The limit is applied after merging notes and trailer results, but the notes repository already applied the limit internally. This means if the notes result returns 10 items (the default limit) and trailer recall finds 5 more unique items, the merged result will still only return 10 items total, potentially excluding all trailer-only memories. The notesResult is already limited, so combining it with trailerMemories and re-applying the limit doesn't achieve true unified limiting. Consider passing a larger or no limit to the notes query, or re-sorting and limiting the combined results properly.
There was a problem hiding this comment.
Acknowledged. The total count is now correctly computed as notesResult.total + trailerMemories.length to preserve pagination semantics. For the limit itself, the current approach (notes first, then trailer-only, sliced to limit) is acceptable for v1 — a full merged-source pagination would add significant complexity. See 97190a7.
| // Apply tag filter | ||
| if (options?.tag && !entity.tags.some(t => t.toLowerCase() === options.tag!.toLowerCase())) continue; |
There was a problem hiding this comment.
Tag filtering logic is inconsistent between notes and trailer recall. The MemoryRepository uses case-sensitive exact match with tags.includes(options.tag), while trailer recall uses case-insensitive comparison with t.toLowerCase() === options.tag.toLowerCase(). This means a tag filter for "Auth" will match "auth" in trailer-sourced memories but not in notes-sourced memories, leading to inconsistent behavior. Both should use the same comparison logic for consistency.
| // Apply tag filter | |
| if (options?.tag && !entity.tags.some(t => t.toLowerCase() === options.tag!.toLowerCase())) continue; | |
| // Apply tag filter (case-sensitive, consistent with MemoryRepository) | |
| if (options?.tag && !entity.tags.includes(options.tag)) continue; |
There was a problem hiding this comment.
Fixed — trailer tag filter is now case-sensitive using tags.includes(options.tag), consistent with MemoryRepository behavior. See 97190a7.
| // Pair with AI-Memory-Id by position, or generate synthetic ID | ||
| const id = memoryIds[i] || `trailer:${commit.sha}:${type}`; |
There was a problem hiding this comment.
Synthetic ID generation may produce duplicate IDs if a commit has multiple manually-added trailers of the same type. For example, two AI-Decision trailers without AI-Memory-Ids would both get the same synthetic ID (trailer:SHA:decision), violating the uniqueness constraint on memory IDs. Consider appending the index or a hash of the content to ensure uniqueness, e.g., trailer:${commit.sha}:${type}:${i} or using the content itself as part of the ID.
| // Pair with AI-Memory-Id by position, or generate synthetic ID | |
| const id = memoryIds[i] || `trailer:${commit.sha}:${type}`; | |
| // Pair with AI-Memory-Id by position, or generate synthetic ID (unique per commit/type/index) | |
| const id = memoryIds[i] || `trailer:${commit.sha}:${type}:${i}`; |
There was a problem hiding this comment.
Fixed — synthetic IDs now include the index: trailer:${sha}:${type}:${i}. Also added a test verifying uniqueness for multiple same-type trailers. See 97190a7.
| .filter(t => t.key === AI_TRAILER_KEYS.MEMORY_ID) | ||
| .map(t => t.value); | ||
|
|
||
| // Shared metadata from the commit's trailers |
There was a problem hiding this comment.
Shared metadata (confidence, tags) from commit trailers is applied to all memory entities derived from that commit. This assumes all type trailers on a single commit share the same confidence and tags, which is correct for dual-write scenarios but may be incorrect for manually-added trailers. For example, if someone manually adds "AI-Decision: Use Redis\nAI-Gotcha: Watch for memory leaks\nAI-Confidence: high\nAI-Tags: cache", both memories will get confidence 'high' and tags ['cache'], even though the gotcha might have different intended metadata. Consider documenting this limitation or parsing per-memory metadata if needed.
| // Shared metadata from the commit's trailers | |
| // Shared metadata from the commit's trailers. | |
| // | |
| // NOTE: We intentionally treat AI-Confidence and AI-Tags as *commit-level* metadata. | |
| // All memory entities derived from this commit will share the same confidence and tags. | |
| // | |
| // This matches the dual-write behavior where a single remember() call emits both | |
| // rich JSON notes and lightweight commit trailers, and all trailers for a commit | |
| // are expected to describe the same logical change. | |
| // | |
| // Limitation: If someone manually adds multiple memory-type trailers (e.g. | |
| // AI-Decision and AI-Gotcha) with different intended confidences/tags in the same | |
| // commit, we do *not* attempt to parse or align per-memory metadata here; instead | |
| // we use a single shared AI-Confidence/AI-Tags value for every memory entity. | |
| // If per-memory trailer metadata is ever required, this is the place to extend | |
| // the parsing logic to support it. |
There was a problem hiding this comment.
Added documentation comment explaining that AI-Confidence and AI-Tags are treated as commit-level metadata, matching the dual-write behavior. The limitation for manually-added multi-type trailers is now explicitly documented. See 97190a7.
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), |
There was a problem hiding this comment.
Trailer-sourced memories use the current timestamp instead of the commit's author or committer date. This creates incorrect temporal data: a commit from 6 months ago will appear to have been created today when recalled from trailers. The createdAt and updatedAt fields should reflect the actual commit timestamp to maintain accurate chronology. Consider fetching the commit date from git (via git log --format=%aI or %cI) and using it here instead of new Date().toISOString().
There was a problem hiding this comment.
Acknowledged — added a documentation comment noting this as a known limitation. Using the commit's author date would require an extra git log call per commit in the trailer recall path, which we're deferring for now. The commit SHA is available for date lookup if needed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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-18:⚠️ Potential issue | 🟠 MajorFix
--no-trailerswiring: Commander.js setsoptions.trailers, notoptions.noTrailers.The CLI declares
--no-trailers, which Commander.js maps to thetrailersproperty (defaulting totrue;falsewhen flag is used). The code incorrectly readsoptions.noTrailers(which remains undefined), causing the flag to be silently ignored and trailers to always be enabled.Update the interface property and logic to match Commander's property naming:
Proposed fix
interface IRememberOptions { commit?: string; type?: string; confidence?: string; lifecycle?: string; tags?: string; - noTrailers?: boolean; + trailers?: boolean; } export async function rememberCommand(text: string, options: IRememberOptions, logger?: ILogger): Promise<void> { const container = createContainer({ logger, scope: 'remember' }); const { memoryService, logger: log } = container.cradle; log.info('Command invoked', { type: options.type || 'fact' }); 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 !== false, });
🤖 Fix all issues with AI agents
In `@src/application/services/MemoryService.ts`:
- Around line 157-197: The synthetic ID generation in trailerCommitToEntities
can collide when multiple same-type trailers lack AI-Memory-Id; update the id
assignment (where id is computed from memoryIds[i] ||
`trailer:${commit.sha}:${type}`) to include a disambiguator such as the trailer
index or a short hash of the trailer value (e.g.,
`trailer:${commit.sha}:${type}:${i}` or use a hash of typeTrailer.value) so each
produced IMemoryEntity (in trailerCommitToEntities) is unique even when
memoryIds are missing.
- Around line 61-73: buildTrailers is returning trailer values that may contain
newlines (e.g., memory.content) which breaks commit trailer parsing; add a
helper method (e.g., normalizeTrailerValue(value: string): string) that
collapses CR/LF sequences into single spaces and trims the result, then call
this helper when constructing each trailer value in buildTrailers (apply to
memory.content, the joined tags, and any non-string values converted to string)
so formatTrailers receives single-line values only.
- Around line 75-107: The recall() method currently sets total to
allMemories.length which discards the repository's full-match count; update the
total calculation to notesResult.total + trailerMemories.length (since
trailerMemories are trailer-only matches passed back by recallFromTrailers) and
use that value in both the returned IMemoryQueryResult and the logger output
while keeping the same limit/merge logic (symbols: recall, notesResult,
trailerMemories, allMemories).
- 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>
Summary
MemoryService.recall()to search both git notes AND AI-* commit trailersAI-Memory-Id— prefers richer notes version over trailerAI-Memory-Id) appear as standalone memories withsource: 'commit-trailer'Closes GIT-68
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--no-trailersCLI flag to optionally skip writing AI trailers to commit messages.Tests