Conversation
…ervices - Domain: ILogger interface, LogLevel type, ILoggerOptions - Infrastructure: Logger (daily file output, ANSI console), NullLogger, factory - Services: optional logger parameter on MemoryService, LiberateService, ContextService - MCP tools: create logger per invocation, pass to services, log errors - CLI: create logger at startup, pass through to all 6 command handlers - Tests: 26 new tests (20 unit + 6 integration), all 161 tests pass Closes GIT-33, GIT-34, GIT-35, GIT-36, GIT-37 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a logging subsystem (ILogger + implementations + factory) and integrates optional logger injection across CLI, commands, services, MCP tools, and tests; replaces direct console output with structured logging and adds tests for logger behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (program)
participant Cmd as Command Handler
participant Svc as Service (e.g., MemoryService)
participant Log as Logger (ILogger)
participant FS as File System
CLI->>Cmd: invoke command (args, logger)
Cmd->>Svc: construct service(..., logger.child({component}))
Cmd->>Log: log info "command invoked" (context)
Svc->>Log: log debug/info/warn/error during processing
Log->>FS: append formatted log line to git-mem-YYYY-MM-DD.log (if file enabled)
Log-->>Cmd: return (fire-and-forget)
(Colored rectangles not required for basic sequence diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63324e3770
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| logDir: process.env['LOG_DIR'] ?? '.git-mem/logs', | ||
| enableConsole: process.env['LOG_CONSOLE'] === 'true', | ||
| enableFile: process.env['LOG_FILE'] !== 'false', |
There was a problem hiding this comment.
loadLoggerOptions() defaults to writing logs to .git-mem/logs and enables file output unless LOG_FILE=false, which means normal CLI/MCP usage now creates files in the working tree for every invocation. Since this repository does not ignore .git-mem/, users will get a dirty git status after read-only commands and can accidentally commit these log files (which may include command/query metadata).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Moved default logDir to os.tmpdir()/git-mem/logs so file logging no longer pollutes the working tree. Also added .git-mem/ to .gitignore as a safety net.
src/infrastructure/logging/Logger.ts
Outdated
| fs.mkdirSync(this.options.logDir, { recursive: true }); | ||
| const filename = `git-mem-${formatDate()}.log`; | ||
| const filepath = path.join(this.options.logDir, filename); | ||
| fs.appendFileSync(filepath, line, 'utf8'); |
There was a problem hiding this comment.
The logger exposes retentionDays via ILoggerOptions and LOG_RETENTION_DAYS, but file writes only append to the daily log and never delete expired files, so retention settings are currently ignored. In long-running or frequent-use environments this causes unbounded log accumulation even when users configure retention explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Added cleanExpiredLogs() method that runs once on first write — parses dates from git-mem-YYYY-MM-DD.log filenames and deletes files older than retentionDays. Skips cleanup when retentionDays <= 0.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/application/services/MemoryService.ts`:
- Around line 25-31: The log in recall() records the original `query` param even
when `options.query` was used; change the method to compute an effective query
(e.g., const effectiveQuery = query ?? options?.query) before calling
this.memoryRepository.query and use that `effectiveQuery` both in the query
payload passed to memoryRepository.query and in the logger?.info call so the
logged "query" reflects the actual value used (refer to recall,
IMemoryQueryOptions, IMemoryQueryResult, memoryRepository.query, and
logger?.info).
In `@src/infrastructure/logging/factory.ts`:
- Line 20: The current retentionDays assignment uses
parseInt(process.env['LOG_RETENTION_DAYS'] ?? '7', 10) || 7 which treats 0 as
falsy and forces a fallback to 7; change it to preserve 0 as a valid value by
parsing into a variable (e.g., const parsed =
parseInt(process.env['LOG_RETENTION_DAYS'] ?? '7', 10)) and then set
retentionDays to parsed if Number.isNaN(parsed) is false, otherwise use 7 (or
use nullish coalescing behavior instead), updating the retentionDays assignment
in factory.ts (symbol: retentionDays) accordingly.
In `@src/infrastructure/logging/Logger.ts`:
- Around line 73-75: child() creates a new Logger but doesn't propagate the
parent's dirCreated state, causing redundant dir creation checks; modify the
Logger constructor usage in child() (and the Logger constructor signature if
needed) to accept and pass through the parent's dirCreated flag (or switch to a
shared state object referenced by both parent and child) so child loggers
inherit the directory-created cache; update any references to Logger(...) in
child() to include this flag/shared state and ensure any code that sets
dirCreated writes to the shared state or the propagated flag.
- Around line 99-108: The writeToFile method calls
fs.mkdirSync(this.options.logDir, { recursive: true }) on every write; add a
cached flag (for example a private property like this.logDirCreated or
initializeLogDir method) to run mkdirSync only once (or lazily once on first
write) and skip subsequent calls, keeping the rest of writeToFile
(filename/appendFileSync) unchanged and ensuring thread-safety if needed for
concurrent writes.
In `@src/mcp-server.ts`:
- Around line 31-34: The catch block creates a separate logger; hoist the logger
creation out of main() to a single module-level logger so both main() and the
main().catch handler reuse it: create a top-level const logger =
createLogger().child({ component: 'mcp-server' }) and remove the createLogger()
call inside the catch handler, then use logger.fatal(...) there; ensure
references to logger inside main() still resolve to the hoisted logger or pass
it into main() if needed so no duplicate logger instances are created.
In `@tests/unit/infrastructure/logging/Logger.test.ts`:
- Around line 13-21: The helper createOptions is always calling createTempDir()
even when the caller provides logDir in overrides; change it so createTempDir()
is only invoked when overrides?.logDir is not provided — e.g., compute a local
logDir = overrides?.logDir ?? createTempDir() and then spread overrides into the
returned object, ensuring the function (named createOptions) no longer allocates
an unused temp directory when overrides.logDir is supplied.
- Log effective query value in MemoryService.recall() instead of raw param - Fix retentionDays parseInt to preserve 0 as valid value via NaN check - Cache mkdirSync with shared state so child loggers inherit dirCreated flag - Implement log retention cleanup on first write (deletes expired log files) - Move default logDir to os.tmpdir() to avoid polluting working trees - Hoist logger to module level in mcp-server.ts to avoid duplicate instances - Avoid unnecessary temp dir allocation in test helper when logDir is overridden - Add .git-mem/ to .gitignore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/infrastructure/logging/Logger.ts`:
- Around line 84-99: The call to JSON.stringify in Logger.log can throw for
circular refs or BigInt; wrap the serialization so logging never throws: replace
the direct JSON.stringify(merged) usage with a safe serialization step inside
Logger.log that tries JSON.stringify(merged) in a try/catch and on failure falls
back to a non-throwing representation (e.g., util.inspect(merged) or a simple
string like "<unserializable context>") and use that result for contextStr;
update the code paths that build contextStr (the merged variable and contextStr
computation) so writeToFile and the console write use the safe string, ensuring
Logger.log, merged, contextStr, writeToFile, formatTimestamp and LEVEL_COLORS
remain the only touched symbols.
In `@tests/unit/infrastructure/logging/factory.test.ts`:
- Around line 7-69: The test suite mutates process.env in multiple it() blocks
and currently restores originals only in the after() hook; change the teardown
hook from after() to afterEach() so the origEnv snapshot saved in before() is
used to restore process.env after every test (affecting the tests under describe
'Logging factory' that call loadLoggerOptions and defaultLogDir), ensuring no
cross-test pollution of environment variables.
In `@tests/unit/infrastructure/logging/Logger.test.ts`:
- Around line 30-37: Change the test-suite teardown from using after() to
afterEach() so tempDirs is cleaned between tests: replace the after() hook that
iterates over tempDirs and calls fs.rmSync(...) with an afterEach() hook that
deletes each dir and then resets tempDirs.length = 0; update references to
tempDirs in the Logger test suite accordingly so each test starts with a clean
state.
| private log(level: LogLevel, message: string, context?: Record<string, unknown>): void { | ||
| if (!this.isLevelEnabled(level)) return; | ||
|
|
||
| const merged = { ...this.bindings, ...context }; | ||
| const contextStr = Object.keys(merged).length > 0 ? ` ${JSON.stringify(merged)}` : ''; | ||
| const timestamp = formatTimestamp(); | ||
| const levelUpper = level.toUpperCase().padEnd(5); | ||
|
|
||
| if (this.options.enableFile) { | ||
| this.writeToFile(`${timestamp} ${levelUpper} ${message}${contextStr}\n`); | ||
| } | ||
|
|
||
| if (this.options.enableConsole) { | ||
| const color = LEVEL_COLORS[level]; | ||
| process.stderr.write(`${color}${timestamp} ${levelUpper}${RESET} ${message}${contextStr}\n`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
JSON.stringify circular references BigInt Node.js throw behavior
💡 Result:
In Node.js (V8), JSON.stringify() throws synchronously with a TypeError in both cases:
- Circular references: when it detects a cycle, it throws
TypeError: Converting circular structure to JSON(often with extra detail about which property “closes the circle”). [2] BigIntvalues: when it encounters aBigInt(e.g.,1n) and there’s no custom serialization, it throwsTypeError: Do not know how to serialize a BigInt. [1]
If you need it not to throw, you must pre-process via a replacer or define a custom toJSON (commonly for BigInt) before calling JSON.stringify(). [1]
🏁 Script executed:
# Find and inspect the Logger.ts file
fd -t f "Logger.ts" src/infrastructure/logging/ -x cat -n {}Repository: TonyCasey/git-mem
Length of output: 5862
🏁 Script executed:
# Check the full context around lines 84-99 including the try-catch wrapper
fd -t f "Logger.ts" src/infrastructure/logging/ -x head -120 {} | tail -50Repository: TonyCasey/git-mem
Length of output: 1712
Guard JSON serialization to prevent logger from throwing.
JSON.stringify throws synchronously on circular structures (TypeError: Converting circular structure to JSON) and BigInt values (TypeError: Do not know how to serialize a BigInt). Currently, this call on line 88 is unprotected—if a caller passes such a context, the exception bubbles out and crashes the logger's caller, violating the stated principle "Logging should never crash the app."
Wrap JSON.stringify in a safe fallback function:
Proposed fix
+function safeStringify(value: unknown): string {
+ try {
+ return JSON.stringify(value);
+ } catch {
+ return '[unserializable]';
+ }
+}
+
private log(level: LogLevel, message: string, context?: Record<string, unknown>): void {
if (!this.isLevelEnabled(level)) return;
const merged = { ...this.bindings, ...context };
- const contextStr = Object.keys(merged).length > 0 ? ` ${JSON.stringify(merged)}` : '';
+ const contextStr = Object.keys(merged).length > 0 ? ` ${safeStringify(merged)}` : '';📝 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.
| private log(level: LogLevel, message: string, context?: Record<string, unknown>): void { | |
| if (!this.isLevelEnabled(level)) return; | |
| const merged = { ...this.bindings, ...context }; | |
| const contextStr = Object.keys(merged).length > 0 ? ` ${JSON.stringify(merged)}` : ''; | |
| const timestamp = formatTimestamp(); | |
| const levelUpper = level.toUpperCase().padEnd(5); | |
| if (this.options.enableFile) { | |
| this.writeToFile(`${timestamp} ${levelUpper} ${message}${contextStr}\n`); | |
| } | |
| if (this.options.enableConsole) { | |
| const color = LEVEL_COLORS[level]; | |
| process.stderr.write(`${color}${timestamp} ${levelUpper}${RESET} ${message}${contextStr}\n`); | |
| } | |
| function safeStringify(value: unknown): string { | |
| try { | |
| return JSON.stringify(value); | |
| } catch { | |
| return '[unserializable]'; | |
| } | |
| } | |
| private log(level: LogLevel, message: string, context?: Record<string, unknown>): void { | |
| if (!this.isLevelEnabled(level)) return; | |
| const merged = { ...this.bindings, ...context }; | |
| const contextStr = Object.keys(merged).length > 0 ? ` ${safeStringify(merged)}` : ''; | |
| const timestamp = formatTimestamp(); | |
| const levelUpper = level.toUpperCase().padEnd(5); | |
| if (this.options.enableFile) { | |
| this.writeToFile(`${timestamp} ${levelUpper} ${message}${contextStr}\n`); | |
| } | |
| if (this.options.enableConsole) { | |
| const color = LEVEL_COLORS[level]; | |
| process.stderr.write(`${color}${timestamp} ${levelUpper}${RESET} ${message}${contextStr}\n`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/infrastructure/logging/Logger.ts` around lines 84 - 99, The call to
JSON.stringify in Logger.log can throw for circular refs or BigInt; wrap the
serialization so logging never throws: replace the direct JSON.stringify(merged)
usage with a safe serialization step inside Logger.log that tries
JSON.stringify(merged) in a try/catch and on failure falls back to a
non-throwing representation (e.g., util.inspect(merged) or a simple string like
"<unserializable context>") and use that result for contextStr; update the code
paths that build contextStr (the merged variable and contextStr computation) so
writeToFile and the console write use the safe string, ensuring Logger.log,
merged, contextStr, writeToFile, formatTimestamp and LEVEL_COLORS remain the
only touched symbols.
| describe('Logging factory', () => { | ||
| // Save original env vars | ||
| const origEnv: Record<string, string | undefined> = {}; | ||
|
|
||
| before(() => { | ||
| origEnv['LOG_LEVEL'] = process.env['LOG_LEVEL']; | ||
| origEnv['LOG_DIR'] = process.env['LOG_DIR']; | ||
| origEnv['LOG_CONSOLE'] = process.env['LOG_CONSOLE']; | ||
| origEnv['LOG_FILE'] = process.env['LOG_FILE']; | ||
| origEnv['LOG_RETENTION_DAYS'] = process.env['LOG_RETENTION_DAYS']; | ||
| }); | ||
|
|
||
| after(() => { | ||
| // Restore env vars | ||
| for (const [key, value] of Object.entries(origEnv)) { | ||
| if (value === undefined) { | ||
| delete process.env[key]; | ||
| } else { | ||
| process.env[key] = value; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| describe('loadLoggerOptions', () => { | ||
| it('should return defaults when no env vars set', () => { | ||
| delete process.env['LOG_LEVEL']; | ||
| delete process.env['LOG_DIR']; | ||
| delete process.env['LOG_CONSOLE']; | ||
| delete process.env['LOG_FILE']; | ||
| delete process.env['LOG_RETENTION_DAYS']; | ||
|
|
||
| const opts = loadLoggerOptions(); | ||
|
|
||
| assert.equal(opts.level, 'info'); | ||
| assert.equal(opts.logDir, defaultLogDir()); | ||
| assert.equal(opts.enableConsole, false); | ||
| assert.equal(opts.enableFile, true); | ||
| assert.equal(opts.retentionDays, 7); | ||
| }); | ||
|
|
||
| it('should read env vars correctly', () => { | ||
| process.env['LOG_LEVEL'] = 'debug'; | ||
| process.env['LOG_DIR'] = '/tmp/custom-logs'; | ||
| process.env['LOG_CONSOLE'] = 'true'; | ||
| process.env['LOG_FILE'] = 'false'; | ||
| process.env['LOG_RETENTION_DAYS'] = '14'; | ||
|
|
||
| const opts = loadLoggerOptions(); | ||
|
|
||
| assert.equal(opts.level, 'debug'); | ||
| assert.equal(opts.logDir, '/tmp/custom-logs'); | ||
| assert.equal(opts.enableConsole, true); | ||
| assert.equal(opts.enableFile, false); | ||
| assert.equal(opts.retentionDays, 14); | ||
| }); | ||
|
|
||
| it('should default to info for invalid LOG_LEVEL', () => { | ||
| process.env['LOG_LEVEL'] = 'invalid'; | ||
|
|
||
| const opts = loadLoggerOptions(); | ||
|
|
||
| assert.equal(opts.level, 'info'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js test runner concurrency default behavior process.env isolation
💡 Result:
- Default isolation mode (CLI):
node --testruns test files with process-level isolation by default (each matched test file runs in its own child process). In this mode, the max number of child processes is controlled by--test-concurrency. The docs also note this is “the default” when describing process isolation mode. [1][2] - Default CLI concurrency: Unless you set
--test-isolation=none,--test-concurrencydefaults toos.availableParallelism() - 1. [1] - What this means for
process.env“isolation”:- With default process isolation, each test file has its own
process.envobject in its own process. Environment variables are inherited at process spawn time (child processenvdefaults to the parent’sprocess.env), so changingprocess.envinside one test file won’t affect other test files (they’re different processes). [3][2] - With
--test-isolation=none, all test files are loaded into the same process and can interact via global state—that includesprocess.env, so mutations can leak across files. [2]
- With default process isolation, each test file has its own
Sources:
[1] Node.js CLI docs (--test-concurrency default and isolation interaction)
[2] Node.js Test Runner docs (execution model; default process isolation; shared-context behavior when isolation is disabled)
[3] Node.js child_process docs (spawn() default env: process.env)
🌐 Web query:
Node.js test runner describe block concurrency isolation subtests
💡 Result:
describe()blocks do not create isolation. In Node’s built-in runner (node:test),describe()/it()are just suite/test helpers that run in the same process and same JS thread as the rest of the tests in that file, sharing globals/module state. (nodejs.org)- “Concurrency” inside a file is cooperative (event-loop) concurrency, not parallel threads/processes. The
concurrencyoption ontest()/subtests controls how many tests are allowed to be in-flight asynchronously within the single-threaded event loop; it does not give you separate processes perdescribe/subtest. (nodejs.org) - Process-level isolation is per test file, not per
describeblock. With default isolation (isolation: 'process'), each test file runs in its own child process;--test-concurrencycontrols how many test files run in parallel. (nodejs.org) - Subtests lifecycle matters: a parent test does not automatically wait for its subtests—if the parent finishes while subtests are still running, remaining subtests are cancelled and treated as failures, and subtest failures fail the parent. (So you generally
awaitsubtests or otherwise keep the parent open.) (nodejs.org)
Implication: if you need true isolation for groups of tests you might think of as “describe blocks,” put them in separate test files (so they can be isolated by process) rather than relying on describe() nesting. (nodejs.org)
Citations:
- 1: https://nodejs.org/api/test.html?utm_source=openai
- 2: https://nodejs.org/api/test.html?utm_source=openai
- 3: https://nodejs.org/api/test.html?utm_source=openai
- 4: https://nodejs.org/download/release/v18.0.0/docs/api/test.html?utm_source=openai
- 5: https://nodejs.org/api/test.html?utm_source=openai
🌐 Web query:
Node.js test runner default concurrency for test() it() blocks sequential or concurrent
💡 Result:
In Node.js’s built-in test runner (node:test), test() (and BDD-style it() when provided by a wrapper) runs tests sequentially by default:
- Default per-test concurrency is
false, meaning only one test runs at a time unless you setconcurrencyin the test options (or inherit it from a parent). [1] - At the runner level, top-level tests are executed with a concurrency of 1 after files are loaded. [1]
So: default = sequential, opt into concurrency via concurrency: true|number. [1]
Sources:
[1] Node.js “Test runner” documentation (node:test) https://nodejs.org/api/test.html
🏁 Script executed:
find . -type f -name "factory.test.ts" | grep -E "logging|infrastructure"Repository: TonyCasey/git-mem
Length of output: 112
🏁 Script executed:
cat -n ./tests/unit/infrastructure/logging/factory.test.ts | head -80Repository: TonyCasey/git-mem
Length of output: 3300
Use afterEach() to restore process.env after each test, preventing cross-test pollution.
Tests mutate global env vars and currently only restore them after the entire suite completes. This allows earlier tests' env changes to affect later tests. Switch after() to afterEach() to ensure each test runs with a clean environment state.
Suggested fix
-import { describe, it, before, after } from 'node:test';
+import { describe, it, before, afterEach } from 'node:test';
describe('Logging factory', () => {
// Save original env vars
const origEnv: Record<string, string | undefined> = {};
before(() => {
origEnv['LOG_LEVEL'] = process.env['LOG_LEVEL'];
origEnv['LOG_DIR'] = process.env['LOG_DIR'];
origEnv['LOG_CONSOLE'] = process.env['LOG_CONSOLE'];
origEnv['LOG_FILE'] = process.env['LOG_FILE'];
origEnv['LOG_RETENTION_DAYS'] = process.env['LOG_RETENTION_DAYS'];
});
- after(() => {
+ afterEach(() => {
// Restore env vars
for (const [key, value] of Object.entries(origEnv)) {
if (value === undefined) {
delete process.env[key];
} else {
process.env[key] = value;
}
}
});🤖 Prompt for AI Agents
In `@tests/unit/infrastructure/logging/factory.test.ts` around lines 7 - 69, The
test suite mutates process.env in multiple it() blocks and currently restores
originals only in the after() hook; change the teardown hook from after() to
afterEach() so the origEnv snapshot saved in before() is used to restore
process.env after every test (affecting the tests under describe 'Logging
factory' that call loadLoggerOptions and defaultLogDir), ensuring no cross-test
pollution of environment variables.
| describe('Logger', () => { | ||
| const tempDirs: string[] = []; | ||
|
|
||
| after(() => { | ||
| for (const dir of tempDirs) { | ||
| fs.rmSync(dir, { recursive: true, force: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Find the Logger test file
find . -name "Logger.test.ts" -o -name "Logger.test.ts"Repository: TonyCasey/git-mem
Length of output: 111
🏁 Script executed:
# Check the actual current state of the file
fd "Logger.test.ts" | head -1 | xargs cat -n 2>/dev/null || echo "File not found in standard locations"Repository: TonyCasey/git-mem
Length of output: 7653
🏁 Script executed:
# Search for the file in tests/unit/infrastructure
find tests/unit -name "*Logger*" -type f 2>/dev/nullRepository: TonyCasey/git-mem
Length of output: 162
🏁 Script executed:
# Verify the established pattern from PreferenceStore.test.ts mentioned in learnings
fd "PreferenceStore.test.ts" | xargs grep -A 10 "afterEach" 2>/dev/null | head -20Repository: TonyCasey/git-mem
Length of output: 43
Change after() to afterEach() for better test isolation.
Each test creates and pushes a temp directory to tempDirs, but cleanup only runs once at the end of all tests in the suite. This can leave residue between tests and makes test failures harder to diagnose. Using afterEach() with tempDirs.length = 0 ensures each test starts with a clean state, matching the established pattern for file-based infrastructure tests.
♻️ Suggested change
-import { describe, it, before, after } from 'node:test';
+import { describe, it, before, afterEach } from 'node:test';
@@
- after(() => {
+ afterEach(() => {
for (const dir of tempDirs) {
fs.rmSync(dir, { recursive: true, force: true });
}
+ tempDirs.length = 0;
});📝 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.
| describe('Logger', () => { | |
| const tempDirs: string[] = []; | |
| after(() => { | |
| for (const dir of tempDirs) { | |
| fs.rmSync(dir, { recursive: true, force: true }); | |
| } | |
| }); | |
| describe('Logger', () => { | |
| const tempDirs: string[] = []; | |
| afterEach(() => { | |
| for (const dir of tempDirs) { | |
| fs.rmSync(dir, { recursive: true, force: true }); | |
| } | |
| tempDirs.length = 0; | |
| }); |
🤖 Prompt for AI Agents
In `@tests/unit/infrastructure/logging/Logger.test.ts` around lines 30 - 37,
Change the test-suite teardown from using after() to afterEach() so tempDirs is
cleaned between tests: replace the after() hook that iterates over tempDirs and
calls fs.rmSync(...) with an afterEach() hook that deletes each dir and then
resets tempDirs.length = 0; update references to tempDirs in the Logger test
suite accordingly so each test starts with a clean state.
1 participant
Summary
ILoggerdomain interface withLogLevel,ILoggerOptionsLogger(daily log files, ANSI console output),NullLogger, and factory functionsloggerparameter intoMemoryService,LiberateService,ContextServiceCloses GIT-33, GIT-34, GIT-35, GIT-36, GIT-37
Test plan
npm run type-check— compiles cleanlynpm run lint— passesnpm test— all 161 tests pass (135 existing + 26 new)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores