[kyle-sexton]

Refs: #10019

Summary

test-windows-secrets has been red since #10019 on one case: "allowed roots: through a link outside D that lands under D → exit 0: expected exit 0, got 2" (for example run 40211741514, PASS=301 FAIL=1). The guard failed safe: it scanned a write it should have exempted. This fixes the Windows path so the case grants as it does on Linux, without granting anything new.

Root cause:

• The test makes the link with mklink /J, and its target is cygpath -w of a /tmp/... path (TEST_TMPDIR is mktemp -d), so the junction stores a Windows temp-folder path.

• On the runner, /tmp is the user temp folder. The log spells the same mktemp directory both as /tmp/tmp.ZAZQuZvuJK/... and as C:/Users/runneradmin/AppData/Local/Temp/tmp.ZAZQuZvuJK/....

• Cygwin, and so Git Bash, follows a junction as a symlink. Its user guide says directory junctions are "recognized as symlink but never generated by Cygwin", and describes the usertemp mount that maps the temp folder to /tmp (Cygwin user guide).

• Before the fix, spd_under_allowed_root passed "<realpath of to-d>/sub/x.md" to cygpath -l -m, and sub does not exist yet. GetLongPathNameW fails "if the file does not exist" (Microsoft Learn).

• The passing case "entry spelled through a link to D → exit 0" sends the same realpath answer for to-d, with nothing appended, through the same spd_cmp_to, and gets a long spelling that matches the root. The fix's first spd_cmp_to call gets exactly that input, so the failing case should now match the same way.

• UNVERIFIED (inference, not run on Windows):

  • that realpath answers the junction in the /tmp/... mount spelling;
  • that the junction target carries the runner's 8.3 name (RUNNER~1);
  • that cygpath -l uses GetLongPathNameW;
  • whether cygpath -l -m on a missing tail prints the short spelling or exits non-zero.

  Either outcome fails the old comparison, and the fix avoids both, so the conclusion does not depend on which one is true.

Fix

• plugins/guardrails/hooks/secret-pattern-detection.sh (spd_under_allowed_root): spd_cmp_to now maps only the existing ancestor's physical path. The components below it, which do not exist, are joined after that. The second spd_cmp_to call only folds case and drive, and on Windows it spawns no cygpath, because its input is already drive-spelled.
• Nothing is loosened. The resolver still runs on the same ancestor, the -ef identity check still applies, and either call failing still returns 1. The joined components do not exist, so they can hold no link, and the target was already refused for .., ., // or a line break. On Linux the compared string is the same as before.
• plugins/guardrails/hooks/secret-pattern-detection.test.sh: a seam test forces the msys arm on any host. A stub resolver answers in a mount spelling, and a stub cygpath expands long names only for a path that exists. It checks that a junction into the root grants, and that a junction to outside and an unresolvable ancestor do not.
• .changes/guardrails/: a patch fragment.

Verification

• bash plugins/guardrails/hooks/secret-pattern-detection.test.sh on Linux: PASS=312 FAIL=0.
• With the hook change reverted and the new test kept: PASS=311 FAIL=1. The new "junction outside D into D, mount-spelled answer → granted" case fails, so the test exercises the fix.
• shellcheck is clean on both files. scripts/check-changelog-fragments.sh --check passes.
• The Windows case cannot run on Linux. The proof for it is this PR's own pr-test-windows run (test-windows-secrets), where "through a link outside D that lands under D → exit 0" must pass.

Related

• feat(guardrails): exempt local-only paths from secret-pattern-detection #10019 added the allowed-roots exemption and this case.
• feat(guardrails): exempt local-only paths from secret-pattern-detection #8761

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-26T13:49:54.824585Z	5d92c9a	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[github-actions]

claude-review reviewed 5d92c9a258b2cd59e68271f4ef7d7d2ad9441e12: 0 finding(s), 0 as inline comments.

No findings. I read the full diff (3 files), the PR context, and spd_under_allowed_root, spd_cmp_to and spd_exempt_anchor in plugins/guardrails/hooks/secret-pattern-detection.sh. I did not run the hook or its tests.

The fix (secret-pattern-detection.sh:787-791): spd_cmp_to now maps only the resolved existing ancestor, and the not-yet-created suffix is joined afterwards. I checked these edge cases:

