fix(guardrails): gate hardcoded-path repo branch on real non-home checkout

[kyle-sexton]

No linked issue — partial fix: this lands F1 of #389 (repo-path branch false positive); #389 stays open until F2 (percent-env token false positive) distributes back from melodic-software/standards, whose components/path-detection/machine-path-patterns.sh is the upstream owner of the affected pattern library (sync-manifest.yml marks the guardrails copy managed — not editable downstream).

F1: hardcoded-path-check.sh engages the repo-path branch only for a real git checkout (git -C ... rev-parse --show-toplevel) that is neither ${HOME:-%USERPROFILE%} nor a home ancestor; missing home resolution fails toward detection. Repro-first test discipline: both stay-quiet repros failed (exit 2) against unmodified code, pass after; new MUST-FIRE case (real non-home checkout root in content → exit 2) guards against over-suppression — the branch previously had zero test coverage.

Verification: hardcoded-path-check.test.sh PASS=34 FAIL=0; shellcheck clean; machine-path-patterns.sh verified byte-identical to the standards source (no downstream skew shipped).

Related

• guardrails: hardcoded-path-check false positives — repo-path branch flags all paths under home; percent-env interpolations flagged #389 (F1 of 2; F2 routed upstream)
• The standards F2 PR will cross-link here when opened

🤖 Generated with Claude Code

[claude]

Claude finished @kyle-sexton's task in 2m 55s —— View job

---

Security Review — PR #759

• Fetch PR diff
• Read changed files
• Analyze for security vulnerabilities
• Post findings

---

No critical or important security issues found. Four files changed: plugin.json (version bump + whitespace only), CHANGELOG.md (docs only), and the two shell files reviewed below.

---

hardcoded-path-check.sh — new SCAN_ROOT gate (lines 127–151)

Injection surfaces — all new variable expansions are double-quoted throughout:

git -C "$PROJECT_ROOT" rev-parse --show-toplevel >/dev/null 2>&1
hook::normalize_path "$PROJECT_ROOT"
hook::normalize_path "${HOME:-${USERPROFILE:-}}"

PROJECT_ROOT is derived from Claude Code's own CLAUDE_PROJECT_DIR env var or from a prior git rev-parse output — not from untrusted file content. No command injection path exists.

Fail-safe defaults — confirmed correct in all three empty-value cases:

State	SCAN_ROOT	Branch behaviour
PROJECT_ROOT empty	""	repo-path branch inactive (pre-existing)
git -C "$PROJECT_ROOT" fails	""	branch suppressed → false-negative risk, but matches stated design intent
HOME and USERPROFILE both empty	PROJECT_ROOT	branch stays active — "fail toward detection" ✓

Ancestor-suppression logic — the [[ ]] glob pattern is directionally correct:

