Conversation
Logs now write to <project>/.git-mem/logs/ instead of ~/.git-mem/logs/ so they are relevant and easy to access per project. 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. |
📝 WalkthroughWalkthroughReplaced home-dir-based default log directory with a Git-root-based lookup: Changes
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 PR changes git-mem’s default file logging directory from a global location under the user’s home directory to a project-local .git-mem/logs directory, while keeping LOG_DIR as an override.
Changes:
- Updated
defaultLogDir()to use a project-local base directory. - Removed the now-unused
osimport.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function defaultLogDir(): string { | ||
| return path.join(os.homedir(), '.git-mem', 'logs'); | ||
| return path.join(process.cwd(), '.git-mem', 'logs'); | ||
| } |
There was a problem hiding this comment.
defaultLogDir() now uses process.cwd(), which is not necessarily the project/repo root (e.g., running git-mem from a subdirectory or launching the MCP server from a different working directory will create .git-mem/logs in that directory). If the intent is truly <project>/.git-mem/logs, consider resolving the git top-level directory (e.g., via git rev-parse --show-toplevel) and using that as the base, with a fallback to process.cwd() when not in a git repo.
There was a problem hiding this comment.
Good catch. Updated to use git rev-parse --show-toplevel with fallback to process.cwd() when not in a git repo.
| export function defaultLogDir(): string { | ||
| return path.join(os.homedir(), '.git-mem', 'logs'); | ||
| return path.join(process.cwd(), '.git-mem', 'logs'); | ||
| } |
There was a problem hiding this comment.
There are unit tests for the logging factory, but none assert the actual path returned by defaultLogDir(). Since this PR changes the default log location, add an assertion that defaultLogDir() matches the expected base directory (whatever the intended definition of "project" is) to prevent regressions.
There was a problem hiding this comment.
Added a unit test in defaultLogDir describe block that asserts the path matches join(gitRoot, '.git-mem', 'logs').
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/infrastructure/logging/factory.ts`:
- Around line 12-14: The logger factory uses process.cwd() directly causing
incorrect log directory when operating on external repos; update defaultLogDir
to accept an optional cwd parameter (defaulting to process.cwd()) and thread
that cwd through createLogger by changing its signature to accept options
including cwd and call loadLoggerOptions(options?.cwd) so the computed basePath
uses the provided cwd; ensure LOG_DIR env var remains a full-path override and
adjust any callers (e.g., where createLogger is invoked from the DI container)
to pass options?.cwd through.
| export function defaultLogDir(): string { | ||
| return path.join(os.homedir(), '.git-mem', 'logs'); | ||
| return path.join(process.cwd(), '.git-mem', 'logs'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing git root detection patterns
echo "=== Checking for git rev-parse usage ==="
rg -n "rev-parse|show-toplevel" --type=ts
echo ""
echo "=== Checking how cwd is threaded through services ==="
rg -nC3 "cwd[?:]\s*string" --type=ts -g '!node_modules'
echo ""
echo "=== Checking if logger creation passes logDir explicitly ==="
rg -nC2 "createLogger|logDir" --type=ts -g '!node_modules' -g '!*.test.ts'
echo ""
echo "=== Checking defaultLogDir usage ==="
rg -nC3 "defaultLogDir" --type=ts -g '!node_modules'
echo ""
echo "=== Checking for --cwd or similar CLI flags ==="
rg -nC2 "cwd|--cwd|process\.chdir" --type=ts -g '!node_modules' -g '!*.test.ts'Repository: TonyCasey/git-mem
Length of output: 50373
🏁 Script executed:
#!/bin/bash
echo "=== Check container initialization and cwd usage ==="
head -80 src/infrastructure/di/container.ts
echo ""
echo "=== Check if createLogger is called with custom logDir anywhere ==="
rg -n "createLogger\(" --type=ts -g '!node_modules' -A2
echo ""
echo "=== Check ILoggerOptions and how logDir is set ==="
rg -n "ILoggerOptions\|logDir" src/infrastructure/logging/factory.ts -A2 -B2Repository: TonyCasey/git-mem
Length of output: 4768
Thread cwd through logger factory to align with service pattern.
The logging system relies on process.cwd(), which may not match the target repository when operating on external repos or from subdirectories. Throughout the codebase, all services (GitClient, NotesService, MemoryRepository, etc.) accept an optional cwd parameter for flexibility. However, the logger is created without considering cwd, breaking this established pattern.
In src/infrastructure/di/container.ts (line 51), the container has access to options?.cwd but doesn't pass it when calling createLogger(). Modify defaultLogDir() to accept an optional cwd parameter and update createLogger to thread it through:
export function defaultLogDir(cwd?: string): string {
const basePath = cwd ?? process.cwd();
return path.join(basePath, '.git-mem', 'logs');
}
export function createLogger(options?: Partial<ILoggerOptions> & { cwd?: string }): ILogger {
const defaults = loadLoggerOptions(options?.cwd);
// ... merge and return
}The LOG_DIR env var still serves as a complete override.
🤖 Prompt for AI Agents
In `@src/infrastructure/logging/factory.ts` around lines 12 - 14, The logger
factory uses process.cwd() directly causing incorrect log directory when
operating on external repos; update defaultLogDir to accept an optional cwd
parameter (defaulting to process.cwd()) and thread that cwd through createLogger
by changing its signature to accept options including cwd and call
loadLoggerOptions(options?.cwd) so the computed basePath uses the provided cwd;
ensure LOG_DIR env var remains a full-path override and adjust any callers
(e.g., where createLogger is invoked from the DI container) to pass options?.cwd
through.
There was a problem hiding this comment.
The updated implementation uses git rev-parse --show-toplevel which already resolves to the repo root regardless of subdirectory. Threading cwd through the logger factory would be over-engineering at this point — the git root detection handles the concern raised.
There was a problem hiding this comment.
@TonyCasey, you're absolutely right — the git rev-parse --show-toplevel approach handles the subdirectory case elegantly and covers the primary concern. The fallback to process.cwd() provides a sensible default when not in a git repo.
I agree that threading cwd explicitly through the logger factory would add complexity that isn't needed for the current use cases. If the need arises to operate on external repositories from a different working directory, it can be addressed then.
Thank you for the clarification!
🧠 Learnings used
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-12T14:53:35.920Z
Learning: Applies to src/infrastructure/**/*.{ts,tsx} : Use ASCII separators `\x1e` (record) and `\x1f` (field) for git log parsing instead of null bytes
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-12T14:53:35.920Z
Learning: Applies to tests/integration/**/*.{ts,tsx} : Create integration tests in `tests/integration/` that instantiate real services against temporary git repositories in `os.tmpdir()` and clean up in `after()` hooks
Learnt from: CR
Repo: TonyCasey/git-mem PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-12T14:53:35.920Z
Learning: Applies to src/application/**/*.{ts,tsx} : Thread `cwd` parameter through all service calls when operating on repositories other than the current working directory
Learnt from: TonyCasey
Repo: TonyCasey/git-mem PR: 21
File: src/commands/init-hooks.ts:69-75
Timestamp: 2026-02-12T15:35:22.928Z
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: TonyCasey
Repo: TonyCasey/git-mem PR: 20
File: src/commands/liberate.ts:17-25
Timestamp: 2026-02-12T13:28:18.656Z
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-12T14:53:35.920Z
Learning: Applies to src/infrastructure/**/*.{ts,tsx} : Organize infrastructure layer in `src/infrastructure/` to implement domain interfaces, including GitClient, NotesService, TrailerService, MemoryRepository, and HeuristicPatterns
Use git rev-parse --show-toplevel to resolve the project root instead of process.cwd(), with fallback to cwd when not in a git repo. Added unit test asserting defaultLogDir() returns .git-mem/logs under git root. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/infrastructure/logging/factory.ts`:
- Around line 13-26: getGitRoot() can hang if the spawned git process stalls;
modify getGitRoot to call execFileSync('git', ['rev-parse','--show-toplevel'], {
encoding: 'utf8', timeout: 2000, stdio: ['ignore','pipe','ignore'] }) (or
equivalent) so the call times out after 2 seconds and avoids unnecessary stdio
piping; keep defaultLogDir() unchanged but rely on updated getGitRoot() to
return null on timeout/exec failure.
In `@tests/unit/infrastructure/logging/factory.test.ts`:
- Around line 32-39: The test calls the real git binary via execFileSync which
makes it an integration test; instead stub the git-root lookup so the unit test
is isolated. In the test for defaultLogDir, replace the direct execFileSync call
by mocking child_process.execFileSync (or the helper that wraps it) to return a
fixed gitRoot string, then assert defaultLogDir() equals join(mockGitRoot,
'.git-mem', 'logs'); ensure you restore the stub after the test. Target the
defaultLogDir() call and the execFileSync symbol when implementing the mock.
| function getGitRoot(): string | null { | ||
| try { | ||
| return execFileSync('git', ['rev-parse', '--show-toplevel'], { | ||
| encoding: 'utf8', | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }).trim(); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| export function defaultLogDir(): string { | ||
| return path.join(os.homedir(), '.git-mem', 'logs'); | ||
| const base = getGitRoot() ?? process.cwd(); | ||
| return path.join(base, '.git-mem', 'logs'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read it
cat -n src/infrastructure/logging/factory.ts | head -30Repository: TonyCasey/git-mem
Length of output: 1217
🏁 Script executed:
# Check package.json for Node.js version constraints
cat package.json | grep -A 5 '"engines"'Repository: TonyCasey/git-mem
Length of output: 132
🏁 Script executed:
# Search for other execFileSync usage to understand patterns
rg -n "execFileSync" --type ts -B 2 -A 5Repository: TonyCasey/git-mem
Length of output: 43814
Add timeout to prevent CLI startup hangs if git stalls.
Since getGitRoot() runs during logger initialization, an unresponsive git command would block CLI startup indefinitely. Add a 2-second timeout and optimize stdio to avoid unnecessary stream management.
🔧 Suggested fix
function getGitRoot(): string | null {
try {
return execFileSync('git', ['rev-parse', '--show-toplevel'], {
encoding: 'utf8',
- stdio: ['pipe', 'pipe', 'pipe'],
+ stdio: ['ignore', 'pipe', 'ignore'],
+ timeout: 2000,
}).trim();
} catch {
return null;
}
}📝 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.
| function getGitRoot(): string | null { | |
| try { | |
| return execFileSync('git', ['rev-parse', '--show-toplevel'], { | |
| encoding: 'utf8', | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| }).trim(); | |
| } catch { | |
| return null; | |
| } | |
| } | |
| export function defaultLogDir(): string { | |
| return path.join(os.homedir(), '.git-mem', 'logs'); | |
| const base = getGitRoot() ?? process.cwd(); | |
| return path.join(base, '.git-mem', 'logs'); | |
| function getGitRoot(): string | null { | |
| try { | |
| return execFileSync('git', ['rev-parse', '--show-toplevel'], { | |
| encoding: 'utf8', | |
| stdio: ['ignore', 'pipe', 'ignore'], | |
| timeout: 2000, | |
| }).trim(); | |
| } catch { | |
| return null; | |
| } | |
| } | |
| export function defaultLogDir(): string { | |
| const base = getGitRoot() ?? process.cwd(); | |
| return path.join(base, '.git-mem', 'logs'); | |
| } |
🤖 Prompt for AI Agents
In `@src/infrastructure/logging/factory.ts` around lines 13 - 26, getGitRoot() can
hang if the spawned git process stalls; modify getGitRoot to call
execFileSync('git', ['rev-parse','--show-toplevel'], { encoding: 'utf8',
timeout: 2000, stdio: ['ignore','pipe','ignore'] }) (or equivalent) so the call
times out after 2 seconds and avoids unnecessary stdio piping; keep
defaultLogDir() unchanged but rely on updated getGitRoot() to return null on
timeout/exec failure.
| describe('defaultLogDir', () => { | ||
| it('should return .git-mem/logs under the git root', () => { | ||
| const gitRoot = execFileSync('git', ['rev-parse', '--show-toplevel'], { | ||
| encoding: 'utf8', | ||
| }).trim(); | ||
|
|
||
| assert.equal(defaultLogDir(), join(gitRoot, '.git-mem', 'logs')); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider mocking git calls or moving this to integration tests.
This unit test executes the real git binary, which couples unit tests to external tooling. Prefer a manual mock for the git-root lookup (or relocate this to an integration test that sets up a temp repo) to keep unit tests isolated. As per coding guidelines "tests/unit/**/*.{ts,tsx}: Create unit tests with manually mocked dependencies (no testing framework) in tests/unit/ directory".
🤖 Prompt for AI Agents
In `@tests/unit/infrastructure/logging/factory.test.ts` around lines 32 - 39, The
test calls the real git binary via execFileSync which makes it an integration
test; instead stub the git-root lookup so the unit test is isolated. In the test
for defaultLogDir, replace the direct execFileSync call by mocking
child_process.execFileSync (or the helper that wraps it) to return a fixed
gitRoot string, then assert defaultLogDir() equals join(mockGitRoot, '.git-mem',
'logs'); ensure you restore the stub after the test. Target the defaultLogDir()
call and the execFileSync symbol when implementing the mock.
Summary
defaultLogDir()from~/.git-mem/logs/(global) to<project>/.git-mem/logs/(project-local)osimportLOG_DIRenv var override still works for custom pathsCloses GIT-63
Test plan
npm test)npm run pre-commit).git-mem/logs/in the project directory after running a git-mem command🤖 Generated with Claude Code
Summary by CodeRabbit