• The suffix always starts with /, so the join is well formed. This holds when the ancestor is /, which gives an empty prefix, and when it is a C:/ drive root, whose trailing slash spd_cmp_to strips.
• The second spd_cmp_to call takes an already drive-spelled string. On Windows it matches the ^[A-Za-z]:/ arm and spawns no cygpath. On POSIX it does the same checks as before on the same string.
• Nothing is loosened. The resolver call, the -ef identity check and the early return 1 on failure are unchanged. The joined components do not exist, so they cannot hold a link.
• Linux behaviour is unchanged, because the string compared is the same as before.

Test (secret-pattern-detection.test.sh): the new simulation drives three cases through the msys arm on a POSIX host:

• A junction into the root is granted.
• A link out of the root is not granted.
• An unresolvable ancestor is not granted.

It is skipped when no symlink can be made or on a Windows host. The stubs match the code path: the resolver answers in the mount spelling, and cygpath expands long names only for a path that exists. The PR's claim that the new case fails with the hook change reverted is author-claimed and I did not verify it.

Unverified Windows behaviour: the PR says the real Windows result, including realpath answering in the /tmp mount spelling and what cygpath -l does for a missing tail, is inference. The PR's own test-windows-secrets run is the proof for that. Reviewing the code alone, I cannot confirm that run goes green.

Closing issue: the PR refs #10019 and closes no issue, so there are no acceptance criteria to check.

Manifests: none changed. The changelog fragment (bump: patch) fits a fix that narrows a fail-safe false positive.

Cost: nothing here can cause a charge.

[github-actions]

claude-security-review reviewed 5d92c9a258b2cd59e68271f4ef7d7d2ad9441e12: 0 finding(s), 0 as inline comments.

No security issues found in this pull request.

critical-findings: 0

What I checked. The change is in spd_under_allowed_root in plugins/guardrails/hooks/secret-pattern-detection.sh (lines 782-791 at the head SHA). The secret_pattern_detection_allowed_roots exemption lets a write skip the secret scan when its real path is strictly under a user-granted root. The change maps only the existing ancestor's physical path through spd_cmp_to, and therefore through cygpath -l -m. It then joins the not-yet-existing suffix and runs spd_cmp_to a second time to fold case and drive. I read the full function and its helpers: spd_exempt_target, spd_exempt_anchor, spd_cmp_to and spd_canon_to.

Why it does not widen the exemption.

• The resolver still runs on the same ancestor. The -ef identity check and the || return 1 on both comparison calls are unchanged, so a resolver or cygpath failure still means the write is scanned.
• The suffix is the part of the target below the nearest existing ancestor, so none of its components exist and none can be a link. spd_exempt_target has already refused .., ., //, a line break, and on POSIX a backslash. On Windows it requires a drive-spelled path, and the second spd_cmp_to call rejects line breaks again.
• After the first call, t is drive-spelled. The second call therefore does not run cygpath on Windows, and on POSIX it adds only the existing backslash and newline refusal. The compared string on Linux is the same as before.
• The final strictly-under test ([[ "$t" == "$r"/* ]]) and the home and filesystem-root refusals are untouched.
• The option is read from the hook's userConfig environment, never from the payload, so the change adds no payload-controlled grant.

Residual notes, not findings.

• Any gap between checking the ancestor and the write landing is a time-of-check/time-of-use window that was already there.
• The author marks the Windows resolver and cygpath behavior as unverified inference. The only failure mode of a wrong inference is the case the PR is fixing: a write that should be exempt is scanned. That fails safe.
• The new seam test uses stub resolver and cygpath functions in a child shell. It covers three cases: a grant through the mount spelling, a link out of the root, and an unresolvable ancestor. Only the grant case is wrapped in the GitHub Actions Windows job's own proof.

The .changes fragment is inert. No trust-control paths from the TRUSTED PATHS list are touched, and no plugin manifests are changed. The PR description contains no embedded instructions to the reviewer.

Cost: the review spends no billed resources.

[kyle-sexton]

test-windows-secrets passed on this head (5d92c9a): job 121609000889. That is the job #10276 reports failing on main with allowed roots: through a link outside D that lands under D → exit 0: expected exit 0, got 2, so this PR should also clear #10276 once it merges.