Conversation
Add a git post-commit hook that automatically writes session metadata (agent, model, timestamp) as a git note on every AI-assisted commit. Implementation: - Add IGitCommitEvent type to HookEvents.ts - Create PostCommitHandler using existing event-driven architecture - Add post-commit.ts with install/uninstall functions - Register handler in DI container on git:commit event - Update init.ts to install post-commit hook alongside prepare-commit-msg - Add postCommit section to IHookConfig interface Tests: - 24 unit tests for PostCommitHandler and hook install/uninstall - 6 integration tests for full commit workflow Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42 AI-Model: claude-opus-4-5-20251101
MCP integration tests were hanging indefinitely because they tried to spawn node with dist/mcp-server.js which doesn't exist without building. Fix: - Create tests/integration/mcp/helpers.ts with shared mcpSession helper - Use tsx to run src/mcp-server.ts directly from TypeScript source - Update all 5 MCP test files to use the shared helper - Tests now complete in ~8 seconds instead of timing out Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42 AI-Model: claude-opus-4-5-20251101
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR introduces a post-commit git hook feature that writes session metadata to git notes when commits are made. It adds a PostCommitHandler class, a post-commit hook installer with git worktree support, configuration integration, and comprehensive test coverage including helper utilities for MCP testing. Changes
Sequence Diagram(s)sequenceDiagram
participant Git as Git (post-commit hook)
participant Hook as post-commit.ts
participant EventBus as Event Bus
participant Handler as PostCommitHandler
participant Env as Environment
participant NotesService as Notes Service
Git->>Hook: Execute post-commit hook<br/>(commit SHA via stdin)
Hook->>Hook: Parse SHA from stdin
Hook->>Hook: Create IGitCommitEvent<br/>(type: 'git:commit', sha, cwd)
Hook->>EventBus: Emit 'git:commit' event
EventBus->>Handler: handle(event: IGitCommitEvent)
Handler->>Env: Read GIT_MEM_AGENT<br/>GIT_MEM_MODEL<br/>CLAUDECODE
alt Agent Detected
Handler->>NotesService: Read existing note<br/>(commit SHA)
alt Note Exists & Valid
NotesService-->>Handler: Parse existing payload
Handler->>Handler: Merge session into<br/>existing payload
else Note Missing or Malformed
Handler->>Handler: Create fresh payload<br/>with session only
end
Handler->>NotesService: Write updated payload
NotesService-->>Git: Success
else No Agent
Handler->>Handler: Log and exit gracefully
Handler-->>Git: No action
end
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)
⚔️ Resolve merge conflicts (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a post-commit git hook that automatically captures session metadata (AI agent, model, and timestamp) as git notes when AI-assisted commits are detected. It also fixes pre-existing MCP integration test timeout issues by switching from compiled dist files to running TypeScript sources directly via tsx.
Changes:
- Implements post-commit hook infrastructure following the same architectural patterns as prepare-commit-msg (hook installation, wrapping, versioning, and event-driven handling)
- Adds PostCommitHandler that writes session metadata to git notes while preserving existing memories
- Consolidates MCP integration test helpers and switches from
node dist/mcp-server.jstotsx src/mcp-server.tsto eliminate timeout issues
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hooks/post-commit.ts | New hook installation/uninstall logic with user hook wrapping support |
| src/application/handlers/PostCommitHandler.ts | Event handler that writes session metadata to git notes |
| src/application/interfaces/IPostCommitHandler.ts | Interface definition for the post-commit handler |
| src/domain/events/HookEvents.ts | Added IGitCommitEvent type for post-commit events |
| src/domain/interfaces/IHookConfig.ts | Added IPostCommitConfig interface |
| src/hooks/utils/config.ts | Added postCommit config section with defaults |
| src/commands/init.ts | Updated to install/uninstall both hooks |
| src/commands/init-hooks.ts | Added postCommit config to buildGitMemConfig |
| src/commands/hook.ts | Added post-commit event mapping and handler |
| src/infrastructure/di/container.ts | Registered PostCommitHandler on event bus |
| tests/unit/hooks/post-commit.test.ts | Comprehensive unit tests for hook install/uninstall/upgrade |
| tests/unit/application/handlers/PostCommitHandler.test.ts | Unit tests for PostCommitHandler with various scenarios |
| tests/integration/hooks/hook-post-commit.test.ts | Integration tests verifying end-to-end functionality |
| tests/integration/hooks/helpers.ts | Updated to include postCommit in config helper |
| tests/integration/mcp/helpers.ts | New shared MCP test helpers using tsx |
| tests/integration/mcp-*.test.ts | Updated all MCP tests to use shared helpers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| id: i + 2, | ||
| ...requests[i], | ||
| }) + '\n'); | ||
| } |
There was a problem hiding this comment.
The mcpSession function doesn't call proc.stdin.end() after writing all requests, unlike sendMcpRequest (line 75). While the process gets killed anyway when responses are received, not ending stdin could potentially cause buffering issues or prevent the server from detecting end-of-input. Consider adding proc.stdin.end() after the loop on line 164 for consistency and to ensure all input is flushed before waiting for responses.
| } | |
| } | |
| proc.stdin.end(); |
There was a problem hiding this comment.
Fixed - added proc.stdin.end() after the loop for consistency with sendMcpRequest.
| * and send JSON-RPC requests through stdio. | ||
| */ | ||
|
|
||
| import { spawn, execFileSync, ChildProcess } from 'child_process'; |
There was a problem hiding this comment.
The ChildProcess type is imported but never used in this file. Remove it from the import statement to keep the code clean.
| import { spawn, execFileSync, ChildProcess } from 'child_process'; | |
| import { spawn, execFileSync } from 'child_process'; |
There was a problem hiding this comment.
Fixed - removed unused ChildProcess import.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/mcp-server.test.ts (1)
8-31:⚠️ Potential issue | 🟡 MinorIntegration test should create a temp git repo and clean up in after()
This test currently runs without a temp repo or teardown. The integration-test guideline calls for creating a real repo in
os.tmpdir()and cleaning it up inafter().As per coding guidelines: "tests/integration/**/*.test.ts: Write integration tests that create real temporary git repos in os.tmpdir(), run actual git commands, and clean up in after() hooks in tests/integration/."🛠️ Suggested adjustment
-import { describe, it } from 'node:test'; +import { describe, it, before, after } from 'node:test'; import assert from 'node:assert/strict'; -import { sendMcpRequest } from './mcp/helpers'; +import { sendMcpRequest, createTestRepo, cleanupRepo } from './mcp/helpers'; describe('Integration: MCP Server', () => { + let repoDir = ''; + let previousCwd = ''; + + before(() => { + previousCwd = process.cwd(); + const repo = createTestRepo('git-mem-mcp-server-'); + repoDir = repo.dir; + process.chdir(repoDir); + }); + + after(() => { + process.chdir(previousCwd); + cleanupRepo(repoDir); + }); + it('should respond to initialize with server info', async () => { const response = await sendMcpRequest({ jsonrpc: '2.0', id: 1,
🤖 Fix all issues with AI agents
In `@src/application/handlers/PostCommitHandler.ts`:
- Around line 14-15: PostCommitHandler currently imports
resolveAgent/resolveModel from infrastructure; remove those direct imports and
inject a domain-facing abstraction (e.g., IAgentResolver) instead. Define
IAgentResolver in the domain layer with methods matching
resolveAgent/resolveModel, update PostCommitHandler to accept an IAgentResolver
via constructor or DI token and call its methods (replace direct
resolveAgent/resolveModel calls). Implement the concrete resolver in
src/infrastructure (class name like AgentResolver) and bind it to IAgentResolver
in the DI container so PostCommitHandler depends only on the domain interface.
Ensure any tests or callers are updated to provide the IAgentResolver mock or
binding.
In `@src/hooks/post-commit.ts`:
- Around line 145-157: The code may throw if the hook file is removed between
isGitMemHook(hookPath) and unlinkSync(hookPath); update the cleanup in the
function that calls isGitMemHook to perform an existence check or wrap
unlinkSync(hookPath) in a safe conditional/try-catch: first verify
existsSync(hookPath) immediately before calling unlinkSync, or catch and ignore
ENOENT errors from unlinkSync, and still attempt to restore the backup via
existsSync(backupPath) and renameSync(backupPath, hookPath) as currently
written; reference isGitMemHook, unlinkSync, existsSync, backupPath, and
renameSync when making the change.
- Around line 25-34: The hook currently silences all error output by appending
"2>/dev/null" to the git-mem invocation inside the HOOK_SCRIPT string, which
hides actionable failures; update HOOK_SCRIPT to preserve stderr (remove
"2>/dev/null") or redirect stderr to a persistent log file (e.g.,
"2>>/var/log/git-mem-post-commit.log") and/or wire a verbosity flag/env var so
users can opt-in to quiet mode; modify the HOOK_SCRIPT template where the line
echo "{\\"sha\\":\\"$SHA\\"}" | git-mem hook post-commit 2>/dev/null || true is
defined to implement the chosen behavior and ensure failures still don't break
commits by keeping the "|| true" if desired.
In `@tests/integration/hooks/hook-post-commit.test.ts`:
- Around line 50-56: The test's afterEach reassigns process.env which doesn't
remove keys added during tests; update the cleanup to restore the environment
in-place using the captured originalEnv: in afterEach, remove any keys from
process.env that are not present in originalEnv and then assign originalEnv's
values back into process.env (use Object.keys on originalEnv and process.env to
detect additions/removals), referencing the beforeEach/afterEach and originalEnv
symbols so the env is fully restored without reassigning process.env.
- Around line 19-21: Replace the local git() function with the shared helper by
importing the exported git helper where other helpers are imported; remove the
duplicate function definition in this test and update any references to call the
imported git function (symbol: git) so the test uses the single source of truth
from the helpers module (ensure the helpers module exports git before removing
the local implementation).
In `@tests/integration/mcp/helpers.ts`:
- Around line 90-112: The current stdout buffering clears the entire stdout
string (stdout = ''), discarding any partial JSON frames; change the logic in
the proc.stdout 'data' handler to preserve the last partial line: split stdout
by '\n' into lines, pop the final element into a variable (partial =
lines.pop()), parse only the remaining full lines (filtering out empty strings)
and push parsed IMcpResponse objects into responses as before, then set stdout =
partial so any incomplete JSON is retained for the next chunk; keep references
to proc.stdout, stdout, responses, and expectedResponses when locating the
handler to modify.
- Around line 13-17: The TSX_BIN constant is currently resolving the Unix shell
script which fails on Windows when spawn() is called without a shell; update the
TSX_BIN definition to choose the correct binary based on platform (use the .cmd
variant on Windows). Modify the TSX_BIN resolution in
tests/integration/mcp/helpers.ts (the TSX_BIN constant) to check
process.platform === 'win32' and resolve to 'node_modules/.bin/tsx.cmd' on
Windows and 'node_modules/.bin/tsx' otherwise so spawn() can execute the correct
file.
In `@tests/unit/application/handlers/PostCommitHandler.test.ts`:
- Around line 32-41: The createMockLogger mock does not fully implement the
ILogger interface: add missing methods trace and fatal, implement
isLevelEnabled(level: string): boolean, and update child to accept bindings?:
Record<string, unknown> and return the same ILogger shape; also align all log
method parameter names and types to (msg: string, context?: Record<string,
unknown>) instead of data?: unknown so signatures match ILogger and tests use
createMockLogger(), trace, fatal, isLevelEnabled, child, debug, info, warn,
error consistently.
In `@tests/unit/hooks/post-commit.test.ts`:
- Around line 15-184: The tests in this file call real git and filesystem APIs
via the local git helper and Node fs functions (git -> execFileSync,
mkdtempSync, writeFileSync, chmodSync, rmSync, existsSync, readFileSync) and
exercise installPostCommitHook/uninstallPostCommitHook against the real FS;
change these unit tests to mock/stub the external effects instead (or move the
whole file to integration tests). Specifically, replace the git(...) helper
calls (which call execFileSync) with a mocked execFileSync, and stub filesystem
operations used in the tests (mkdtempSync, writeFileSync, chmodSync, rmSync,
existsSync, readFileSync, mkdirSync) so installPostCommitHook and
uninstallPostCommitHook are exercised in-memory; alternatively relocate this
test to tests/integration and keep current real I/O behavior there.
| import { resolveAgent, resolveModel } from '../../infrastructure/detect-agent'; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Decouple application layer from infrastructure utilities.
PostCommitHandler imports resolveAgent/resolveModel from src/infrastructure, which breaks the application→domain-only dependency rule. Please inject an application-friendly abstraction (e.g., IAgentResolver in domain) and bind it in the DI container, or relocate agent detection to a domain/application utility.
As per coding guidelines, "src/{application,infrastructure}/**/*.ts: Implement domain interfaces in src/infrastructure/ and depend on domain layer only from src/application/."
🤖 Prompt for AI Agents
In `@src/application/handlers/PostCommitHandler.ts` around lines 14 - 15,
PostCommitHandler currently imports resolveAgent/resolveModel from
infrastructure; remove those direct imports and inject a domain-facing
abstraction (e.g., IAgentResolver) instead. Define IAgentResolver in the domain
layer with methods matching resolveAgent/resolveModel, update PostCommitHandler
to accept an IAgentResolver via constructor or DI token and call its methods
(replace direct resolveAgent/resolveModel calls). Implement the concrete
resolver in src/infrastructure (class name like AgentResolver) and bind it to
IAgentResolver in the DI container so PostCommitHandler depends only on the
domain interface. Ensure any tests or callers are updated to provide the
IAgentResolver mock or binding.
There was a problem hiding this comment.
Acknowledged. For this PR, keeping the direct import since resolveAgent/resolveModel are pure functions with no external dependencies (just process.env reads). Creating an abstraction layer would add complexity without clear benefit for these simple utilities. Will consider for future refactoring if this pattern spreads.
There was a problem hiding this comment.
@TonyCasey, understood! That's a reasonable pragmatic call for these simple environment readers. If they grow dependencies or need different behavior in tests down the line, the abstraction can be introduced then. Thanks for considering it for future refactoring if the pattern expands. 👍
🧠 Learnings used
Learnt from: TonyCasey
Repo: TonyCasey/git-mem PR: 20
File: src/commands/liberate.ts:17-25
Timestamp: 2026-02-12T13:28:25.372Z
Learning: In src/commands/**/*.ts and src/mcp/tools/**/*.ts: Use the Awilix DI container pattern via createContainer() instead of manual dependency construction. Command and tool handlers should create a container with appropriate options (logger, scope, enrich) and resolve services from container.cradle (e.g., const { memoryService, logger: log } = container.cradle).
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-14T09:43:48.659Z
Learning: Applies to src/{application,infrastructure}/**/*.ts : Implement domain interfaces in src/infrastructure/ and depend on domain layer only from src/application/
Learnt from: TonyCasey
Repo: TonyCasey/git-mem PR: 21
File: src/commands/init-hooks.ts:69-75
Timestamp: 2026-02-12T15:35:31.520Z
Learning: In src/commands/**/*.ts: Commands that require domain services (IMemoryService, IContextService, ILiberateService, etc.) should use the Awilix DI container pattern via createContainer() and resolve services from container.cradle. Simple configuration/initialization commands that only perform file I/O operations (e.g., init-mcp, init-hooks) should accept an optional ILogger parameter directly without DI container overhead.
</learning>
<!-- [remove_learning]
e6547207-68e3-47f5-b1b7-4eedeeeec6ea
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-12T13:30:15.764Z
Learning: Applies to src/commands/**/*.ts : Use manual dependency construction instead of a DI container. Each command/tool handler must construct its dependency chain fresh per invocation: NotesService → MemoryRepository → MemoryService
Learnt from: CR
Repo: TonyCasey/lisa PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-30T20:19:52.042Z
Learning: Applies to src/lib/**/*.{ts,tsx} : Domain layer must have no dependencies on infrastructure or application layers. Application layer depends only on domain interfaces.
Learnt from: TonyCasey
Repo: TonyCasey/git-mem PR: 21
File: src/hooks/session-start.ts:11-13
Timestamp: 2026-02-12T15:34:53.411Z
Learning: In src/**/*.{ts,tsx}: Internal relative imports (e.g., '../infrastructure/di', './utils/stdin') should NOT use `.js` extensions. The project is CommonJS and only requires `.js` extensions when importing external ESM dependencies like modelcontextprotocol/sdk with esModuleInterop: true.
Learnt from: CR
Repo: TonyCasey/lisa PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-30T20:19:52.042Z
Learning: For new features: 1) Define domain interface in `src/lib/domain/`, 2) Create handler in `src/lib/application/handlers/`, 3) Implement infrastructure in `src/lib/infrastructure/`, 4) Wire up DI in `src/lib/services.ts`, 5) Add CLI command if needed, 6) Write tests mirroring source structure.
| const HOOK_SCRIPT = `#!/bin/sh | ||
| ${HOOK_FINGERPRINT} | ||
| # Writes session metadata as a git note when an AI-assisted session is detected. | ||
|
|
||
| # Get the commit SHA | ||
| SHA=$(git rev-parse HEAD) | ||
|
|
||
| # Pipe JSON to git-mem hook handler | ||
| echo "{\\"sha\\":\\"$SHA\\"}" | git-mem hook post-commit 2>/dev/null || true | ||
| `; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Suppressing stderr may hide actionable errors.
The 2>/dev/null in the hook script suppresses all stderr output from git-mem hook post-commit. While this prevents noisy output for users, it could make debugging difficult if there are configuration issues or unexpected failures.
Consider logging to a file or using a verbosity flag, or at minimum documenting this behavior.
🤖 Prompt for AI Agents
In `@src/hooks/post-commit.ts` around lines 25 - 34, The hook currently silences
all error output by appending "2>/dev/null" to the git-mem invocation inside the
HOOK_SCRIPT string, which hides actionable failures; update HOOK_SCRIPT to
preserve stderr (remove "2>/dev/null") or redirect stderr to a persistent log
file (e.g., "2>>/var/log/git-mem-post-commit.log") and/or wire a verbosity
flag/env var so users can opt-in to quiet mode; modify the HOOK_SCRIPT template
where the line echo "{\\"sha\\":\\"$SHA\\"}" | git-mem hook post-commit
2>/dev/null || true is defined to implement the chosen behavior and ensure
failures still don't break commits by keeping the "|| true" if desired.
There was a problem hiding this comment.
Acknowledged. The stderr suppression is intentional to keep commits fast and non-intrusive. For debugging, users can run git-mem hook post-commit manually with GIT_MEM_LOG_LEVEL=debug. Will document this in the README.
There was a problem hiding this comment.
@TonyCasey, thank you for the clarification! That's a reasonable trade-off—keeping the commit flow fast and non-intrusive while providing a debug path via GIT_MEM_LOG_LEVEL=debug for manual troubleshooting. Documenting this in the README will help users who need to investigate issues. Appreciate the thoughtful design decision here!
✏️ Learnings added
Learnt from: TonyCasey
Repo: TonyCasey/git-mem PR: 49
File: src/hooks/post-commit.ts:25-34
Timestamp: 2026-02-14T10:03:02.254Z
Learning: In src/hooks/post-commit.ts, the post-commit hook script intentionally suppresses stderr with `2>/dev/null` to keep commits fast and non-intrusive; users can debug by running `git-mem hook post-commit` manually with `GIT_MEM_LOG_LEVEL=debug`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-14T09:43:48.659Z
Learning: Applies to .git-mem.json : Use .git-mem.json at project root for hook configuration, which is loaded by src/hooks/utils/config.ts and must never throw
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-14T09:43:48.659Z
Learning: Run `npm run pre-commit` before committing, which runs type-check and lint
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-14T09:43:48.659Z
Learning: Applies to tests/integration/**/*.test.ts : Write integration tests that create real temporary git repos in os.tmpdir(), run actual git commands, and clean up in after() hooks in tests/integration/
Learnt from: TonyCasey
Repo: TonyCasey/git-mem PR: 22
File: src/hooks/utils/config.ts:30-50
Timestamp: 2026-02-12T17:16:42.308Z
Learning: In src/hooks/utils/config.ts, the .git-mem.json config loader intentionally avoids per-field type validation because the config is internal to hook entry points and already falls back to defaults on parse failure; adding type guards for individual fields would add complexity without meaningful safety improvement.
Learnt from: CR
Repo: TonyCasey/lisa PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-06T11:57:30.539Z
Learning: Applies to src/project/.claude/hooks/**/*.ts : Hooks must be written in TypeScript in src/project/.claude/hooks/ and compiled to .claude/hooks/ via npm run build
- Remove unused ChildProcess import - Add proc.stdin.end() after writing requests for consistency Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42 AI-Model: claude-opus-4-5-20251101
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/integration/mcp/helpers.ts`:
- Around line 32-34: The spawned server process (created with spawn(TSX_BIN,
[SERVER_PATH]) and assigned to proc) pipes stderr but never reads it, so server
error output is lost on test failures; update the test helper to listen to
proc.stderr 'data' events (collect into a buffer/string) and include that
collected stderr text in any rejection/error messages or logs (e.g., when the
helper rejects/startup fails) so failures surface server errors for easier
debugging while keeping stdio as ['pipe','pipe','pipe'].
| const proc = spawn(TSX_BIN, [SERVER_PATH], { | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider capturing stderr for easier debugging.
stderr is piped but never read. When tests fail, any server error messages will be lost, making debugging harder.
💡 Suggested enhancement
const proc = spawn(TSX_BIN, [SERVER_PATH], {
stdio: ['pipe', 'pipe', 'pipe'],
});
let stdout = '';
+ let stderr = '';
let resolved = false;
+
+ proc.stderr.on('data', (data: Buffer) => {
+ stderr += data.toString();
+ });Then include stderr in the rejection message if available:
const timeout = setTimeout(() => {
if (!resolved) {
resolved = true;
proc.kill();
- reject(new Error('MCP server did not respond within 5 seconds'));
+ reject(new Error(`MCP server did not respond within 5 seconds${stderr ? `\nstderr: ${stderr}` : ''}`));
}
}, 5000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const proc = spawn(TSX_BIN, [SERVER_PATH], { | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| }); | |
| const proc = spawn(TSX_BIN, [SERVER_PATH], { | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| }); | |
| let stdout = ''; | |
| let stderr = ''; | |
| let resolved = false; | |
| proc.stderr.on('data', (data: Buffer) => { | |
| stderr += data.toString(); | |
| }); |
🤖 Prompt for AI Agents
In `@tests/integration/mcp/helpers.ts` around lines 32 - 34, The spawned server
process (created with spawn(TSX_BIN, [SERVER_PATH]) and assigned to proc) pipes
stderr but never reads it, so server error output is lost on test failures;
update the test helper to listen to proc.stderr 'data' events (collect into a
buffer/string) and include that collected stderr text in any rejection/error
messages or logs (e.g., when the helper rejects/startup fails) so failures
surface server errors for easier debugging while keeping stdio as
['pipe','pipe','pipe'].
There was a problem hiding this comment.
Good suggestion for debugging. Will add stderr capture as a follow-up improvement - the tests are working fine without it currently.
There was a problem hiding this comment.
@TonyCasey, sounds good! Since this is a nice-to-have enhancement for debugging rather than a critical issue, deferring it makes sense.
Would you like me to open a GitHub issue to track adding stderr capture to the MCP test helpers as a follow-up improvement?
🧠 Learnings used
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: tests/integration/n8n/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:35:18.080Z
Learning: Applies to tests/integration/n8n/src/n8n/workflows/typescript/mcp-test/**/*.ts : Use TypeScript with `npx tsc` to compile to JavaScript before running test files in MCP test suite
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: apps/n8n-workflows/mcp-test/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:34:55.341Z
Learning: Applies to apps/n8n-workflows/mcp-test/**/test-runner.ts : Store test results in unified JSON format in src/n8n/workflows/typescript/mcp-test/logs/unified/ directory with separate directories for HTTP requests, responses, and n8n executions
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: tests/integration/n8n/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:35:18.080Z
Learning: Applies to tests/integration/n8n/src/n8n/workflows/typescript/mcp-test/**/*test*.ts : Store test result logs in unified format at `src/n8n/workflows/typescript/mcp-test/logs/unified/unified_log_*.json`
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-14T09:43:48.659Z
Learning: Applies to tests/integration/**/*.test.ts : Write integration tests that create real temporary git repos in os.tmpdir(), run actual git commands, and clean up in after() hooks in tests/integration/
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: apps/n8n-workflows/mcp/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:35:06.992Z
Learning: Applies to apps/n8n-workflows/mcp/**/mcp-test/**/*.{ts,js} : Always use the mcp-test TypeScript testing framework for testing services, not manual testing
Learnt from: CR
Repo: TonyCasey/lisa PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-30T20:19:52.042Z
Learning: Applies to tests/integration/**/*.{test,spec}.{ts,tsx} : Integration tests require environment setup: Local Docker for MCP/Neo4j or Zep Cloud API credentials. Set RUN_*_INTEGRATION_TESTS=1 environment variable to enable.
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: tests/integration/n8n/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:35:18.080Z
Learning: Applies to tests/integration/n8n/src/configs/workflows.ts : Configure MCP workflows to send events to `http://localhost:3003/emit-event` so SSE listener captures events
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: tests/integration/n8n/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:35:18.080Z
Learning: Applies to tests/integration/n8n/src/configs/workflows.ts : Add workflow configuration to `src/configs/workflows.ts` when testing new MCP workflows
Learnt from: TonyCasey
Repo: TonyCasey/lisa PR: 166
File: tests/unit/src/lib/infrastructure/services/PreferenceStore.test.ts:15-285
Timestamp: 2026-02-01T13:40:07.340Z
Learning: In tests/unit/**/PreferenceStore.test.ts and similar file-based infrastructure service tests: File-based services (PreferenceStore, SessionCaptureService, CronService, etc.) use real filesystem operations with unique temp directories (fs.mkdtempSync) and cleanup in afterEach. This is the established pattern in the codebase for testing services whose purpose is file I/O. Tests remain fast (~80ms), isolated, and deterministic.
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-09T09:34:42.025Z
Learning: Use Clean SSE Testing Framework located at tests/integration/n8n/ for testing all MCP workflows with real-time event streaming
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: tests/integration/n8n/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:35:18.080Z
Learning: Applies to tests/integration/n8n/src/n8n/workflows/typescript/mcp-test/**/sse-listener.ts : Create pretty-formatted SSE event logs in JSON format at `/logs/sse/sse-events-YYYY-MM-DD.log` with structured event data including timestamp, event type, source, session ID, and payload
Learnt from: TonyCasey
Repo: TonyCasey/lisa PR: 27
File: tests/unit/src/lib/infrastructure/services/MemoryService.timeout.test.ts:136-223
Timestamp: 2026-01-23T15:19:20.556Z
Learning: In tests/unit/**/MemoryService.timeout.test.ts and similar timeout tests: TonyCasey prefers using real timers with conservative thresholds (e.g., 500ms for operations expected to complete in <50ms) to test actual timeout behavior, rather than fake timers. This approach is intentional to validate production-like conditions, with a plan to adjust if CI flakiness occurs.
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: apps/n8n-workflows/mcp-test/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:34:55.341Z
Learning: Applies to apps/n8n-workflows/mcp-test/**/sse-listener.ts : SSE listeners must emit events to http://localhost:3003/emit-event endpoint to ensure events are captured by the test suite
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: apps/n8n-workflows/mcp/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:35:06.992Z
Learning: Applies to apps/n8n-workflows/mcp/**/mcp-test/**/*.{ts,js} : Test all AI model connections before deployment to verify OpenAI and Claude integration
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: apps/n8n-workflows/mcp-test/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:34:55.341Z
Learning: Start SSE Listener/Client first by compiling TypeScript and running node dist/sse-listener.js on port 3003 before testing MCP workflows
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: apps/n8n-workflows/mcp-test/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:34:55.341Z
Learning: Applies to apps/n8n-workflows/mcp-test/**/workflows.ts : Add workflow configuration to src/configs/workflows.ts when testing new MCP workflows
Learnt from: TonyCasey
Repo: TonyCasey/git-mem PR: 20
File: src/commands/liberate.ts:17-25
Timestamp: 2026-02-12T13:28:25.372Z
Learning: In src/commands/**/*.ts and src/mcp/tools/**/*.ts: Use the Awilix DI container pattern via createContainer() instead of manual dependency construction. Command and tool handlers should create a container with appropriate options (logger, scope, enrich) and resolve services from container.cradle (e.g., const { memoryService, logger: log } = container.cradle).
Learnt from: CR
Repo: TonyCasey/agenisys PR: 0
File: apps/n8n-workflows/mcp/CLAUDE.md:0-0
Timestamp: 2025-12-09T09:35:06.992Z
Learning: Applies to apps/n8n-workflows/mcp/**/mcp-test/**/*.{ts,js} : Validate AI-generated content using AI content indicators such as 'strategy', 'analysis', 'recommendations', 'optimization'
Learnt from: TonyCasey
Repo: TonyCasey/git-mem PR: 49
File: src/hooks/post-commit.ts:25-34
Timestamp: 2026-02-14T10:03:02.254Z
Learning: In src/hooks/post-commit.ts, the post-commit hook script intentionally suppresses stderr with `2>/dev/null` to keep commits fast and non-intrusive; users can debug by running `git-mem hook post-commit` manually with `GIT_MEM_LOG_LEVEL=debug`.
- Fix mock logger to implement full ILogger interface (trace, fatal, isLevelEnabled) - Fix MCP helpers stdout buffering to preserve partial JSON frames - Add Windows compatibility for tsx binary path - Fix env restoration in integration tests (properly remove added keys) - Export git helper from hooks/helpers and reuse in tests - Add TOCTOU protection to unlinkSync in post-commit uninstall - Move post-commit hook tests from unit to integration (uses real fs) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42 AI-Model: claude-opus-4-5-20251101
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private readPayload(sha: string, cwd: string): INotePayload { | ||
| const raw = this.notesService.read(sha, undefined, cwd); | ||
| if (!raw) return {}; | ||
|
|
||
| try { | ||
| const parsed = JSON.parse(raw); | ||
| if (typeof parsed === 'object' && parsed !== null) { | ||
| return parsed as INotePayload; | ||
| } | ||
| return {}; | ||
| } catch { | ||
| // Malformed JSON — return empty and let write overwrite | ||
| this.logger?.warn('Malformed note JSON, will overwrite', { sha }); | ||
| return {}; |
There was a problem hiding this comment.
PostCommitHandler.readPayload assumes the existing git note JSON is an object shaped like {memories?: ...}. However, MemoryRepository explicitly supports legacy note formats where the note is a bare array (memories[]) or a single memory object. In those cases this handler will either (a) fail to persist the new session field because JSON.stringify on an array drops custom properties, or (b) corrupt the note by turning the single memory object into a payload that MemoryRepository later treats as a memory (including the session field). Consider normalizing parsed notes the same way as MemoryRepository.readPayload (wrap arrays/single objects into a { memories: [...] } payload) before adding/updating session, so session metadata is reliably written without breaking existing memory notes.
| const proc = spawn(TSX_BIN, [SERVER_PATH], { | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); |
There was a problem hiding this comment.
sendMcpRequest() spawns the MCP server without setting cwd or env. Since src/mcp-server.ts logs an "MCP server running" info message and the default logger writes to .git-mem/logs under the git root (when LOG_FILE is not 'false'), this test helper can create log files in the repository root during integration tests. Consider spawning with a temp cwd (or at least passing env overrides like LOG_FILE=false / LOG_CONSOLE=false) to avoid leaking artifacts outside the test repo and to keep test runs hermetic.
| @@ -0,0 +1,241 @@ | |||
| /** | |||
| * post-commit hook — unit tests | |||
There was a problem hiding this comment.
File header says "post-commit hook — unit tests", but these tests exercise install/uninstall against real temp git repos (integration-style). Consider renaming the header comment (or describe blocks) to avoid confusion about the test level/purpose.
| * post-commit hook — unit tests | |
| * post-commit hook — integration tests |
| /** Map CLI event names to internal event bus types. */ | ||
| export const EVENT_MAP: Record<string, HookEventType> = { | ||
| 'session-start': 'session:start', | ||
| 'session-stop': 'session:stop', | ||
| 'prompt-submit': 'prompt:submit', | ||
| 'post-commit': 'git:commit', | ||
| }; |
There was a problem hiding this comment.
The top-of-file usage comment for the hook command still lists only session-start/session-stop/prompt-submit, but this PR adds the 'post-commit' event to EVENT_MAP. Consider updating the usage/examples in the header comment to include git-mem hook post-commit so docs stay in sync with supported events.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/hooks/post-commit.ts`:
- Around line 86-133: The upgrade branch inside installPostCommitHook currently
overwrites an existing wrapper and drops the user's backup; change the
isGitMemHook && !isCurrentVersion branch to detect if backupPath exists and, if
so, reconstruct the wrapper (use the same wrapperScript logic currently used
when wrapping: prepend the BACKUP_HOOK lines to HOOK_SCRIPT), write that wrapper
to hookPath, set wrapped = true, chmod it and return { installed: true, wrapped:
true, hookPath }; otherwise keep the existing behavior of writing HOOK_SCRIPT
and returning wrapped: false. Ensure you still call chmodSync(hookPath, 0o755)
before returning.
In `@tests/integration/hooks/post-commit-install.test.ts`:
- Around line 1-17: The file header comment incorrectly calls these "unit tests"
even though they operate on real git repos and filesystem state; update the
top-of-file comment block to say "integration tests" (or "integration tests —
operate on real git repositories and filesystem state") and adjust any short
description lines accordingly so it accurately reflects integration scope;
locate the header comment at the top of
tests/integration/hooks/post-commit-install.test.ts (the block above the
imports) and replace "unit tests" with the corrected wording.
In `@tests/integration/mcp/helpers.ts`:
- Around line 174-177: The git helper and the similar createTestRepo and
cleanupRepo functions are duplicated across test files; extract them into a
single shared test-utility module (e.g., a shared git-helpers module), move the
implementations of git, createTestRepo, and cleanupRepo into that module, export
them, and update any files that currently define duplicates to import these
functions instead; ensure signatures for git(args: string[], cwd: string):
string and the create/cleanup functions remain unchanged and update any relative
imports in tests to use the new shared module.
| export function installPostCommitHook(cwd?: string): IHookInstallResult { | ||
| const gitDir = findGitDir(cwd || process.cwd()); | ||
| const hooksDir = join(gitDir, 'hooks'); | ||
| const hookPath = join(hooksDir, 'post-commit'); | ||
| const backupPath = join(hooksDir, 'post-commit.user-backup'); | ||
|
|
||
| // Already installed and up-to-date — idempotent | ||
| if (isGitMemHook(hookPath) && isCurrentVersion(hookPath)) { | ||
| return { installed: false, wrapped: false, hookPath }; | ||
| } | ||
|
|
||
| // Outdated git-mem hook — upgrade in-place | ||
| if (isGitMemHook(hookPath) && !isCurrentVersion(hookPath)) { | ||
| writeFileSync(hookPath, HOOK_SCRIPT); | ||
| chmodSync(hookPath, 0o755); | ||
| return { installed: true, wrapped: false, hookPath }; | ||
| } | ||
|
|
||
| let wrapped = false; | ||
|
|
||
| // Existing non-git-mem hook — wrap it | ||
| if (existsSync(hookPath)) { | ||
| // Guard: don't overwrite an existing backup from a failed previous install | ||
| if (existsSync(backupPath)) { | ||
| throw new Error( | ||
| `Backup hook already exists at ${backupPath}. ` + | ||
| 'Remove it manually or run --uninstall-hooks first.', | ||
| ); | ||
| } | ||
| renameSync(hookPath, backupPath); | ||
| wrapped = true; | ||
|
|
||
| // Create a wrapper that calls the user's hook first, then ours. | ||
| // Uses a path relative to the hook's directory so it survives repo moves. | ||
| const wrapperScript = HOOK_SCRIPT.replace( | ||
| '#!/bin/sh', | ||
| '#!/bin/sh\n# Wrapped existing hook — original saved as post-commit.user-backup\n' + | ||
| 'BACKUP_HOOK="$(dirname "$0")/post-commit.user-backup"\n' + | ||
| 'if [ -x "$BACKUP_HOOK" ]; then\n "$BACKUP_HOOK" "$@" || exit $?\nfi', | ||
| ); | ||
| writeFileSync(hookPath, wrapperScript); | ||
| } else { | ||
| writeFileSync(hookPath, HOOK_SCRIPT); | ||
| } | ||
|
|
||
| chmodSync(hookPath, 0o755); | ||
| return { installed: true, wrapped, hookPath }; | ||
| } |
There was a problem hiding this comment.
Upgrade path should preserve wrapped user hooks.
If a hook was wrapped (backup exists), the upgrade branch overwrites the wrapper with the plain hook, so the user hook never runs. Consider rebuilding the wrapper when a backup is present and returning wrapped: true.
🐛 Suggested fix
// Outdated git-mem hook — upgrade in-place
if (isGitMemHook(hookPath) && !isCurrentVersion(hookPath)) {
- writeFileSync(hookPath, HOOK_SCRIPT);
- chmodSync(hookPath, 0o755);
- return { installed: true, wrapped: false, hookPath };
+ const hasBackup = existsSync(backupPath);
+ const script = hasBackup
+ ? HOOK_SCRIPT.replace(
+ '#!/bin/sh',
+ '#!/bin/sh\n# Wrapped existing hook — original saved as post-commit.user-backup\n' +
+ 'BACKUP_HOOK="$(dirname "$0")/post-commit.user-backup"\n' +
+ 'if [ -x "$BACKUP_HOOK" ]; then\n "$BACKUP_HOOK" "$@" || exit $?\nfi',
+ )
+ : HOOK_SCRIPT;
+ writeFileSync(hookPath, script);
+ chmodSync(hookPath, 0o755);
+ return { installed: true, wrapped: hasBackup, hookPath };
}🤖 Prompt for AI Agents
In `@src/hooks/post-commit.ts` around lines 86 - 133, The upgrade branch inside
installPostCommitHook currently overwrites an existing wrapper and drops the
user's backup; change the isGitMemHook && !isCurrentVersion branch to detect if
backupPath exists and, if so, reconstruct the wrapper (use the same
wrapperScript logic currently used when wrapping: prepend the BACKUP_HOOK lines
to HOOK_SCRIPT), write that wrapper to hookPath, set wrapped = true, chmod it
and return { installed: true, wrapped: true, hookPath }; otherwise keep the
existing behavior of writing HOOK_SCRIPT and returning wrapped: false. Ensure
you still call chmodSync(hookPath, 0o755) before returning.
| /** | ||
| * post-commit hook — unit tests | ||
| * | ||
| * Tests installPostCommitHook / uninstallPostCommitHook against real temp git repos. | ||
| */ | ||
|
|
||
| import { describe, it, before, after } from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { execFileSync } from 'child_process'; | ||
| import { mkdtempSync, writeFileSync, readFileSync, existsSync, chmodSync, rmSync, mkdirSync } from 'fs'; | ||
| import { join } from 'path'; | ||
| import { tmpdir } from 'os'; | ||
| import { installPostCommitHook, uninstallPostCommitHook } from '../../../src/hooks/post-commit'; | ||
|
|
||
| function git(args: string[], cwd: string): string { | ||
| return execFileSync('git', args, { encoding: 'utf8', cwd }).trim(); | ||
| } |
There was a problem hiding this comment.
Header comment should reflect integration scope.
These tests operate on real git repos and filesystem state, so calling them “unit tests” is misleading.
✏️ Suggested fix
- * post-commit hook — unit tests
+ * post-commit hook — integration tests📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * post-commit hook — unit tests | |
| * | |
| * Tests installPostCommitHook / uninstallPostCommitHook against real temp git repos. | |
| */ | |
| import { describe, it, before, after } from 'node:test'; | |
| import assert from 'node:assert/strict'; | |
| import { execFileSync } from 'child_process'; | |
| import { mkdtempSync, writeFileSync, readFileSync, existsSync, chmodSync, rmSync, mkdirSync } from 'fs'; | |
| import { join } from 'path'; | |
| import { tmpdir } from 'os'; | |
| import { installPostCommitHook, uninstallPostCommitHook } from '../../../src/hooks/post-commit'; | |
| function git(args: string[], cwd: string): string { | |
| return execFileSync('git', args, { encoding: 'utf8', cwd }).trim(); | |
| } | |
| /** | |
| * post-commit hook — integration tests | |
| * | |
| * Tests installPostCommitHook / uninstallPostCommitHook against real temp git repos. | |
| */ | |
| import { describe, it, before, after } from 'node:test'; | |
| import assert from 'node:assert/strict'; | |
| import { execFileSync } from 'child_process'; | |
| import { mkdtempSync, writeFileSync, readFileSync, existsSync, chmodSync, rmSync, mkdirSync } from 'fs'; | |
| import { join } from 'path'; | |
| import { tmpdir } from 'os'; | |
| import { installPostCommitHook, uninstallPostCommitHook } from '../../../src/hooks/post-commit'; | |
| function git(args: string[], cwd: string): string { | |
| return execFileSync('git', args, { encoding: 'utf8', cwd }).trim(); | |
| } |
🤖 Prompt for AI Agents
In `@tests/integration/hooks/post-commit-install.test.ts` around lines 1 - 17, The
file header comment incorrectly calls these "unit tests" even though they
operate on real git repos and filesystem state; update the top-of-file comment
block to say "integration tests" (or "integration tests — operate on real git
repositories and filesystem state") and adjust any short description lines
accordingly so it accurately reflects integration scope; locate the header
comment at the top of tests/integration/hooks/post-commit-install.test.ts (the
block above the imports) and replace "unit tests" with the corrected wording.
| /** Helper to run git commands. */ | ||
| export function git(args: string[], cwd: string): string { | ||
| return execFileSync('git', args, { encoding: 'utf8', cwd }).trim(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared test utilities.
This git helper is nearly identical to the one in test/integration/hooks/helpers.ts. The same applies to createTestRepo and cleanupRepo below. Consider consolidating these into a shared test utility module (e.g., tests/integration/shared/git-helpers.ts) to reduce duplication.
🤖 Prompt for AI Agents
In `@tests/integration/mcp/helpers.ts` around lines 174 - 177, The git helper and
the similar createTestRepo and cleanupRepo functions are duplicated across test
files; extract them into a single shared test-utility module (e.g., a shared
git-helpers module), move the implementations of git, createTestRepo, and
cleanupRepo into that module, export them, and update any files that currently
define duplicates to import these functions instead; ensure signatures for
git(args: string[], cwd: string): string and the create/cleanup functions remain
unchanged and update any relative imports in tests to use the new shared module.
Summary
Changes
Post-commit hook implementation:
IGitCommitEventtype to HookEvents.tsPostCommitHandlerusing existing event-driven architecturepost-commit.tswith install/uninstall functions (same pattern as prepare-commit-msg)git:commiteventinit.tsto install post-commit hook alongside prepare-commit-msgpostCommitsection to IHookConfig interfaceMCP test fixes:
tests/integration/mcp/helpers.tswith sharedmcpSessionhelpersrc/mcp-server.tsdirectly from TypeScript sourceTest plan
git mem init --hooksinstalls both prepare-commit-msg and post-commit hooks🤖 Generated with Claude Code
Summary by CodeRabbit
New Features