fix(guardrails): hoist the Shared defang out of the per-candidate loop by kyle-sexton · Pull Request #1840 · melodic-software/claude-code-plugins

kyle-sexton · 2026-07-31T01:51:08Z

Summary

hpp::scan_text's macOS block defanged Shared tokens inside a per-candidate while read loop,
spawning a sed and a grep for every candidate line. The loop's only escape was the trailing
head -3, and that short-circuit fires when candidates survive the defang. On a block where
every candidate is a legitimate Users/Shared reference, none survives, nothing is ever written,
head -3 never closes the pipe, and the loop runs to completion.

So the guard was slowest on precisely the innocent content the exclusion exists to serve, and
fastest on violations — the wrong way round for something with a hook timeout. A PreToolUse guard
killed at its timeout fails open, which makes this a correctness bug, not a speed one. This
plugin has been bitten by that before (#1345).

The defang now runs once over the whole candidate block:

One sed over the candidate block. sed is line-oriented in this pipeline — no N/H multiline
commands, and $ anchors per line in both shapes — so hoisting cannot change any individual
line's result.
One grep -nE over the defanged block yields the block-relative indices of the survivors; awk
then selects those lines from the original block by NR. The reported entry therefore still
carries its original line number and original un-defanged text, never the defanged copy.
A block containing no Shared token at all skips the pipeline entirely via a bash-builtin
substring test. The defang is a provable no-op there, so the common case costs nothing.

grep -E remains the sole matcher and awk does no regex work, so no second regex dialect enters
and the shared HPP_* bodies stay the single source of truth.

The survivor re-test also strips grep -n's <n>: line-number prefix before matching. Hoisting
made that necessary and it is easy to miss: the re-test runs over the numbered candidate lines,
so a violation at column 0 arrives as <n>:/Users/… and can no longer satisfy the left
boundary's ^ alternative. It matched anyway only because that class also accepts : — a member
added for yaml/docker value position, which owes this pipeline nothing. Narrowing the class for its
own stated purpose would therefore have silently dropped a violation the first pass had already
flagged. Verified by rerunning the pipeline with : removed from the class: the column-0 survivor
set goes from [2] to [] unstripped, and stays [2] stripped. The strip is one more expression on
the sed the defang already runs, so it adds no process, and a column-0 case now pins it.

Detection semantics are otherwise unchanged. Only the hoisting was ported — this repo's
_posix_boundary and its [^A-Za-z0-9._-] defang boundary class are untouched.

One adjacent fail-open fix: the candidate assignment gains an explicit || true. Its trailing
grep -v exits non-zero whenever nothing survives the Windows exclusion (the common clean case),
and this library is sourced by commit-time hooks whose shell options it does not control — aborting
there under set -e would fail open in the same way.

Measured

A matched pair: one machine, one harness, the same two corpora driven through the hook, with
only lib/path-detection/hardcoded-path-patterns.sh swapped between the sides. Spawn counts come
from grep/sed shims on PATH; wall clock is EPOCHREALTIME around the hook invocation with the
payload precomputed outside the timed region.

Corpus — 100 vs 400 Shared-only lines	Per-candidate loop	Hoisted
`grep`/`sed` spawns at 100	210 (100 `sed` + 110 `grep`)	12
`grep`/`sed` spawns at 400	proportional	12
Wall clock at 100	22s (median of 3: 21.2 / 22.0 / 23.5)	11s (median of 5, range 9.5–13.9)
Wall clock at 400	288s	10s (median of 5, range 5.0–15.2)

Spawn count is the exact figure; wall clock is its consequence. The per-candidate shape grew 13x
for a 4x input increase — super-linear, because fork pressure compounds — while the hoisted shape
is flat and its spread is machine noise. A control corpus of the same size carrying no Shared
token cost 1.9–4.0s on both sides, which places the delta in the defang rather than in payload
size.

The figures in the issue (108s per-line against 0.92s hoisted) are #1792's own measurement on a
different machine, not this pair.

Test plan

Regression cases in hardcoded-path-check.test.sh, plus the existing suite:

Bounded, not per-candidate — the headline pin. It counts subprocesses, not seconds.
grep/sed shims on PATH tally every spawn the hook makes, and the tally must not move when
the input quadruples. A companion assertion fails if the shims never fire, so the equality cannot
pass vacuously.

This replaces a wall-clock ratio assertion, and the reason is the most reviewer-relevant fact in
the PR: the timing assertion failed on unchanged code. Repeats of the identical 400-line
corpus measured 5.0s and 15.2s, so the noise floor was wider than the 4x signal the ratio existed
to detect, and the gate reported FAIL: Shared defang scales with candidate count: 4s at 100 lines, 18s at 400 on a green branch. Widening the tolerance would have left a gate that cannot
discriminate the bug it guards. A count is exact, load-invariant, and pins the property the fix
actually establishes — a constant subprocess count.
Shared-padded block — four Shared-only lines then a real user path at line 5, so the violation
sits beyond the first three candidates. Asserts the reported entry carries the original file
line number (5:cd …) and that the defanged spelling never appears in output. This is exactly
where a block-relative index would leak through in place of the file-relative one.
Violation at column 0 inside a Shared block — pins the prefix strip described above. It passes
both with and without the strip today, which is the point: it guards the coupling rather than a
live defect, and it fails the moment : leaves the boundary class if the strip is ever reverted.
All candidates excluded as Windows paths — the candidate block is empty after the -v stage,
which under pipefail reports failure. Asserts exit 0 and silence, distinguishing a clean pass
from a silent abort.
Existing Shared cases (bare Shared at EOL, Shared + user path on one line, SharedStuff,
trailing punctuation, quoted forms) all still pass, pinning that the exclusion stays match-level.

Results on this branch:

ok: spawn shims active (12 grep/sed spawns at 100 lines)
ok: Shared defang is bounded, not per-candidate (12 spawns at 100 lines, 12 at 400)
PASS=81 FAIL=0

Version bumped 0.18.4 → 0.18.5 with a matching ## [0.18.5] CHANGELOG entry. (The branch
originally claimed 0.18.4; #1821 released that version on main while this was open, so it was
renumbered when this branch rebased rather than co-owning a released version.)

Gates, all run from the worktree root after the rebase:

Gate	Result
`bash plugins/guardrails/hooks/hardcoded-path-check.test.sh`	PASS=81 FAIL=0
`shellcheck` (lib + test)	clean
`shfmt -d` (lib + test)	clean
`scripts/check-shell-portability.sh origin/main`	PASS — 2 files, no unexcused GNU-only constructs
`scripts/check-changed-skills.sh origin/main`	PASS — no changed skills to gate
`scripts/check-changelog-parity.sh --check`	PASS
`scripts/check-changelog-parity.sh --check-bump origin/main`	PASS
`scripts/check-changelog-parity.sh --check-order`	PASS — 71 changelogs, newest-first, no duplicates
`scripts/check-cross-plugin-source-drift.sh --check`	PASS — no unregistered or drifted clusters
`scripts/check-silent-skips.sh`	PASS
`scripts/validate-plugins.sh`	PASS — manifests + catalog
`markdownlint-cli2 "plugins/guardrails/*/.md"`	PASS — 0 issues, 3 files

Fixes guardrails: per-line Shared defang blows the hook timeout on Shared-heavy content (fails open) #1792
Refs fix(guardrails): machine-path right boundary — segment class, not trailing separator #1095 — the match-level Shared carve-out this reports on
Refs guardrails: PreToolUse guards fail open — killed at their declared timeout on 86% of runs #1345 — prior incident: guards killed at their timeout
Refs melodic-software/medley#1685 — the same block-hoisted shape over the same
machine-path-patterns.sh bodies. Porting it here reconverges the two drivers rather than leaving
them forked. That PR also carries a derived _seg_end and a :- boundary addition which are
not included here — those are separate semantic changes belonging to their own issues.

