Skip to content

fix: use project-local .git-mem/logs/ for log directory (GIT-63) #31

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.

Sign up for GitHub

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

Merged
TonyCasey merged 2 commits into from
Feb 12, 2026
Merged

fix: use project-local .git-mem/logs/ for log directory (GIT-63) #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84d837a
fix: use project-local .git-mem/logs/ for log directory (GIT-63)
TonyCasey Feb 12, 2026
8d41bb9
fix: address PR review — use git root, add defaultLogDir test (GIT-63)
TonyCasey Feb 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/infrastructure/logging/factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as os from 'node:os';
import * as path from 'node:path';
import { execFileSync } from 'node:child_process';
import { ILogger, ILoggerOptions, LogLevel } from '../../domain/interfaces/ILogger';
import { Logger } from './Logger';
import { NullLogger } from './NullLogger';
Expand All @@ -10,8 +10,20 @@ function isValidLogLevel(value: string): value is LogLevel {
return VALID_LEVELS.includes(value as LogLevel);
}

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');
Comment on lines +13 to +26
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, check if the file exists and read it
cat -n src/infrastructure/logging/factory.ts | head -30

Repository: 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 5

Repository: 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.

Suggested change
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.

}
Comment on lines 24 to 27
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

@TonyCasey TonyCasey Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Updated to use git rev-parse --show-toplevel with fallback to process.cwd() when not in a git repo.

Comment on lines 24 to 27
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

@TonyCasey TonyCasey Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a unit test in defaultLogDir describe block that asserts the path matches join(gitRoot, '.git-mem', 'logs').

Comment on lines 24 to 27
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 -B2

Repository: 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.

Copy link
Copy Markdown
Owner Author

@TonyCasey TonyCasey Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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


export function loadLoggerOptions(): ILoggerOptions {
Expand Down
12 changes: 12 additions & 0 deletions tests/unit/infrastructure/logging/factory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { describe, it, before, after } from 'node:test';
import assert from 'node:assert/strict';
import { join } from 'path';
import { execFileSync } from 'child_process';
import { createLogger, createNullLogger, defaultLogDir, loadLoggerOptions } from '../../../../src/infrastructure/logging/factory';
import { Logger } from '../../../../src/infrastructure/logging/Logger';
import { NullLogger } from '../../../../src/infrastructure/logging/NullLogger';
Expand Down Expand Up @@ -27,6 +29,16 @@ describe('Logging factory', () => {
}
});

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'));
});
Comment on lines +32 to +39
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

});

describe('loadLoggerOptions', () => {
it('should return defaults when no env vars set', () => {
delete process.env['LOG_LEVEL'];
Expand Down