• "$_pr_norm" == "$_home_norm" → checkout is HOME → suppress ✓
• "$_home_norm" == "$_pr_norm"/* → HOME is under the project root → project is an ancestor of HOME → suppress ✓

hook::normalize_path is a lexical-only transform (backslash→slash, drive-letter case fold) — no external commands, no injection risk. Symlinks are not resolved, which is consistent with how the function is used elsewhere in hook-utils.sh and is a pre-existing behaviour unmodified by this PR.

TOCTOU note (SUGGESTION, low confidence): The new code runs git -C "$PROJECT_ROOT" rev-parse as a second git check after PROJECT_ROOT was already resolved from a prior git call (line 121). If .git were removed between the two invocations, the second check fails and SCAN_ROOT stays empty — the repo-path branch goes silent rather than blocking. This is an intentional fail-safe posture per the comment, and the race is not exploitable in practice (it requires an attacker to be able to delete .git mid-hook-execution, which implies they already own the filesystem).

hardcoded-path-check.test.sh — new test cases

No security concerns. Tests run in isolated $TEST_TMPDIR subdirectories with a proper trap ... EXIT cleanup. New git init calls are scoped inside throwaway directories and don't touch any shared state.

---

Verdict: safe to merge from a security perspective. The change narrows an overly broad detection gate using well-quoted shell code and documented fail-safe defaults. No new injection surfaces, no authorization gaps, no credential exposure.

[claude]

Claude finished @kyle-sexton's task in 2m 47s —— View job

---

PR Review

• Gather context and read changed files
• Review hardcoded-path-check.sh logic
• Review hardcoded-path-check.test.sh coverage
• Review plugin.json changes
• Review CHANGELOG.md
• Post findings

---

Summary

This is a clean, targeted fix. The new SCAN_ROOT gate correctly limits the repo-path detection branch to real non-home git checkouts, and the test suite now has explicit coverage (MUST-FIRE + two stay-quiet repros) for what was previously a zero-coverage path. The overall logic is sound and the fail-toward-detection stance on missing home resolution is exactly right.

A few observations below, all minor.

---

Logic correctness

Gate condition — hardcoded-path-check.sh L140–151

if [[ -n "$_home_norm" && ( "$_pr_norm" == "$_home_norm" || "$_home_norm" == "$_pr_norm"/* ) ]]; then

The two suppress conditions are:

1. "$_pr_norm" == "$_home_norm" → project root equals home ✓ (F1b covers this)
2. "$_home_norm" == "$_pr_norm"/* → home is inside project root → project root is an ancestor of home ✓

Condition 2 is correct but has no dedicated test. An edge case like PROJECT_ROOT=/home HOME=/home/user exercises this path and is the only realistic scenario not represented in the test suite. It's an exotic setup and failing toward detection if the branch suppresses wrongly here would be a false negative, so this isn't blocking — but a single test case would close the gap completely.

Symlinks not resolved — the SCAN_ROOT comparison uses hook::normalize_path (slash-fold only), not hook::physical_path. If HOME is a symlink to the same directory as PROJECT_ROOT, the comparison will be string-unequal and SCAN_ROOT will be armed. Since this fails toward detection (a false positive, not a false negative) it's acceptable, but it's a known gap shared with the scope guard.

Second git rev-parse call — When PROJECT_ROOT is already resolved from CLAUDE_PROJECT_DIR, the new code calls git -C "$PROJECT_ROOT" rev-parse --show-toplevel again to confirm it's a git repo. This is intentional (CLAUDE_PROJECT_DIR can point at a non-git directory) and correct, though it spawns an extra subprocess on every hook invocation. Not a problem in practice.

---

Test coverage

F1a (test line 126) tests the non-git gate: C:\Users\bob doesn't exist on Linux so git rev-parse fails, SCAN_ROOT stays empty, detection is suppressed. The home-equality check is never reached. The comment is accurate ("NON-git home"), but readers looking for "home-equality suppression" won't find it here.

F1b (test line 135) covers the git-checkout-equals-home case.

MUST-FIRE (test line 64) guards against over-suppression. This is the most important addition — the branch previously had zero test coverage.

The gap is the missing "project root is a proper ancestor of home" case (condition 2 above). F1a + F1b + MUST-FIRE together give confidence the common scenarios are solid.

---

plugin.json

The only functional change is the version bump 0.9.1 → 0.9.2. The remaining 200-line diff is a reformatting from 2-space indent to tabs. This is harmless but makes the diff harder to skim and will show up as a blame change on every key in the file. If the project has a preferred JSON style, aligning here avoids future noise; if not, consider whether the reformat is worth it.

---

CHANGELOG

Well-written. Accurately describes the bug, the gate logic, and the upstream split. No issues.

---

Verdict

Approve with minor notes. The fix is correct, the fail-toward-detection invariant is preserved, and the test suite is materially better than before. The ancestor-of-home gap and the symlink edge case are both fail-toward-detection, so they're not blocking. The plugin.json reformat is cosmetic noise.

[claude]

Claude finished @kyle-sexton's task in 3m 33s —— View job

---

PR Review #759

• Gather context
• Read changed files
• Review hardcoded-path-check.sh logic
• Review hardcoded-path-check.test.sh coverage
• Review plugin.json and CHANGELOG.md
• Post findings

---

Summary

The fix is correct and well-reasoned. The SCAN_ROOT gate cleanly decouples the telemetry/relative-path variable (PROJECT_ROOT) from the scanner argument, and the three-condition check (real git checkout ∧ not home ∧ not ancestor-of-home) matches the PR description exactly. No security regressions; shellquoting is sound throughout.

---

Logic Walkthrough

SCAN_ROOT derivation (hardcoded-path-check.sh lines 139–151):

if [[ -n "$PROJECT_ROOT" ]] &&
  git -C "$PROJECT_ROOT" rev-parse --show-toplevel >/dev/null 2>&1; then
  ...
  if [[ -n "$_home_norm" && ( "$_pr_norm" == "$_home_norm" || "$_home_norm" == "$_pr_norm"/* ) ]]; then
    SCAN_ROOT=""   # project dir IS home, or IS an ancestor of home
  else
    SCAN_ROOT="$PROJECT_ROOT"
  fi
fi

All four cases resolve correctly:

Scenario	SCAN_ROOT	Expected
Non-git dir (CLAUDE_PROJECT_DIR is home without a .git)	""	suppress ✓
Git checkout == $HOME (dotfiles-as-home)	""	suppress ✓
Git checkout is an ancestor of $HOME (e.g. root = /home, home = /home/user)	""	suppress ✓
Git checkout is a normal subdirectory below home	"$PROJECT_ROOT"	detect ✓
$HOME/$USERPROFILE both unset	"$PROJECT_ROOT" (fail toward detection)	detect ✓

The "$_home_norm" == "$_pr_norm"/* glob correctly anchors on the path-segment boundary — /* requires a literal / separator, so /homework does not match /home/*. That's the same anchoring pattern used elsewhere in hook-utils.sh (e.g. hook::read_file_path line 206).

Pre-filter interaction (hardcoded-path-patterns.sh line 74): The F1a Windows test still reaches the pre-filter (Users matches), the OS-context and home-path branches are all suppressed or miss, and project_root="" skips the repo-path block — exit 0 as expected.

---

Findings

1. Missing test — ancestor-of-home suppression (minor gap)

The "$_home_norm" == "$_pr_norm"/* branch (project root is an ancestor of $HOME) has no dedicated test. F1b only covers the equal-case. A test for e.g. PROJECT_ROOT=/tmp/anc, HOME=/tmp/anc/user would complete the matrix. The failure mode if the suppression didn't work would be false positives (annoying, not a security regression), but coverage of a novel branch is low.

Fix this →

2. Windows USERPROFILE fallback is CI-untestable (acknowledged limitation)

The ${USERPROFILE:-} path in "$(hook::normalize_path "${HOME:-${USERPROFILE:-}}")" is not exercised by any test because CI runs Linux. F1a uses a Windows-format string but the git check fails for a filesystem reason, not because of the home comparison. This is a known cross-platform testing gap (the test suite notes "runtime-assembled machine paths") — not a blocker, but worth tracking against a future Windows runner.

---

Non-issues

• Double git invocation: git check-ignore runs on $CLAUDE_PROJECT_DIR; the new git rev-parse runs on $PROJECT_ROOT. These are the same variable when CLAUDE_PROJECT_DIR is set, and the second call is gated on PROJECT_ROOT being non-empty. Minor redundancy, not a correctness concern.
• PROJECT_ROOT vs actual git root mismatch: When CLAUDE_PROJECT_DIR is a subdirectory of a repo, SCAN_ROOT is set to CLAUDE_PROJECT_DIR (not the git root). This is the same behavior as before this PR — the branch always searched for PROJECT_ROOT as a literal substring, not the actual toplevel. Existing, pre-existing, not introduced here.
• Comment length: The 12-line block comment is detailed but warranted given the subtlety of the invariant.
• CHANGELOG.md / plugin.json: Correct semver bump (bug fix → patch), clean changelog entry, description unchanged. ✓

---

Verdict

Approve pending the ancestor-of-home test gap (item 1, minor). The core fix is sound, the must-fire regression case guards against over-suppression, and the fail-toward-detection semantics on missing HOME are correct. CHANGELOG and version are clean.

[claude]

Claude finished @kyle-sexton's task in 2m 23s —— View job

---

Security Review — PR #759

• Load tools
• Fetch PR diff
• Read changed shell files
• Analyze for security vulnerabilities
• Post findings

---

No security issues found.

Two of the four changed files are noise (plugin.json — version bump only; CHANGELOG.md — docs only). The security-relevant changes are confined to hardcoded-path-check.sh and hardcoded-path-check.test.sh.

---

hardcoded-path-check.sh — new SCAN_ROOT gate (L139–L151)

Injection surfaces — every new variable expansion touching a command invocation is double-quoted:

git -C "$PROJECT_ROOT" rev-parse --show-toplevel >/dev/null 2>&1
hook::normalize_path "$PROJECT_ROOT"
hook::normalize_path "${HOME:-${USERPROFILE:-}}"

PROJECT_ROOT originates from CLAUDE_PROJECT_DIR (host-process env var) or an earlier git rev-parse stdout — not from untrusted file content. HOME/USERPROFILE are env vars; their values land in _home_norm which is used only inside a [[ ]] string comparison, never in a command position. No injection path.

hook::normalize_path is a pure string transform (backslash→slash, optional drive-letter fold via printf '%s') — no subshell commands that could be injected into.

Fail-safe analysis — three empty-value paths, all verified correct:

Condition	_home_norm	Result
PROJECT_ROOT empty	—	SCAN_ROOT="" — repo-path branch stays inactive (pre-existing)
git -C "$PROJECT_ROOT" rev-parse fails	—	SCAN_ROOT="" — repo-path branch suppressed; documented intent (fail-safe open on git absence)
Both HOME and USERPROFILE unset	""	[[ -n "$_home_norm" ]] is false → else branch → SCAN_ROOT="$PROJECT_ROOT" — branch stays active ("fail toward detection") ✓

The third case is the critical one: a missing home resolution never silently disables the guard.

Gate logic:

if [[ -n "$_home_norm" && ( "$_pr_norm" == "$_home_norm" || "$_home_norm" == "$_pr_norm"/* ) ]]; then

• Condition 1 (_pr_norm == _home_norm): project root equals home → suppress ✓
• Condition 2 (_home_norm == _pr_norm/*): home is inside the project root (project root is an ancestor of home) → suppress ✓ (e.g. PROJECT_ROOT=/home, HOME=/home/user — suppressing /home as a "repo path" is correct)

Env-var manipulation: an attacker who can set HOME to an arbitrary value could influence the gate, but the worst reachable outcome is false-positive suppression (the repo-path branch goes quiet), not detection bypass of the home-path branches (Linux user path, macOS user path, Windows user path) which are independent and unaffected by SCAN_ROOT. The repo-path branch is also the least critical of the detection families — it catches repo-root strings that leak, not raw home directories.

git -C TOCTOU: if .git is deleted between the earlier CLAUDE_PROJECT_DIR assignment and the new git rev-parse check, SCAN_ROOT stays "" and the repo-path branch is silently disabled. This matches the documented fail-safe posture. The race is not exploitable: deleting .git mid-hook-execution requires filesystem write access, which already implies full ownership.

---

hardcoded-path-check.test.sh — new test cases

Test scaffolding is clean: git init calls are scoped to $TEST_TMPDIR subdirectories and cleaned up by the existing trap 'rm -rf "$TEST_TMPDIR"' EXIT. No shared state, no credential exposure, no unsafe invocations.

---

Verdict: safe to merge. The gate narrows overly broad detection using well-quoted shell code, correct fail-toward-detection defaults, and no new command execution surfaces.

[claude]

Claude finished @kyle-sexton's task in 1m 25s —— View job

---

Security Review — PR #759

• Fetch PR diff
• Read changed shell files
• Analyze for security vulnerabilities
• Post findings

---

No security issues found. Two of the four changed files are noise (plugin.json — version bump only; CHANGELOG.md — docs only). The security-relevant changes are confined to hardcoded-path-check.sh and hardcoded-path-check.test.sh.

---

hardcoded-path-check.sh — new SCAN_ROOT gate (L139–L151)

Injection surfaces — every new variable expansion in a command position is double-quoted:

git -C "$PROJECT_ROOT" rev-parse --show-toplevel >/dev/null 2>&1
hook::normalize_path "$PROJECT_ROOT"
hook::normalize_path "${HOME:-${USERPROFILE:-}}"

PROJECT_ROOT originates from CLAUDE_PROJECT_DIR (host-process env var) or an earlier git rev-parse stdout, not from untrusted file content. HOME/USERPROFILE land in _home_norm, which is used only inside a [[ ]] string comparison — never in a command position. hook::normalize_path is a pure string transform (backslash→slash + optional drive-letter case fold via printf '%s') with no exec path. No injection surface introduced.

Fail-safe / fail-toward-detection analysis — three empty-value paths, all correct:

Condition	_home_norm	SCAN_ROOT	Expected
PROJECT_ROOT empty	—	""	repo-path branch inactive (pre-existing)
git -C "$PROJECT_ROOT" rev-parse fails	—	""	branch suppressed; documented intent
Both HOME and USERPROFILE unset	""	"$PROJECT_ROOT" (else branch)	branch armed — "fail toward detection" ✓

The third case is the critical one from a security standpoint: a missing home env var never silently disables the guard.

Gate logic correctness (L146):

if [[ -n "$_home_norm" && ( "$_pr_norm" == "$_home_norm" || "$_home_norm" == "$_pr_norm"/* ) ]]; then

The /* glob anchors on a path-segment boundary — /homework cannot match /home/*. Both suppress conditions are directionally correct and are not bypassable without controlling HOME/USERPROFILE (environment-level access, already beyond this hook's threat model).

Env-var manipulation impact ceiling — an attacker who can set HOME to an arbitrary value could force SCAN_ROOT="", silencing the repo-path branch only. The home-path detection branches (Linux user path, macOS user path, Windows user path) run independently and are entirely unaffected by SCAN_ROOT. The worst reachable outcome is a false-negative on the repo-path branch, not a bypass of the primary home-path detections.

TOCTOU note — if .git is removed between the earlier CLAUDE_PROJECT_DIR resolution and the new git rev-parse check, SCAN_ROOT stays "" and the repo-path branch goes silent. Matches the documented fail-safe posture; the race requires filesystem write access, which already implies full host ownership.

---

hardcoded-path-check.test.sh — new test cases

Test scaffolding is clean: git init calls are scoped to $TEST_TMPDIR subdirectories and cleaned up by the existing trap 'rm -rf "$TEST_TMPDIR"' EXIT. HOME overrides are constrained to those subdirectories. No shared state, no credential exposure, no unsafe invocations introduced.

---

Verdict: safe to merge from a security perspective. The gate narrows overly broad detection using well-quoted shell code, correct fail-toward-detection defaults for missing HOME, and no new command-execution surfaces.

[claude]

Claude finished @kyle-sexton's task in 1m 47s —— View job

---

PR Review #759

• Gather context
• Read changed files and PR diff
• Review logic correctness
• Review test coverage
• Review CLAUDE.md alignment
• Post findings

---

Summary

Three commits, two of which are cleanup-only. The functional fix (5f50c3a) is correct and the two follow-up commits (79733e5, f6a30dd) address feedback from the prior review rounds: plugin.json formatting noise is resolved and issue-number back-references have been removed from comments. One gap from the prior reviews is still open (ancestor-of-home test case); everything else is solid.

---

Commit Breakdown

5f50c3a — core fix: SCAN_ROOT gate. Correct and already analyzed in depth in earlier review rounds.

79733e5 — restores plugin.json to its original 2-space indent (undoes the formatter-introduced tab reformat), drops issue ref from the hook comment. Directly addresses the "plugin.json reformat adds blame noise" note from the prior review. ✓

f6a30dd — drops two surviving #389 / issue back-references from test comments. Rewrites to describe behavior instead. ✓

---

Logic Correctness

Gate at hardcoded-path-check.sh L140–151 is unchanged from the initial commit and is correct across all five scenarios:

Scenario	SCAN_ROOT	Expected
PROJECT_ROOT empty	""	branch inactive ✓
git rev-parse fails (non-git dir)	""	branch suppressed ✓
checkout == $HOME (dotfiles-as-home)	""	suppress ✓
checkout is an ancestor of $HOME (e.g. PROJECT_ROOT=/home, HOME=/home/user)	""	suppress ✓
normal repo below home	"$PROJECT_ROOT"	detect ✓
$HOME/$USERPROFILE both unset	"$PROJECT_ROOT"	detect (fail toward detection) ✓

---

Open Gap — Ancestor-of-Home Test Case

Both prior review rounds flagged that the "$_home_norm" == "$_pr_norm"/* branch (home is inside project root) has no dedicated test. That gap persists in this revision.

The failure mode is a false positive (the repo-path branch stays active when it should suppress), not a security regression. But this is the third review cycle and the branch still has zero coverage for that condition.

Add the test →

---

No Issues With

• F1a / F1b test labels — these are scenario identifiers in assert messages, not tracker references. The cleanup commit correctly removed the #389 and issue references; F1a/F1b are classification labels and are fine.
• Security — no new surfaces. Double-quoting is consistent throughout; hook::normalize_path is a pure string transform. Fail-toward-detection invariant holds on missing home resolution.
• plugin.json — version bump only (0.9.1 → 0.9.2), correct patch bump for a bug fix, formatting matches the original file exactly.
• CHANGELOG.md — accurate and well-written.
• CLAUDE.md alignment — repo-agnostic (no hardcoded paths in plugin source), no PII, semver bumped, comment hygiene clean after the latest commit.

---

Verdict

Approve with the ancestor-of-home test still pending. The fix is correct, the follow-up commits cleanly addressed the prior review feedback, and the fail-toward-detection stance means the missing test is a coverage gap rather than a correctness or security issue. The PR is safe to merge; the test can follow as a small separate commit or on the next bump.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f6a30dd6c1

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 2m 1s —— View job

---

Security Review — PR #759

• Fetch PR diff and context
• Read changed shell files
• Analyze for security vulnerabilities
• Post findings

---

No security issues found. Two files are noise (plugin.json — version bump only; CHANGELOG.md — docs). The security-relevant surface is confined to the new SCAN_ROOT gate in hardcoded-path-check.sh introduced across four commits, with the latest (a1a879b) upgrading the comparison target from PROJECT_ROOT to the actual git toplevel.

---

hardcoded-path-check.sh — SCAN_ROOT gate (L143–L157)

Injection surfaces — all new variable expansions in command positions are double-quoted:

_toplevel="$(git -C "$PROJECT_ROOT" rev-parse --show-toplevel 2>/dev/null)"
_tl_norm="$(hook::normalize_path "$_toplevel")"
_home_norm="$(hook::normalize_path "${HOME:-${USERPROFILE:-}}")"

PROJECT_ROOT originates from CLAUDE_PROJECT_DIR (host env var set by Claude Code) or an earlier git rev-parse stdout — not from untrusted file content. _toplevel is git's own canonical output (the .git-root directory path) and lands in a double-quoted assignment, then passes only into hook::normalize_path (a pure string transform — backslash→slash, optional drive-letter fold via printf '%s'; no exec path) and a [[ ]] comparison. HOME/USERPROFILE are env vars; their derived value _home_norm is used only inside [[ ]] and never in a command position. No injection surface.

[[ ]] glob matching on the RHS — "$_home_norm" == "$_tl_norm"/*: in bash [[ ]], the RHS of == is a glob pattern, so if _tl_norm contained metacharacters ([, ?, *), the pattern could behave unexpectedly. Git toplevel paths on real filesystems are extremely unlikely to contain glob metacharacters, and the failure mode if the pattern mismatch occurred would be the repo-path branch staying active (false positive for the detection hook), not a bypass of any security gate. This is a correctness observation, not an exploitable vulnerability.

Fail-toward-detection analysis — four empty-value paths, all correct:

Condition	_toplevel	SCAN_ROOT	Behaviour
PROJECT_ROOT empty	—	""	repo-path branch inactive (pre-existing)
git rev-parse fails (non-git dir)	""	""	branch suppressed; documented intent
checkout == $HOME; or $HOME is inside checkout	set	""	suppress (F1b and new F1c case) ✓
normal repo below home	set	"$PROJECT_ROOT"	detect ✓
HOME/USERPROFILE both unset	set	"$PROJECT_ROOT" (else branch)	branch armed — "fail toward detection" ✓

The critical invariant — unset home never silently disables the guard — is confirmed correct.

Env-var manipulation ceiling — an attacker who controls HOME can force SCAN_ROOT="", silencing only the repo-path branch. The home-path detection branches (Linux user path, macOS user path, Windows user path) run independently and are unaffected by SCAN_ROOT. The worst reachable outcome is a false negative on the least-critical detection family, not a bypass of primary home-path detection.

git safe.directory on a1a879b's second git rev-parse — if PROJECT_ROOT is owned by a different user, modern git refuses the call and writes to stderr (suppressed by 2>/dev/null); _toplevel is empty; SCAN_ROOT stays "". The repo-path branch goes silent — matches the documented fail-safe posture and is not exploitable.

TOCTOU — if .git is deleted between the PROJECT_ROOT assignment and the new rev-parse, _toplevel is empty and SCAN_ROOT="". Requires filesystem write access to .git mid-hook-execution, which implies full host ownership. Not exploitable in practice.

---

hardcoded-path-check.test.sh — new test scaffolding

New git init calls are scoped to $TEST_TMPDIR subdirectories (F1b uses $TEST_TMPDIR/githome, F1c uses $TEST_TMPDIR/homerepo, etc.) and cleaned up by the existing trap 'rm -rf "$TEST_TMPDIR"' EXIT. HOME overrides are constrained to those throwaway paths. No shared state, no credential exposure, no unsafe invocations introduced.

---

Verdict: safe to merge from a security perspective. The a1a879b change tightens the gate to compare against the git toplevel rather than PROJECT_ROOT, correctly closing the chezmoi/home-is-checkout side-door identified by Codex. Quoting is sound throughout; fail-toward-detection semantics on missing home are preserved; no new command-execution surfaces introduced.

[claude]

Claude finished @kyle-sexton's task in 2m 22s —— View job

---

PR Review #759 — a1a879b

• Gather context and identify latest commit
• Read changed files at HEAD
• Review a1a879b — toplevel capture fix
• Check test coverage (ancestor-of-home gap)
• Post findings

---

What changed in a1a879b

This commit fixes the Codex P2 finding: when $HOME is itself a git checkout (chezmoi-style dotfiles) and CLAUDE_PROJECT_DIR is a subdirectory of home, the old code compared PROJECT_ROOT (the subdir) against $HOME. The subdir is neither equal to home nor an ancestor of home, so the gate passed and SCAN_ROOT was armed — causing every path rooted under the subdir to hard-deny.

The fix captures _toplevel from git -C "$PROJECT_ROOT" rev-parse --show-toplevel and runs the home comparison against that instead. SCAN_ROOT is still set to PROJECT_ROOT (not _toplevel) when the gate passes, because the scanner looks for the project dir as a literal substring, not the repo root.

---

Logic Walkthrough

hardcoded-path-check.sh L144–157:

if [[ -n "$PROJECT_ROOT" ]]; then
  _toplevel="$(git -C "$PROJECT_ROOT" rev-parse --show-toplevel 2>/dev/null)"
  if [[ -n "$_toplevel" ]]; then
    _tl_norm="$(hook::normalize_path "$_toplevel")"
    _tl_norm="${_tl_norm%/}"
    _home_norm="$(hook::normalize_path "${HOME:-${USERPROFILE:-}}")"
    _home_norm="${_home_norm%/}"
    if [[ -n "$_home_norm" && ( "$_tl_norm" == "$_home_norm" || "$_home_norm" == "$_tl_norm"/* ) ]]; then
      SCAN_ROOT="" # enclosing checkout is home or an ancestor of home — suppress
    else
      SCAN_ROOT="$PROJECT_ROOT"
    fi
  fi
fi

Full scenario matrix — all correct:

Scenario	_toplevel	SCAN_ROOT	Expected
PROJECT_ROOT empty	—	""	branch inactive ✓
Non-git dir (git fails)	""	""	branch suppressed ✓
Checkout equals home	$HOME	""	suppress ✓
Checkout is ancestor of home (e.g. /home, HOME=/home/user)	/home	""	suppress ✓
Home is checkout, project is subdir of home (Codex case)	$HOME	""	suppress ✓ (was broken before)
Real non-home checkout, project = repo root	non-home path	$PROJECT_ROOT	detect ✓
Real non-home checkout, project = subdir of repo	non-home toplevel	$PROJECT_ROOT (subdir)	detect ✓
HOME/USERPROFILE both unset	non-home path	$PROJECT_ROOT	detect (fail-toward-detection) ✓

---

Test Coverage

Three new/updated test cases in hardcoded-path-check.test.sh:

F1b update (L153–157): now derives HOME from git ... --show-toplevel output rather than the raw path. Correct and important — on Git Bash/macOS, git may return a canonicalized form (e.g. /private/tmp/...) that differs from what was passed in. Setting HOME to the same form git returns ensures the string comparison in the hook isn't trivially bypassed by a path-form mismatch. The added comment at L142–146 explains this well.

F1c (L164–171): the exact Codex scenario. HOMEREPO is a git checkout, HOME=$HOMEREPO_TL, CLAUDE_PROJECT_DIR=$HOMEREPO/Desktop. The hook discovers _toplevel=$HOMEREPO_TL = $HOME → suppressed → exit 0. This was the zero-coverage blind spot that the Codex finding identified; it now has a direct repro.

SUBREPO MUST-FIRE (L73–79): guards against over-suppression from the toplevel check. CLAUDE_PROJECT_DIR=$SUBREPO/pkg, checkout at $SUBREPO, HOME=$TEST_TMPDIR/elsewhere2. _toplevel=$SUBREPO ≠ home → SCAN_ROOT="$SUBREPO/pkg" → content "at $SUBREPO/pkg/x" fires → exit 2. This verifies that using _toplevel for the home gate doesn't accidentally suppress real subdirectory checkouts.

---

Remaining Gap (pre-existing, not introduced here)

The ancestor-of-home suppression ("$_home_norm" == "$_tl_norm"/*) — where the checkout root is an ancestor of $HOME, e.g. _toplevel=/home, HOME=/home/user — still has no dedicated test. This is the same gap flagged in prior review rounds. It remains fail-toward-detection (false positive) rather than a security regression, and the condition itself is not touched by this commit.

---

No Issues With

• Shell quoting: all new expansions are double-quoted; _tl_norm/_home_norm land only in [[ ]] comparisons, never in command positions.
• SCAN_ROOT = PROJECT_ROOT not _toplevel: correct — the scanner looks for the project dir literal, not the repo root. The SUBREPO test confirms this.
• _toplevel empty on git failure: the inner if [[ -n "$_toplevel" ]] gates correctly; SCAN_ROOT stays "" (fail-safe open, documented intent).
• Fail-toward-detection on missing HOME: unchanged — [[ -n "$_home_norm" ]] is false when both env vars are unset, else-branch fires, SCAN_ROOT="$PROJECT_ROOT".

---

Verdict

Approve. The fix is correct, directly addresses the Codex P2 finding, and the test suite now has explicit coverage for all five meaningful scenarios (non-git, checkout=home, home-is-checkout+subdir, real-non-home-root, real-non-home-subdir). PASS=38 FAIL=0. The ancestor-of-home test gap is pre-existing and not blocking.