Overlap with concurrent guardrails work

Sibling lanes are working #1814/#1811/#1810 (git wrapper argv parsing) on other branches. Different
code path — those touch the commit-guard argv helpers, this touches the path-detection driver — so
no functional overlap is expected. Both land in the same plugin, so CHANGELOG.md and
plugin.json's version are the likely textual conflict points; whichever merges second rebases the
version bump. Rebased onto current origin/main.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 920a21f7c6

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

The macOS block defanged Shared tokens inside a per-candidate `while read` loop, spawning a sed and a grep per line. The loop's only escape was the trailing `head -3`, which fires when candidates SURVIVE the defang — so a block whose candidates are all legitimate Users/Shared references never short-circuited and ran to completion. The guard was slowest on exactly the innocent content the exclusion exists to serve, and a guard killed at its hook timeout fails OPEN. The defang now runs once over the whole candidate block. sed is line-oriented here (no N/H commands, and `$` anchors per line in both shapes), so hoisting cannot change any individual line's result. A grep -nE over the defanged block gives the block-relative survivor indices, and awk selects those lines from the ORIGINAL block by NR, so the reported entry keeps its original line number and original un-defanged text. grep -E stays the sole matcher; awk does no regex work, so the shared HPP_* bodies remain the single source of truth. A block with no Shared token skips the pipeline via a bash-builtin substring test, where the defang is a provable no-op. The survivor re-test strips grep -n's `<n>:` line-number prefix before matching. Hoisting made that necessary: the re-test runs over the NUMBERED candidate lines, so a violation at column 0 arrives as `<n>:/Users/…` and can no longer satisfy the left boundary's `^` alternative. It matched anyway only because that class also accepts ":", a member added for yaml/docker value position which owes this pipeline nothing — narrowing the class for its own stated purpose would have silently dropped a violation the first pass had already flagged. Rerunning the pipeline with ":" removed from the class takes the column-0 survivor set from [2] to [] unstripped, and leaves it [2] stripped. The strip is one more expression on the sed the defang already runs, so it adds no process, and a column-0 case pins it. The candidate assignment gains an explicit `|| true`: its trailing grep -v exits non-zero whenever nothing survives the Windows exclusion, and this library is sourced by commit-time hooks whose shell options it does not control. Aborting there under `set -e` would fail open too. Subprocess count is now constant rather than proportional to the candidate count, and the regression case pins that count directly rather than timing the run: grep/sed shims on PATH tally every spawn the hook makes, and the tally must not move when the input quadruples. Measured through the hook on Git Bash over the same corpora, swapping only the pattern lib: 210 spawns at 100 Shared-only lines per-line (100 sed + 110 grep) against 12 at either size hoisted, with wall clock rising 22s to 288s across a 4x input increase against a flat ~10s. A wall-clock ratio could not carry that pin. Elapsed time here is dominated by process-spawn latency, and repeats of the identical 400-line corpus measured 5.0s and 15.2s — a spread wider than the 4x signal the ratio existed to detect, which made the timing assertion report a scaling failure on unchanged code. Widening its tolerance would have left a gate that cannot discriminate the bug it guards; a count is exact and load-invariant. A companion assertion fails if the shims never fire, so the equality cannot pass vacuously. Fixes #1792 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW

claude · 2026-07-31T04:34:23Z

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

Security review

Gather context (PR diff, changed files)
Review hardcoded-path-patterns.sh changes for injection / shell-safety issues
Review test file changes
Review plugin.json / CHANGELOG.md changes
Post findings

Scope reviewed: the 4 files changed in this PR (hardcoded-path-patterns.sh, hardcoded-path-check.test.sh, plugin.json, CHANGELOG.md), diffed against origin/main. plugin.json/CHANGELOG.md are a version bump and prose only — no security surface.

Findings: none

This is a shell-guard hook (hpp::scan_text), a defensive control, not an execution path for untrusted input:

