-
Notifications
You must be signed in to change notification settings - Fork 0
feat: dual-write trailers in MemoryService.remember() (GIT-67) #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7dd735b
4080eec
8fcd93f
4e83310
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,26 +2,68 @@ | |||||||||||||||||||||||||||||||||||||||
| * MemoryService | ||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||
| * Application service that orchestrates memory operations. | ||||||||||||||||||||||||||||||||||||||||
| * Delegates storage to IMemoryRepository. | ||||||||||||||||||||||||||||||||||||||||
| * Dual-writes to git notes (rich JSON) and commit trailers | ||||||||||||||||||||||||||||||||||||||||
| * (lightweight, natively queryable metadata). | ||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| import type { IMemoryService } from '../interfaces/IMemoryService'; | ||||||||||||||||||||||||||||||||||||||||
| import type { IMemoryRepository, IMemoryQueryOptions, IMemoryQueryResult } from '../../domain/interfaces/IMemoryRepository'; | ||||||||||||||||||||||||||||||||||||||||
| import type { IMemoryEntity, ICreateMemoryOptions } from '../../domain/entities/IMemoryEntity'; | ||||||||||||||||||||||||||||||||||||||||
| import type { IMemoryEntity, ICreateMemoryOptions, MemoryType } from '../../domain/entities/IMemoryEntity'; | ||||||||||||||||||||||||||||||||||||||||
| import type { ITrailerService } from '../../domain/interfaces/ITrailerService'; | ||||||||||||||||||||||||||||||||||||||||
| import type { ITrailer } from '../../domain/entities/ITrailer'; | ||||||||||||||||||||||||||||||||||||||||
| import { AI_TRAILER_KEYS } from '../../domain/entities/ITrailer'; | ||||||||||||||||||||||||||||||||||||||||
| import type { ILogger } from '../../domain/interfaces/ILogger'; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const MEMORY_TYPE_TO_TRAILER_KEY: Record<MemoryType, string> = { | ||||||||||||||||||||||||||||||||||||||||
| decision: AI_TRAILER_KEYS.DECISION, | ||||||||||||||||||||||||||||||||||||||||
| gotcha: AI_TRAILER_KEYS.GOTCHA, | ||||||||||||||||||||||||||||||||||||||||
| convention: AI_TRAILER_KEYS.CONVENTION, | ||||||||||||||||||||||||||||||||||||||||
| fact: AI_TRAILER_KEYS.FACT, | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| export class MemoryService implements IMemoryService { | ||||||||||||||||||||||||||||||||||||||||
| constructor( | ||||||||||||||||||||||||||||||||||||||||
| private readonly memoryRepository: IMemoryRepository, | ||||||||||||||||||||||||||||||||||||||||
| private readonly logger?: ILogger, | ||||||||||||||||||||||||||||||||||||||||
| private readonly trailerService?: ITrailerService, | ||||||||||||||||||||||||||||||||||||||||
| ) {} | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| remember(text: string, options?: ICreateMemoryOptions): IMemoryEntity { | ||||||||||||||||||||||||||||||||||||||||
| const memory = this.memoryRepository.create(text, options); | ||||||||||||||||||||||||||||||||||||||||
| this.logger?.info('Memory stored', { id: memory.id, type: memory.type, sha: memory.sha }); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Dual-write: also add AI-* trailers to the commit (opt-out via trailers: false) | ||||||||||||||||||||||||||||||||||||||||
| if (options?.trailers !== false && this.trailerService) { | ||||||||||||||||||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| return memory; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| private buildTrailers(memory: IMemoryEntity): ITrailer[] { | ||||||||||||||||||||||||||||||||||||||||
| const trailers: ITrailer[] = [ | ||||||||||||||||||||||||||||||||||||||||
| { key: MEMORY_TYPE_TO_TRAILER_KEY[memory.type], value: memory.content }, | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+53
to
+55
|
||||||||||||||||||||||||||||||||||||||||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed — added normalizeTrailerValue() that replaces embedded newlines with spaces before writing trailers. Applied to both the content value and tags. See 97190a7.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,61 @@ export class TrailerService implements ITrailerService { | |
| } | ||
| } | ||
|
|
||
| addTrailers(trailers: readonly ITrailer[], cwd?: string): void { | ||
| // Only operate on AI-* trailers to match readTrailers/parseTrailerBlock behavior. | ||
| const aiTrailers = trailers.filter(t => t.key.startsWith(AI_TRAILER_PREFIX)); | ||
| if (aiTrailers.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 = aiTrailers.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'] } | ||
| ); | ||
|
Comment on lines
+95
to
+121
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 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
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 1190 🏁 Script executed: Length of output: 3394
Could you verify:
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) [filename: src/domain/entities/ITrailer.ts] (lines 40-40) [filename: src/domain/interfaces/ITrailerService.ts] (lines 38-78) [filename: src/domain/interfaces/ITrailer.ts] (lines 14-21) [filename: src/domain/interfaces/ITrailerService.ts] (lines 26-31) 🧠 Learnings used |
||
| } | ||
|
|
||
| buildCommitMessage(message: string, trailers: readonly ITrailer[]): string { | ||
| if (trailers.length === 0) return message; | ||
|
|
||
| const trailerBlock = this.formatTrailers(trailers); | ||
| const trimmed = message.trimEnd(); | ||
|
|
||
| // Detect existing trailer block: lines after the last blank line | ||
| // must all match "Key: Value" format (git trailer convention). | ||
| if (this.hasTrailerBlock(trimmed)) { | ||
| return `${trimmed}\n${trailerBlock}\n`; | ||
| } | ||
|
|
||
| return `${trimmed}\n\n${trailerBlock}\n`; | ||
| } | ||
|
|
||
| private hasTrailerBlock(message: string): boolean { | ||
| const lastBlankIdx = message.lastIndexOf('\n\n'); | ||
| if (lastBlankIdx === -1) return false; | ||
|
|
||
| const afterBlank = message.slice(lastBlankIdx + 2).trim(); | ||
| if (!afterBlank) return false; | ||
|
|
||
| const lines = afterBlank.split('\n').filter(l => l.trim().length > 0); | ||
| return lines.length > 0 && lines.every(l => /^[\w-]+:\s/.test(l)); | ||
| } | ||
|
|
||
| private parseTrailerBlock(block: string): ITrailer[] { | ||
| const trailers: ITrailer[] = []; | ||
| const lines = block.split('\n'); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,20 +7,25 @@ import { tmpdir } from 'os'; | |
| import { MemoryService } from '../../../../src/application/services/MemoryService'; | ||
| import { MemoryRepository } from '../../../../src/infrastructure/repositories/MemoryRepository'; | ||
| import { NotesService } from '../../../../src/infrastructure/services/NotesService'; | ||
| import { TrailerService } from '../../../../src/infrastructure/services/TrailerService'; | ||
|
|
||
| function git(args: string[], cwd: string): string { | ||
| return execFileSync('git', args, { encoding: 'utf8', cwd }).trim(); | ||
| } | ||
|
|
||
| describe('MemoryService', () => { | ||
| let service: MemoryService; | ||
| let serviceWithTrailers: MemoryService; | ||
| let trailerService: TrailerService; | ||
| let repoDir: string; | ||
| let commitSha: string; | ||
|
|
||
| before(() => { | ||
| const notesService = new NotesService(); | ||
| const memoryRepo = new MemoryRepository(notesService); | ||
| trailerService = new TrailerService(); | ||
| service = new MemoryService(memoryRepo); | ||
| serviceWithTrailers = new MemoryService(memoryRepo, undefined, trailerService); | ||
|
|
||
| repoDir = mkdtempSync(join(tmpdir(), 'git-mem-memsvc-test-')); | ||
| git(['init'], repoDir); | ||
|
|
@@ -53,6 +58,100 @@ describe('MemoryService', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('remember with trailers', () => { | ||
| it('should write AI-* trailers to the commit', () => { | ||
| // Create a fresh commit for this test | ||
| writeFileSync(join(repoDir, 'trailer-test.txt'), 'trailer'); | ||
| git(['add', '.'], repoDir); | ||
| git(['commit', '-m', 'feat: trailer test'], repoDir); | ||
|
|
||
| serviceWithTrailers.remember('Use Redis for caching', { | ||
| cwd: repoDir, | ||
| type: 'decision', | ||
| confidence: 'high', | ||
| tags: 'cache, infra', | ||
| }); | ||
|
|
||
| const trailers = trailerService.readTrailers('HEAD', repoDir); | ||
| assert.ok(trailers.find(t => t.key === 'AI-Decision' && t.value === 'Use Redis for caching')); | ||
| assert.ok(trailers.find(t => t.key === 'AI-Confidence' && t.value === 'high')); | ||
| assert.ok(trailers.find(t => t.key === 'AI-Tags' && t.value === 'cache, infra')); | ||
| assert.ok(trailers.find(t => t.key === 'AI-Memory-Id')); | ||
| }); | ||
|
|
||
| it('should map memory type to correct trailer key', () => { | ||
| writeFileSync(join(repoDir, 'gotcha-test.txt'), 'gotcha'); | ||
| git(['add', '.'], repoDir); | ||
| git(['commit', '-m', 'fix: gotcha test'], repoDir); | ||
|
|
||
| serviceWithTrailers.remember('Watch out for null tokens', { | ||
| cwd: repoDir, | ||
| type: 'gotcha', | ||
| }); | ||
|
|
||
| const trailers = trailerService.readTrailers('HEAD', repoDir); | ||
| assert.ok(trailers.find(t => t.key === 'AI-Gotcha')); | ||
| assert.ok(!trailers.find(t => t.key === 'AI-Decision')); | ||
| }); | ||
|
|
||
| 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); | ||
| }); | ||
|
Comment on lines
+97
to
+112
|
||
|
|
||
| it('should not fail when trailerService is not injected', () => { | ||
| writeFileSync(join(repoDir, 'no-svc.txt'), 'plain'); | ||
| git(['add', '.'], repoDir); | ||
| git(['commit', '-m', 'feat: no service'], repoDir); | ||
|
|
||
| // service (without trailerService) should not throw | ||
| const memory = service.remember('Plain memory', { cwd: repoDir }); | ||
| assert.ok(memory.id); | ||
| }); | ||
|
|
||
| it('should include AI-Memory-Id linking trailer to notes entry', () => { | ||
| writeFileSync(join(repoDir, 'link-test.txt'), 'link'); | ||
| git(['add', '.'], repoDir); | ||
| git(['commit', '-m', 'feat: link test'], repoDir); | ||
|
|
||
| const memory = serviceWithTrailers.remember('Linked memory', { | ||
| cwd: repoDir, | ||
| type: 'convention', | ||
| }); | ||
|
|
||
| const trailers = trailerService.readTrailers('HEAD', repoDir); | ||
| const memoryId = trailers.find(t => t.key === 'AI-Memory-Id'); | ||
| assert.ok(memoryId); | ||
| assert.equal(memoryId.value, memory.id); | ||
| }); | ||
|
|
||
| it('should not write AI-Tags trailer when tags are empty', () => { | ||
| writeFileSync(join(repoDir, 'no-tags.txt'), 'notags'); | ||
| git(['add', '.'], repoDir); | ||
| git(['commit', '-m', 'feat: no tags test'], repoDir); | ||
|
|
||
| serviceWithTrailers.remember('No tags here', { | ||
| cwd: repoDir, | ||
| type: 'fact', | ||
| }); | ||
|
|
||
| const trailers = trailerService.readTrailers('HEAD', repoDir); | ||
| assert.ok(!trailers.find(t => t.key === 'AI-Tags')); | ||
| }); | ||
| }); | ||
|
|
||
| describe('recall', () => { | ||
| it('should find memories by query', () => { | ||
| const result = service.recall('JWT', { cwd: repoDir }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -41,6 +41,7 @@ describe('createContainer', () => { | |||||||||
| assert.ok(cradle.notesService); | ||||||||||
| assert.ok(cradle.gitClient); | ||||||||||
| assert.ok(cradle.memoryRepository); | ||||||||||
| assert.ok(cradle.trailerService); | ||||||||||
| assert.ok(cradle.eventBus); | ||||||||||
| assert.ok(cradle.triageService); | ||||||||||
| assert.ok(cradle.memoryService); | ||||||||||
|
|
@@ -162,6 +163,24 @@ describe('createContainer', () => { | |||||||||
| }); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| describe('trailerService', () => { | ||||||||||
| it('should resolve with expected interface', () => { | ||||||||||
| const container = createContainer(); | ||||||||||
| const { trailerService } = container.cradle; | ||||||||||
|
|
||||||||||
| assert.equal(typeof trailerService.readTrailers, 'function'); | ||||||||||
| assert.equal(typeof trailerService.formatTrailers, 'function'); | ||||||||||
| assert.equal(typeof trailerService.queryTrailers, 'function'); | ||||||||||
|
||||||||||
| assert.equal(typeof trailerService.queryTrailers, 'function'); | |
| assert.equal(typeof trailerService.queryTrailers, 'function'); | |
| assert.equal(typeof trailerService.addTrailers, 'function'); | |
| assert.equal(typeof trailerService.buildCommitMessage, 'function'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.