Skip to content

refactor(hooks): hoist the repo-relative path helper into lib/hook-utils.sh - #3463

Merged
kyle-sexton merged 9 commits into
mainfrom
claude/coupling-hook-file-rel
Aug 28, 2026
Merged

refactor(hooks): hoist the repo-relative path helper into lib/hook-utils.sh#3463
kyle-sexton merged 9 commits into
mainfrom
claude/coupling-hook-file-rel

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

No linked issue

Summary

A ~9-line FILE_REL / cygpath block was hand-copied into 12 hook payload scripts across 10 plugins, and only four of the twelve copies carried the #1133 absolute-path-leak fix. Second-ranked route-lane finding from the /coupling:reduce pass merged in #3457.

grep -rl '_file_lm=$(cygpath -lm "$FILE"' plugins/ now returns nothing.

Fix

hook::repo_relative_path added to lib/hook-utils.sh — the repo's existing single source of truth for shared hook shell code — implementing the cygpath handling and the basename degrade that eight of the twelve copies lacked. scripts/sync-hook-utils.sh propagates it to all 17 carrying plugins under the existing hook-utils-sync CI lane, so no new script, lane, or registry entry was needed.

The four that already had the degrade are actionlint-check.sh and guardrails' cli-flag-verify.sh, skill-reference-verify.sh and stale-path-verify.sh. An earlier revision of this description said "only one copy" and named only actionlint; that was wrong, and contradicted the CHANGELOG entries this PR ships. Corrected here after an independent verifier measured the twelve copies at origin/main.

All 17 carriers get a version bump and CHANGELOG entry, not just the 10 touched: --check-bump requires it for every carrier of a changed lib.

The finding had behavioral reach, and fixing it exposed a second defect

In typos-format the value feeds the tool invocation, not just telemetry. Its guard was [[ -n "$root" && -n "$FILE_REL" && "$FILE_REL" != "$FILE" ]]. Under the old code a failed strip left FILE_REL == FILE, so the guard fell through to the absolute path. Introducing a basename degrade makes it != FILE — so a naive conversion would have silently pointed typos at a nonexistent path and dropped real findings. The hook now branches on FILE_REL_DEGRADED.

Exactly three of the twelve sites feed a tool rather than telemetry — ruff-format.sh, typos-format.sh, actionlint-check.sh — and all three carry that branch. Independent verification enumerated all twelve and confirmed no fourth tool-feeding site exists.

The helper returns 0 when the path is genuinely repo-relative and 1 (plus HOOK_REPO_RELATIVE_DEGRADED=1) when it degraded, matching the existing hook::repo_root / hook::physical_path idiom — the return status, not the global, is the usable channel under command substitution.

Verification

Fixture: a real git repo plus a symlink to it, so file_path arrives in the symlink spelling while git rev-parse --show-toplevel answers with the physical path, and the prefix strip cannot match.

eol-normalizer telemetry data.file:

before (origin/main)  /tmp/.../demo/linkrepo/x.txt
after                 x.txt

typos-format, file one directory deep, three variants — the middle row is the change with the degrade branch removed, showing the regression the branch prevents:

before   findings=[{"typo":"delibrate",...}]  data.file=/tmp/.../linkrepo/sub/deep.md   advisory mentions typo: 1
naive    findings=[]                          data.file=deep.md                          advisory mentions typo: 0
after    findings=[{"typo":"delibrate",...}]  data.file=deep.md                          advisory mentions typo: 1

An independent verifier reproduced that guard-removal experiment from its own fixture across all three tool-feeding sites, and found actionlint the worst case of the three: with the branch removed it emits no output at all and exits 0, a completely silent findings loss rather than a tool-break message.

guarded guard removed
ruff F401 + F821 reported E902 No such file or directory
typos typo reported argument note.md is not found
actionlint both violations reported no output, exit 0

The new cases are not vacuous: mutating each hook in place makes its suite fail precisely on the new symlinked-root case (ruff FAIL=2, typos FAIL=1, actionlint FAIL=1), and pass unmutated.

Gates: sync-hook-utils.sh --check and --check-bump origin/main, check-changelog-parity.sh --check and --check-bump, validate-plugins.sh, check-purged-em-dashes.sh, shellcheck -x on all changed shell, check-shell-portability.sh, markdownlint-cli2 on 17 CHANGELOGs, typos, editorconfig-checker — all pass. All 17 vendored copies independently confirmed to share one sha256 with lib/hook-utils.sh, and the set of plugins that source it is exactly the set that carries a copy (17 = 17, nothing on either side of the comm).

