fix: bind eleven more failure-token probes to their probe, and quote a spaced skill dir - #3474
Merged
Merged
Conversation
…a spaced skill dir #3469 fixed five filtered probes whose `||` fallback fires spuriously under `set -o pipefail`, and stopped there. The same shape is still live at eleven more sites across six plugins, four of them introduced by #3468 -- the commit immediately before the one that diagnosed the bug class. The mechanism is unchanged. `guard >/dev/null 2>&1 && probe | cap || echo TOKEN` takes the pipeline's exit status. Without pipefail that is the cap's, always 0. With pipefail it is the last non-zero status, which two ordinary situations produce: a filter matching nothing, and the writer taking SIGPIPE when the cap closes the pipe. Either fires the failure token on a healthy probe. ai-slop:audit is the worst and was live, not latent. detect.sh --show-config emits 10 lines against a head -8 cap, writing one at a time, so head always closes the pipe mid-run: PIPESTATUS reads 141 0. The probe rendered the full correct config and then appended "detector unavailable" beneath it. It reproduced from any directory with no configuration at all. knowledge is the one with a consequence beyond a wrong string. video-digest states "STOP if the pre-computed context above shows MISSING for yt-dlp, ffmpeg, or ImageMagick. Cloud agents without the media toolchain fail closed." #3468 fixed that gate's fail-open defect and shipped this inversion in its place, so the gate would refuse to run on a machine that HAS the toolchain. Whether the token fires is a race between the tool writing its banner and the cap closing the pipe, so it scales with output size. Measured here over 30 runs per size against a synthetic tool on PATH, installed and working: 41 lines fired 29/30, 5 lines fired 19/30, 1 line fired 0/30. So ffmpeg fails on nearly every run, magick (5 lines) is a genuine race, and yt-dlp (1 line) is latent by shape only. An intermittent false MISSING is harder to diagnose than a consistent one; all three are fixed regardless. Also fixed here, same shape: course-digest's two probes, firecrawl:firecrawl (pre-existing, and the idiom the fleet copied), provenance:audit's two detector probes, docs-hygiene:audit-derivability -- whose capture-first form protects only the git call, leaving its awk-and-head data run exposed past 20 dirty files -- and code-tidying:tidy, where a bare third "unknown" line was appended to well-formed output. Two of the eleven are latent by shape, not observed failures, and the provenance changelog says so plainly rather than claiming a live fix: list-corpus.sh --show-config emits 7 lines against a head -10 cap, and check-stamps.sh is piped into tail -3, which drains its input and cannot raise SIGPIPE. Both were verified to behave identically before and after in three states under both settings. Separately, ${CLAUDE_SKILL_DIR} is now quoted in ai-slop:audit and provenance:audit. Installed under a path containing a space, the unquoted expansion made a working detector report itself unavailable. firecrawl:update already quoted it; these now match. Pre-existing, but these commits are what made the token reachable at all -- before them the same failure rendered empty. Quoting changes the literal command string, and Bash permission rules are globs over that literal string, so the unquoted script grants no longer match the quoted invocations. Three companion rules are added, one per quoted script: Bash("${CLAUDE_SKILL_DIR}/scripts/detect.sh":*) in ai-slop, and the list-corpus and check-stamps equivalents in provenance. The unquoted rules are kept because both skill bodies still instruct unquoted invocation elsewhere. Each pair names one script under the same ${CLAUDE_SKILL_DIR} anchor with the same :* argument scope, so nothing is authorized that the plugin could not already run. No other grant changed: every other rewritten command keeps its original leading token, which is why code-tidying:tidy takes the ||-inside-the-brace-group form rather than the brace-group-first one. The remedy elsewhere is the brace group #3469 established: the data pipeline sits in { ...; :; }, closed by a command that cannot fail, so the || is reachable only when the guard short-circuits. No new idiom, and no $ expansion introduced beyond the quoted ${CLAUDE_SKILL_DIR}; audit-derivability keeps the pre-existing $s capture, which is the part that legitimately drives its ||. Every site proven by execution in three states -- tool or script absent, output under the cap, output over the cap -- with and without pipefail. The five sites #3469 fixed were re-verified at 3,000 dirty files under both settings and are correct as shipped, as is claude-ops:observability, whose sed drains its input. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UmrZGdp1dgbZuPCy7tcRJo
claude Bot
added a commit
that referenced
this pull request
Aug 28, 2026
…five wrong keeps (#3475) #3468 closed 16 cross-plugin path citations under ADR 0018 and kept 23 on a test it never wrote down. Its own verifier reported '3 keeps refuted; 4 cannot verify' and that reasoning was unreadable, because the verifier ran as a remote session the agent could not read back. An independent verifier settled it: five are wrong, not three, and it found why this recurred. The dividing test #3468 applied -- fix a citation that is an ADDRESS for an obligation, keep one that is EVIDENCE about this tree at a moment in time -- appears nowhere in ADR 0018. Clause 2 reads unconditionally. But ADR 0018 is itself a docs/** file and path-cites plugin skill privates five times, quoting their contents inline. Either it violates itself on its own first page, or clause 2 is scoped to citations that function as an address. Only the second reading is coherent, and an unwritten rule is what let one commit rule two ways in two files. This writes it down, as an Amendment following ADR 0011's convention: the evidence/address test, scripts/lib being outside the entry-surface carve-out, and how plugin-level non-skill trees generalize. The five wrong keeps. shell-test-helpers cited a private scripts/lib/ path -- and that is a self-contradiction inside c66f26c itself, which added detector-findings 2.8.1 fixing the byte-identical shape and stating the ground, then kept it one file over. Three line and step pins are dropped as the same class 2.7.1 removed a :414 pin for; each citation already quotes its content inline, so the drop is lossless. records.json:3's keep-ground was false: the sweep kept it because generation overwrites hand-edits, but that store's own note says 'Hand-editable and human-gated'. Recorded as open remainder rather than swept: twelve citations c66f26c created itself, every fix having written a changelog entry quoting the path it removed, so the population is 16 fixed and 35 kept rather than 16 and 23; three trees excluded by fiat with no carve-out authorizing it; 51 citations into plugin-level non-skill trees; and a stale dometrain security record whose own version-bump re-trigger has fired unnoticed. A fifth review round found two more false claims, both stale statuses. '34 to 32' was wrong: re-deriving all 34 rows against the roster's own text test finds 22 already closed, twelve of them by #3380, the same commit that last wrote the roster and left its own 'all 34 re-verified' line standing over twelve citations it had just deleted. This change set republished that status in a file whose line 18 reads 'the check is the text, never the status and never the line number.' And the spec still shipped the probe shape #3469 removed, presented as verified in three states, two of which print the failure token under pipefail. Refs #3468, refs #3469, refs #3460. Sibling #3474.
claude Bot
added a commit
that referenced
this pull request
Aug 28, 2026
…test, and record the pipefail question (#3476) The last five findings from an independent security review of #3460/#3468/ #3469. #3474 and #3475 closed the probe-shape and citation halves; this closes the filter half and writes the open question into the convention. Four probes silently under-reported any filename git status --porcelain C-quotes. A plain space is enough to defeat grep '\.md$' and awk '{print $NF}'. Executed against a repo holding seven files per extension (plain, space, single quote, double quote, semicolon, pipe, embedded newline), each of the four went from catching 4 of 7 to 7 of 7. Two correct forms already shipped in the same two plugins, so this was drift rather than a missing idea: the .md sites took audit-noise's grep form, which keeps their leading token and so keeps their grant valid, and dissolve-comments took audit-comment-residue's -z NUL parse. The residual is stated in the changelogs rather than hidden. audit-comment-residue's parity test could not see an over-reporting preview. Its loop only checked one direction, so mutating the SKILL.md awk to drop its rename skip made the preview emit a phantom path while detect.sh audited five real ones, and the suite still passed 53/53. A reverse loop now asserts every previewed path is one detect.sh audited: the same mutation fails 1/54 naming the phantom, and reverted it passes 54/54. The forward loop is byte-identical. claude-ops:observability rendered 'unknown' both outside a repo and inside one whose toplevel directory is literally named unknown. The failure case now renders a distinct token. Six labels asserted 'empty = none' in two states the probe does not establish: the brace group's : makes the outer || unreachable, which also masks a filter-stage failure and a guard/data double invocation. The labels now say 'empty = none matched or the probe returned nothing'. No plumbing restructured; the ~25 unfiltered probes were left alone because their claim is already true. The durable fix is in precompute-context.md: whether Claude Code runs ! injections under pipefail is undocumented, the brace group is correct under both settings while guard && pipeline || token is correct under only one, and the brace group's own price is that an internal failure renders empty, which is why the label must carry it. The recheck trigger now covers shell options. Two derivations unrelated to the review's table both return the same six filtered probes: the true set is six, four defective, not five and three. #3474 fixed audit-derivability's SIGPIPE shape hours earlier and left its filter untouched, so both defects landed on one line; this branch was rebuilt onto current main and carries both, since committing either alone would have reverted the other. 70 pipefail cells all exit 0; affected-tests real exit 0 with 17/17 suites; changelog parity including --check-preserved across 306 headings; 3061 plugin files validated; markdownlint clean over 173 files. No grant widened or broken. Refs #3474, refs #3475, refs #3469, refs #3468, refs #3460.
claude Bot
added a commit
that referenced
this pull request
Aug 28, 2026
…, and flag a fired security re-trigger (#3477) Every one of #3468's ADR-0018 citation fixes wrote a CHANGELOG entry quoting the path it removed, creating twelve new instances of the shape it was closing. The population at main is 16 fixed and 35 kept, not 16 and 23. #3475 wrote the governing test into ADR 0018; this applies it to those twelve so a later re-derivation does not re-open them. All twelve are keep-correct, established row by row and anchored on text rather than line numbers. A dated changelog entry quoting a citation it removed is the evidence case exactly. No CHANGELOG was edited, so no convention bump is owed. Three sub-rulings the class needed, now written into the spec rather than left implicit. config-cascade's three rows are the only clause-3 question in the set: they are plugin-relative and resolve against nothing, and the entry says so in the same sentence. Clause 3 governs addresses a doc offers, not strings it quotes in order to report them broken, and forcing resolution would delete the finding. The other nine resolve on disk today, verified one path at a time across eleven distinct targets. No row carries a line or step pin, the part the amendment says rots first. One excluded on purpose and recorded so it is not re-opened: plugins/review/reference/topic-docs.md, a plugin-level non-skill tree the amendment routes to its own pass, which the entry declares a keep rather than quoting as removed. Separately, MIGRATION-PLAYBOOK.md records a dometrain security review performed at 0.1.0 and states that a version bump adding a new trust surface re-triggers it. The manifest now reads 0.2.7, eleven releases later. The note added states the reviewed version, the shipping version, that the ACCEPT below it describes 0.1.0 only, and that the re-review is owed and deliberately not performed here. It stops short of asserting a trust surface was added: that is the condition the clause turns on and cannot be settled by reading the page. Two checks were run to avoid claiming otherwise and neither settles it, which the note says. The spec's dometrain entry pinned MIGRATION-PLAYBOOK.md:943, a line this change's own edit would have invalidated. Re-anchored on the record's heading text. That decay rule has been violated three times across this sweep; this is the first time it was caught before landing. Recorded because two earlier claims about it were wrong in opposite directions: this worker's definition declares the Agent tool and the harness withheld it at spawn, so nesting is gated by depth realized as pre-launch tool omission. There is no refusal string because there is no call to refuse. Refs #3476, refs #3475, refs #3474, refs #3469, refs #3468, refs #3460.
claude Bot
added a commit
that referenced
this pull request
Aug 28, 2026
…reopened it three times (#3478) The L4 roster's Group 2, eight intra-plugin path-form citations, was the last block anyone believed open. All eight were already closed, by 6c7a103 (#3380) on 2026-08-26 -- the same commit that wrote the roster. Its own message names what it did: eight citations written with an implied base of the plugin root while the real base was reference/, none of which resolved for any reader. Verified one at a time against the roster's own text test. All eight now read ${CLAUDE_PLUGIN_ROOT}/skills/<skill>/<path> and all seven distinct targets exist on disk. Zero plugins/** files edited, so no bump and no plugin changelog entry: nothing needed changing. The reason four separate passes each re-derived this roster from scratch is one sentence. #3380 fixed 32 of 34 rows and left its summary asserting that all 34 path:line citations were re-verified on 2026-08-26 and every one resolves -- standing over citations it had just deleted. A fifth-round audit caught that for Group 1, where 22 of 32 were already closed and twelve of those by #3380 itself. This closes the same hole for Group 2. The generalizable rule is now in the sweep spec: a record that fixes findings and updates its own summary in the same commit must update the summary, or the summary outranks the fix for every later reader. The roster's inventory rows are left verbatim, per its own decay rule that the inventory is the part that cannot be re-derived, and per the precedent that #3474 and #3475 both closed rows without touching the file. Only an additive closure stamp is appended. On whether ADR 0018 reaches this class, the honest answer is that its encapsulation half does not. Clause 1 legalises intra-plugin citations and names this exact citing surface. Clause 2 cannot apply, because both files ship inside one plugin so the runtime absence motivating it cannot occur, and the 2026-08-28 amendment's fix-an-address / keep-evidence test divides clause 2 applications only. Only clause 3 reaches Group 2, and clause 3 is a resolvability rule rather than an encapsulation one: had these been open, the remedy would have been path form and nothing else. The clause that earned its keep is the ADR's own observation that proximity did not prevent them. A second derivation making no reference to the roster resolved every citation token in every plugin-level reference/, context/ and agents/ tree plus every plugin README against the base its own form implies: 52 tokens, 0 clause-3 failures. Group 3's two anchors were also closed by #3380. 34 closed, 0 open. No fresh-context verifier reviewed this diff. Nested spawning is unavailable at this depth: the Agent tool is withheld pre-launch, so there is no call to refuse and no refusal string. A second mechanical derivation was substituted and every line reference re-checked against the live tree, which is weaker than a fresh context and is recorded as such. Refs #3477, refs #3476, refs #3475, refs #3474, refs #3469, refs #3468.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No linked issue
Summary
#3469 fixed five filtered probes whose
||fallback fires spuriously underset -o pipefail, and stopped there. Two independent verifiers found the same shape still live at eleven more sites across six plugins — four of them introduced by #3468, the commit immediately before the one that diagnosed the bug class.The mechanism is unchanged.
guard >/dev/null 2>&1 && probe | cap || echo TOKENtakes the pipeline's exit status. Without pipefail that is the cap's, always 0. With pipefail it is the last non-zero status, which two ordinary situations produce: a filter matching nothing, and the writer taking SIGPIPE when the cap closes the pipe. Either fires the failure token on a healthy probe.Fix
The two that matter
ai-slop:auditwas live and unconditional.detect.sh --show-configemits 10 lines against ahead -8cap, writing them one at a time, soheadcloses the pipe mid-run every time.PIPESTATUSreads141 0. The probe rendered the full correct config and then appendeddetector unavailablebeneath it — reproducible from any directory, with no configuration at all.knowledgehad a consequence beyond a wrong string.video-digest/SKILL.mdstates: "STOP if the pre-computed context above shows MISSING for yt-dlp, ffmpeg, or ImageMagick. Cloud agents without the media toolchain fail closed." #3468 fixed that gate's fail-open defect and shipped this inversion in its place, so the gate would refuse to run on a machine that has the toolchain.Measured over 30 runs each, tool installed and working, under pipefail:
ffmpegmagickyt-dlpIt is load-dependent:
ffmpegnear-deterministic,magicka race,yt-dlplatent by output size. An intermittent falseMISSINGis harder to diagnose than a consistent one. All three are fixed regardless. After the fix: 0/30 at every size.Also fixed
course-digest's two probes;firecrawl:firecrawl(pre-existing, and the idiom the fleet copied);provenance:audit's two detector probes (latent by shape only — its changelog says so rather than claiming a live fix);docs-hygiene:audit-derivability, whose capture-first form protects only thegitcall and leaves itsawk-and-headdata run exposed past 20 dirty.mdfiles; andcode-tidying:tidy, where a bare thirdunknownwas appended to well-formed output.Grants
Eight sites keep their original leading token, so every existing grant still matches.
One grant did move, and an earlier draft of this commit message denied it. Quoting
${CLAUDE_SKILL_DIR}changes the literal command string, and Bash permission rules are globs over that string, soBash(${CLAUDE_SKILL_DIR}/scripts/detect.sh:*)stops matching"${CLAUDE_SKILL_DIR}/scripts/detect.sh". Three companion quoted rules were added (ai-slopdetect.sh; provenancelist-corpus.sh,check-stamps.sh) with the unquoted ones retained, because both skill bodies still instruct unquoted invocation elsewhere. Same script, same anchor, same:*scope. Nothing widened.The quoting itself fixes a real defect: installed under a path containing a space, the unquoted expansion made a working detector report itself unavailable.
firecrawl:updatealready quoted it; these now match.Verification
Every site executed in three states, with and without
set -o pipefail:Roster completeness was re-derived by a method unrelated to the first grep: a parser over every extracted injection tracking brace depth and quote state, flagging a top-level
|followed by a top-level||. It found zero of these eleven (confirming they are fixed) and nine survivors, all correctly-drawn declines:claude-memory,implement,observabilityandaudit-comment-residueare structurally immune (wc/tr/tail/seddrain their input);audit-noise:17,rename-references,find-handoffare buffer-bounded rather than immune — measured to fire 0/5 at 1,000 producer lines and 5/5 at 5,000, andaudit-noise:17runs at 61 lines and fired 0/10.Open, and stated rather than assumed
Whether Claude Code runs
!injections underpipefailremains undocumented. The skills page specifies working directory, stderr merging, timeout, output size and exit-code semantics, but names no shell options. That governs whether these are live or latent, not whether they are defects — the brace-group form is correct under both settings, so the exposure is avoidable regardless.Related
Refs #3469, refs #3468, refs #3460.
Generated by Claude Code