From d71394b59e5fc48434eaa6504b86654877daafd4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 7 Aug 2026 08:28:25 -0400 Subject: [PATCH 1/7] fix(guardrails): address stranded post-merge review findings Unverified batch preserved from a session that ended before its findings were falsified against origin/main. Touches block-dangerous-git and skill-reference-verify plus their test suites. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/guardrails/CHANGELOG.md | 43 +++ .../guardrails/hooks/block-dangerous-git.sh | 17 +- .../hooks/block-dangerous-git.test.sh | 21 ++ .../hooks/skill-reference-verify.sh | 309 ++++++++++++------ .../hooks/skill-reference-verify.test.sh | 138 ++++++++ 5 files changed, 430 insertions(+), 98 deletions(-) diff --git a/plugins/guardrails/CHANGELOG.md b/plugins/guardrails/CHANGELOG.md index 15671b8e8b..39e6c83400 100644 --- a/plugins/guardrails/CHANGELOG.md +++ b/plugins/guardrails/CHANGELOG.md @@ -3,6 +3,49 @@ All notable changes to the `guardrails` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.20.0] + +### Fixed + +- **`block-dangerous-git`'s hash-width probe ignored a wrapper's chdir, so an unsafe + `--force-with-lease` passed.** A lease expectation is judged against the hash width of the + repository the push will run in, and the probe replays git's own repository-locating globals to + find it. It could not replay a WRAPPER's relocation: `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 `env -C git push + --force-with-lease=main:<40-hex>` probed the invoking SHA-1 directory, read the 40-hex expectation + as an immutable object id, and allowed the push. Where git actually runs, that same word is an + ordinary movable ref name, which is the exact hole `--force-with-lease` exists to close. + `hook::git_resolve_index` already records the relocation in `HOOK_GIT_RESOLVED_WRAPPER_DIRS` — it + is the only parser that can tell a real `env -C ` from the `-C` in `env -u -C git`, which + moves nothing — and the probe now replays it as leading `-C` words, ahead of git's own, so the two + compose in execution order under git's rules rather than being modelled. Covered for `env -C`, + `env --chdir=`, `sudo -D`, the composition with git's own `-C`, and the `env -u -C` non-chdir. + +- **`skill-reference-verify` reported references an Edit never touched, missed short substring + edits, and could spend its whole 30-second timeout on one large Edit.** Partial-Edit + reconstruction located the hunk by line and then filtered the whole line by word token. All three + defects were that filter: an untouched broken reference sharing a physical line with the hunk was + readmitted by any word it happened to share (`legacy` in both the edited prose and + `` `/alpha:ghost-legacy` ``); an Edit replacing fewer than four lowercase characters produced no + token at all, so reconstruction gave up and every such edit went uncovered; and locating spent two + full-file `grep` processes per hunk line, which a thousand-line Edit turned into a timeout — an + advisory lost entirely, after delaying the tool call to get there. Reconstruction now reads the + file once and keeps only the inline-code spans whose extent OVERLAPS the located anchor. Overlap + is exact where the token filter was approximate, has no minimum length to clear, and costs no + subprocess per hunk line. The occurrence-uniqueness gate is unchanged: an anchor that cannot say + which occurrence the edit landed on is still dropped rather than unioned. + +- **`skill-reference-verify` reported a valid command as unresolved when its plugin declared custom + skill paths.** Resolution hard-coded `plugins//skills/`, but a manifest's `skills` key + holds a path or array of paths that ADD to that directory, and a declared path may point straight + at a directory holding `SKILL.md` ([Plugins + reference](https://code.claude.com/docs/en/plugins-reference), "Path behavior rules"). A skill + loaded from a declared location now resolves, as does the documented single-skill layout (a root + `SKILL.md` with no `skills/` subdirectory and no `skills` key). That layout is honoured under its + stated conditions only — a root `SKILL.md` beside a populated `skills/` is not loaded by Claude + Code, so accepting it would suppress the advisory for a command that does not exist. + ## [0.19.0] ### Changed diff --git a/plugins/guardrails/hooks/block-dangerous-git.sh b/plugins/guardrails/hooks/block-dangerous-git.sh index f488223f74..a8c028ab31 100755 --- a/plugins/guardrails/hooks/block-dangerous-git.sh +++ b/plugins/guardrails/hooks/block-dangerous-git.sh @@ -256,13 +256,28 @@ lease_expect_is_immutable() { # git word and the subcommand. Two-word options whose value is consumed the same # way hook::git_resolve_subcommand consumes it, so the walk cannot desynchronize # from the parser's own. +# +# The slice starts AT the git word, as it must: this walk cannot know which of a +# wrapper's own options take a value, so reading `env -u -C git …` as git's `-C` +# would relocate the probe on an option that belongs to `env`'s `-u`. That +# scoping is what makes the replay below necessary — a wrapper's chdir is a real +# relocation the slice deliberately cannot see, and hook::git_resolve_index is +# the single parser that can tell the two apart. +# +# A wrapper's chdir happens before git starts, so git's own locating options +# compose onto it: it is replayed as LEADING `-C` words, which git applies +# cumulatively in argv order, and the composition then falls out of git's own +# rules rather than being modelled here. # shellcheck disable=SC2329 # reached via the hook::bash_parse_segments callback chain collect_git_locating_opts() { local gi="$1" sub_idx="$2" shift 2 local -a w=("$@") - local j=$((gi + 1)) + local j=$((gi + 1)) wdir git_locating_opts=() + for wdir in ${HOOK_GIT_RESOLVED_WRAPPER_DIRS[@]+"${HOOK_GIT_RESOLVED_WRAPPER_DIRS[@]}"}; do + git_locating_opts+=(-C "$wdir") + done while ((j < sub_idx)); do case "${w[j]}" in -C | --git-dir | --work-tree | --namespace) diff --git a/plugins/guardrails/hooks/block-dangerous-git.test.sh b/plugins/guardrails/hooks/block-dangerous-git.test.sh index be949c6f0a..db31a4a9f4 100755 --- a/plugins/guardrails/hooks/block-dangerous-git.test.sh +++ b/plugins/guardrails/hooks/block-dangerous-git.test.sh @@ -82,6 +82,27 @@ run "git --git-dir=/.git push --force-with-lease=main:<40-hex> (blo run "git -C push --force-with-lease=main:<40-hex> (width undeterminable, fail-closed block)" "git -C $NOT_A_REPO push --force-with-lease=main:$SHA1_OID origin main" 2 run "git -c x=y -C push --force-with-lease=main:<64-hex> (config value skipped, not mistaken for a path, allowed)" "git -c x=y -C $REPO_SHA256 push --force-with-lease=main:$SHA256_OID origin main" 0 +# A WRAPPER's chdir moves git just as git's own -C does, and the width probe must +# follow it. The locating-option walk is deliberately scoped to `[git, subcommand)` +# — it cannot know which wrapper options take a value — so the relocation reaches +# it only through hook::git_resolve_index, the one parser that can tell `env -C +# ` (a real chdir) from the `-C` in `env -u -C git` (the operand of -u). +# Losing it probed the INVOKING directory, where a 40-hex lease reads as an object +# id while the push runs where it is a movable ref name. +run "env -C git push --force-with-lease=main:<40-hex> (40-hex is a name where git runs, blocked)" "env -C $REPO_SHA256 git push --force-with-lease=main:$SHA1_OID origin main" 2 +run "env -C git push --force-with-lease=main:<64-hex> (object id where git runs, allowed)" "env -C $REPO_SHA256 git push --force-with-lease=main:$SHA256_OID origin main" 0 +run_in "$REPO_SHA256" "env -C git push --force-with-lease=main:<64-hex> (64-hex is a name where git runs, blocked)" "env -C $REPO_SHA1 git push --force-with-lease=main:$SHA256_OID origin main" 2 +run "env --chdir= git push --force-with-lease=main:<40-hex> (long form, blocked)" "env --chdir=$REPO_SHA256 git push --force-with-lease=main:$SHA1_OID origin main" 2 +run "sudo -D git push --force-with-lease=main:<40-hex> (sudo's chdir, blocked)" "sudo -D $REPO_SHA256 git push --force-with-lease=main:$SHA1_OID origin main" 2 +# The mirror image: an option that only LOOKS like a chdir must not move the probe. +# GNU env's `-u NAME` consumes the next word, so the `-C` in `env -u -C git` is +# the variable name and git never moves — the lease is judged where it stands. +run "env -u -C git push --force-with-lease=main:<40-hex> (-C is -u's operand, no chdir, allowed)" "env -u -C git push --force-with-lease=main:$SHA1_OID origin main" 0 +# A wrapper chdir composes AHEAD of git's own -C, in that order: env relocates +# before git starts, so a relative `-C` resolves against the wrapper's directory. +run "env -C git -C push --force-with-lease=main:<64-hex> (composed, allowed)" "env -C $TEST_TMPDIR git -C repo-sha256 push --force-with-lease=main:$SHA256_OID origin main" 0 +run "env -C git -C push --force-with-lease=main:<40-hex> (composed, blocked)" "env -C $TEST_TMPDIR git -C repo-sha256 push --force-with-lease=main:$SHA1_OID origin main" 2 + # The width probe is the guard's only subprocess, and a command may carry many # lease expectations. Counting real git invocations catches the cache being lost # to a subshell — a per-expectation probe would spawn one git each and push a diff --git a/plugins/guardrails/hooks/skill-reference-verify.sh b/plugins/guardrails/hooks/skill-reference-verify.sh index bcbdb1a2f9..6430bac4b5 100755 --- a/plugins/guardrails/hooks/skill-reference-verify.sh +++ b/plugins/guardrails/hooks/skill-reference-verify.sh @@ -96,12 +96,22 @@ shopt -u nullglob # A plugin's command namespace is its manifest `name`, which need not equal its # directory name. Build the name → directory map from the manifests themselves so # a renamed directory or a name override resolves correctly. -declare -A PLUGIN_DIR=() +# +# The manifest also decides WHERE that plugin's skills live. Its `skills` key +# holds a path or an array of paths, each relative to the plugin root, and those +# ADD to the conventional `skills/` directory rather than replacing it — verified +# against the Plugins reference (, +# "Path behavior rules", fetched 2026-08-06). Collect them per plugin so a skill +# loaded from a declared location resolves like any other. +declare -A PLUGIN_DIR=() PLUGIN_SKILL_PATHS=() for m in "${manifests[@]}"; do pdir="${m%/.claude-plugin/plugin.json}" pname=$(jq -r '.name // empty' "$m" 2>/dev/null | tr -d '\r') [[ -n "$pname" ]] || pname="${pdir##*/}" PLUGIN_DIR["$pname"]="$pdir" + PLUGIN_SKILL_PATHS["$pname"]=$( + jq -r '.skills // empty | if type == "array" then .[] else . end' "$m" 2>/dev/null | tr -d '\r' + ) done # Extract a plugin skill's frontmatter `name`, or nothing when it declares none. @@ -115,6 +125,35 @@ skill_frontmatter_name() { head -1 } +# The directories a plugin's skills are loaded from: the conventional `skills/`, +# plus every path its manifest declares, plus the plugin root when the manifest +# declares nothing and the root itself is the skill. +# +# All three shapes come from the Plugins reference +# (, fetched 2026-08-06): +# declared paths are relative to the plugin root and start with `./` (the `skills` +# key also accepts `.`, and both `.` and `./` denote the root); they ADD to the +# default `skills/` scan; and a plugin with a root SKILL.md, no `skills/` +# subdirectory and no `skills` key auto-loads as a single-skill plugin. That last +# condition is honoured as written rather than widened — a root SKILL.md sitting +# beside a populated `skills/` is not loaded, and accepting it would suppress the +# advisory for a command Claude Code does not actually offer. +# +# Call as: skill_roots -> $roots +# shellcheck disable=SC2154 # roots is the caller's frame, per the call contract +skill_roots() { + local pdir="$1" declared="$2" rel + roots=("$pdir/skills") + while IFS= read -r rel; do + [[ -n "$rel" ]] || continue + rel="${rel#./}" + rel="${rel%/}" + if [[ -z "$rel" || "$rel" == "." ]]; then roots+=("$pdir"); else roots+=("$pdir/$rel"); fi + done <<<"$declared" + [[ -z "$declared" && ! -d "$pdir/skills" && -f "$pdir/SKILL.md" ]] && roots+=("$pdir") + return 0 +} + # A plugin skill's command segment comes from its frontmatter `name` when set, # and from the directory name ONLY when it does not. # @@ -126,18 +165,34 @@ skill_frontmatter_name() { # A directory alone is also not a skill: it must carry a SKILL.md, or a leftover # empty `skills//` would suppress the advisory for a command that never # existed. +# +# A root may BE a skill directory rather than a directory of them — a declared +# path can point straight at a directory holding SKILL.md — so each root is tried +# both ways. skill_resolves() { - local pdir="$1" skill="$2" sd fname - local direct="$pdir/skills/$skill/SKILL.md" - if [[ -f "$direct" ]]; then - fname=$(skill_frontmatter_name "$direct") - # No declared name → the directory name IS the command segment. - [[ -z "$fname" || "$fname" == "$skill" ]] && return 0 - fi - for sd in "$pdir"/skills/*/SKILL.md; do - [[ -f "$sd" ]] || continue - fname=$(skill_frontmatter_name "$sd") - [[ -n "$fname" && "$fname" == "$skill" ]] && return 0 + local pdir="$1" skill="$2" declared="$3" root sd sdir fname + local -a roots=() + skill_roots "$pdir" "$declared" + for root in "${roots[@]}"; do + if [[ -f "$root/$skill/SKILL.md" ]]; then + fname=$(skill_frontmatter_name "$root/$skill/SKILL.md") + # No declared name → the directory name IS the command segment. + [[ -z "$fname" || "$fname" == "$skill" ]] && return 0 + fi + if [[ -f "$root/SKILL.md" ]]; then + fname=$(skill_frontmatter_name "$root/SKILL.md") + sdir="${root%/}" + if [[ -n "$fname" ]]; then + [[ "$fname" == "$skill" ]] && return 0 + else + [[ "${sdir##*/}" == "$skill" ]] && return 0 + fi + fi + for sd in "$root"/*/SKILL.md; do + [[ -f "$sd" ]] || continue + fname=$(skill_frontmatter_name "$sd") + [[ -n "$fname" && "$fname" == "$skill" ]] && return 0 + done done return 1 } @@ -157,113 +212,173 @@ emit_refs() { sed -nE 's|^(/[a-z][a-z0-9-]*:[a-z][a-z0-9-]*)([[:space:]].*)?$|\1|p' } +# Reconstruction is bounded on three axes, because this hook has a 30s timeout +# (hooks.json) and a guard that loses its advisory to a timeout has stopped +# guarding. A file too large to hold in a shell variable is not reconstructed at +# all; a hunk stops contributing once it has yielded this many code spans, which is +# far above any real edit given that only spans the anchor reaches are kept; and an +# anchor stops being counted one occurrence past the cap, which is all the +# uniqueness gate needs to know. +RECONSTRUCT_MAX_CHARS=4194304 +RECONSTRUCT_MAX_SPANS=40 +RECONSTRUCT_MAX_OCCURRENCES=40 + +# Append to the caller's $ctx every inline-code span in whose extent +# OVERLAPS [, ), and count them in the caller's $nspan. +# +# Span boundaries are exactly the ones emit_refs' own `\`[^\`]+\`` finds, so what +# is kept here and what is parsed there cannot disagree: leftmost match, at least +# one character between the backticks (an adjacent pair opens nothing), and never +# across a newline, since grep matches within a line. +# +# Call as: collect_overlapping_spans +# shellcheck disable=SC2154 # ctx and nspan are the caller's frame, per the docblock +collect_overlapping_spans() { + local region="$1" hs="$2" he="$3" + local i=0 a b rest pre after inner + while :; do + rest="${region:i}" + pre="${rest%%\`*}" + # No backtick left in the region — nothing further can open a span. + [[ "$pre" == "$rest" ]] && return 0 + a=$((i + ${#pre})) + after="${region:a+1}" + inner="${after%%\`*}" + # No closing backtick anywhere after this one: no span can open here or later. + [[ "$inner" == "$after" ]] && return 0 + # An empty inner (an adjacent pair) or one spanning a newline is not a match + # for grep either; it retries from the next character, so this walk does too. + if [[ -z "$inner" || "$inner" == *$'\n'* ]]; then + i=$((a + 1)) + continue + fi + b=$((a + 1 + ${#inner})) + # The span occupies [a, b] inclusive of both backticks; the hunk [hs, he). + if ((a < he && b >= hs)); then + ctx+="\`$inner\`"$'\n' + ((++nspan >= RECONSTRUCT_MAX_SPANS)) && return 0 + fi + i=$((b + 1)) + done +} + +# Locate every occurrence of in the caller's $content, into the caller's +# $offs. Stops one past the occurrence cap: the uniqueness gate only needs to know +# whether there is more than one, and `replace_all` stops contributing context at +# the cap anyway. +# +# Occurrences, not matching lines. Counting lines is not enough: two occurrences +# on one physical line are a single line, and that is a real shape — inserting +# `legacy` into a line that already carries an untouched `` `/alpha:ghost-legacy` `` +# leaves the anchor twice on that line, and reporting the reference would be an +# advisory about text this call never wrote. +# +# Call as: anchor_offsets -> $offs +# shellcheck disable=SC2154 # content and offs are the caller's frame, per the call contract +anchor_offsets() { + local anchor="$1" alen=${#1} + local rest="$content" base=0 pre + offs=() + while [[ "$rest" == *"$anchor"* ]]; do + pre="${rest%%"$anchor"*}" + offs+=($((base + ${#pre}))) + ((${#offs[@]} > RECONSTRUCT_MAX_OCCURRENCES)) && return 0 + base=$((base + ${#pre} + alen)) + rest="${content:base}" + done +} + # Partial-replacement context reconstruction (Edit only). The same shape lives in -# stale-path-verify and cli-flag-verify; stale-path-verify was fixed for this -# defect class by 65b4f67c (#1432) and this one goes one step further with the -# uniqueness requirement below, so the three are not yet identical. +# stale-path-verify and cli-flag-verify, which still take the whole located line +# and filter it by word token (stale-path-verify was fixed for the line-anchoring +# half of this defect class by 65b4f67c); this one keeps only the part of the line +# the hunk reaches, so the three are not identical. # # An Edit may replace an arbitrary substring: swapping `setup` for `ghost` inside # an existing `/alpha:setup` leaves `/alpha:ghost` on disk, but the hunk is the # bare word `ghost` and carries no command for emit_refs to find, so a # newly-broken reference would be silently missed. # -# Recover bounded context: the edit is already applied by PostToolUse time, so -# pull from disk only the lines the hunk's OWN TEXT UNIQUELY locates, scan those, -# and keep only references whose plugin or skill segment contains one of the -# hunk's word tokens. Two gates, and the first is where diff-scope actually lives: +# Recover bounded context POSITIONALLY. The edit is already applied by PostToolUse +# time, so every line of new_string is on disk verbatim and its own text locates +# the exact span of the file this call wrote. Diff-scope is then an overlap test +# rather than a word-token heuristic. Two gates: # -# 1. LOCATE by the hunk's own lines, and only where a line OCCURS exactly once -# in the file. Every line of new_string is on disk verbatim, so a unique -# occurrence IS where the edit landed. Anything repeated cannot say which -# copy that was, so it is dropped rather than unioned. Occurrences, not -# matching lines — two copies on one physical line are a single grep hit and -# would otherwise slip through. Exception: under `replace_all` every -# occurrence is a place this call edited, so all are kept. -# 2. FILTER the references found there by the hunk's word tokens. +# 1. LOCATE by the hunk's own lines, never by its tokens, and only where a line +# OCCURS exactly once in the file. A unique occurrence IS where the edit +# landed. Anything repeated cannot say which copy that was, so it is dropped +# rather than unioned. Exception: under `replace_all` every occurrence is a +# place this call edited, so all are kept. +# 2. KEEP only the inline-code spans whose extent OVERLAPS the located anchor. # -# Gate 2 alone is not sufficient and was never the guarantee: a token short -# enough to occur in unrelated prose is also short enough to be a substring of an -# untouched skill segment, so it would pass the reference through. Uniqueness at -# gate 1 is what keeps the guard inside the diff. +# Gate 2 is what a word-token filter could never be: exact. Sharing a physical line +# with the hunk is not evidence the edit wrote a reference, so a span the anchor +# does not reach is dropped no matter how many words it happens to share with the +# hunk. And overlap has no minimum length to clear, so an Edit replacing two +# characters inside a command segment is scoped as precisely as one replacing +# twenty — where the token filter, needing a token long enough to filter on, simply +# produced none and gave up. # # What this does NOT claim: the `replace_all` branch keeps every occurrence, # including one that pre-existed the edit and merely happens to read the same — # nothing in the payload separates those. Reconstruction is a best effort under an # advisory guard, not a proof that every reported line was written by this call. +# +# Cost is ONE read of the file, then in-memory scans — no subprocess per hunk line. +# The per-anchor `grep` this replaced spawned two processes per line of the hunk, +# which a large multiline Edit turned into the hook's timeout. reconstruct_partial_edit() { [[ "$TOOL" == "Edit" && -f "$FILE" ]] || return 0 - # The token filter reads the hunk with any COMPLETE reference removed first. A - # complete reference is already handled by the direct scan, and leaving it in - # would contribute its own plugin name as a token — `alpha` then passes every - # reference to that plugin. What remains is the genuinely bare edited text. - local residue - residue=$(printf '%s' "$SCAN_CONTENT" | sed -E 's#/[a-z][a-z0-9-]*:[a-z][a-z0-9-]*##g') - # Minimum token length. A substring match on a very short token matches almost - # any segment — stripping a reference out of `Run `/alpha:x` now.` leaves the - # fragment `un`, which substring-matches `untouched-ghost`. 4 characters is the - # shortest command segment worth filtering on; below that the token carries no - # filtering power. - local -a toks=() - mapfile -t toks < <(printf '%s' "$residue" | grep -oE '[a-z][a-z0-9-]{3,}' 2>/dev/null | sort -u) - ((${#toks[@]})) || return 0 - # Lines are located by the hunk's own lines, never by its tokens. Every line of - # new_string is on disk verbatim, so it matches the line the edit landed in; a - # token, being shorter, also matches lines the edit never touched — a bare - # `legacy` in unrelated prose pulls in every `/alpha:*-legacy` reference on any - # line, and an untouched broken one among them would fire. + local content + content=$(<"$FILE") || return 0 + # new_string arrived with CR stripped, so a CRLF file must be searched the same + # way or no anchor would ever locate. Offsets are read back off this normalized + # text only, never off the bytes on disk. + content=${content//$'\r'/} + ((${#content} <= RECONSTRUCT_MAX_CHARS)) || return 0 + local -a anchors=() mapfile -t anchors < <(printf '%s' "$SCAN_CONTENT" | grep -vE '^[[:space:]]*$' 2>/dev/null) ((${#anchors[@]})) || return 0 - # An anchor is used ONLY when it OCCURS exactly once in the file — occurrences, - # not matching lines. Counting lines is not enough: two occurrences on one - # physical line are one grep hit, and that is a real shape — inserting `legacy` - # into a line that already carries an untouched `` `/alpha:ghost-legacy` `` - # leaves the anchor twice on that line, and reporting the reference would be an - # advisory about text this call never wrote. Occurrence uniqueness subsumes line - # uniqueness (one occurrence can only be on one line), so it is the only gate. - # - # A non-unique anchor cannot say WHICH occurrence the edit landed on, so it is - # dropped rather than unioned. Cost: a missed advisory when an edit lands in - # text that repeats verbatim elsewhere in the file. For a detect-then-judge - # guard that is the right side of the trade — it is degraded far worse by being - # wrong when it speaks than by staying quiet. - local anchor occ ctx="" - local -a hits=() + + # A repeated anchor line resolves identically every time, so scan each once. + declare -A seen=() + local ctx="" nspan=0 anchor alen off hs he head tail + local -a offs=() for anchor in "${anchors[@]}"; do - mapfile -t hits < <(grep -F -- "$anchor" "$FILE" 2>/dev/null) - ((${#hits[@]})) || continue - # `replace_all` is the one case where repetition is expected rather than - # ambiguous: every occurrence is a place THIS call edited, so all of them are - # in scope and uniqueness must not be required. Accepted narrowing — a line - # that independently contained `new_string` and was never touched is kept too, - # since nothing in the payload distinguishes it from an edited one. - if [[ "$REPLACE_ALL" == "true" ]]; then - for occ in "${hits[@]}"; do ctx+="$occ"$'\n'; done - continue - fi - occ=$(grep -o -F -- "$anchor" "$FILE" 2>/dev/null | grep -c .) - ((occ == 1)) || continue - ctx+="${hits[0]}"$'\n' + alen=${#anchor} + ((alen)) || continue + [[ -n "${seen[$anchor]:-}" ]] && continue + seen["$anchor"]=1 + anchor_offsets "$anchor" + ((${#offs[@]})) || continue + # A non-unique anchor cannot say WHICH occurrence the edit landed on, so it is + # dropped rather than unioned. Cost: a missed advisory when an edit lands in + # text that repeats verbatim elsewhere in the file. For a detect-then-judge + # guard that is the right side of the trade — it is degraded far worse by being + # wrong when it speaks than by staying quiet. + [[ "$REPLACE_ALL" == "true" ]] || ((${#offs[@]} == 1)) || continue + for off in "${offs[@]}"; do + # The physical line the anchor sits in: back to the newline before it, + # forward to the newline after. A reference the edit landed inside is only + # whole when read from the line, not from the anchor alone — which is why + # the line is read at all, and why only the part of it the anchor reaches + # may be kept. + head="${content:0:off}" + head="${head##*$'\n'}" + hs=${#head} + tail="${content:off+alen}" + tail="${tail%%$'\n'*}" + he=$((hs + alen)) + collect_overlapping_spans "${content:off-hs:he+${#tail}}" "$hs" "$he" + ((nspan >= RECONSTRUCT_MAX_SPANS)) && break 2 + done done [[ -n "$ctx" ]] || return 0 + local saved="$SCAN_CONTENT" - SCAN_CONTENT=$(printf '%s' "$ctx" | grep -vE '^[[:space:]]*$' | head -40) - local ref seg plug skl - while IFS= read -r ref; do - [[ -n "$ref" ]] || continue - plug="${ref#/}" - plug="${plug%%:*}" - skl="${ref##*:}" - # SUBSTRING match, not equality: an Edit can replace part of a segment - # (`up` -> `host` turns `/alpha:setup` into `/alpha:sethost`), so the hunk - # token is a substring of the segment rather than the whole of it. - for seg in "${toks[@]}"; do - if [[ "$plug" == *"$seg"* || "$skl" == *"$seg"* ]]; then - printf '%s\n' "$ref" - break - fi - done - done < <(emit_refs) + SCAN_CONTENT="$ctx" + emit_refs SCAN_CONTENT="$saved" } @@ -290,7 +405,7 @@ for ref in "${REFS[@]}"; do # PLUGIN-SCOPE GATE: only adjudicate a plugin this repo owns. pdir="${PLUGIN_DIR[$plugin]:-}" [[ -n "$pdir" ]] || continue - skill_resolves "$pdir" "$skill" && continue + skill_resolves "$pdir" "$skill" "${PLUGIN_SKILL_PATHS[$plugin]:-}" && continue UNRESOLVED+=("$ref") done diff --git a/plugins/guardrails/hooks/skill-reference-verify.test.sh b/plugins/guardrails/hooks/skill-reference-verify.test.sh index 01b6edab98..95ea662efb 100755 --- a/plugins/guardrails/hooks/skill-reference-verify.test.sh +++ b/plugins/guardrails/hooks/skill-reference-verify.test.sh @@ -365,6 +365,144 @@ assert_contains "reference reachable both ways → reported" "$OUT" \ assert_contains "reference reachable both ways → counted once" "$OUT" \ "1 skill reference(s) do not resolve" +# Sharing a physical LINE with the hunk is not evidence the edit wrote a +# reference. Here the anchor is unique — so the uniqueness gate is satisfied and +# cannot help — and it sits on the same line as an untouched broken reference, +# which a shared word (`legacy`) then readmitted through the token filter. Only +# keeping the part of the line the anchor OVERLAPS separates them. +ONELINE="$REPO/oneline.md" +printf 'Stale `/alpha:ghost-legacy` here. The legacy naming convention was updated today.\n' \ + >"$ONELINE" +OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" \ + <<<"$(edit_json "$ONELINE" 'The legacy naming convention was updated today.')" 2>&1) +RC=$? +assert_exit "unique anchor beside an untouched reference → exit 0" 0 "$RC" +assert_silent "a reference the anchor does not overlap is not reported" "$OUT" + +# The same line, edited INSIDE the reference this time: overlap must still admit +# it, or the fix above would have bought scope by going blind. +ONELINE2="$REPO/oneline2.md" +printf 'Stale `/alpha:ghost-legacy` here. The legacy naming convention was updated today.\n' \ + >"$ONELINE2" +OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" <<<"$(edit_json "$ONELINE2" 'ghost-legacy')" 2>&1) +assert_contains "an anchor inside the reference still reports it" "$OUT" \ + "UNRESOLVED_SKILL: /alpha:ghost-legacy" + +# A SHORT substring edit. Replacing `up` with `xx` inside `/alpha:setup` leaves +# `/alpha:setxx` on disk and a two-character hunk; a minimum token length left +# every such edit uncovered, while overlap has no length to clear. +SHORT="$REPO/short.md" +printf 'Run `/alpha:setxx` to begin.\n' >"$SHORT" +OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" <<<"$(edit_json "$SHORT" 'xx')" 2>&1) +RC=$? +assert_exit "two-character Edit hunk → exit 0" 0 "$RC" +assert_contains "two-character Edit hunk → containing reference recovered" "$OUT" \ + "UNRESOLVED_SKILL: /alpha:setxx" + +# A short anchor is still subject to the uniqueness gate — it buys no scope. +SHORT2="$REPO/short2.md" +printf 'Run `/alpha:setxx` and note the xx convention.\n' >"$SHORT2" +OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" <<<"$(edit_json "$SHORT2" 'xx')" 2>&1) +assert_silent "a short anchor occurring twice is still ambiguous" "$OUT" + +# A large multiline Edit must not cost a subprocess per hunk line. The hook's own +# timeout is 30 s (hooks.json) and a per-line full-file grep spent it, losing the +# advisory entirely; the ceiling here is deliberately loose so the case fails only +# on the defect, not on a slow host. +BIGDOC="$REPO/big.md" +BIGHUNK="$TEST_TMPDIR/bighunk.txt" +: >"$BIGDOC" +: >"$BIGHUNK" +{ + for ((i = 1; i <= 1000; i++)); do printf 'Line %s of the large replaced passage.\n' "$i"; done +} >"$BIGHUNK" +{ + cat "$BIGHUNK" + printf 'Run `/alpha:ghost-big` at the end.\n' +} >"$BIGDOC" +big_start=$SECONDS +OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" \ + <<<"$(edit_json "$BIGDOC" "$(cat "$BIGHUNK")")" 2>&1) +RC=$? +big_elapsed=$((SECONDS - big_start)) +assert_exit "1000-line Edit hunk → exit 0" 0 "$RC" +if ((big_elapsed < 30)); then + ok "1000-line Edit hunk stays inside the hook's 30s timeout (${big_elapsed}s)" +else + bad "1000-line Edit hunk took ${big_elapsed}s, at or past the hook's 30s timeout" +fi + +# ============ MANIFEST-DECLARED SKILL PATHS ================================= +# A manifest may declare `skills` paths, which ADD to the conventional `skills/` +# directory rather than replacing it. A command loaded from a declared location is +# real, so reporting it unresolved is a false advisory. +mk_plugin gamma gamma +mk_skill gamma conventional +mkdir -p "$REPO/plugins/gamma/custom/extras/declared" +printf -- '---\nname: declared\ndescription: x\n---\n' \ + >"$REPO/plugins/gamma/custom/extras/declared/SKILL.md" +# A declared path may point straight AT a skill directory, not only at a directory +# of them. +mkdir -p "$REPO/plugins/gamma/solo" +printf -- '---\nname: solo-command\ndescription: x\n---\n' \ + >"$REPO/plugins/gamma/solo/SKILL.md" +MSYS_NO_PATHCONV=1 jq -n '{name:"gamma",version:"0.1.0",skills:["./custom/extras/","./solo/"]}' \ + >"$REPO/plugins/gamma/.claude-plugin/plugin.json" + +OUT=$(run 'Run `/gamma:declared`.') +RC=$? +assert_exit "manifest-declared skill path → exit 0" 0 "$RC" +assert_silent "skill under a manifest-declared path resolves" "$OUT" + +OUT=$(run 'Run `/gamma:solo-command`.') +assert_silent "declared path pointing AT a skill directory resolves" "$OUT" + +# Declared paths ADD to `skills/`; the conventional directory must still resolve. +OUT=$(run 'Run `/gamma:conventional`.') +assert_silent "declared paths add to skills/, they do not replace it" "$OUT" + +# The guard must not go blind for a plugin that declares paths — a command in +# neither location still fires. +OUT=$(run 'Run `/gamma:nowhere`.') +assert_contains "declared paths do not suppress a genuinely missing command" "$OUT" \ + "UNRESOLVED_SKILL: /gamma:nowhere" + +# A declared skill's DIRECTORY name is no more an alias than a conventional one's. +OUT=$(run 'Run `/gamma:solo`.') +assert_contains "declared skill's directory name is NOT an alias" "$OUT" \ + "UNRESOLVED_SKILL: /gamma:solo" + +# A string `skills` value is as valid as an array. +mk_plugin delta delta +mkdir -p "$REPO/plugins/delta/elsewhere/one" +printf -- '---\nname: one\ndescription: x\n---\n' \ + >"$REPO/plugins/delta/elsewhere/one/SKILL.md" +MSYS_NO_PATHCONV=1 jq -n '{name:"delta",version:"0.1.0",skills:"./elsewhere/"}' \ + >"$REPO/plugins/delta/.claude-plugin/plugin.json" +OUT=$(run 'Run `/delta:one`.') +assert_silent "a string skills value resolves like a one-element array" "$OUT" + +# The single-skill plugin layout: a SKILL.md at the plugin root, no `skills/` +# subdirectory and no `skills` key, auto-loads as one skill named by its +# frontmatter. +mk_plugin epsilon epsilon +printf -- '---\nname: eps-command\ndescription: x\n---\n' >"$REPO/plugins/epsilon/SKILL.md" +OUT=$(run 'Run `/epsilon:eps-command`.') +assert_silent "root SKILL.md with no skills/ and no manifest key auto-loads" "$OUT" +OUT=$(run 'Run `/epsilon:missing`.') +assert_contains "single-skill plugin still fires for a command it does not have" "$OUT" \ + "UNRESOLVED_SKILL: /epsilon:missing" + +# The auto-load applies only under its documented conditions. A root SKILL.md +# BESIDE a populated `skills/` is not loaded, so it must not resolve — accepting it +# would suppress the advisory for a command Claude Code does not offer. +mk_plugin zeta zeta +mk_skill zeta real +printf -- '---\nname: zeta-root\ndescription: x\n---\n' >"$REPO/plugins/zeta/SKILL.md" +OUT=$(run 'Run `/zeta:zeta-root`.') +assert_contains "root SKILL.md beside a populated skills/ is not auto-loaded" "$OUT" \ + "UNRESOLVED_SKILL: /zeta:zeta-root" + # ============================ KILL SWITCH =================================== OUT=$(CLAUDE_PROJECT_DIR="$REPO" CLAUDE_PLUGIN_OPTION_SKILL_REFERENCE_VERIFY_ENABLED=false \ From ab05f8f163fdb2a2ab8b06f8adb9e8ab32e03419 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 8 Aug 2026 07:00:35 -0400 Subject: [PATCH 2/7] fix(guardrails): correct a dead SHA citation and record the unmodelled skills-path exception The reconstruction docblock cited 65b4f67c for the stale-path-verify line-anchoring fix. That SHA is a pre-squash branch commit and is not an ancestor of main; the commit that actually landed it is a2d98f8a (#1432). The skill_roots docblock stated the manifest `skills` additive rule without its marketplace-root exception, where declared subdirectories replace the default scan instead. Record the exception and why it is left unmodelled. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/guardrails/hooks/skill-reference-verify.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/plugins/guardrails/hooks/skill-reference-verify.sh b/plugins/guardrails/hooks/skill-reference-verify.sh index 6430bac4b5..69e92f7f43 100755 --- a/plugins/guardrails/hooks/skill-reference-verify.sh +++ b/plugins/guardrails/hooks/skill-reference-verify.sh @@ -103,6 +103,14 @@ shopt -u nullglob # against the Plugins reference (, # "Path behavior rules", fetched 2026-08-06). Collect them per plugin so a skill # loaded from a declared location resolves like any other. +# +# One documented exception is NOT modelled: for a marketplace entry whose `source` +# resolves to the marketplace root, declared subdirectories REPLACE the default +# `skills/` scan. Modelling it would mean reading marketplace.json to learn how +# each entry resolves, and the cost of not modelling it is bounded — the default +# stays in the search set, so at worst a reference resolves that Claude Code would +# not offer and this advisory stays quiet. Staying quiet is the failure this guard +# is allowed to have; a false alarm is not. declare -A PLUGIN_DIR=() PLUGIN_SKILL_PATHS=() for m in "${manifests[@]}"; do pdir="${m%/.claude-plugin/plugin.json}" @@ -291,7 +299,7 @@ anchor_offsets() { # Partial-replacement context reconstruction (Edit only). The same shape lives in # stale-path-verify and cli-flag-verify, which still take the whole located line # and filter it by word token (stale-path-verify was fixed for the line-anchoring -# half of this defect class by 65b4f67c); this one keeps only the part of the line +# half of this defect class by a2d98f8a); this one keeps only the part of the line # the hunk reaches, so the three are not identical. # # An Edit may replace an arbitrary substring: swapping `setup` for `ghost` inside From e414162a1f815177ed71bda26f3f1dc4f8cdb896 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 9 Aug 2026 13:58:30 -0400 Subject: [PATCH 3/7] docs(guardrails): state why the wrapper-chdir fix is a minor bump The guard's acceptance behavior changes in both directions, so the entry says so rather than leaving a reader to infer the bump from the diff. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/guardrails/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugins/guardrails/CHANGELOG.md b/plugins/guardrails/CHANGELOG.md index adf3503da9..23cd82b4b4 100644 --- a/plugins/guardrails/CHANGELOG.md +++ b/plugins/guardrails/CHANGELOG.md @@ -21,6 +21,9 @@ All notable changes to the `guardrails` plugin are documented here. Format follo moves nothing — and the probe now replays it as leading `-C` words, ahead of git's own, so the two compose in execution order under git's rules rather than being modelled. Covered for `env -C`, `env --chdir=`, `sudo -D`, the composition with git's own `-C`, and the `env -u -C` non-chdir. + **Acceptance behavior changes** (hence a minor bump): a wrapped push whose lease is a movable name + where git runs is refused where it was allowed, and one whose lease is a real object id there is + allowed where the misplaced probe refused it. - **`skill-reference-verify` reported references an Edit never touched, missed short substring edits, and could spend its whole 30-second timeout on one large Edit.** Partial-Edit From 155fb11bfacb7ee56bc855027f09c65ff0f60f75 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 9 Aug 2026 14:33:16 -0400 Subject: [PATCH 4/7] fix(guardrails): locate the Edit hunk whole instead of rescanning per line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dropping the per-line subprocesses removed only half the reconstruction cost. The per-line RESCAN remained, and it is anchors times file size: a thousand span-free hunk lines cost 82 s against the hook's 30-second budget on a Windows/Git Bash host. Nothing bounded the product — the span cap stops early only when the hunk's lines carry code spans, which is why the earlier measurement missed it. The hunk reaches disk contiguously, so locate it whole: one scan for the whole edit. The span set is unchanged, because 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. Same measurement: 11 s at a thousand lines, 11 s at four thousand. Locating whole also scopes better than the walk it replaces — a hunk whose every line repeats but whose whole text does not used to be dropped as ambiguous line by line, and now resolves to the one place it names. The walk stays as a fallback for a hunk no longer on disk verbatim, under a total scanning budget rather than an anchor count. anchor_offsets now does one pattern operation per iteration instead of two; testing with `[[ == *"$anchor"* ]]` and then stripping searched the same text twice, which doubled the term the whole-hunk locate is dominated by. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/guardrails/CHANGELOG.md | 20 +++++- .../hooks/skill-reference-verify.sh | 65 +++++++++++++++---- .../hooks/skill-reference-verify.test.sh | 38 +++++++++-- 3 files changed, 102 insertions(+), 21 deletions(-) diff --git a/plugins/guardrails/CHANGELOG.md b/plugins/guardrails/CHANGELOG.md index 23cd82b4b4..e21aa5655c 100644 --- a/plugins/guardrails/CHANGELOG.md +++ b/plugins/guardrails/CHANGELOG.md @@ -35,9 +35,23 @@ All notable changes to the `guardrails` plugin are documented here. Format follo full-file `grep` processes per hunk line, which a thousand-line Edit turned into a timeout — an advisory lost entirely, after delaying the tool call to get there. Reconstruction now reads the file once and keeps only the inline-code spans whose extent OVERLAPS the located anchor. Overlap - is exact where the token filter was approximate, has no minimum length to clear, and costs no - subprocess per hunk line. The occurrence-uniqueness gate is unchanged: an anchor that cannot say - which occurrence the edit landed on is still dropped rather than unioned. + is exact where the token filter was approximate, and has no minimum length to clear. The + occurrence-uniqueness gate is unchanged: an anchor that cannot say which occurrence the edit + landed on is still dropped rather than unioned. + + The timeout half needed both halves of its cost removed. Dropping the subprocesses left the + per-line RESCAN, which is anchors TIMES file size — measured on a Windows/Git Bash host, a + thousand span-free hunk lines still cost 82 s against the 30-second budget. The hunk is written to + disk contiguously, so it is now located WHOLE: one scan for the entire edit, and the span set is + the same one the per-line walk produced, 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. The same measurement is + now 11 s at a thousand lines and 11 s at four thousand — the hunk-size term is gone. Locating + whole is also strictly better scoping: a hunk whose every line repeats but whose whole text does + not used to be dropped as ambiguous line by line, and now resolves to the one place it names. The + per-line walk survives as a fallback for a hunk that is no longer on disk verbatim — another + PostToolUse hook reformatting the file between the write and this read is the realistic cause — + under a total scanning budget, because bounding the anchor count or the file size alone leaves + their product free. - **`skill-reference-verify` reported a valid command as unresolved when its plugin declared custom skill paths.** Resolution hard-coded `plugins//skills/`, but a manifest's `skills` key diff --git a/plugins/guardrails/hooks/skill-reference-verify.sh b/plugins/guardrails/hooks/skill-reference-verify.sh index 69e92f7f43..3a9f62c74a 100755 --- a/plugins/guardrails/hooks/skill-reference-verify.sh +++ b/plugins/guardrails/hooks/skill-reference-verify.sh @@ -220,16 +220,23 @@ emit_refs() { sed -nE 's|^(/[a-z][a-z0-9-]*:[a-z][a-z0-9-]*)([[:space:]].*)?$|\1|p' } -# Reconstruction is bounded on three axes, because this hook has a 30s timeout +# Reconstruction is bounded on four axes, because this hook has a 30s timeout # (hooks.json) and a guard that loses its advisory to a timeout has stopped # guarding. A file too large to hold in a shell variable is not reconstructed at # all; a hunk stops contributing once it has yielded this many code spans, which is -# far above any real edit given that only spans the anchor reaches are kept; and an +# far above any real edit given that only spans the anchor reaches are kept; an # anchor stops being counted one occurrence past the cap, which is all the -# uniqueness gate needs to know. +# uniqueness gate needs to know; and the per-line FALLBACK below gets a total +# scanning budget rather than an anchor count, because its cost is anchors TIMES +# file size and bounding either factor alone leaves the product free. Measured on +# the slowest host available (Windows/Git Bash): one anchor over a 41 KB file costs +# ~66 ms, so 2 MB of total scanning is ~3 s — inside the budget with the fixed +# per-invocation overhead accounted for. A big file therefore buys fewer anchors, +# down to one. RECONSTRUCT_MAX_CHARS=4194304 RECONSTRUCT_MAX_SPANS=40 RECONSTRUCT_MAX_OCCURRENCES=40 +RECONSTRUCT_MAX_SCAN_CHARS=2097152 # Append to the caller's $ctx every inline-code span in whose extent # OVERLAPS [, ), and count them in the caller's $nspan. @@ -281,14 +288,22 @@ collect_overlapping_spans() { # leaves the anchor twice on that line, and reporting the reference would be an # advisory about text this call never wrote. # +# ONE pattern operation per iteration. `${rest%%"$anchor"*}` both tests and locates: +# it returns $rest unchanged when there is no match, and a literal `==` on that +# result is a memcmp, not a second search. Asking `[[ $rest == *"$anchor"* ]]` first +# and then stripping searched the same text twice, which doubled the cost of the +# whole-hunk locate — where the anchor is the size of the edit and the search is the +# dominant term. +# # Call as: anchor_offsets -> $offs # shellcheck disable=SC2154 # content and offs are the caller's frame, per the call contract anchor_offsets() { local anchor="$1" alen=${#1} local rest="$content" base=0 pre offs=() - while [[ "$rest" == *"$anchor"* ]]; do + while :; do pre="${rest%%"$anchor"*}" + [[ "$pre" == "$rest" ]] && return 0 offs+=($((base + ${#pre}))) ((${#offs[@]} > RECONSTRUCT_MAX_OCCURRENCES)) && return 0 base=$((base + ${#pre} + alen)) @@ -312,11 +327,12 @@ anchor_offsets() { # the exact span of the file this call wrote. Diff-scope is then an overlap test # rather than a word-token heuristic. Two gates: # -# 1. LOCATE by the hunk's own lines, never by its tokens, and only where a line +# 1. LOCATE by the hunk's own text, never by its tokens, and only where that text # OCCURS exactly once in the file. A unique occurrence IS where the edit # landed. Anything repeated cannot say which copy that was, so it is dropped # rather than unioned. Exception: under `replace_all` every occurrence is a -# place this call edited, so all are kept. +# place this call edited, so all are kept. The hunk is located WHOLE, falling +# back to line by line only when the whole no longer matches. # 2. KEEP only the inline-code spans whose extent OVERLAPS the located anchor. # # Gate 2 is what a word-token filter could never be: exact. Sharing a physical line @@ -332,9 +348,12 @@ anchor_offsets() { # nothing in the payload separates those. Reconstruction is a best effort under an # advisory guard, not a proof that every reported line was written by this call. # -# Cost is ONE read of the file, then in-memory scans — no subprocess per hunk line. -# The per-anchor `grep` this replaced spawned two processes per line of the hunk, -# which a large multiline Edit turned into the hook's timeout. +# Cost is ONE read of the file and, normally, ONE scan of it — no subprocess per +# hunk line and no rescan per hunk line either. The `grep` this replaced spawned two +# processes per line of the hunk; scanning in-memory removed the processes but left +# the per-line rescan, which is anchors TIMES file size and still spent the timeout +# on a thousand-line hunk. Locating the hunk whole is what removes the factor; the +# scanning budget bounds the fallback that cannot. reconstruct_partial_edit() { [[ "$TOOL" == "Edit" && -f "$FILE" ]] || return 0 local content @@ -345,14 +364,32 @@ reconstruct_partial_edit() { content=${content//$'\r'/} ((${#content} <= RECONSTRUCT_MAX_CHARS)) || return 0 - local -a anchors=() - mapfile -t anchors < <(printf '%s' "$SCAN_CONTENT" | grep -vE '^[[:space:]]*$' 2>/dev/null) - ((${#anchors[@]})) || return 0 + local ctx="" nspan=0 anchor alen off hs he head tail cap + local -a offs=() anchors=() + + # The hunk is written to disk CONTIGUOUSLY, so locate it WHOLE first — one scan + # for the entire edit instead of one per line. The loop below is then driven by a + # single anchor, and the span set it produces is the same one the per-line walk + # produced: an anchor's extent is the text the edit wrote on its own line, and the + # whole hunk's extent is the union of exactly those. Multi-line is no obstacle — + # the extent is bounded by offset, and a code span never crosses a newline. + anchor_offsets "$SCAN_CONTENT" + if ((${#offs[@]})) && { [[ "$REPLACE_ALL" == "true" ]] || ((${#offs[@]} == 1)); }; then + anchors=("$SCAN_CONTENT") + else + # FALLBACK, for a hunk that is no longer on disk verbatim — another PostToolUse + # hook may have reformatted the file between the write and this read — or one + # whose whole text repeats. Individual lines can still locate, so walk them; the + # budget is what keeps that walk from costing anchors TIMES file size. + mapfile -t anchors < <(printf '%s' "$SCAN_CONTENT" | grep -vE '^[[:space:]]*$' 2>/dev/null) + ((${#anchors[@]})) || return 0 + cap=$((RECONSTRUCT_MAX_SCAN_CHARS / (${#content} + 1))) + ((cap < 1)) && cap=1 + ((${#anchors[@]} > cap)) && anchors=("${anchors[@]:0:cap}") + fi # A repeated anchor line resolves identically every time, so scan each once. declare -A seen=() - local ctx="" nspan=0 anchor alen off hs he head tail - local -a offs=() for anchor in "${anchors[@]}"; do alen=${#anchor} ((alen)) || continue diff --git a/plugins/guardrails/hooks/skill-reference-verify.test.sh b/plugins/guardrails/hooks/skill-reference-verify.test.sh index 95ea662efb..4cd19e9838 100755 --- a/plugins/guardrails/hooks/skill-reference-verify.test.sh +++ b/plugins/guardrails/hooks/skill-reference-verify.test.sh @@ -405,10 +405,14 @@ printf 'Run `/alpha:setxx` and note the xx convention.\n' >"$SHORT2" OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" <<<"$(edit_json "$SHORT2" 'xx')" 2>&1) assert_silent "a short anchor occurring twice is still ambiguous" "$OUT" -# A large multiline Edit must not cost a subprocess per hunk line. The hook's own -# timeout is 30 s (hooks.json) and a per-line full-file grep spent it, losing the -# advisory entirely; the ceiling here is deliberately loose so the case fails only -# on the defect, not on a slow host. +# A large multiline Edit must cost neither a subprocess NOR a rescan per hunk line. +# Both are anchors times file size; removing only the subprocess left a thousand-line +# hunk spending the hook's own 30 s timeout (hooks.json) and losing the advisory. The +# hunk is located whole, so this is one scan. The ceiling is deliberately loose so +# the case fails only on the defect, not on a slow host. +# Every line here is span-free ON PURPOSE: a hunk whose lines carry code spans hits +# the span cap and stops early, which would hide the per-line rescan rather than +# measure it. BIGDOC="$REPO/big.md" BIGHUNK="$TEST_TMPDIR/bighunk.txt" : >"$BIGDOC" @@ -432,6 +436,32 @@ else bad "1000-line Edit hunk took ${big_elapsed}s, at or past the hook's 30s timeout" fi +# The whole-hunk locate is a fast path, not the only one. When the text on disk is +# no longer the hunk verbatim — another PostToolUse hook reformatting the file +# between the write and this read is the realistic cause — the per-line walk must +# still recover the reference. Here a blank line separates two lines the Edit wrote +# together, so the whole hunk cannot be found and only the lines can. +SPLIT="$REPO/split.md" +printf 'First edited line.\n\nSecond line with `/alpha:ghost-split`.\n' >"$SPLIT" +OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" \ + <<<"$(edit_json "$SPLIT" 'First edited line. +Second line with `/alpha:ghost-split`.')" 2>&1) +RC=$? +assert_exit "hunk no longer contiguous on disk → exit 0" 0 "$RC" +assert_contains "per-line fallback still recovers the reference" "$OUT" \ + "UNRESOLVED_SKILL: /alpha:ghost-split" + +# A multi-line hunk whose every LINE repeats but whose whole text does not is where +# the whole-hunk locate is strictly better than the per-line walk: the walk drops +# every anchor as ambiguous, while the hunk itself names exactly one place. +PAIR="$REPO/pair.md" +printf 'shared line\nother\nshared line\ntail with `/alpha:ghost-pair`.\n' >"$PAIR" +OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" \ + <<<"$(edit_json "$PAIR" 'shared line +tail with `/alpha:ghost-pair`.')" 2>&1) +assert_contains "a unique whole hunk resolves lines that individually repeat" "$OUT" \ + "UNRESOLVED_SKILL: /alpha:ghost-pair" + # ============ MANIFEST-DECLARED SKILL PATHS ================================= # A manifest may declare `skills` paths, which ADD to the conventional `skills/` # directory rather than replacing it. A command loaded from a declared location is From 3c60f3afc610964383c73ff71d3a641971ed7f60 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 9 Aug 2026 16:48:51 -0400 Subject: [PATCH 5/7] perf(guardrails): stop recomputing the anchor offsets the caller already has The whole-hunk locate leaves this anchor's offsets in `$offs`, and the loop immediately recomputed them. One redundant full-file scan per anchor is the same per-anchor cost the whole-hunk locate exists to remove. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/guardrails/hooks/skill-reference-verify.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/plugins/guardrails/hooks/skill-reference-verify.sh b/plugins/guardrails/hooks/skill-reference-verify.sh index 3a9f62c74a..5a9310f4a2 100755 --- a/plugins/guardrails/hooks/skill-reference-verify.sh +++ b/plugins/guardrails/hooks/skill-reference-verify.sh @@ -364,7 +364,7 @@ reconstruct_partial_edit() { content=${content//$'\r'/} ((${#content} <= RECONSTRUCT_MAX_CHARS)) || return 0 - local ctx="" nspan=0 anchor alen off hs he head tail cap + local ctx="" nspan=0 anchor alen off hs he head tail cap located_whole=0 local -a offs=() anchors=() # The hunk is written to disk CONTIGUOUSLY, so locate it WHOLE first — one scan @@ -376,6 +376,8 @@ reconstruct_partial_edit() { anchor_offsets "$SCAN_CONTENT" if ((${#offs[@]})) && { [[ "$REPLACE_ALL" == "true" ]] || ((${#offs[@]} == 1)); }; then anchors=("$SCAN_CONTENT") + # $offs already holds this anchor's offsets; the loop must not pay for them twice. + located_whole=1 else # FALLBACK, for a hunk that is no longer on disk verbatim — another PostToolUse # hook may have reformatted the file between the write and this read — or one @@ -395,7 +397,7 @@ reconstruct_partial_edit() { ((alen)) || continue [[ -n "${seen[$anchor]:-}" ]] && continue seen["$anchor"]=1 - anchor_offsets "$anchor" + if ((located_whole)); then located_whole=0; else anchor_offsets "$anchor"; fi ((${#offs[@]})) || continue # A non-unique anchor cannot say WHICH occurrence the edit landed on, so it is # dropped rather than unioned. Cost: a missed advisory when an edit lands in From 3e95be502205ffda6f58610f2c7f07725677fd54 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 9 Aug 2026 17:16:30 -0400 Subject: [PATCH 6/7] fix(guardrails): bound reconstruction on the scan's real, quadratic cost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The caps were set against a linear cost model the scan does not have. Measured on a quiescent Windows/Git Bash host, best of three, one anchor_offsets scan costs 0.07 s at 32 KiB, 0.24 s at 64, 0.53 s at 96, 1.07 s at 128, 2.18 s at 192 and 3.94 s at 256 — quadratic, because bash's `%%` pattern strip walks the string instead of indexing it. Extrapolated, the previous 4 MiB file cap allowed a SINGLE scan of about eighteen minutes, so the worst case had never been bounded, only moved off the per-line loop. Reconstruction now stops above 256 KiB, where one scan is still ~4 s, and the fallback's anchor cap falls on the same curve rather than being a flat count or a total-characters budget: 123 anchors at 32 KiB, 30 at 64, 7 at 128, 1 at 256. Above the file cap the direct hunk scan is untouched, so a complete reference is still reported and only partial-edit recovery stops — the permitted direction. Tests: a case that combines a large file WITH the fallback path, which neither the existing timing case (whole-hunk fast path) nor the correctness cases (three lines) reached, and a case above the file cap. Both assert what the bound delivers, not only that it exists — the scale case's hunk carries a bare substring of the reference, so the direct scan cannot report it and a bound that admitted nothing would fail. Also: the unresolved-reference advisory named plugins//skills/ as the place searched whatever the manifest declared. It now lists the directories the search actually covered, from the same skill_roots the resolution used. The message ends by telling the reader to confirm against the tree, so naming the wrong part of it is the one thing it cannot do. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/guardrails/CHANGELOG.md | 23 +++++- .../hooks/skill-reference-verify.sh | 70 ++++++++++++++----- .../hooks/skill-reference-verify.test.sh | 63 +++++++++++++++++ 3 files changed, 137 insertions(+), 19 deletions(-) diff --git a/plugins/guardrails/CHANGELOG.md b/plugins/guardrails/CHANGELOG.md index e21aa5655c..a0285ebcff 100644 --- a/plugins/guardrails/CHANGELOG.md +++ b/plugins/guardrails/CHANGELOG.md @@ -49,9 +49,19 @@ All notable changes to the `guardrails` plugin are documented here. Format follo whole is also strictly better scoping: a hunk whose every line repeats but whose whole text does not used to be dropped as ambiguous line by line, and now resolves to the one place it names. The per-line walk survives as a fallback for a hunk that is no longer on disk verbatim — another - PostToolUse hook reformatting the file between the write and this read is the realistic cause — - under a total scanning budget, because bounding the anchor count or the file size alone leaves - their product free. + PostToolUse hook reformatting the file between the write and this read is the realistic cause. + + Measuring the scan itself then contradicted the bound that had been placed on it. One scan is not + linear in file size, it is QUADRATIC — 0.07 s at 32 KiB, 0.24 s at 64, 1.07 s at 128, 3.94 s at + 256 on the same host, because bash's `%%` pattern strip walks the string rather than indexing it. + A 4 MiB file, which the previous cap allowed, is ~18 minutes for a SINGLE scan, so the guard's + worst case had never actually been bounded, only moved. Reconstruction now stops above 256 KiB, + where one scan is still ~4 s, and the fallback's anchor cap falls along that same curve instead of + being a flat count — 123 anchors at 32 KiB down to one at 256. Above the cap the direct hunk scan + is unaffected, so a complete reference is still reported and only partial-edit recovery stops, + which is this guard's permitted failure direction. Covered by a case that puts a large file and + the fallback path together, which neither the timing case (whole-hunk fast path) nor the + correctness cases (three lines) had done. - **`skill-reference-verify` reported a valid command as unresolved when its plugin declared custom skill paths.** Resolution hard-coded `plugins//skills/`, but a manifest's `skills` key @@ -63,6 +73,13 @@ All notable changes to the `guardrails` plugin are documented here. Format follo stated conditions only — a root `SKILL.md` beside a populated `skills/` is not loaded by Claude Code, so accepting it would suppress the advisory for a command that does not exist. + The advisory's own text was wrong the same way the resolution had been: it named + `plugins//skills/` as the place searched, whatever the manifest declared. The message now + lists the directories the search actually covered, built from the same `skill_roots` the + resolution used. The advisory ends by telling the reader to confirm against the tree, and pointing + them at the wrong part of it is the one instruction that cannot survive being wrong. A plugin + using the conventional layout still reads exactly as before. + ## [0.20.0] ### Changed diff --git a/plugins/guardrails/hooks/skill-reference-verify.sh b/plugins/guardrails/hooks/skill-reference-verify.sh index 5a9310f4a2..640b69e413 100755 --- a/plugins/guardrails/hooks/skill-reference-verify.sh +++ b/plugins/guardrails/hooks/skill-reference-verify.sh @@ -222,21 +222,45 @@ emit_refs() { # Reconstruction is bounded on four axes, because this hook has a 30s timeout # (hooks.json) and a guard that loses its advisory to a timeout has stopped -# guarding. A file too large to hold in a shell variable is not reconstructed at -# all; a hunk stops contributing once it has yielded this many code spans, which is -# far above any real edit given that only spans the anchor reaches are kept; an -# anchor stops being counted one occurrence past the cap, which is all the -# uniqueness gate needs to know; and the per-line FALLBACK below gets a total -# scanning budget rather than an anchor count, because its cost is anchors TIMES -# file size and bounding either factor alone leaves the product free. Measured on -# the slowest host available (Windows/Git Bash): one anchor over a 41 KB file costs -# ~66 ms, so 2 MB of total scanning is ~3 s — inside the budget with the fixed -# per-invocation overhead accounted for. A big file therefore buys fewer anchors, -# down to one. -RECONSTRUCT_MAX_CHARS=4194304 +# guarding. Two of the four are set from a measured cost curve rather than a +# round number, because the cost they bound is QUADRATIC and a linear estimate +# of it is wrong by orders of magnitude at the sizes that matter. +# +# One anchor_offsets scan, measured on the slowest host available (Windows/Git +# Bash, quiescent, best of three): 0.07 s at 32 KiB, 0.24 s at 64 KiB, 0.53 s at +# 96 KiB, 1.07 s at 128 KiB, 2.18 s at 192 KiB, 3.94 s at 256 KiB. That is +# ~0.065 s x (KiB/32)^2 — bash's `%%` pattern strip walks the string it searches +# rather than indexing it, so the scan is quadratic in FILE size no matter how +# short the anchor is. Extrapolated: ~67 s at 1 MiB, ~18 minutes at 4 MiB. +# +# * RECONSTRUCT_MAX_CHARS — above this, reconstruction does not run at all, +# because a SINGLE scan of a larger file already exceeds the hook's budget. +# Complete references in the hunk are still reported by the direct scan; only +# partial-edit recovery is skipped, which is this guard's permitted failure +# direction. 256 KiB is far above any real markdown file and is where one +# scan is still ~4 s. +# * RECONSTRUCT_FALLBACK_SCAN_BUDGET — the per-line fallback scans once per +# anchor, so its cap must fall as the file grows, on the same curve: the +# anchor cap is this budget divided by (KiB)^2. 126000 holds the fallback's +# total scanning near 8 s at every file size on that host — 123 anchors at +# 32 KiB, 30 at 64 KiB, 7 at 128 KiB, 1 at 256 KiB. A flat anchor count +# cannot do this, and neither can a total-characters budget, which assumes +# the linear cost this scan does not have. +# +# The other two are ordinary counts: a hunk stops contributing once it has +# yielded this many code spans, far above any real edit given that only spans the +# anchor reaches are kept; and an anchor stops being counted one occurrence past +# the cap, which is all the uniqueness gate needs to know. +# +# Deferred alternative, with its trigger: replacing the bash search with a +# linear-time locate would retire the file cap instead of living under it. It is +# not done here because every portable option is a subprocess whose offsets are +# BYTES where these are characters, or a line-oriented tool that cannot match a +# multi-line hunk at all. Revisit if a real markdown file ever exceeds the cap. +RECONSTRUCT_MAX_CHARS=262144 RECONSTRUCT_MAX_SPANS=40 RECONSTRUCT_MAX_OCCURRENCES=40 -RECONSTRUCT_MAX_SCAN_CHARS=2097152 +RECONSTRUCT_FALLBACK_SCAN_BUDGET=126000 # Append to the caller's $ctx every inline-code span in whose extent # OVERLAPS [, ), and count them in the caller's $nspan. @@ -364,7 +388,7 @@ reconstruct_partial_edit() { content=${content//$'\r'/} ((${#content} <= RECONSTRUCT_MAX_CHARS)) || return 0 - local ctx="" nspan=0 anchor alen off hs he head tail cap located_whole=0 + local ctx="" nspan=0 anchor alen off hs he head tail cap kib located_whole=0 local -a offs=() anchors=() # The hunk is written to disk CONTIGUOUSLY, so locate it WHOLE first — one scan @@ -385,7 +409,9 @@ reconstruct_partial_edit() { # budget is what keeps that walk from costing anchors TIMES file size. mapfile -t anchors < <(printf '%s' "$SCAN_CONTENT" | grep -vE '^[[:space:]]*$' 2>/dev/null) ((${#anchors[@]})) || return 0 - cap=$((RECONSTRUCT_MAX_SCAN_CHARS / (${#content} + 1))) + kib=$(((${#content} + 1023) / 1024)) + ((kib < 1)) && kib=1 + cap=$((RECONSTRUCT_FALLBACK_SCAN_BUDGET / (kib * kib))) ((cap < 1)) && cap=1 ((${#anchors[@]} > cap)) && anchors=("${anchors[@]:0:cap}") fi @@ -493,10 +519,22 @@ emit_tel() { if ((${#UNRESOLVED[@]} > 0)); then hook::ctx_append "skill-reference-verify: ${#UNRESOLVED[@]} skill reference(s) do not resolve in $FILE" hook::ctx_append "This repo owns each plugin named below, so it can say the skill is not there:" + # Name the directories the search ACTUALLY covered, from the same skill_roots + # the resolution used. Naming only `skills/` understates the search for a plugin + # whose manifest declares paths, and the advisory ends by telling the reader to + # confirm against the tree — pointing them at the wrong part of it is the one + # thing that instruction cannot survive. For the conventional layout this still + # renders exactly `plugins//skills/`. + declare -a roots=() for r in "${UNRESOLVED[@]}"; do plugin="${r#/}" plugin="${plugin%%:*}" - hook::ctx_append " UNRESOLVED_SKILL: $r (no such skill under plugins/${PLUGIN_DIR[$plugin]##*/}/skills/)" + skill_roots "${PLUGIN_DIR[$plugin]}" "${PLUGIN_SKILL_PATHS[$plugin]:-}" + searched="" + for root in "${roots[@]}"; do + searched+="${searched:+, }plugins/${root#"$PLUGINS_DIR/"}/" + done + hook::ctx_append " UNRESOLVED_SKILL: $r (no such skill under $searched)" done hook::ctx_append "" hook::ctx_append "Detect-then-judge: this is a prompt for your verdict, not a determination." diff --git a/plugins/guardrails/hooks/skill-reference-verify.test.sh b/plugins/guardrails/hooks/skill-reference-verify.test.sh index 4cd19e9838..c92d0f67a0 100755 --- a/plugins/guardrails/hooks/skill-reference-verify.test.sh +++ b/plugins/guardrails/hooks/skill-reference-verify.test.sh @@ -462,6 +462,69 @@ tail with `/alpha:ghost-pair`.')" 2>&1) assert_contains "a unique whole hunk resolves lines that individually repeat" "$OUT" \ "UNRESOLVED_SKILL: /alpha:ghost-pair" +# SCALE meets the FALLBACK. The two cases above force the fallback but on three +# lines, and the timing case above is large but takes the whole-hunk fast path, so +# neither measures what the original defect actually cost: one scan PER ANCHOR over +# a big file. This case combines them — a file near RECONSTRUCT_MAX_CHARS and a +# many-line hunk that a reformat has made non-contiguous — and it is the case the +# fallback's anchor cap exists for. +# +# The cap is what the assertions are about, so they assert what a bound DELIVERS, +# not merely that it exists. At this file size the budget buys about one anchor, so +# the first hunk line carries the reference: a bound that admitted nothing would +# pass a timing-only check while having stopped guarding. +# +# The hunk's first line is a bare SUBSTRING of the reference, never the reference +# itself, so the direct hunk scan cannot see it and only reconstruction can report +# it — the assertion below would otherwise pass on the direct scan alone and prove +# nothing about the fallback. +FBIG="$REPO/fallback-big.md" +FBIGHUNK="$TEST_TMPDIR/fallback-hunk.txt" +{ + printf 'ghost-fallback\n' + for ((i = 1; i <= 400; i++)); do printf 'Body line %s of the reformatted passage.\n' "$i"; done +} >"$FBIGHUNK" +# On disk that first line reads as part of a code span, so the hunk is not +# contiguous here and the whole-hunk locate cannot match it. ~200 KiB of unrelated +# prose follows, putting the file just under RECONSTRUCT_MAX_CHARS. +{ + printf 'Opening line with `/alpha:ghost-fallback` in it.\n' + for ((i = 1; i <= 400; i++)); do printf 'Body line %s of the reformatted passage.\n' "$i"; done + for ((i = 1; i <= 2600; i++)); do + printf 'Filler paragraph %s padding this file toward the reconstruction cap.\n' "$i" + done +} >"$FBIG" +fb_start=$SECONDS +OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" <<<"$(edit_json "$FBIG" "$(cat "$FBIGHUNK")")" 2>&1) +RC=$? +fb_elapsed=$((SECONDS - fb_start)) +assert_exit "large file forced onto the fallback path → exit 0" 0 "$RC" +if ((fb_elapsed < 30)); then + ok "fallback at scale stays inside the hook's 30s timeout (${fb_elapsed}s)" +else + bad "fallback at scale took ${fb_elapsed}s, at or past the hook's 30s timeout" +fi +assert_contains "the capped fallback still reports the reference it admits" "$OUT" \ + "UNRESOLVED_SKILL: /alpha:ghost-fallback" + +# Above the cap, reconstruction does not run at all — one scan of a file that size +# would spend the budget by itself. The direct hunk scan is unaffected, so a +# COMPLETE reference in the hunk is still reported; only partial-edit recovery +# stops, which is this guard's permitted failure direction. +OVER="$REPO/over-cap.md" +{ + printf 'Complete reference `/alpha:ghost-over` written by this edit.\n' + for ((i = 1; i <= 4200; i++)); do + printf 'Filler paragraph %s pushing this file past the reconstruction cap.\n' "$i" + done +} >"$OVER" +OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" \ + <<<"$(edit_json "$OVER" 'Complete reference `/alpha:ghost-over` written by this edit.')" 2>&1) +RC=$? +assert_exit "file past RECONSTRUCT_MAX_CHARS → exit 0" 0 "$RC" +assert_contains "the direct scan still reports a complete reference above the cap" "$OUT" \ + "UNRESOLVED_SKILL: /alpha:ghost-over" + # ============ MANIFEST-DECLARED SKILL PATHS ================================= # A manifest may declare `skills` paths, which ADD to the conventional `skills/` # directory rather than replacing it. A command loaded from a declared location is From 764e1929c84c4e9c0af6ed5e0fc5168aea132ce4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 9 Aug 2026 17:47:01 -0400 Subject: [PATCH 7/7] fix(guardrails): pin the reconstruction cap by behavior, not by wall clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retunes the two measured bounds and stops asserting them with a clock. The first pair (256 KiB file cap, 126000 budget) was derived from an isolated scan curve that is a FLOOR rather than the cost: it times 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.31 s at 200 KiB) and the whole-hunk probe pays a scan before the fallback runs at all. End to end that pair spent 21 s of a 30 s budget. The cap is now 128 KiB with a 60000 budget, calibrated against the hook rather than the curve. The scale case no longer asserts wall time. On this host the same fixture read 21 s loaded and a smaller one 23 s, against an isolated scan of ~1 s at that size — a timing assertion that noisy fails on load and passes on a regression that happens to run on a quiet box. It now pins the cap from both sides instead: a reference on a hunk line inside the anchor cap is still reported, one on a line far past it is not. Both are reachable only through reconstruction, so neither can pass on the direct hunk scan. Two assertions added where a prefix match was hiding the subject: the advisory's directory list is now asserted whole for a plugin with declared paths, and the conventional layout is pinned to the rendering it always had. The file-size gate moves ahead of the CR strip. Every step past that gate is a whole-string operation, so a gate placed after one of them leaves that one unbounded; measuring the raw text also errs toward not reconstructing. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/guardrails/CHANGELOG.md | 21 ++++--- .../hooks/skill-reference-verify.sh | 36 +++++++---- .../hooks/skill-reference-verify.test.sh | 61 ++++++++++++------- 3 files changed, 77 insertions(+), 41 deletions(-) diff --git a/plugins/guardrails/CHANGELOG.md b/plugins/guardrails/CHANGELOG.md index a0285ebcff..dec7937cf6 100644 --- a/plugins/guardrails/CHANGELOG.md +++ b/plugins/guardrails/CHANGELOG.md @@ -55,13 +55,20 @@ All notable changes to the `guardrails` plugin are documented here. Format follo linear in file size, it is QUADRATIC — 0.07 s at 32 KiB, 0.24 s at 64, 1.07 s at 128, 3.94 s at 256 on the same host, because bash's `%%` pattern strip walks the string rather than indexing it. A 4 MiB file, which the previous cap allowed, is ~18 minutes for a SINGLE scan, so the guard's - worst case had never actually been bounded, only moved. Reconstruction now stops above 256 KiB, - where one scan is still ~4 s, and the fallback's anchor cap falls along that same curve instead of - being a flat count — 123 anchors at 32 KiB down to one at 256. Above the cap the direct hunk scan - is unaffected, so a complete reference is still reported and only partial-edit recovery stops, - which is this guard's permitted failure direction. Covered by a case that puts a large file and - the fallback path together, which neither the timing case (whole-hunk fast path) nor the - correctness cases (three lines) had done. + worst case had never actually been bounded, only moved. Reconstruction now stops above 128 KiB, + and the fallback's anchor cap falls along that same curve instead of being a flat count — 58 + anchors at 32 KiB, 14 at 64, 3 at 128. Above the cap the direct hunk scan is unaffected, so a + complete reference is still reported and only partial-edit recovery stops, which is this guard's + permitted failure direction. Both numbers are calibrated end to end against the hook rather than + from the isolated curve, which understates the cost: it times an anchor that matches near the end, + where one strip walks the file and the second is free, while a no-match strip walks it twice and + the whole-hunk probe pays a scan before the fallback runs at all. Covered by a case that puts a + large file and the fallback path TOGETHER, which neither the timing case (whole-hunk fast path) + nor the correctness cases (three lines) reached. That case asserts what the cap DOES rather than + how long it takes — one reference inside the cap is still reported, one past it is not — because a + wall-clock bound there measures the host: the same fixture read 21 s loaded and a smaller one 23 s, + against an isolated scan of ~1 s at that size. A timing assertion that noisy fails on load and + passes on a regression that happens to run on a quiet box. - **`skill-reference-verify` reported a valid command as unresolved when its plugin declared custom skill paths.** Resolution hard-coded `plugins//skills/`, but a manifest's `skills` key diff --git a/plugins/guardrails/hooks/skill-reference-verify.sh b/plugins/guardrails/hooks/skill-reference-verify.sh index 640b69e413..cb73861c96 100755 --- a/plugins/guardrails/hooks/skill-reference-verify.sh +++ b/plugins/guardrails/hooks/skill-reference-verify.sh @@ -233,19 +233,27 @@ emit_refs() { # rather than indexing it, so the scan is quadratic in FILE size no matter how # short the anchor is. Extrapolated: ~67 s at 1 MiB, ~18 minutes at 4 MiB. # +# Those figures are the FLOOR, not the cost: they time an anchor that matches near +# the end, so one strip walks the file and the second is free. A no-match strip +# walks it twice (2.31 s at 200 KiB, measured the same way), and the whole-hunk +# probe pays a scan of its own before the fallback runs at all. Both bounds below +# are therefore calibrated end to end against the hook, not from that table. +# # * RECONSTRUCT_MAX_CHARS — above this, reconstruction does not run at all, -# because a SINGLE scan of a larger file already exceeds the hook's budget. -# Complete references in the hunk are still reported by the direct scan; only +# because scanning a larger file already spends the hook's budget. Complete +# references in the hunk are still reported by the direct scan; only # partial-edit recovery is skipped, which is this guard's permitted failure -# direction. 256 KiB is far above any real markdown file and is where one -# scan is still ~4 s. +# direction. 128 KiB is far above any real markdown file this guard reads +# (CHANGELOGs, the one shape that grows without bound, are excluded upstream). # * RECONSTRUCT_FALLBACK_SCAN_BUDGET — the per-line fallback scans once per # anchor, so its cap must fall as the file grows, on the same curve: the -# anchor cap is this budget divided by (KiB)^2. 126000 holds the fallback's -# total scanning near 8 s at every file size on that host — 123 anchors at -# 32 KiB, 30 at 64 KiB, 7 at 128 KiB, 1 at 256 KiB. A flat anchor count -# cannot do this, and neither can a total-characters budget, which assumes -# the linear cost this scan does not have. +# anchor cap is this budget divided by (KiB)^2. 60000 holds the whole +# invocation near 7 s at every file size on that host — 58 anchors at 32 KiB, +# 14 at 64 KiB, 3 at 128 KiB. A flat anchor count cannot do this, and neither +# can a total-characters budget, which assumes the linear cost this scan does +# not have. The end-to-end scale case in the test suite is what holds these +# two numbers honest; an earlier pair passed the same case at 21 s of a 30 s +# budget, which is a bound in name only. # # The other two are ordinary counts: a hunk stops contributing once it has # yielded this many code spans, far above any real edit given that only spans the @@ -257,10 +265,10 @@ emit_refs() { # not done here because every portable option is a subprocess whose offsets are # BYTES where these are characters, or a line-oriented tool that cannot match a # multi-line hunk at all. Revisit if a real markdown file ever exceeds the cap. -RECONSTRUCT_MAX_CHARS=262144 +RECONSTRUCT_MAX_CHARS=131072 RECONSTRUCT_MAX_SPANS=40 RECONSTRUCT_MAX_OCCURRENCES=40 -RECONSTRUCT_FALLBACK_SCAN_BUDGET=126000 +RECONSTRUCT_FALLBACK_SCAN_BUDGET=60000 # Append to the caller's $ctx every inline-code span in whose extent # OVERLAPS [, ), and count them in the caller's $nspan. @@ -382,11 +390,15 @@ reconstruct_partial_edit() { [[ "$TOOL" == "Edit" && -f "$FILE" ]] || return 0 local content content=$(<"$FILE") || return 0 + # Gate on the size BEFORE touching the string: every step past this point is a + # whole-string operation, so a gate that runs after one of them has left that one + # unbounded. Measuring the raw text rather than the normalized text also errs + # toward not reconstructing, which is the safe direction here. + ((${#content} <= RECONSTRUCT_MAX_CHARS)) || return 0 # new_string arrived with CR stripped, so a CRLF file must be searched the same # way or no anchor would ever locate. Offsets are read back off this normalized # text only, never off the bytes on disk. content=${content//$'\r'/} - ((${#content} <= RECONSTRUCT_MAX_CHARS)) || return 0 local ctx="" nspan=0 anchor alen off hs he head tail cap kib located_whole=0 local -a offs=() anchors=() diff --git a/plugins/guardrails/hooks/skill-reference-verify.test.sh b/plugins/guardrails/hooks/skill-reference-verify.test.sh index c92d0f67a0..3cfc2b6763 100755 --- a/plugins/guardrails/hooks/skill-reference-verify.test.sh +++ b/plugins/guardrails/hooks/skill-reference-verify.test.sh @@ -117,6 +117,10 @@ mkdir -p "$REPO/plugins/alpha/skills/ghost" OUT=$(run 'Run `/alpha:ghost`.') assert_contains "skills/ dir without SKILL.md → still unresolved" "$OUT" \ "UNRESOLVED_SKILL: /alpha:ghost" +# Naming the searched directories must not have changed how the conventional +# layout reads — one root, rendered exactly as it always was. +assert_contains "a plugin declaring no paths still reads as plugins//skills/" "$OUT" \ + "(no such skill under plugins/alpha/skills/)" # A frontmatter name carrying a trailing YAML comment is a valid rename and must # resolve; the old end-of-line-anchored parser extracted nothing. @@ -469,43 +473,50 @@ assert_contains "a unique whole hunk resolves lines that individually repeat" "$ # many-line hunk that a reformat has made non-contiguous — and it is the case the # fallback's anchor cap exists for. # -# The cap is what the assertions are about, so they assert what a bound DELIVERS, -# not merely that it exists. At this file size the budget buys about one anchor, so -# the first hunk line carries the reference: a bound that admitted nothing would -# pass a timing-only check while having stopped guarding. +# What the cap DOES is asserted, not how long it takes. A wall-clock bound here +# measures the host: the same fixture read 21 s loaded and the smaller one below +# read 23 s, while the isolated scan it is supposed to be bounding is ~1 s at this +# size. A timing assertion that noisy is worse than none — it fails on load and +# passes on a regression that happens to run on a quiet box. The scan cost itself +# is measured directly (see the constants' docblock); what a test can pin +# deterministically is the cap's BEHAVIOR, so this case pins it from both sides. # -# The hunk's first line is a bare SUBSTRING of the reference, never the reference -# itself, so the direct hunk scan cannot see it and only reconstruction can report -# it — the assertion below would otherwise pass on the direct scan alone and prove -# nothing about the fallback. +# Both references are reached only through reconstruction: each hunk line carrying +# one is a bare SUBSTRING, never the reference itself, so the direct hunk scan +# cannot see either and an assertion cannot pass on the direct scan alone. FBIG="$REPO/fallback-big.md" FBIGHUNK="$TEST_TMPDIR/fallback-hunk.txt" { printf 'ghost-fallback\n' - for ((i = 1; i <= 400; i++)); do printf 'Body line %s of the reformatted passage.\n' "$i"; done + for ((i = 2; i <= 300; i++)); do printf 'Body line %s of the reformatted passage.\n' "$i"; done + printf 'ghost-deep\n' + for ((i = 302; i <= 401; i++)); do printf 'Body line %s of the reformatted passage.\n' "$i"; done } >"$FBIGHUNK" -# On disk that first line reads as part of a code span, so the hunk is not -# contiguous here and the whole-hunk locate cannot match it. ~200 KiB of unrelated -# prose follows, putting the file just under RECONSTRUCT_MAX_CHARS. +# On disk those two lines read as part of code spans, so the hunk is not contiguous +# here and the whole-hunk locate cannot match it. Unrelated prose follows, putting +# the file just under RECONSTRUCT_MAX_CHARS (128 KiB). { printf 'Opening line with `/alpha:ghost-fallback` in it.\n' - for ((i = 1; i <= 400; i++)); do printf 'Body line %s of the reformatted passage.\n' "$i"; done - for ((i = 1; i <= 2600; i++)); do + for ((i = 2; i <= 300; i++)); do printf 'Body line %s of the reformatted passage.\n' "$i"; done + printf 'Deep line with `/alpha:ghost-deep` in it.\n' + for ((i = 302; i <= 401; i++)); do printf 'Body line %s of the reformatted passage.\n' "$i"; done + for ((i = 1; i <= 1450; i++)); do printf 'Filler paragraph %s padding this file toward the reconstruction cap.\n' "$i" done } >"$FBIG" -fb_start=$SECONDS OUT=$(CLAUDE_PROJECT_DIR="$REPO" bash "$HOOK" <<<"$(edit_json "$FBIG" "$(cat "$FBIGHUNK")")" 2>&1) RC=$? -fb_elapsed=$((SECONDS - fb_start)) assert_exit "large file forced onto the fallback path → exit 0" 0 "$RC" -if ((fb_elapsed < 30)); then - ok "fallback at scale stays inside the hook's 30s timeout (${fb_elapsed}s)" -else - bad "fallback at scale took ${fb_elapsed}s, at or past the hook's 30s timeout" -fi +# The bound ADMITS: the first hunk line is inside the anchor cap, so its reference +# is still recovered. A cap that had collapsed to nothing would stop guarding while +# passing every timing check ever written. assert_contains "the capped fallback still reports the reference it admits" "$OUT" \ "UNRESOLVED_SKILL: /alpha:ghost-fallback" +# The bound BINDS: hunk line 301 is far past the cap this file size buys, so its +# reference is not reached. Asserting the silence is what makes the cap observable +# without a clock — if the anchor walk ever went unbounded again, this fails. +assert_absent "the fallback stops at the anchor cap instead of walking the hunk" "$OUT" \ + "UNRESOLVED_SKILL: /alpha:ghost-deep" # Above the cap, reconstruction does not run at all — one scan of a file that size # would spend the budget by itself. The direct hunk scan is unaffected, so a @@ -514,7 +525,7 @@ assert_contains "the capped fallback still reports the reference it admits" "$OU OVER="$REPO/over-cap.md" { printf 'Complete reference `/alpha:ghost-over` written by this edit.\n' - for ((i = 1; i <= 4200; i++)); do + for ((i = 1; i <= 2200; i++)); do printf 'Filler paragraph %s pushing this file past the reconstruction cap.\n' "$i" done } >"$OVER" @@ -559,6 +570,12 @@ assert_silent "declared paths add to skills/, they do not replace it" "$OUT" OUT=$(run 'Run `/gamma:nowhere`.') assert_contains "declared paths do not suppress a genuinely missing command" "$OUT" \ "UNRESOLVED_SKILL: /gamma:nowhere" +# The message names WHERE it looked, and the advisory's next line tells the reader +# to confirm against the tree — so the full rendering is asserted, not its prefix. +# A prefix match passes on any wrong directory list, which is the whole subject +# here. +assert_contains "the advisory names every directory the search covered" "$OUT" \ + "(no such skill under plugins/gamma/skills/, plugins/gamma/custom/extras/, plugins/gamma/solo/)" # A declared skill's DIRECTORY name is no more an alias than a conventional one's. OUT=$(run 'Run `/gamma:solo`.')