Suites: affected-tests.sh --run 122/122 shell suites pass after the merge with main widened the diff, 0 FAIL. The 7 Python suites it names as NOT RUN all pass. lib/hook-utils.test.sh PASS=221 FAIL=0, with all 12 repo_relative_path cases running in both the cyg and nocyg arms. git diff -- '*.test.sh' | grep '^-[^-]' is empty: no test line was deleted or loosened anywhere in the diff.

Stated limits, not glossed: powershell-format's suite skips its behavioral half (no PSScriptAnalyzer), confirmed a genuine pre-existing environment gate untouched by this diff. go-format's suite self-skipped on the authoring machine for want of goimports, but ran in full during verification on a machine that has it: PASS=54 FAIL=0, including the data.file repo-relative assertion. One portability-ok: annotation was added to a new hook-utils.test.sh UNC fixture, because check-shell-portability.sh reads the literal \s/\b inside '\\srv\share\b.md' as GNU regex escapes; verified a genuine false positive by reproducing the report on a minimal fixture carrying only that line. No test was weakened or skipped to get green.

Merge with main

Three CHANGELOG version collisions were resolved by keeping main's shipped entry verbatim and renumbering this PR's above it (claude-ops 0.38.18, instruction-placement 0.11.12, rate-limit-guard 0.7.20), with each manifest updated to match. Verified as a clean union: exactly six files differ from both parents, and they are precisely those three CHANGELOGs and their three manifests, so nothing else could have been silently resolved. Each CHANGELOG's diff against origin/main is a pure insertion at the top.

Known gap, deliberately not widened into here

Verification found two further hand-rolled copies of this computation in guardrailssecret-pattern-detection.sh and hardcoded-path-check.sh — that the completeness grep missed because they use a different spelling (CLAUDE_PROJECT_DIR plus backslash folding, no cygpath). Their redaction case lacks the UNC arm, so secret-pattern-detection.sh still emits a full \\srv\share\... path in data.file when the root is unset. Pre-existing, not a regression from this change, and hardcoded-path-check.sh is unreachable (its scope guard guarantees the root is set). Filed as a follow-up rather than expanding a 68-file PR.

Related

Follow-up to #3457. Refs ADR 0019 (docs/adr/0019-share-code-across-plugins-by-vendoring-with-a-sync-gate.md), issue #1133.


Generated by Claude Code

claude added 5 commits August 28, 2026 07:11
A nine-line FILE_REL / cygpath block was hand-copied into 12 hook payload
scripts across 10 plugins. Only actionlint's copy carried the #1133 fix: when
the repo-root prefix strip does not match (mount/symlink mismatch, or a cygpath
that answers for one side and not the other), the "repo-relative" value is still
the whole absolute path, which embeds the developer's username and breaks the
telemetry schema's data.file contract. Guardrails' three copies had grown an
equivalent redaction independently, under a different variable name. The other
eight leaked.

lib/hook-utils.sh now owns it as hook::repo_relative_path, propagated to all 17
carrying plugins by scripts/sync-hook-utils.sh. It answers on the same three
channels the neighboring hook::repo_root and hook::physical_path use: stdout, a
return code, and HOOK_REPO_RELATIVE_DEGRADED.

That status matters at one call site. typos-format feeds FILE_REL to the tool
itself (TYPOS_ARG), not only to telemetry, and typos runs from the repo root, so
a redacted bare basename there names a different file or none at all. That hook
now reads the return status and keeps the absolute path when the helper
degraded. Measured on a symlinked-repo fixture with a file one directory deep:
before, data.file carried the absolute path and the typo was reported; with the
helper but no status branch, data.file was redacted and the scan went silent;
with both, data.file is "deep.md" and the typo is still reported.

The helper also redacts UNC paths, which the guardrails case patterns did not
match, and drops actionlint's basename subprocess.

Every carrying plugin's version and CHANGELOG are bumped: the plugin version is
the update cache key, so an unbumped carrier never delivers the lib change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QkMx972GYEQ3vCb98pLw8m
…ions

Review found that ruff-format feeds FILE_REL to Ruff itself, behind the same
guard typos-format had, so the new basename redaction pointed Ruff at a
nonexistent path and real findings vanished from an advisory hook. actionlint
consumes FILE_REL the same way, and has since it grew the degrade on main.

Audited all 12 converted sites for tool-argument consumption: 3 feed a tool
(actionlint, ruff-format, typos-format), 9 are telemetry-only. All three now
branch on FILE_REL_DEGRADED and fall back to the absolute path.

Adds symlinked-root regression cases to the actionlint, ruff-format, and
typos-format suites; each fails without its guard. The lib cases now drive both
helper arms in child shells with a curated PATH (empty, or a stub cygpath), so
the Windows normalization arm is exercised on every host rather than skipped.

Also: an empty repo root no longer shaves the leading slash and returns a
near-absolute path with a success status, and the contract block documents the
set -e hazard of taking the status from a bare assignment.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QkMx972GYEQ3vCb98pLw8m
…file-rel

# Conflicts:
#	plugins/actionlint/CHANGELOG.md
#	plugins/autonomy/CHANGELOG.md
#	plugins/claude-ops/CHANGELOG.md
#	plugins/context-guard/CHANGELOG.md
#	plugins/go-format/CHANGELOG.md
#	plugins/guardrails/CHANGELOG.md
#	plugins/powershell-format/CHANGELOG.md
#	plugins/rate-limit-guard/CHANGELOG.md
#	plugins/source-control/CHANGELOG.md
#	plugins/typos-format/CHANGELOG.md
…ases

The child-shell probe is invoked as "$BASH" so it resolves off the deliberately
near-empty PATH; shellcheck cannot then tell the quoted argument is a bash
script and reads its child positional parameters as unexpanded by mistake, so
the site carries a reasoned SC2016 waiver. The symlink fixture's intentional
misspelling gets the repo's spellchecker:disable-line directive, the same
convention the sibling fixtures in that file already use.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QkMx972GYEQ3vCb98pLw8m
…file-rel

# Conflicts:
#	plugins/claude-ops/CHANGELOG.md
#	plugins/desktop-notification/CHANGELOG.md
#	plugins/rate-limit-guard/CHANGELOG.md
#	plugins/source-control/CHANGELOG.md
@github-actions github-actions Bot deleted a comment from claude Bot Aug 28, 2026
@github-actions github-actions Bot deleted a comment from claude Bot Aug 28, 2026
@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

claude added 3 commits August 28, 2026 08:57
…not flag

The two new lib fixtures spelled their absolute paths as C:/Users/dev/... and
/home/dev/..., which is exactly the shape the hygiene lane's
machine-specific-paths detector hunts. They are synthetic, but the detector
cannot tell, and it is one of the ungated whole-repo scanners, so it blocked
the merge.

The cases only need a drive-letter absolute path and a POSIX absolute path, so
they now use C:/proj/app/a/b.md and /srv/proj/repo/a.txt. Confirmed against the
detector this repo actually pins (ci-workflows @ c265418): its five bodies key
on Users, /home/, and a literal `repos` checkout parent, none of which the new
spellings contain. Running that exact script over the tree is clean, and
restoring the old spellings reproduces both failures.

Assertions are unchanged: same 12 cases, PASS=221, and disabling the empty-root
guard still fails the empty-root case on the new path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QkMx972GYEQ3vCb98pLw8m
# Conflicts:
#	plugins/claude-ops/CHANGELOG.md
#	plugins/instruction-placement/CHANGELOG.md
#	plugins/rate-limit-guard/CHANGELOG.md
@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

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


Security review complete

  • Gather context (PR diff, changed files, skill instructions)
  • Run skip-gate check — PR is open, has security-relevant surface (path handling feeding telemetry and tool invocations), no prior successful review at this head applies
  • Review shared hook::repo_relative_path helper in lib/hook-utils.sh
  • Review each of the 12 rewritten hook call sites for correct guard usage
  • Review scripts/sync-hook-utils.sh propagation for tampering risk
  • Check for any GitHub Actions / permissions changes
  • Post findings

No security issues found.

What I checked:

  • hook::repo_relative_path (lib/hook-utils.sh#L613-L671) — the redaction-on-failure logic is sound: any path the prefix strip can't anchor to the repo root (POSIX-absolute, drive-letter, or UNC) degrades to its basename rather than leaking an absolute path (which would embed the developer's username, the original actionlint: membership guard silently skips lint on 8.3 short paths + cd-failure reads as clean + abs-path leak in telemetry #1133 leak). The empty-root guard is correct — without it, ${file#""/} would only shave a leading slash and hand back an absolute-looking path with a success status, defeating the redaction. Verified against the case-pattern logic by hand; the accompanying test cases in lib/hook-utils.test.sh (mount-form vs. drive-letter, empty root, root-as-file) cover the same edges.

  • Tool-feeding call sites (typos-format, ruff-format, actionlint-check) — this is the risk class the PR body itself calls out: a degraded (redacted) FILE_REL is a bare basename, and resolving that against the repo root as a tool argument would silently scan the wrong file (or none), dropping real findings rather than leaking anything. All three sites correctly branch on FILE_REL_DEGRADED/return-status before using FILE_REL as the tool argument, falling back to the absolute $FILE when degraded. actionlint/typos/ruff invocations pass the path after a literal --, so no argument-injection risk from filenames starting with -.

  • set -e interaction — none of the rewritten hooks use set -e (they use set -uo pipefail), so the helper's return 1 on degrade can't trigger an unintended early exit in a bare FILE_REL=$(hook::repo_relative_path ...) assignment. The two sites that use set -e-sensitive patterns already guard with || FILE_REL_DEGRADED=1.

  • Telemetry-only call sites (bash-format, biome-format, eol-normalizer, go-format, markdown-format, powershell-format, guardrails/*) — these only feed data.file, never a tool invocation, so the degrade-vs-relative distinction doesn't matter there; a redacted basename in telemetry is the intended, safe outcome.

  • scripts/sync-hook-utils.sh — unchanged by this PR (git diff origin/main...HEAD -- scripts/sync-hook-utils.sh is empty); the existing sync/--check-bump gate is reused as claimed, no new propagation surface.

  • No .github/workflows/** changes in this PR — nothing to review on the Actions/permissions front.

This PR is itself a security fix (closing the absolute-path/username leak on 16 of 17 carriers that lacked the degrade) applied uniformly through the existing vendoring mechanism, and it correctly identifies and fixes the follow-on defect (the typos-format guard) that a naive port of the fix would have introduced.

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

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


Code review complete

  • Gather context (PR diff, CLAUDE.md/AGENTS.md, REVIEW.md, hook-utils changes)
  • Understand the request (invoked /review:code-review per repo custom instructions)
  • Review lib/hook-utils.sh new helper + tests
  • Review carrier hook scripts (actionlint, bash-format, biome-format, eol-normalizer, go-format, guardrails, markdown-format, powershell-format, ruff-format, typos-format)
  • Review sync/version-bump/changelog mechanics
  • Post inline findings (if any) and summary

No blocking findings. This is a clean, well-verified refactor:

  • hook::repo_relative_path in lib/hook-utils.sh (added at L613) correctly consolidates the cygpath-normalization + basename-degrade logic, and matches the existing hook::repo_root/hook::physical_path idiom of using the return status (not just the global HOOK_REPO_RELATIVE_DEGRADED) as the channel readable under command substitution.
  • I checked every carrier that calls the helper directly (actionlint-check.sh, bash-format.sh, biome-format.sh, eol-normalizer.sh, go-format.sh, markdown-format.sh, powershell-format.sh, ruff-format.sh, typos-format.sh, and guardrails' cli-flag-verify.sh/skill-reference-verify.sh/stale-path-verify.sh). The three hooks that feed the value into an actual tool invocation (actionlint-check.sh, ruff-format.sh, typos-format.sh) all correctly branch on FILE_REL_DEGRADED before using FILE_REL as the lint target — this is exactly the regression the PR body calls out, and it's fixed consistently everywhere it applies. The telemetry-only carriers correctly rely on the helper's built-in basename redaction without needing the degraded flag.
  • Confirmed none of the callers use set -e, so the bare FILE_REL=$(hook::repo_relative_path ...) assignments (without || :) in the telemetry-only carriers are safe — the hazard the function's own doc comment warns about doesn't apply here.
  • Verified all 17 plugins/*/hooks/hook-utils.sh copies are byte-identical to lib/hook-utils.sh (sync integrity holds), and spot-checked several CHANGELOG entries / plugin.json version bumps — consistent patch bumps with accurate descriptions, including for carriers with no functional hook change (e.g. autonomy, correctly noting "this plugin's hooks do not call it").
  • New test coverage (hook-utils.test.sh, actionlint-check.test.sh, ruff-format.test.sh, typos-format.test.sh) exercises exactly the symlinked-repo-root scenario described in the PR body, both the "real finding still surfaces" and "telemetry stays redacted" assertions, on both the cygpath and non-cygpath arms.
  • Security-relevant aspects (absolute-path/username leak in telemetry) are pre-existing behavior from actionlint: membership guard silently skips lint on 8.3 short paths + cd-failure reads as clean + abs-path leak in telemetry #1133 being consolidated, not newly introduced, and this repo has a dedicated claude-security-review lane, so I've scoped that out per REVIEW.md.

Nothing here rises to a reportable finding under this lane's high-signal bar.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

Last security-reviewed head: cffec15671caa8503ccba45d1a4d10e5ae9ad39c. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

# Conflicts:
#	plugins/claude-ops/CHANGELOG.md
@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 57s —— View job


Security review — skip gate hit

  • Gather context (PR diff since last review, custom instructions)
  • Check skip gate
  • Post result

Skip gate: this head already has a successful security review that still applies.

The last completed security review covered 1747fb69537077c2e828422c66c385ace5729b4 (comment above, "Last security-reviewed head"). The only commits added since then, up to the current head cffec15671caa8503ccba45d1a4d10e5ae9ad39c, are:

git diff --stat 1747fb69537077c2e828422c66c385ace5729b4 cffec15671caa8503ccba45d1a4d10e5ae9ad39c touches only docs, skill markdown, and CHANGELOGs for unrelated plugins (ai-slop, code-tidying, docs-hygiene, firecrawl, knowledge, playbooks, provenance, claude-ops skill doc) — none of it overlaps this PR's actual changed-file set (lib/hook-utils.sh, the 12 hook payload scripts, or any carrier's hooks/hook-utils.sh/CHANGELOG.md/plugin.json). No new security-relevant surface was introduced since the prior review.

Per the skip-gate rule, posting nothing further beyond this note.

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 59s —— View job


Code review complete

  • Gather context (PR diff, changed files, /review:code-review skill instructions)
  • Check skip gate: does the current head still match a prior successful review from this lane?
  • Confirm the diff since that review is unchanged
  • Post findings

Skip gate: this lane's prior review still applies. This run was triggered by a synchronize (push of cffec15), but the only commit added since the last successful review of this lane (at 1747fb6, see the review comment above) is cffec15, a merge of origin/main into this branch. That merge pulled in unrelated main-line changes (ai-slop, code-tidying, docs-hygiene, firecrawl, knowledge, playbooks, provenance, and docs) and resolved one conflict in plugins/claude-ops/CHANGELOG.md.

I confirmed:

  • git diff origin/main...HEAD --stat is still exactly 68 files changed, 1653 insertions(+), 229 deletions(-) — identical to the PR body's stated diff size, so the merge introduced no new content into this PR's own diff.
  • The claude-ops CHANGELOG/manifest conflict resolution is clean: no leftover conflict markers, 0.38.19 entry correctly documents that this plugin's hooks don't call the new helper but the vendored copy is bumped for sync parity — consistent with the PR's stated "all 17 carriers get a version bump" policy. (plugins/claude-ops/CHANGELOG.md#L6)

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), there's nothing new to review — the prior code-review pass on this PR (posted above, no blocking findings after reviewing hook::repo_relative_path, all rewritten carrier hooks, and the sync/version-bump mechanics) still covers the current head. No new inline comments filed.

@kyle-sexton
kyle-sexton merged commit af67243 into main Aug 28, 2026
60 checks passed
@kyle-sexton
kyle-sexton deleted the claude/coupling-hook-file-rel branch August 28, 2026 15:30
kyle-sexton added a commit that referenced this pull request Aug 28, 2026
#3463 certified its hoist complete with a grep scoped to the cygpath
spelling, so two hand-rolled copies of the same computation survived in
guardrails in a different spelling. secret-pattern-detection.sh's
redaction tested only the POSIX-absolute and drive-letter spellings, never
UNC, and this guard deliberately scans on when no project dir is set, so
a \\server\share\file path matched neither arm and the whole share path,
server name included, landed in the telemetry envelope's data.file.

Both hooks now call hook::repo_relative_path, which carries the UNC arm.
secret-pattern-detection.sh pairs it with hook::repo_root so a file with
no project dir is reported relative to its own checkout rather than
collapsing to a basename. hardcoded-path-check.sh's scope guard made the
leaking shape unreachable there, so its emitted value is unchanged and
the copy is removed only so a third divergent one cannot reappear. Both
computations move inside emit_tel, behind the telemetry gate, so an
unwired run pays nothing.

A trailing-separator trim the copies carried was dropped in the first
revision and restored after review: the helper strips "$root/", so a root
ending in a separator forms the prefix /repo// and degrades every
in-project file to its basename. Trimmed at these two call sites rather
than in the shared helper, since every other caller passes
git rev-parse --show-toplevel, which never returns one.

All 17 selected suites pass; secret-pattern-detection 65/0 and
hardcoded-path-check 100/0, each demonstrated failing first against the
unfixed hooks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants