feat(guardrails): add the skill-reference-verify claim guard - #1319
Conversation
Closes #1270 Two advisory PostToolUse guards on Write|Edit, siblings of `cli-flag-verify`: same defect class — a confident specific that was never checked — with different oracles. **asserted-path-verify.** Flags a repo-relative path asserted in markdown, in a code span or a link target, that does not exist in the working tree. Deterministic oracle, advisory action: a citation can be deliberately forward-looking and PostToolUse runs after the write, so it cannot tell. A first-segment gate is what keeps it quiet: a candidate is adjudicated only when its leading directory is in the repo, so another project's paths, package paths, globs, placeholders, and third-party references never fire. Line and range citation suffixes are stripped, which matters because that is this repo's dominant citation form. **skill-reference-verify.** Flags a `/plugin:skill` reference that does not resolve. Declared **detect-then-judge**, not deterministic: globbing a plugins tree is exact only where the reference is locally owned, so the finding is a prompt for a human verdict, never a determination and never an auto-fix. Gated twice — inert outside a marketplace repo, and within one it adjudicates only a plugin that repo's own manifests own. Resolution goes through manifest `name` and skill frontmatter `name`, so a renamed directory still matches. Both follow the `cli-flag-verify` pattern exactly: diff-scope only, per-guard kill switch defaulting true, `statusMessage` per handler, telemetry with repo-relative path redaction, and a co-located test carrying MUST-fire, MUST-stay-quiet, kill-switch, empty-stdin, missing-prerequisite, and telemetry cases. `lib/hook-utils.sh` is unchanged. Notice keys are unique plugin-wide, as `require-jq-notice-isolation.test.sh` now proves across 11 hooks. **A third guard was scoped and dropped.** A version-versus-manifest guard had no buildable trigger: its only in-repo shape is already covered by `check-changelog-parity.sh --check-bump`, and the residual prose surface is historical, minimum-floor, and planned version claims that a manifest-compare oracle reads wrong. The enumeration is recorded on #1270 rather than shipped as a guard that never fires correctly. **Boy Scout.** README counts were stale before this change: prose said "nine safety guards" and the table omitted `block-convention-violation` while ten were wired. Counts are now measured against the manifest toggle set, the missing row is present, and a new enforceability-tier section states each guard's oracle class so the detect-then-judge guard cannot be read as deterministic. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI `hygiene` failed with `shellcheck=failure` on #1284. Three findings, all real. `asserted-path-verify.sh`'s first-segment gate declared `local seg`. `hook-utils.sh` declares `local -a seg` in its bash parser and ShellCheck resolves sourced files (`external-sources=true`, `source-path=SCRIPTDIR`), so the string assignment read as an array-to-string type change — SC2178 plus SC2128 on the expansion. Renamed to `first_seg`, with the collision recorded at the site so it is not reintroduced. The same file's candidate-rejection `case` had no default branch, which the repo's `.shellcheckrc` requires via `enable=add-default-case` (SC2249). Added, stating that anything unmatched stays a candidate for the gates below. Both test files tripped SC2016 46 times: their markdown fixtures are single-quoted because backticks are the code-span delimiters the hooks scan for, and the placeholder fixtures contain `$`/`{}` on purpose. Nothing there may expand. Disabled file-wide with the rationale rather than annotating ~45 lines. `skill-reference-verify.sh` was already clean. Verified after the rename that the first-segment gate still yields the same findings on the same fixture. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five findings on #1284, all real. Each is pinned by a repro-first case. **asserted-path-verify — titled and bracketed link destinations were skipped.** The extractor forbade whitespace inside the parenthesized run, so a valid `[guide](docs/missing.md "Guide")` was rejected wholesale and its missing target never checked. It now takes the whole run and keeps the leading destination token, handling both title-quote styles and the `<dest>` form. **asserted-path-verify — percent-encoded destinations were tested literally.** `[g](docs/my%20file.md)` names `docs/my file.md` and reported the encoded string. Destinations are now decoded before the existence test, but only when every `%` begins a valid two-hex-digit escape, so a literal `%` in a filename is not mangled. The traversal guard is re-applied AFTER decoding — `%2e%2e%2f` decodes to `../` and is only visible at that point. **asserted-path-verify — link destinations resolved against the wrong base.** A markdown link resolves relative to the document, not the repo root, so `docs/guide.md` linking `assets/x.png` means `docs/assets/x.png`. With an `assets/` at both the root and the document directory, a valid link was reported missing. Tokens now carry their kind: a link is accepted when either base resolves, a code-span citation stays repo-root-relative, which is this repo's convention for citing a file in prose. Both the first-segment gate and the existence test run per base. **skill-reference-verify — a bare directory counted as a skill.** A leftover `skills/<name>/` with no SKILL.md suppressed the advisory for a command that does not exist. It now requires the SKILL.md. **skill-reference-verify — a trailing YAML comment defeated name extraction.** `name: renamed # public command` is valid YAML and a valid rename, but the end-of-line-anchored parser extracted nothing, so the skill was reported unresolved. Comments are stripped before matching. asserted-path-verify 45 -> 56 cases, skill-reference-verify 38 -> 40, both green. ShellCheck clean against the repo rcfile. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… resolve parent-relative links Three more review findings on #1284, plus a flaky-harness fix found while verifying them. **A renamed skill's directory name was acting as an alias.** A `skills/legacy-dir/SKILL.md` declaring `name: renamed` made BOTH `/plugin:renamed` and `/plugin:legacy-dir` resolve. The second is not a command, and treating it as one suppressed the advisory for exactly the stale pre-rename references this guard exists to catch. The directory name is now the command segment only when the file declares no frontmatter name. **Argument-bearing invocations were never scanned.** The pattern required the closing backtick immediately after the skill name, so `/plugin:skill --apply` — the common form in this repo — was skipped entirely. The reference is now the leading command token of a code span rather than the whole span. **Parent-relative link destinations were rejected wholesale**, so a legitimate `[missing](../gone.md)` was never checked. They are now resolved the way a renderer would, against the document's directory. Two earlier attempts at that last fix were wrong in the same way, and both would have passed on Linux while silently doing nothing on Windows. Computing the document prefix as `${DOC_DIR#$REPO_ROOT}` never matches, because `git rev-parse --show-toplevel` returns a drive path (`C:/Users/…`) while `dirname "$FILE"` returns the MSYS form (`/tmp/…`). cygpath does not reconcile them either: the MSYS `/tmp` mount resolves through an 8.3 short name, so one side reads `KYLESE~1` and the other `KyleSexton`. The fix is to ask git — `rev-parse --show-prefix` answers in one universe. Because the result is then repo-relative, ascending past its own root IS leaving the repo, so lexical_normalize's own failure is the containment test and no absolute path is involved. **Harness: pin the stdin read bound.** `hook::buffer_stdin` bounds its fd0 read at `CLAUDE_PLUGIN_OPTION_STDIN_READ_TIMEOUT`, default 2 s, and a timeout makes an advisory hook exit 0 silently. One invocation costs 10-20 s of wall time on a loaded Windows/Git Bash box, so the default bound yielded empty output and a FAIL that read as a detection defect — load-dependent, which is why the same code passed 40/40 earlier and 44/45 later. Raised to 30 s in both harnesses: these cases test detection, not the read bound. asserted-path-verify 56 -> 61 cases, skill-reference-verify 40 -> 45, both green. ShellCheck clean against the repo rcfile. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
A real-world noise measurement across all 975 tracked markdown files found that 56 of this guard's 63 findings (89%) were CHANGELOG rename entries — content the hook's own advisory calls correct as written. A CHANGELOG is an append-only historical record: an entry saying a skill was renamed MUST keep naming the old command, so every rename this repo ever makes permanently adds an unresolvable reference to one. CHANGELOGs are also touched every release, which is where the noise is felt most. Excluding them moves the guard from 3.4% of files firing at 6% precision to 0.51% firing at 57% precision, keeping all four genuine findings: `/claude-ops:claude-observability` (the skill is `observability`), `/skill-quality:skill-quality check` (renamed to `check`, two sites), and `/planning:architect close-out` (renamed to `plan`). Deliberately narrow, and basename-scoped rather than substring-matched. `docs/topics/*/PLAN.md` completion records are arguably the same append-only shape, but two of the four real findings live there — a broader "historical by contract" rule would cost half the signal. Recorded at the site so the tension is visible to whoever revisits it. skill-reference-verify 45 -> 48 cases, green. ShellCheck clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-verify alone Closes #1270 A full-corpus noise measurement withdrew one of the two guards. Recorded here because the number, not a judgment call, is the reason. **asserted-path-verify: withdrawn.** Swept across all 975 tracked markdown files it fired on 231 of them — 23.7%, roughly one in four writes — producing 389 findings with **zero** true positives. The oracle never misfired; every finding was a scoping problem. 72% were consumer-project config paths (`.claude/**` and similar) that a doc describes for a CONSUMING repo and that correctly do not exist in a marketplace, and its first-segment gate passed only because this repo happens to carry same-named top-level directories. 17% were subtree-relative citations resolvable against a skill or plugin root. Fixing the three dominant causes still left ~4% firing at zero true positives. A guard that fires on a quarter of writes and is never right trains people to ignore every advisory, including the real ones its sibling produces. Per `docs/conventions/hook-precision/README.md`'s over-fire discipline it does not ship. The measurement is carried on #1314 for rescoping — the sweep covered one repo and that repo is the pathological case, so the guard may be mis-scoped rather than unsound. **skill-reference-verify: ships.** 4 true positives on the same corpus, each verified individually against the skill tree. After excluding CHANGELOGs as append-only historical records it fires on 0.51% of files at 57% precision. Guard counts reconcile three ways at 11: README table rows, manifest `*_enabled` toggles, and wired hook scripts. Catalog regenerated. `check-changelog-parity --check-bump`, `check-silent-skips` and ShellCheck all pass. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
![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: cda86a36fa
ℹ️ 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".
Review finding on #1319. An Edit may replace an arbitrary substring, so swapping `setup` for `ghost` inside an existing `/alpha:setup` leaves `/alpha:ghost` on disk while the hunk is the bare word `ghost` — carrying no command for emit_refs to find, so a newly-broken reference was silently missed. Mirrors the sibling `cli-flag-verify`'s reconstruct_partial_edit, which already solves this exact shape: the edit is applied by PostToolUse time, so pull from disk only the lines carrying one of the hunk's word tokens, scan those, and keep only references whose plugin or skill segment appears in the hunk. That token filter preserves the diff-scope contract — a pre-existing unrelated reference sharing one of those lines never fires. The anchor is the token rather than the line because a bare-word hunk carries no positional information. Reconstruction runs only when the hunk itself yielded no reference; a full-command hunk already scans correctly and must not be re-scanned from disk. Three cases pin it: bare-word hunk recovers the containing reference, an untouched broken neighbour stays unreported, and a full-command hunk takes the direct path with no disk leakage. ShellCheck clean against the repo rcfile. The contract suite could not be re-run to completion locally — a single hook invocation costs 10-20 s on this loaded Windows/Git Bash box and the suite exceeded the available window — so CI's Linux run is the authority on the full suite here, and it gates the merge. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 5m 33s —— View job PR Review: feat(guardrails): add the skill-reference-verify claim guard
OverallThe guard is well-built: the two-tier gate (plugins-root → plugin-scope) correctly limits the oracle to contexts where it has authority, the CHANGELOG carve-out is justified and measured, the advisory-only posture is appropriate, and the Finding 1 — Kill switch table is missing two rows (documentation)
The "Per-hook kill switches" table has 9 rows but there are 11 guards. Both The PR description says "missing row added" for Expected additions: | block-convention-violation | `block_convention_gate_enabled` |
| skill-reference-verify | `skill_reference_verify_enabled` |Finding 2 —
|
|
Claude finished @kyle-sexton's task in 4m 20s —— View job Security Review — PR #1319
No critical or important security vulnerabilities found. Reviewed: What was checkedInjection surfaces in
File-path handling
Plugin manifest
Path traversal via skill name
JSON construction Telemetry JSON is built entirely through Advisory exit semantics Every code path exits 0. The hook cannot block a tool call — it only produces One suggestionSUGGESTION — confidence: medium
In SummaryThe new hook is well-scoped and follows the same defensive patterns as its sibling |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 639bc4a4c9
ℹ️ 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".
…onstruction Two review findings on #1319, both in the reconstruction added one commit earlier, plus a diff-scope regression the first fix introduced. **Substring edits were missed.** The filter required the hunk token to equal a whole plugin or skill segment, but an Edit replaces arbitrary substrings: `up` -> `host` turns `/alpha:setup` into `/alpha:sethost`, where the token `host` locates the line but is only part of the segment. Now matches on substring. **Mixed hunks were missed.** Reconstruction ran only when the direct scan found nothing, so a hunk carrying both a complete reference and a substring change to another reported only the complete one. Reconstruction now runs on every Edit; CHECKED already dedupes a reference reachable both ways, and a case pins that it is counted once. **Regression from that second fix, caught by an existing case.** Running reconstruction unconditionally meant a hunk containing a complete reference contributed its own plugin name as an anchor token — `alpha` then substring-matched every reference to that plugin, including untouched ones on neighbouring lines. Complete references are now stripped before anchor extraction, since the direct scan already owns them. That still leaked: stripping the reference out of "Run `/alpha:x` now." leaves the fragment `un`, which substring-matches `untouched-ghost`. Anchor tokens now require 4+ characters — below that a token carries no locating power, only collisions. Verified directly on all three shapes as well as through the suite: a full-command hunk reports only its own reference, a bare-word hunk recovers the containing one, and a mixed hunk reports both — none leaking the untouched neighbour. skill-reference-verify 48 -> 59 cases, green. ShellCheck clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 51s —— View job PR Review: feat(guardrails): add the skill-reference-verify claim guard
Verdict on
|
|
Claude finished @kyle-sexton's task in 3m 6s —— View job Security Review — PR #1319
No security vulnerabilities found. Reviewed: What was checked
The token-extraction chain in
Reconstruction tokens in File path handling —
Plugin manifest name —
Frontmatter extraction
JSON output
Advisory exit semantics Every code path exits 0. The hook cannot block a tool call — it only produces Test harness The test harness creates a synthetic marketplace under
The new HOOK_TELEMETRY_SINK execution
SummaryThe new hook follows the same defensive posture as its |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0c41f2b09
ℹ️ 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".
…d precision (#1357) Closes #1352 ## Summary Adds `docs/adr/0003-verification-guards-earn-default-on-by-measured-precision.md`, recording the reusable lesson from the #1270 guard program. Docs-only. Three guards were scoped on sound oracles. One shipped (#1319, 0.51% firing at 57% precision). Two were withdrawn: - **version-vs-manifest** � dropped before implementation. Its only in-repo surface is already covered by `scripts/check-changelog-parity.sh --check-bump`, and the residual prose surface is historical, minimum-floor, and planned version claims a manifest compare reads *wrong*. - **asserted-path** � built to 61 contract cases, fully reviewed, then withdrawn on measurement: **23.7% of all 975 tracked markdown files fired, 389 findings, zero true positives** (#1314 carries the sweep). The generalizable finding is that **a sound oracle is necessary but not sufficient**. The path guard's oracle was exact � no candidate resolved at the repo root, every finding was a scoping failure. A passing contract suite proves the oracle; only a corpus sweep proves the scoping. Four rules recorded: measure against the real corpus before shipping default-on; report the number in the PR; treat zero true positives as disqualifying however sound the oracle; distinguish wrong-oracle from wrong-scope, because that decides deletion versus rescoping. It also records the cost honestly � measuring immediately after the guard first worked, rather than after polish and a review round, would have saved that round. That is the actionable part for whoever builds the next one. ## Test plan - `markdownlint-cli2` on the new file � 0 errors - Every cross-reference verified to resolve: `docs/adr/0002-�md`, `docs/conventions/hook-precision/README.md`, `docs/PLUGIN-PHILOSOPHY.md`, `scripts/check-changelog-parity.sh` - House format matches 0001 and 0002 (`# Title`, Status/Date bullets, Context, Decision, Consequences) - Docs-only; no plugin version or CHANGELOG bump applies ## Related Extends [ADR 0002](docs/adr/0002-default-on-ai-review-advisory-with-earned-promotion.md) one step earlier in the lifecycle � 0002 governs promoting an advisory gate to blocking on demonstrated precision, this governs whether it ships default-on at all, on the same evidentiary basis. Defers to `docs/conventions/hook-precision/README.md` for post-ship over-fire discipline rather than restating it. #1270 (scoping, amended twice), #1319 (shipped guard, merged), #1284 (closed), #1314 (withdrawn guard and its measurement). --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… rescope skill-reference-verify reconstruction (#2100) No linked issue ## Summary Discharges six stranded bot review threads against the `guardrails` plugin, all filed on already-merged PRs. One is a defeated security guard, the rest are `skill-reference-verify` correctness and timeout-budget defects. Four further threads raised on this PR are also addressed below. ## Fix **`block-dangerous-git` — the hash-width probe ignored a wrapper's chdir (thread on #1275).** A `--force-with-lease` expectation is judged immutable only when it is an object id of the hash width of the repository the push will run in. `collect_git_locating_opts` reads only the slice between the git word and the subcommand — as it must, since that walk cannot know which of `env`'s or `sudo`'s options take a value — so a wrapper's relocation was invisible to it. `env -C <sha256-repo> git push --force-with-lease=main:<40-hex>` therefore probed the invoking SHA-1 directory, read the 40-hex word as an object id, and allowed the push; where git actually runs that word is an ordinary movable ref name, which is exactly the hole `--force-with-lease` exists to close. `hook::git_resolve_index` already records the relocation in `HOOK_GIT_RESOLVED_WRAPPER_DIRS` — the only parser that tells a real `env -C <dir>` from the `-C` in `env -u -C git`, which moves nothing — and the probe now replays those directories as leading `-C` words so they compose ahead of git's own under git's rules rather than being modelled. This mirrors the migration `848df9e9` (#1785) made in `block-noncanonical-commit`. **`skill-reference-verify` — partial-Edit reconstruction (threads on #1319 and #1466, one span).** The old shape located the hunk by line and then filtered the whole physical line by word token. Three defects, all that filter: an untouched broken reference sharing a line with the hunk was readmitted by any word it happened to share; an Edit replacing fewer than four lowercase characters produced no token at all, so every short-substring edit went uncovered; and locating spent two full-file `grep` processes per hunk line, which a large Edit turned into the hook's 30s timeout. Reconstruction now keeps only the inline-code spans whose extent OVERLAPS the located anchor. The occurrence-uniqueness gate is unchanged. **`skill-reference-verify` — the cost model behind the timeout fix was wrong, twice.** Removing the subprocesses left a per-line RESCAN, so the hunk is now located WHOLE — one scan for the whole edit, producing the same span set, since a line anchor's extent is the text the edit wrote on that line and the whole hunk's extent is the union of exactly those. Measuring the scan itself then contradicted the bound placed on it: one scan is QUADRATIC in file size, not linear, because bash's `%%` pattern strip walks the string rather than indexing it. The previous 4 MiB file cap therefore allowed a single scan of roughly eighteen minutes — the worst case had been moved off the per-line loop, not bounded. Both caps are now set from the measured curve. **`skill-reference-verify` — manifest-declared skill paths (thread on #1319).** Resolution hard-coded `plugins/<plugin>/skills/`. Per the [Plugins reference](https://code.claude.com/docs/en/plugins-reference) (fetched 2026-08-09), `skills` is a `string|array` whose paths ADD to the default `skills/` scan, a path may point straight at a directory holding `SKILL.md`, and a root `SKILL.md` with no `skills/` and no `skills` key auto-loads as a single-skill plugin. All three now resolve. The documented marketplace-root exception is deliberately not modelled and is recorded as such at the call site — leaving it out only ever suppresses an advisory, never invents one. The advisory's own text carried the same hard-coded assumption and now lists the directories the search actually covered. ## Verification **Security defect, reproduced before and after** against the same fixture tree (SHA-1 and SHA-256 repos), hook cwd = the SHA-1 repo unless noted. `origin/main`'s `block-dangerous-git.sh` vs this branch's: | case | pre-fix | post-fix | want | | :-- | :-- | :-- | :-- | | `env -C <sha256> git push --force-with-lease=main:<40-hex>` | **ALLOWED** | BLOCKED | BLOCKED | | `env -C <sha256> git push --force-with-lease=main:<64-hex>` | **BLOCKED** | ALLOWED | ALLOWED | | `env -C <sha1> git push …:<64-hex>` (cwd = sha256) | **ALLOWED** | BLOCKED | BLOCKED | | `env --chdir=<sha256> git push …:<40-hex>` | **ALLOWED** | BLOCKED | BLOCKED | | `sudo -D <sha256> git push …:<40-hex>` | **ALLOWED** | BLOCKED | BLOCKED | | `sudo --chdir=<sha256> git push …:<40-hex>` | **ALLOWED** | BLOCKED | BLOCKED | | `bash -c 'env -C <sha256> git push …:<40-hex>'` | **ALLOWED** | BLOCKED | BLOCKED | | `env -C <parent> git -C repo-sha256 …:<64-hex>` | **BLOCKED** | ALLOWED | ALLOWED | | `env -u -C git push …:<40-hex>` (`-C` is `-u`'s operand) | ALLOWED | ALLOWED | ALLOWED | The last row is the control that keeps the fix honest: an option that only looks like a chdir still moves nothing, so the guard did not simply get stricter. Two rows flip BLOCKED → ALLOWED, which a fail-closed regression could not produce. **Scan cost, measured rather than assumed.** One `anchor_offsets` scan, isolated, Windows/Git Bash, quiescent, best of three: | file size | 32 KiB | 64 KiB | 96 KiB | 128 KiB | 192 KiB | 256 KiB | | :-- | :-- | :-- | :-- | :-- | :-- | :-- | | one scan | 0.07s | 0.24s | 0.53s | 1.07s | 2.18s | 3.94s | That is ~0.065s × (KiB/32)² — quadratic. Those figures are a FLOOR, not the cost: they time an anchor matching near the end, so one strip walks the file and the second is free, while a no-match strip walks it twice (2.31s at 200 KiB) and the whole-hunk probe pays a scan before the fallback runs at all. So the two bounds are calibrated end to end, not from the table: `RECONSTRUCT_MAX_CHARS` is 128 KiB, and the fallback's anchor cap is `RECONSTRUCT_FALLBACK_SCAN_BUDGET / (KiB)²` — 58 anchors at 32 KiB, 14 at 64, 3 at 128. Above the file cap the direct hunk scan is untouched, so a complete reference is still reported and only partial-edit recovery stops. **End-to-end, the shape the defect actually lived in** (a hunk of distinct span-free lines, so the span cap never binds and every anchor would rescan). `origin/main` vs this branch, same fixture: | hunk | file size | `origin/main` | this branch | | :-- | :-- | :-- | :-- | | 1 line | <1 KiB | 10.5s | 0.8s | | 100 lines | ~4 KiB | 135.8s | — | | 500 lines | ~19 KiB | 778.8s | — | | 1000 lines | ~38 KiB | (not run) | 1.0s | Baseline per-invocation overhead on this host is 0.8–1s quiescent, so the branch numbers are the scan, not the harness. An earlier revision of this PR reported far flatter pre-fix numbers; that benchmark used hunk lines carrying inline code spans, which trip `RECONSTRUCT_MAX_SPANS` and stop the loop after 40 anchors — it measured the capped path, not the defect. The table above is the corrected measurement. A 4000-line row from that revision is dropped rather than restated: at ~156 KiB it now exceeds the file cap, so it would time the skip path, not reconstruction. **Why the scale test asserts behavior instead of wall time.** The new large-file fallback case pins the cap from both sides — a reference inside the anchor cap is still reported, one past it is not — rather than timing it. On this host the same fixture read 21s loaded and a smaller one 23s, against an isolated scan of ~1s at that size; a timing assertion that noisy fails on load and passes on a regression that happens to run on a quiet box. The scan cost is measured directly instead, in the constants' docblock. **Gates run from the worktree root, all green:** `shellcheck -x` on the four changed shell files; `markdownlint-cli2` on the changelog; `check-changelog-parity.sh --check`, `--check-bump origin/main`, `--check-order`; `check-shell-portability.sh origin/main`; `sync-hook-utils.sh --check` and `--check-bump`; `check-cross-plugin-source-drift.sh --check`; `validate-plugins.sh`; `check-changed-skills.sh origin/main`. Contract suites: `block-dangerous-git.test.sh` 341 pass / 0 fail; `skill-reference-verify.test.sh` 96 pass / 0 fail (see also the CI `plugin-gate` job, which runs both on Linux). **Four threads raised on this PR.** `Xp-3r` (quadratic rescan) and `XqJ0e` (nothing bounds the anchor count) are both discharged by the whole-hunk locate plus `RECONSTRUCT_FALLBACK_SCAN_BUDGET`; the suite fixture the first was measured against at 35s now runs in 0s. `XqJ1e` (the advisory hard-coded `plugins/<x>/skills/`) is fixed and asserted on its full rendering, not a prefix. `XrcOx` (no test combines a large file with the fallback path) is the case described above. **Not fixed here, flagged instead:** `block-convention-violation.sh`'s `effective_dir` (`:186-201`) scans *every* word for `-C`, with no `[git, subcommand)` slice and no wrapper replay — the pre-`848df9e9` shape, failing the opposite direction from the one fixed here. It accepts a `-C` that moves nothing (`env -u -C git`) and a `-C` after the subcommand (`git commit -C HEAD`, reuse-message), so `effective_dir` can name the wrong repository. Different defect class, needs its own tests; not widened into this PR. ## Related - Review thread on #1275 — `block-dangerous-git` wrapper-chdir hash probe (the security defect) - Review threads on #1319 and #1466 — `skill-reference-verify` reconstruction and manifest skill paths - #1785 (`848df9e9`) — the wrapper-chdir parser in the shared lib this fix consumes - #1466 (`527dcd85`) — already landed the hunk-line anchoring; a further thread on it needed no new fix - #1432 (`a2d98f8a`) — the sibling `stale-path-verify` fix the reconstruction docblock cites --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ction to the lines the Edit wrote (#2153) ## Disposition: fixed, not closed won't-fix The issue offered three dispositions and flagged the defect as possibly payload-inherent. It is not, and that is the whole reason this ships as a fix. Partial-edit reconstruction separates an occurrence the call wrote from a coincidental one by requiring the anchor to occur exactly once. `replace_all` is precisely where that rule is suspended, on the reasoning that there every occurrence *is* the edit's own footprint. It is not: after `ghost` replaces `setup` everywhere, the `ghost` inside a pre-existing `ghost-old` matches the anchor too. The issue's premise — "nothing in the payload distinguishes a `ghost` this call wrote from the `ghost` inside a pre-existing `ghost-old`" — **holds for `tool_input` and fails for `tool_response`.** The Edit tool's structured output carries `structuredPatch`, which marks the lines the call actually wrote with a leading `+`. Under `replace_all` only, an occurrence is now kept just when its physical line is one the patch reports as written. The suspended uniqueness rule gets an external witness instead of nothing. Both halves were confirmed against pages **fetched 2026-08-10**, per the repo's fresh-docs mandate: - `PostToolUse` input "includes both `tool_input`, the arguments sent to the tool, and `tool_response`, the result it returned. The exact schema for both depends on the tool", and that field is "the tool's structured `Output` object" — <https://code.claude.com/docs/en/hooks>, "PostToolUse input". - `Output` for Edit is `FileEditOutput`, whose `structuredPatch` is `Array<{oldStart, oldLines, newStart, newLines, lines: string[]}>` — <https://code.claude.com/docs/en/agent-sdk/typescript>, "Edit". ## Why line TEXT and not line numbers Numbers are wrong the moment another PostToolUse hook reformats the file between the write and this read — the exact case the reconstruction fallback already exists for. And mapping a character offset back to a line number costs a whole-prefix scan per occurrence, which would reintroduce the quadratic term 0.21.0 spent a release removing. Text matching is a hash lookup and survives renumbering. Its one imprecision is conservative: an untouched line whose text duplicates an edited one is kept, so the filter can only ever drop findings the payload itself calls untouched. ## Deliberately inert outside its one case - A multi-line `new_string` is **not** filtered: its anchor extent spans several lines, matches no single patch line, and filtering would erase every finding rather than narrow them. - A payload with no `tool_response`, and every non-`replace_all` Edit, behaves exactly as before — the filter is inert by construction, not by a flag. ## Verification The table below was produced by running the **shipped test file** against the **unmodified `origin/main` hook** in a pristine worktree — not against a separate throwaway fixture. That distinction is load-bearing here; see the traps below. | assertion | pre | post | discriminates? | |---|---|---|---| | the WRITTEN reference is still reported | pass | pass | no — must not regress | | **the UNTOUCHED reference is not reported** | **FAIL** | **pass** | **yes** | | genuine multi-site: both refs survive the filter | pass | pass | no — proves no findings lost | | liveness: empty target yields nothing at all | pass | pass | no — see below | **Liveness**, using the technique the issue names: the identical payload is run against a truncated, empty target file. The hook is silent there, so every finding in the real fixture demonstrably came from **reading the file** rather than from the payload text. A filter that merely echoed `new_string` back would have spoken in both. Suite: **106 pass, 0 fail**. ## Two traps hit and fixed while building this Both are recorded because both produced a green assertion for the wrong reason, which is the failure mode this sweep exists to stop. 1. The first draft of the payload builder passed the diff lines as jq `--args` positionals. Every line starts with `-`, `+` or a space; jq parsed the leading `-` as an option and died on `Unknown option -u`. The payload came back empty, the hook went silent, and `assert_absent "the UNTOUCHED reference is not reported"` went **green** — while testing nothing at all. It was caught only because the paired `assert_contains` failed alongside it. Lines now reach jq on stdin. This is also why the PRE column above is measured with the shipped test file rather than the standalone fixture: the two artifacts had already diverged behaviorally once. 2. The positive needle `UNRESOLVED_SKILL: /alpha:ghost` is a substring of the `/alpha:ghost-old` line it exists to exclude, so it was sound only as long as the paired absence assertion stayed next to it. It now carries its own right boundary. ## Not verified A live PostToolUse payload carrying `structuredPatch` was **not** directly observed — no hook-event capture existed on the authoring machine to read. The schema is documented (above, fetched today) and corroborated by real Edit records in Claude Code's own transcript JSONL, which carry `originalFile` and a `structuredPatch` with `+`/`-`/` `-prefixed lines. If the field never arrives in a hook payload, the filter never engages and nothing regresses — but a reviewer with a hook-event capture could close this gap in one grep. ## Adversarial verification status A fresh-context adversarial verifier was spawned for this PR and did **not** return a verdict before the authoring session ended — the machine was saturated by concurrent agents and every spawned verifier stalled inside a long test sweep. Treat this PR as carrying the author's own evidence only. What partially substitutes for it, and why it is not nothing: the PRE column in the table above was produced by running the **shipped test file** against the **unmodified `origin/main` hook** in a pristine worktree, which is precisely the headline check such a verifier performs. What is still unverified by a second party is the "can you break it" attack surface and the payload-supply question called out below. Closes #2129 ## Related - #1319 — where the parent finding was filed - #2100 — closed the parent finding via span-overlap scoping; this is the residual it left - #1938 — the stranded post-merge review-findings sweep - #2113 — the other guardrails defect from this sweep, shipped separately - Another agent is concurrently bumping `guardrails` for #2124, and #2113's branch bumps to the same 0.24.0; whichever of the three merges later will need to re-bump. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
1 participant
Closes #1270
Supersedes #1284 — same work, rebased onto
mainafter #853 landed, and narrowed from two guards to one on measurement. #1284's branch could not be force-pushed, so this is a fresh branch; its review history is worth reading, since all 13 findings there were real.Summary
One advisory
PostToolUseguard onWrite|Edit:skill-reference-verifyflags a`/plugin:skill`reference in markdown that does not resolve.Declared detect-then-judge, not deterministic. Globbing a plugins tree is exact only where the reference is locally owned — in a consuming repo it may name a plugin from another marketplace, or one simply not installed. Per
conventions/engineering/enforceability-tiers.mdthat means advisory plus a human verdict, never an auto-fix.Gated twice so the oracle only runs where it is meaningful: inert outside a marketplace repo, and within one it adjudicates only a plugin that repo's own manifests own. Resolution goes through manifest
nameand skill frontmattername. A renamed skill's directory name is deliberately not an alias — treating it as one would suppress exactly the stale pre-rename references this guard exists to catch. The reference is the leading command token of a code span, so argument-bearing invocations (/plugin:skill --apply) are scanned.Follows
cli-flag-verifyexactly: diff-scope only (scan what the call wrote, never re-read from disk), kill switch defaulting true,statusMessage, telemetry with repo-relative path redaction,lib/hook-utils.shuntouched.Boy Scout: README counts were stale before this change — prose said "nine safety guards" while ten were wired and the table omitted
block-convention-violation. Counts now measured against the manifest toggle set, missing row added, plus a new enforceability-tier section so the detect-then-judge guard cannot be read as deterministic.A guard was built and withdrawn
asserted-path-verifyshipped in #1284 and is not in this PR. A full-corpus sweep — all 975 tracked markdown files, each fed as a real payload — measured:The oracle never misfired; every finding was a scoping problem. 72% were consumer-project config paths (
.claude/**and similar) that a doc describes for a consuming repo and that correctly do not exist in a marketplace — its first-segment gate passed only because this repo happens to carry same-named top-level directories. 17% were subtree-relative citations resolvable against a skill or plugin root. Fixing the three dominant causes still left ~4% firing at zero true positives.A guard that fires on a quarter of writes and is never right trains people to ignore every advisory, including the real ones. Per
docs/conventions/hook-precision/README.md's over-fire discipline, it does not ship. The measurement is carried on #1314 for rescoping rather than discarded — the sweep covered one repo and that repo is the pathological case, so the guard may be mis-scoped rather than unsound.Test plan
skill-reference-verify.test.sh— 48 cases, covering MUST-fire, MUST-stay-quiet, kill-switch, empty-stdin, missing-prerequisite and telemetry. Stay-quiet coverage includes unowned plugins, manifest-less directories, uppercase non-command tokens, unbackticked prose, non-markdown files, non-Write|Edittools, files outsideCLAUDE_PROJECT_DIR, and CHANGELOGs.require-jq-notice-isolation.test.shproves the notice key is unique plugin-wide by glob discovery.Guard counts reconcile three ways at 11 — README table rows, manifest
*_enabledtoggles, wired hook scripts. Catalog regenerated vianode scripts/generate-catalog.mjs.Gates:
validate-plugins.sh,check-silent-skips.sh,check-changelog-parity.sh --checkand--check-bump origin/main, ShellCheck against the repo rcfile — all pass.Related
Closes #1270, whose scope was amended twice during the build: three guards → two (a version-vs-manifest guard had no buildable trigger, already covered by
check-changelog-parity --check-bump), then two → one on the measurement above.#1314 carries the withdrawn guard's rescope. #1284 is superseded. #853 is the merge whose
0.14.3bump forced this rebase — the collision #1284's body predicted.Review status, stated plainly: the contract suite and repo gates are verified, and the noise measurement above is the empirical case for the one guard that ships. A subagent security review was dispatched twice and never reported, so this PR relies on the repo's own
security-review,reviewand cross-vendor checks. A shell hook running on everyWrite/Editis a code-execution trust surface — please do not merge on the author's word alone.