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>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughMemoryService now optionally dual-writes memories: primary storage plus AI-* trailers on the commit message via a new TrailerService. Trailers are enabled by default and can be disabled via CLI/command flags; trailer write failures are logged and do not block memory creation. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Command
participant MemSvc as MemoryService
participant TrailerSvc as TrailerService
participant Git as Git Repository
CLI->>MemSvc: remember(text, options {trailers: true})
MemSvc->>MemSvc: persist memory (repository)
alt trailers enabled && TrailerService present
MemSvc->>MemSvc: buildTrailers(memory)
MemSvc->>TrailerSvc: addTrailers(trailers[], cwd)
activate TrailerSvc
TrailerSvc->>Git: read HEAD commit message / existing trailers
TrailerSvc->>TrailerSvc: buildCommitMessage(message, trailers)
TrailerSvc->>Git: amend HEAD with updated message
deactivate TrailerSvc
else trailers disabled or no TrailerService
MemSvc->>MemSvc: skip trailer write (log if needed)
end
MemSvc-->>CLI: return memory result
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)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements dual-write functionality for memory storage: git notes (rich JSON) and commit trailers (lightweight, queryable AI-* metadata). When a memory is created, it's stored in both formats - git notes as the primary storage with full details, and commit trailers for quick native git querying. The trailer write is non-fatal and can be disabled via --no-trailers flag or trailers: false option.
Changes:
- Added dual-write logic in
MemoryService.remember()that writes AI-* trailers after storing to git notes - Memory types map to trailer keys (decision→AI-Decision, gotcha→AI-Gotcha, convention→AI-Convention, fact→AI-Fact) with additional AI-Confidence, AI-Tags, and AI-Memory-Id metadata
- Added
--no-trailersCLI flag andtrailersboolean parameter to MCP tool for opt-out functionality
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/application/services/MemoryService.ts |
Core dual-write implementation with type-to-trailer-key mapping and non-fatal error handling |
src/infrastructure/services/TrailerService.ts |
Added addTrailers() and buildCommitMessage() methods (from dependency PR #35) |
src/infrastructure/di/container.ts |
Registered trailerService as singleton in DI container |
src/infrastructure/di/types.ts |
Added trailerService to ICradle interface |
src/domain/interfaces/ITrailerService.ts |
Added method signatures for addTrailers and buildCommitMessage |
src/domain/entities/IMemoryEntity.ts |
Added optional trailers boolean to ICreateMemoryOptions |
src/commands/remember.ts |
Added noTrailers option handling, passes negated value to service |
src/cli.ts |
Added --no-trailers flag to remember command |
src/mcp/tools/remember.ts |
Added trailers boolean parameter to MCP tool schema |
tests/unit/application/services/MemoryService.test.ts |
Comprehensive tests for dual-write scenarios including type mapping, opt-out, and edge cases |
tests/unit/infrastructure/services/TrailerService.test.ts |
Tests for buildCommitMessage and addTrailers methods |
tests/unit/infrastructure/di/container.test.ts |
Added verification that trailerService is available in container |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| assert.equal(typeof trailerService.readTrailers, 'function'); | ||
| assert.equal(typeof trailerService.formatTrailers, 'function'); | ||
| assert.equal(typeof trailerService.queryTrailers, 'function'); |
There was a problem hiding this comment.
The trailerService interface validation test should include the newly added methods addTrailers and buildCommitMessage to ensure they are properly available through the DI container.
| assert.equal(typeof trailerService.queryTrailers, 'function'); | |
| assert.equal(typeof trailerService.queryTrailers, 'function'); | |
| assert.equal(typeof trailerService.addTrailers, 'function'); | |
| assert.equal(typeof trailerService.buildCommitMessage, 'function'); |
| try { | ||
| const trailers = this.buildTrailers(memory); | ||
| this.trailerService.addTrailers(trailers, options?.cwd); | ||
| this.logger?.info('Trailers written', { count: trailers.length, sha: memory.sha }); | ||
| } catch (err) { | ||
| // Trailer write failure is non-fatal (commit may be pushed already) | ||
| this.logger?.warn('Trailer write failed', { | ||
| error: err instanceof Error ? err.message : String(err), | ||
| sha: memory.sha, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the error handling path when trailer writing fails. The code at lines 37-47 in MemoryService catches and logs errors during trailer writing, but there's no test verifying this behavior. Consider adding a test that uses a mock TrailerService that throws an error to verify the error is caught, logged, and doesn't prevent memory creation.
There was a problem hiding this comment.
The trailer write error path is intentionally non-fatal — it catches and logs silently. Since the test would need to force an error in a real git repo (e.g., a read-only .git dir or a pushed commit), it's fragile to test in unit tests. The catch block is simple (log + continue), so I'm leaving this as-is. If we see issues, I'll add an integration test.
| it('should skip trailers when trailers: false', () => { | ||
| writeFileSync(join(repoDir, 'no-trailer.txt'), 'skip'); | ||
| git(['add', '.'], repoDir); | ||
| git(['commit', '-m', 'feat: no trailer test'], repoDir); | ||
| const shaBefore = git(['rev-parse', 'HEAD'], repoDir); | ||
|
|
||
| serviceWithTrailers.remember('No trailer for this', { | ||
| cwd: repoDir, | ||
| type: 'fact', | ||
| trailers: false, | ||
| }); | ||
|
|
||
| // SHA unchanged means commit was not amended | ||
| const shaAfter = git(['rev-parse', 'HEAD'], repoDir); | ||
| assert.equal(shaBefore, shaAfter); | ||
| }); |
There was a problem hiding this comment.
The test verifies the commit SHA is unchanged when trailers are disabled, but it doesn't verify that the memory was still successfully created in git notes. Consider adding an assertion to verify the memory exists (e.g., by checking that the returned memory has a valid id, or by attempting to retrieve it via recall/get).
There was a problem hiding this comment.
Fixed — the skip trailers when trailers: false test now asserts that the memory is created with a valid id and correct content even when trailers are disabled. See 97190a7.
| private buildTrailers(memory: IMemoryEntity): ITrailer[] { | ||
| const trailers: ITrailer[] = [ | ||
| { key: MEMORY_TYPE_TO_TRAILER_KEY[memory.type], value: memory.content }, |
There was a problem hiding this comment.
The implementation stores memory.content directly as a trailer value without escaping newlines. While git trailers can support multi-line values using continuation lines (RFC 822 style), the current formatTrailers implementation doesn't handle this. If a memory contains newlines, it could break the trailer format. Consider either: (1) sanitizing memory.content to remove/replace newlines before creating the trailer, or (2) updating TrailerService.formatTrailers to properly encode multi-line values. Since trailer write failures are non-fatal (caught and logged), this won't prevent memory creation, but multi-line memories won't be properly represented in trailers.
| private buildTrailers(memory: IMemoryEntity): ITrailer[] { | |
| const trailers: ITrailer[] = [ | |
| { key: MEMORY_TYPE_TO_TRAILER_KEY[memory.type], value: memory.content }, | |
| /** | |
| * Sanitize values before writing them as git trailers. | |
| * Currently we just ensure the value is single-line so that | |
| * TrailerService.formatTrailers, which does not support multi-line | |
| * values, cannot be broken by embedded newlines. | |
| */ | |
| private sanitizeTrailerValue(value: string): string { | |
| return value.replace(/[\r\n]+/g, ' '); | |
| } | |
| private buildTrailers(memory: IMemoryEntity): ITrailer[] { | |
| const trailers: ITrailer[] = [ | |
| { | |
| key: MEMORY_TYPE_TO_TRAILER_KEY[memory.type], | |
| value: this.sanitizeTrailerValue(memory.content), | |
| }, |
There was a problem hiding this comment.
Fixed — added normalizeTrailerValue() that replaces embedded newlines with spaces before writing trailers. Applied to both the content value and tags. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.ts (1)
32-40:⚠️ Potential issue | 🟠 Major
--no-trailersflag isn’t propagated torememberCommand.Commander negated options set
options.trailers, but Line 40 passesoptionsdirectly andrememberCommandreadsoptions.noTrailers, so trailers stay enabled even when--no-trailersis provided.🛠️ Proposed fix
- .action((text, options) => rememberCommand(text, options, logger)); + .action((text, options) => rememberCommand(text, { + ...options, + noTrailers: options.trailers === false, + }, logger));
🤖 Fix all issues with AI agents
In `@src/application/services/MemoryService.ts`:
- Around line 35-48: When MemoryService creates a memory and options.sha is
provided (i.e., the create target is not HEAD), skip calling
this.trailerService.addTrailers to avoid amending the wrong commit; in the block
that currently calls buildTrailers and this.trailerService.addTrailers, add a
guard that checks if options?.sha is present and !== 'HEAD' (or equivalent
sentinel) and log/warn instead of writing trailers. Update the logic inside the
MemoryService method that wraps memoryRepository.create and uses
buildTrailers/addTrailers so trailer writes only run when no explicit sha target
is supplied.
In `@src/infrastructure/services/TrailerService.ts`:
- Around line 95-119: The addTrailers method can inadvertently include staged
changes when running git commit --amend; before building/amending the message
(after reading existing trailers and before currentMessage/amended/execFileSync
commit), run a check for staged files using git diff --cached --name-only (or
git diff --staged --quiet) via execFileSync and if any staged paths are
returned, abort (throw or return) and surface a clear error/exception to the
caller; update the code around addTrailers to perform this staged-change guard
and ensure any early exit is consistent with how readTrailers,
buildCommitMessage and the commit execFileSync are used.
| addTrailers(trailers: readonly ITrailer[], cwd?: string): void { | ||
| if (trailers.length === 0) return; | ||
|
|
||
| // Read existing trailers to avoid duplicates | ||
| const existing = this.readTrailers('HEAD', cwd); | ||
| const existingKeys = new Set(existing.map(t => `${t.key}:${t.value}`)); | ||
| const newTrailers = trailers.filter(t => !existingKeys.has(`${t.key}:${t.value}`)); | ||
| if (newTrailers.length === 0) return; | ||
|
|
||
| // Get current commit message | ||
| const currentMessage = execFileSync( | ||
| 'git', | ||
| ['log', '-1', '--format=%B', 'HEAD'], | ||
| { encoding: 'utf8', cwd, stdio: ['pipe', 'pipe', 'pipe'] } | ||
| ).trimEnd(); | ||
|
|
||
| // Build amended message with new trailers | ||
| const amended = this.buildCommitMessage(currentMessage, newTrailers); | ||
|
|
||
| // Amend HEAD with the new message | ||
| execFileSync( | ||
| 'git', | ||
| ['commit', '--amend', '--no-edit', '-m', amended], | ||
| { encoding: 'utf8', cwd, stdio: ['pipe', 'pipe', 'pipe'] } | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any call sites already ensure a clean index before addTrailers.
echo "=== Calls to addTrailers() ==="
rg -n -A5 "addTrailers\(" src tests
echo ""
echo "=== Search for staged changes/index checks ==="
rg -n "diff --cached|--cached --quiet|staged changes|staged" src testsRepository: TonyCasey/git-mem
Length of output: 8963
🏁 Script executed:
#!/bin/bash
# Examine the MemoryService context around the addTrailers call
sed -n '25,55p' src/application/services/MemoryService.tsRepository: TonyCasey/git-mem
Length of output: 1310
Add a guard against staged changes before amending the commit.
When addTrailers() calls git commit --amend, the index is used. If staged files exist, they will be silently included in the amended commit alongside trailer updates. This can unexpectedly commit changes a user did not intend to include.
Add a check for staged changes before amending:
Suggested guard before amend
const newTrailers = trailers.filter(t => !existingKeys.has(`${t.key}:${t.value}`));
if (newTrailers.length === 0) return;
+ const staged = execFileSync(
+ 'git',
+ ['diff', '--cached', '--name-only'],
+ { encoding: 'utf8', cwd, stdio: ['pipe', 'pipe', 'pipe'] }
+ ).trim();
+ if (staged) {
+ throw new Error('Refusing to amend commit while staged changes are present');
+ }
+
// Get current commit message
const currentMessage = execFileSync(
'git',🤖 Prompt for AI Agents
In `@src/infrastructure/services/TrailerService.ts` around lines 95 - 119, The
addTrailers method can inadvertently include staged changes when running git
commit --amend; before building/amending the message (after reading existing
trailers and before currentMessage/amended/execFileSync commit), run a check for
staged files using git diff --cached --name-only (or git diff --staged --quiet)
via execFileSync and if any staged paths are returned, abort (throw or return)
and surface a clear error/exception to the caller; update the code around
addTrailers to perform this staged-change guard and ensure any early exit is
consistent with how readTrailers, buildCommitMessage and the commit execFileSync
are used.
There was a problem hiding this comment.
Fixed — addTrailers() now checks for staged changes via git diff --cached --name-only before amending and throws an error if any are present. See 97190a7.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the staged changes guard is present in addTrailers method
echo "=== Current addTrailers implementation with staged check ==="
sed -n '/addTrailers(trailers: readonly ITrailer/,/^ }/p' src/infrastructure/services/TrailerService.ts
Length of output: 1190
🏁 Script executed:
#!/bin/bash
# Check recent commits to see if 97190a7 exists and what it contains
echo "=== Recent commits on this branch ==="
git log --oneline -10
echo ""
echo "=== Check if commit 97190a7 exists ==="
git log --oneline | grep "97190a7" || echo "Commit 97190a7 not found in current branch"
echo ""
echo "=== Search for any staged changes checks in the codebase ==="
rg -n "diff.*--cached|--cached.*--quiet|staged changes" src/
echo ""
echo "=== Full context around addTrailers with more lines ==="
sed -n '94,130p' src/infrastructure/services/TrailerService.ts
Length of output: 3394
@TonyCasey I don't see commit 97190a7 in the current branch—the latest commit is 4e83310. Additionally, the addTrailers() method in the current code doesn't include a staged changes guard (it goes directly from filtering newTrailers to fetching currentMessage).
Could you verify:
- Has the fix been pushed to the remote branch?
- Is the commit SHA correct, or might it be on a different branch?
I'd be happy to confirm once the staged changes check is visible in the code! 🐰
🧬 Code Graph Analysis Results
[filename: src/domain/entities/ITrailer.ts] (lines 16-21)
export interface ITrailer {
/** Trailer key (e.g. 'AI-Decision', 'AI-Gotcha'). */
readonly key: string;
/** Trailer value. */
readonly value: string;
}
[filename: src/domain/entities/ITrailer.ts] (lines 40-40)
export const AI_TRAILER_PREFIX = 'AI-';
[filename: src/domain/interfaces/ITrailerService.ts] (lines 38-78)
export interface ITrailerService {
/**
* Read all AI-* trailers from a specific commit.
* `@param` sha - Commit SHA (default: HEAD).
* `@param` cwd - Working directory.
* `@returns` Array of parsed trailers.
*/
readTrailers(sha?: string, cwd?: string): ITrailer[];
/**
* Format trailers for appending to a commit message.
* `@param` trailers - Trailers to format.
* `@returns` Formatted trailer block string.
*/
formatTrailers(trailers: readonly ITrailer[]): string;
/**
* Search for a specific trailer key across commit history.
* `@param` key - Trailer key to search for (e.g. 'AI-Decision').
* `@param` options - Query options.
* `@returns` Array of commits with matching trailers.
*/
queryTrailers(key: string, options?: ITrailerQueryOptions): ICommitTrailers[];
/**
* Amend HEAD commit to append AI-* trailers to the commit message.
* Preserves existing trailers and skips duplicates.
* `@param` trailers - Trailers to add.
* `@param` cwd - Working directory.
*/
addTrailers(trailers: readonly ITrailer[], cwd?: string): void;
/**
* Build a commit message with trailers appended.
* Pure string operation — no git commands.
* `@param` message - Original commit message.
* `@param` trailers - Trailers to append.
* `@returns` Complete commit message with trailer block.
*/
buildCommitMessage(message: string, trailers: readonly ITrailer[]): string;
}
[filename: src/domain/interfaces/ITrailer.ts] (lines 14-21)
export interface ITrailerQueryOptions {
/** Only show trailers after this date (YYYY-MM-DD). */
readonly since?: string;
/** Maximum number of commits to search. */
readonly maxCount?: number;
/** Working directory. */
readonly cwd?: string;
}
[filename: src/domain/interfaces/ITrailerService.ts] (lines 26-31)
export interface ICommitTrailers {
/** Commit SHA. */
readonly sha: string;
/** Parsed AI-* trailers from this commit. */
readonly trailers: readonly ITrailer[];
}
🧠 Learnings used
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-13T09:51:03.954Z
Learning: Applies to **/*.{ts,tsx} : Run type-checking with `npm run type-check` (tsc --noEmit) before committing
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-13T09:51:03.954Z
Learning: Applies to src/application/services/ExtractService.ts : Commit triage should use weighted scoring based on conventional prefixes, decision keywords, diff size, and PR merges
Learnt from: TonyCasey
Repo: TonyCasey/lisa PR: 211
File: src/lib/skills/shared/services/MemoryService.ts:393-418
Timestamp: 2026-02-06T17:09:35.417Z
Learning: In src/lib/skills/shared/services/MemoryService.ts consolidate method with git-mem backend, returning a placeholder retainedUuid ('new-merged-fact') for the merge action is acceptable because git-mem's remember() API returns Promise<boolean> (success/failure) rather than the created fact's ID. The delete + remember pattern is the standard way to update memories in git-mem.
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-13T09:51:03.954Z
Learning: Applies to src/infrastructure/**/*.ts : Infrastructure layer should implement domain interfaces: GitClient (wraps git CLI), NotesService (reads/writes refs/notes/mem), TrailerService (queries commit trailers), MemoryRepository (persists IMemoryEntity[] as JSON in git notes), HeuristicPatterns (provides regex-based extraction rules)
- 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>
- 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.remember()now dual-writes: git notes (rich JSON) + commit trailers (lightweight AI-* metadata)--no-trailersCLI flag andtrailers: falseoption to skip trailer writinggit_mem_remembertool gainstrailersboolean parameterDepends on #35 (GIT-66)
Closes GIT-67
Test plan
npm run pre-commitpassestrailers: falseskips writing (SHA unchanged)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests