fix(hook): make hook-augment work on Windows drive-letter paths#619
fix(hook): make hook-augment work on Windows drive-letter paths#619catsmonster wants to merge 1 commit into
Conversation
48a2738 to
26e85d5
Compare
|
Huge thanks for opening this PR and for the work you put into it. The maintainer shop is currently full, so this may sit for a bit before it gets a proper review. We will come back to this as soon as possible with real feedback; I wanted to make sure it did not sit unacknowledged in the meantime. |
|
Thanks, this is targeted at #618. Before merge, please remove generated/co-author/session metadata from the non-merge commit. If CI flags DCO on the merge commit, please refresh the branch; otherwise please include Windows |
2e1e290 to
c234f17
Compare
There was a problem hiding this comment.
Pull request overview
Fixes hook-augment on Windows by removing POSIX-only absolute-path assumptions that caused the augmenter to always early-exit (drive-letter cwd), enabling the Grep/Glob PreToolUse context augmentation to run on Windows paths while preserving the “never block a tool” behavior.
Changes:
- Introduces
cbm_hook_path_is_abs()to recognize both POSIX (/...) and Windows drive-rooted (X:/...) absolute paths. - Normalizes Windows
cwdbackslashes (\) to forward slashes (/) before validation and walk-up resolution. - Updates the walk-up loop and parent-climb termination to work correctly with drive-letter roots, and adds a regression test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/cli/hook_augment.c |
Adds cross-platform absolute-path detection, normalizes Windows cwd, and makes the directory walk-up logic work with drive-letter roots. |
src/cli/cli.h |
Exposes cbm_hook_path_is_abs() for direct test coverage. |
tests/test_cli.c |
Adds regression coverage for Windows drive-letter absolute-path handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* True for an absolute path we can walk up: POSIX "/..." or Windows drive | ||
| * "X:/..." (callers normalize '\\' to '/' first). Declared in cli.h so the | ||
| * Windows drive-letter handling (#618) has direct regression coverage. */ |
There was a problem hiding this comment.
Done in 4bb9eff — the comment now states the predicate also treats a bare X: as absolute (matching d[2] == '\0' and the cbm_hook_path_is_abs("C:") test case).
| /* True for an absolute path the augmenter can walk up: POSIX "/..." or Windows | ||
| * drive "X:/..." (callers normalize '\\' to '/' first). Exposed for tests — | ||
| * regression coverage for the Windows drive-letter no-op (#618). */ |
There was a problem hiding this comment.
Done in 4bb9eff — the comment now states the predicate also treats a bare X: as absolute (matching d[2] == '\0' and the cbm_hook_path_is_abs("C:") test case).
|
Both points addressed. 1. Co-author/session metadata removed. Rebased onto latest 2. Windows # dummy repo at C:\tmp\smoke-demo — app.js defining function smokeTestHandler()
$ codebase-memory-mcp cli index_repository '{"repo_path":"C:/tmp/smoke-demo"}'
{"project":"C-tmp-smoke-demo","nodes":6,"edges":5,...,"status":"indexed"}
$ PAYLOAD='{"hook_event_name":"PreToolUse","tool_name":"Grep","cwd":"C:/tmp/smoke-demo","tool_input":{"pattern":"smokeTestHandler"}}'
# BEFORE — release v0.8.1 (POSIX-only path guard):
$ printf '%s' "$PAYLOAD" | codebase-memory-mcp-0.8.1 hook-augment | wc -c
0
# AFTER — this PR:
$ printf '%s' "$PAYLOAD" | ./build/c/codebase-memory-mcp hook-augment
{"hookSpecificOutput":{"hookEventName":"PreToolUse","additionalContext":"[codebase-memory] 1 graph symbol(s) match \"smokeTestHandler\" (structured context; your search results below are unaffected):\n- C-tmp-smoke-demo.app.smokeTestHandler app.js Function"}}Same indexed project, same drive-letter |
hook-augment's PreToolUse Grep/Glob augmenter required POSIX-style '/'-prefixed absolute paths in two guards, so a Windows cwd (C:\repo or C:/repo) always failed: the _WIN32 cwd check returned early, and the walk-up loop guard (dir[0] == '/') never iterated. The result was a structural no-op on Windows — zero additionalContext for every Grep/Glob, even for an exactly-indexed symbol. Add cbm_hook_path_is_abs(), accepting POSIX "/..." and Windows "X:/..." roots; normalize backslashes to forward slashes on the Windows cwd; and stop the parent-climb at the drive root as well as the POSIX root. POSIX behavior is unchanged. Adds a cli_hook_augment_path_is_abs regression test (the predicate is exposed as cbm_hook_path_is_abs via cli.h) covering POSIX and Windows drive roots — it fails on the pre-fix POSIX-only guard and passes after. Verified with that test plus a standalone walk self-test (full path resolves first, then climbs to one level below the drive root, symmetric to POSIX); clang-format clean on all three files; clang -fsyntax-only on the translation unit. Not built through the full toolchain locally (no make/zlib) — relying on Windows CI to confirm the build. Fixes DeusData#618 Signed-off-by: catsmonster <shaked.brand@gmail.com>
c234f17 to
4bb9eff
Compare
What
Fixes the
hook-augmentPreToolUse Grep/Glob augmenter being a structural no-op on Windows.Closes #618.
Why
src/cli/hook_augment.chad two POSIX-only path guards that a Windows drive-lettercwd(C:\repo/C:/repo) can never satisfy:cbm_cmd_hook_augment,_WIN32branch —if (!cwd || cwd[0] != '/')returned early on every invocation (a Windows absolute path never starts with/).ha_resolve_and_querywalk-up loop —for (... && dir[0] == '/'; ...)never iterated for aC:/...path, and the parent-climb terminator assumed a/-root.cbm_project_name_from_pathalready handles Windows paths (it produced the indexed project name from theC:\...root), so the fix is purely in these path assumptions.How
cbm_hook_path_is_abs()— true for POSIX"/..."and Windows"X:/..."roots.\→/on the Windowscwdbefore validation.cbm_hook_path_is_abs(dir); stop the parent-climb at the drive root (X:/) as well as the POSIX root.cwd[0] != '/'→getcwdfallback is preserved via the predicate).Testing
The predicate is exposed as
cbm_hook_path_is_abs(declared incli.h) so it has direct regression coverage.tests/test_cli.c::cli_hook_augment_path_is_abs(registered in theclisuite): asserts the predicate accepts/...,C:/Users/me/proj,C:/,C:,d:/...and rejectsrelative/path,proj,"",NULL. It fails on the old POSIX-only guard and passes after the fix. Being pure string logic, it also runs on the Linux CI.C:\Users\me\projthe resolver queriesC:/Users/me/proj→C:/Users/me→C:/Users(stops one below the drive root), exactly symmetric to POSIX/home/u/proj→/home/u→/home.clang-formatclean on all three changed files;clang -fsyntax-only -I src -I vendored src/cli/hook_augment.cpasses.make/zliblocally, so I'm relying on Windows CI to confirm the compile/link. Flagging that explicitly.clang-tidy note: my additions follow the file's existing idiom (
[4096]buffers, uncheckedsnprintf, drive-index access) that the surrounding code already uses, so I kept them consistent rather than adding one-off named-constants/void-casts. (CI runslint.sh --ci, which skips clang-tidy.)Notes
The "never block a tool" guarantee is untouched — every path still ends in
exit 0with output written exactly once. This only changes whether context is added on Windows; it can still never deny a tool. Fits under the Windows umbrella tracker #394.