fix(guardrails): machine-path right boundary — segment class, not trailing separator - #1095
Merged
Merged
Conversation
…iling separator The five machine-path bodies required a separator AFTER the child segment, inverting detection both ways (#1093): a real bare path value at end of line ("root = <drive>:/Dev/GitHub") was missed, while the space-permitting segment class let prose satisfy the requirement by greedily consuming words until a later slash on the same line — the hook flagged a comment while the actual violations passed clean. The classes now exclude whitespace and the double quote and the mandatory trailing separator is gone: bare values at a natural boundary (EOL, whitespace, quote) are detected, prose spans cannot match, and a bare root with no child segment still never matches. The driver's /Users/Shared exclusion covers the new bare form. 15 regression cases added; suite 59/0, shellcheck clean. Synced-component note: machine-path-patterns.sh is a managed standards component — the identical change lands upstream in melodic-software/standards components/path-detection/ so the next sync does not revert it. Closes #1093 Co-Authored-By: Claude Fable 5 (200k context) <noreply@anthropic.com>
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ab01a68b8
ℹ️ 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".
… on the same line Codex review: the widened Shared exclusion was a line-level grep -v, so a line holding both a bare Shared path and a user-specific path dropped entirely, silently passing the real violation (the old separator-form exclusion did not trigger on the bare Shared value, so the second path was reported). Each candidate line is now re-tested with its Shared tokens defanged under boundary guards (a SharedStuff segment stays flagged); only lines whose sole matches are Shared drop out, and the original line is reported. Suite 62/0. Co-Authored-By: Claude Fable 5 (200k context) <noreply@anthropic.com>
Contributor
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
Contributor
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6770a1b0b2
ℹ️ 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".
Contributor
Author
|
🔒 babysit-prs lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, safe tier). Will fix clear branch-owned findings and push; will not resolve threads or merge (safe tier). |
…n char Codex round 2: the segment class admits shell/prose punctuation, so "cd /Users/Shared;" and a single-quoted Shared value matched the body while the enumerated defang boundaries (separator, whitespace, double quote, EOL) missed them — new false positives on common Shared-directory command shapes. The defang boundary is now any character that cannot continue a real directory name (word chars, dot, hyphen) instead of an enumerated delimiter list; SharedStuff stays flagged. Two boundary regression cases added; suite 64/0. Co-Authored-By: Claude Fable 5 (200k context) <noreply@anthropic.com>
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Contributor
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb91732e47
ℹ️ 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".
Codex round 3: the pattern lib has always assigned the macOS/Linux bodies a driver-side left-boundary prefix, and the ci-workflows/medley verification drivers carry one — this hook driver never did. With separator-bounded bodies the gap mostly hid (a URL needed a trailing slash after the home root to false-match); the bare-value form widened it to any URL suffix, e.g. an https URL ending in /home/alice flagged as a Linux user path. The driver now prefixes both slash-rooted bodies with a boundary (line start, whitespace, quote, backtick, paren, "=", ":" for yaml/docker value position, or file://), mirroring the verification drivers' set plus ":". URL negatives and a colon-prefixed positive pinned; suite 68/0. Co-Authored-By: Claude Fable 5 (200k context) <noreply@anthropic.com>
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Contributor
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b04ed3d82
ℹ️ 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".
kyle-sexton
added a commit
to melodic-software/standards
that referenced
this pull request
Jul 23, 2026
…ing separator (#255) ## Summary The five `HPP_*` bodies required a separator AFTER the child segment, which inverted detection both ways in a live consumer incident (melodic-software/claude-code-plugins#1093): - A real bare path value at end of line (`root = <drive>:/Dev/GitHub`) has no trailing separator and was **missed** — a false negative on exactly the config-value shape the bodies exist to catch. - Prose satisfied the requirement anyway: the space-permitting segment class greedily consumed words until a later slash on the same line, flagging a comment as a "Windows repo path" while the actual violations passed clean. Fix: the segment classes now exclude whitespace and the double quote, and the mandatory trailing separator is gone. Bare values at a natural boundary (EOL, whitespace, quote) are detected; prose spans cannot match (at least one non-space child character must follow the root); a root with NO child segment (`C:\Users`, a lone `D:/repos/`) still never matches. ## Deliberately flipped pins `machine-path-patterns.test.sh` pinned "bare roots stay clean" as a regression guard. Those pins are flipped, not worked around: that design's only stated rationale was prose safety, which the whitespace-excluding class now provides without the false-negative cost. New pins cover both directions of the old inversion (bare values must flag; prose spans must not — including a span-equality assertion that the prose line's match is the path token itself, never a greedy span). README updated to match (cross-doc reconciliation done in-PR; no other doc references the old boundary semantics). ## Consumer impact (manifest targets) Semantic widening reaches all three mapped targets on next sync — bare home/checkout values will start flagging: - **claude-code-plugins** (`path-detection-guardrails`): already carries the identical body file plus a bare-form update to its `/Users/Shared` exclusion stage — melodic-software/claude-code-plugins#1095. Local and upstream copies are byte-identical. - **medley** (`path-detection-tools`): its hook-layer driver has the same `/Users/Shared/` exclusion shape (`tools/shared/path-detection/hardcoded-path-patterns.sh`) and needs the same one-line bare-form follow-up when this syncs. - **ci-workflows** (`path-detection-action`): no affected exclusion stage found in `check-machine-specific-paths.sh`. ## Verification - `components/path-detection/machine-path-patterns.test.sh` **34/0** - shellcheck clean on both files (the escaped body's `SC1003` disable became unnecessary — no trailing backslash remains) 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Related No linked issue — the driving incident is tracked downstream: melodic-software/claude-code-plugins#1093 (consumer bug report), fixed there by melodic-software/claude-code-plugins#1095 with this component change mirrored byte-identically. Prior art: #250 (checkout-root broadening from the same consumer chain). Co-authored-by: Claude Fable 5 (200k context) <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Jul 23, 2026
…git work tree (#1098) ## Summary The #1039 fix skips `hardcoded-path-check` when `CLAUDE_PROJECT_DIR` is unset — but Claude Code sets a project dir for **any** directory (per current official hooks docs), home-directory sessions being the common case. There the scope guard passed while every per-file exemption rung was unreachable: - the `*.claude/hooks/*` / `*.claude/projects/*` carve-outs don't cover machine-local plugin config (`~/.claude/<plugin>.conf`); - `git check-ignore` errors outside a work tree, so the gitignore allowlist can never fire; - the only remaining escape was the global `hardcoded_path_check_enabled=false` kill switch. Reproduced live (producer incident): a `Write` of `~/.claude/repo-fleet-hygiene.conf` — a machine-local gitconfig-format file whose entire purpose is naming machine paths — was blocked with no per-file exemption. Fix: the scope guard also skips when `git -C "$CLAUDE_PROJECT_DIR" rev-parse --is-inside-work-tree` doesn't report a working tree — the same rationale as #1039 and the hook's stated philosophy: hardcoded paths only harm portable repo artifacts, and a non-worktree project dir has none. Bare repos skip too (no working tree, no tracked portable artifacts at that path). ## Test restructure The fixture root (`$TEST_TMPDIR`) is now git-initialized so active-project scan cases still run under the new guard, and the carve-out pins were re-pointed at **real** work trees — previously they used nonexistent project dirs (`/some/repo`, a fake home), which the new skip would have turned into trivially-passing tests that no longer exercised the carve-outs. New pins: non-worktree skip (machine-local `.claude/*.conf` shape) and same-content-in-real-work-tree still blocks. ## Verification - `hardcoded-path-check.test.sh` **72/0**; shellcheck clean - guardrails `0.12.2` → `0.12.3` + CHANGELOG; README scoping bullet updated ## Related - Producer finding: handoff-inbox item `20260723-014618-guardrails-hpp-win-repo-pattern-gaps` (Finding 2) - Prior art: #1038 / #1039 (no-project skip); sibling finding #1093 (merged as #1095) Closes #1094 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 (200k context) <noreply@anthropic.com>
This was referenced Jul 23, 2026
kyle-sexton
added a commit
that referenced
this pull request
Jul 31, 2026
#1840) ## 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 | ## Related - Fixes #1792 - Refs #1095 — the match-level Shared carve-out this reports on - Refs #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.com/claude-code) https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The five machine-path bodies in
machine-path-patterns.shrequired a separator AFTER the child segment, which inverted detection both ways (#1093, reproduced live):root = <drive>:/Dev/GitHub) has no trailing separator and was missed — a false negative on exactly the config-value shape the guard exists to catch.<drive>:/Projects/x - personal repos (reference/reading only here)was flagged as "Windows repo path detected" while the actual violations passed clean.Fix: all five bodies now exclude whitespace and the double quote from the child-segment class and drop the mandatory trailing separator. Bare values at a natural boundary (EOL, whitespace, quote) are detected; prose spans cannot match (the class requires at least one non-space child character after the root); a bare ROOT with no child segment (
C:/Dev, a lone/home) still never matches. The driver's/Users/Sharedexclusion now covers the bare-at-EOL form.Synced-component constraint
machine-path-patterns.shis a managed standards component (melodic-software/standardscomponents/path-detection/, distributed by the sync bot). The identical pattern change is being filed upstream so the next sync does not revert this fix — upstream PR linked below once open. Local and upstream copies must stay byte-identical.Verification
hardcoded-path-check.test.sh59/0 (15 new regression cases: bare values in all five body shapes incl. JSON-escaped, greedy-prose negative, root-plus-whitespace negative, bareShared)SC1003disable became unnecessary — no trailing backslash remains in the escaped body)0.12.1→0.12.2+ CHANGELOGRelated
20260723-014618-guardrails-hpp-win-repo-pattern-gaps(Finding 1)Closes #1093
🤖 Generated with Claude Code