No injection surface. $content (the scanned command text — the closest thing to "untrusted input" here) is only ever piped to grep/sed/awk via printf '%s' "$content" | ... (stdin), never interpolated into a shell command line or passed to eval. The one place a shell variable is threaded into a tool invocation is awk -v keep="$_keep" '...' (hardcoded-path-patterns.sh#L202), which uses -v — the safe way to pass a shell string into awk without it being re-parsed as code.
_shared/_posix_boundary/HPP_MACOS_USER_BODY are fixed literals, not attacker-influenced, and _posix_boundary/HPP_MACOS_USER_BODY are untouched by this PR (confirmed via diff).
No fail-open regression. The new || true on the candidate assignment (hardcoded-path-patterns.sh#L175-L176) only absorbs the expected non-zero exit from grep -v when the Windows exclusion consumes every candidate (the clean case) — it doesn't swallow errors from the actual match logic, and downstream violations accumulation is otherwise unchanged. This is the correct direction for a set -e-hostile sourcing context (commit-time hooks), consistent with the PR's stated goal.
The block-relative index re-mapping is sound. Traced the two-pass grep -n → strip-prefix → defang → re-grep -n → cut -d: -f1 → awk NR selection (L175-L206) by hand against both the new column-0 regression test and a few adversarial line shapes (embedded colons, digit-leading original text, multiple Shared occurrences per line). The sed 's|^[0-9]*:||' strip is unambiguous — the injected line-number prefix is a pure digit run immediately followed by :, so it can't be confused with digits that are part of the original text. No case found where a genuine violation is silently dropped or where a benign line is misreported as a different (wrong) source line.
This refactor actually removes a latent fragility rather than adding one: pre-PR, the per-candidate retest matched a column-0 violation only by accident, via the left-boundary class also accepting : (intended for key: /path yaml/docker syntax) once grep -n's own prefix was prepended. This PR's explicit prefix-strip decouples the retest from that coincidence, which is a hardening, not a regression.

Non-issue noted for completeness (not reported as a finding)

hardcoded-path-check.test.sh generates a shim script via heredoc with exec '$_real' "\$@" (hardcoded-path-check.test.sh#L306-L310), where $_real is interpolated inside single quotes without escaping embedded quotes. In isolation that's a quoting fragility, but _real comes from command -v grep/command -v sed in the CI/test environment — not attacker-controlled input — so there's no realistic path to exploit it here. Flagging only for completeness per the "unsafe handling" hunt category; not a vulnerability in this PR's threat model, and out of scope for a fix.

No CRITICAL, IMPORTANT, or SUGGESTION-severity security findings for this PR.

claude · 2026-07-31T04:36:31Z

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

Code review complete ✅

Gather context (PR diff, changed files, CLAUDE.md)
Review hardcoded-path-patterns.sh hoisting logic
Review hardcoded-path-check.test.sh new tests (incl. Codex's timing-flakiness note)
Review plugin.json / CHANGELOG.md changes
Post findings

No blocking issues found. This is a correctness fix (fail-open under hook timeout) with strong test coverage, and I traced the actual regex/pipeline mechanics rather than taking the PR description at face value. Details below, scoped to correctness/architecture/tests/maintainability per this lane's remit (security is the dedicated security lane's scope).

Core logic (`hardcoded-path-patterns.sh`)

Traced the hoisted pipeline at L167-L208 line by line against the per-line original (git show HEAD diff):

Block-relative → file-relative index mapping is correct. sed never drops or adds lines (substitution only), so the second grep -nE's NR after the strip corresponds 1:1 with awk's NR over the original $_cands stream. Verified against the "Shared-padded block" test (L252-L266), which pins exactly this.
The s/^[0-9]*:/ prefix strip (L199-L200) is correct and not ambiguous: since grep -n's own N: separator is the first colon in the stream, [0-9]* can never over-consume into content-embedded digits (a content byte immediately after the true prefix's colon halts the digit run before it starts). I confirmed by hand-tracing a 3:45:something case.
Worth noting for context, not as a defect: the strip is currently redundant with existing behavior — the boundary class _posix_boundary already accepts : as a left-boundary character, so an unstripped N:/Users/… candidate would already satisfy the boundary via the : alternative, not just ^. The commit message is explicit about this (decoupling from the : member's unrelated purpose — yaml/docker value position — so a future narrowing of that class doesn't silently reintroduce this bug). Cheap (one more sed expression, no extra process) and it's pinned by the column-0 test, so this is a reasonable defensive addition rather than premature complexity.
_cands=... || true (L175-L176) correctly prevents a set -e abort (fail-open) when the Windows exclusion drops every candidate; covered by the "all Windows-excluded" test.
The $_cands != *"$_shared"* fast path is a sound no-op proof: when the literal /Users/Shared substring is absent from the whole block, no per-candidate defang could possibly change any match.

Codex's P2 finding (wall-clock flakiness)

Already resolved in this revision. The single commit on this branch (30e7842) replaces timing-based assertions with a PATH-shimmed subprocess-count pin (L299-L353): SPAWNS_LARGE == SPAWNS_SMALL after quadrupling the corpus, with a companion assertion that the shims actually fired (guards against a vacuous pass). No SECONDS/wall-clock assertion remains in the file — confirmed by grep. Codex's review was against an earlier revision (920a21f7c6) than the current HEAD.

Test coverage

Enumerated the branches in the hoisted logic (empty _cands, no-Shared-token fast path, Shared-only full-drop, mixed survivor beyond head -3's first 3 slots, column-0 survivor, SharedStuff non-boundary case, spawn-count bound) — all have a dedicated case in hardcoded-path-check.test.sh. No gap found.

Minor/non-blocking

PR description says "Version bumped 0.18.2 → 0.18.3" but the diff (and CHANGELOG.md) actually shows 0.18.4 → 0.18.5 — consistent with the PR's own note about a sibling lane rebasing the version bump. Not a defect, just stale prose in the description; plugin.json and CHANGELOG.md agree with each other.

Overall: solid, well-tested fix for a real fail-open bug, and it stays within the stated scope (hoisting only — the _seg_end/:- boundary changes from the sibling PR are correctly left out).

github-actions · 2026-07-31T04:40:22Z

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

chatgpt-codex-connector Bot reviewed Jul 31, 2026

View reviewed changes

Comment thread plugins/guardrails/hooks/hardcoded-path-check.test.sh Outdated

kyle-sexton force-pushed the fix/1792-hoist-shared-defang branch from 920a21f to 30e7842 Compare July 31, 2026 04:33

kyle-sexton mentioned this pull request Jul 31, 2026

guardrails: hardcoded-path-check VIOLATIONS capture strips block-terminator newlines, running output together #1854

Closed

kyle-sexton merged commit 3287616 into main Jul 31, 2026
34 checks passed

kyle-sexton deleted the fix/1792-hoist-shared-defang branch July 31, 2026 14:48

Navigation Menu

Uh oh!

fix(guardrails): hoist the Shared defang out of the per-candidate loop - #1840

fix(guardrails): hoist the Shared defang out of the per-candidate loop#1840
kyle-sexton merged 1 commit into
mainfrom
fix/1792-hoist-shared-defang

kyle-sexton commented Jul 31, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

claude Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

claude Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

kyle-sexton commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Measured

Test plan

Related

Overlap with concurrent guardrails work

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

claude Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security review

Findings: none

Non-issue noted for completeness (not reported as a finding)

Uh oh!

claude Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review complete ✅

Core logic (hardcoded-path-patterns.sh)

Codex's P2 finding (wall-clock flakiness)

Test coverage

Minor/non-blocking

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kyle-sexton commented Jul 31, 2026 •

edited

Loading

claude Bot commented Jul 31, 2026 •

edited

Loading

claude Bot commented Jul 31, 2026 •

edited

Loading

Core logic (`hardcoded-path-patterns.sh`)