Conversation
- Update config.ts to read from .git-mem/.git-mem.yaml - Export getConfigPath() and getConfigDir() helpers - Export CONFIG_DIR and CONFIG_FILE constants - Update integration test helpers to write YAML config - Update all unit and integration tests This also completes GIT-93 (update integration test helpers). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42 AI-Model: claude-opus-4-5-20251101 AI-Decision: migrate config from JSON to YAML (GIT-89). - Update config.ts to read from .git-mem/.git-mem.yaml AI-Confidence: medium AI-Tags: hooks, utils, tests, integration, unit AI-Lifecycle: project AI-Memory-Id: ea93b196 AI-Source: heuristic
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR migrates the git-mem configuration system from JSON format (.git-mem.json in root) to YAML format (.git-mem.yaml in a dedicated .git-mem directory). It introduces public helper functions for config path resolution and updates all related test files accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR migrates hook configuration from a root-level JSON file to a YAML file under a dedicated .git-mem/ directory, updating config loading utilities and test helpers accordingly.
Changes:
- Switch
loadHookConfig()to read.git-mem/.git-mem.yamland parse with theyamlpackage. - Export config path utilities/constants (
getConfigPath(),getConfigDir(),CONFIG_DIR,CONFIG_FILE) for reuse. - Update unit/integration tests and helpers to write YAML config files in the new location.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/hooks/utils/config.ts |
Introduces YAML-based config loading and exports config path helpers/constants. |
tests/unit/hooks/utils/config.test.ts |
Updates unit tests to cover new config helpers and YAML config reading/merging. |
tests/integration/hooks/helpers.ts |
Updates integration helper to write YAML config under .git-mem/. |
tests/integration/hooks/hook-commit-msg.test.ts |
Updates commit-msg integration test setup to write YAML config under .git-mem/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function loadHookConfig(cwd?: string): IHookConfig { | ||
| const dir = cwd ?? process.cwd(); | ||
| const configPath = join(dir, '.git-mem.json'); | ||
| const configPath = getConfigPath(cwd); | ||
|
|
||
| if (!existsSync(configPath)) { | ||
| return DEFAULTS; | ||
| } | ||
|
|
||
| try { | ||
| const raw = JSON.parse(readFileSync(configPath, 'utf8')) as Record<string, unknown>; | ||
| const raw = parseYaml(readFileSync(configPath, 'utf8')) as Record<string, unknown>; | ||
| const rawHooks = (raw.hooks ?? {}) as Partial<IHooksConfig>; |
There was a problem hiding this comment.
loadHookConfig() now only looks for .git-mem/.git-mem.yaml. However, the CLI init flows in this repo still create and deep-merge .git-mem.json (e.g., src/commands/init.ts, src/commands/init-hooks.ts), meaning user config written by init will be silently ignored by hooks. Either update the init commands to write the YAML file in the new location, or add a backward-compatible fallback here that reads legacy .git-mem.json if the YAML file is missing.
| /** | ||
| * Get the full path to the config file. | ||
| * @param cwd - Working directory (defaults to process.cwd()) | ||
| * @returns Absolute path to .git-mem/.git-mem.yaml | ||
| */ | ||
| export function getConfigPath(cwd?: string): string { | ||
| const dir = cwd ?? process.cwd(); | ||
| return join(dir, CONFIG_DIR, CONFIG_FILE); | ||
| } | ||
|
|
||
| /** | ||
| * Get the path to the config directory. | ||
| * @param cwd - Working directory (defaults to process.cwd()) | ||
| * @returns Absolute path to .git-mem/ | ||
| */ | ||
| export function getConfigDir(cwd?: string): string { | ||
| const dir = cwd ?? process.cwd(); | ||
| return join(dir, CONFIG_DIR); |
There was a problem hiding this comment.
The JSDoc for getConfigPath() / getConfigDir() says the returned path is absolute, but join(cwd, ...) will return a relative path if the caller passes a relative cwd. Consider either resolving cwd (e.g., via resolve()) or adjusting the docs to say the path is relative when cwd is relative.
| it('should return path to .git-mem/.git-mem.yaml', () => { | ||
| const testDir = '/some/dir'; | ||
| const result = getConfigPath(testDir); | ||
| assert.equal(result, '/some/dir/.git-mem/.git-mem.yaml'); | ||
| }); | ||
|
|
||
| it('should use process.cwd() when no cwd provided', () => { | ||
| const result = getConfigPath(); | ||
| assert.ok(result.endsWith('.git-mem/.git-mem.yaml')); | ||
| }); |
There was a problem hiding this comment.
These assertions hard-code POSIX separators (/some/dir/... and .endsWith('.git-mem/.git-mem.yaml')). Since getConfigPath() uses path.join(), the output is platform-dependent and these tests will fail on Windows. Consider asserting using join(testDir, CONFIG_DIR, CONFIG_FILE) and using path.sep-safe checks for the process.cwd() cases.
Summary
config.tsto read from.git-mem/.git-mem.yamlinstead of.git-mem.jsonyamlpackagegetConfigPath(),getConfigDir(),CONFIG_DIR,CONFIG_FILEfor reuseThis also completes GIT-93 (update integration test helpers).
Closes GIT-89
Closes GIT-93
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
.git-mem/subdirectory instead of project root for better organization.