fix(claude-config): stop audit-pass asking the model to test a substituted placeholder - #2403
Merged
Merged
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 39s —— View job Security Review — PR #2403
No security issues found. Scope reviewed (all 4 changed files, via
Why this is out of scope for the vulnerability classes I was asked to hunt for:
This PR is a low-risk, prose-only documentation fix with no security-relevant surface. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 644de273e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cursor
Bot
force-pushed
the
fix/2280-audit-pass-placeholder-conditions
branch
from
August 12, 2026 08:36
644de27 to
f5ab88f
Compare
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging. Re-run the job to retry the review; a new push also retries it only if the caller's Re-running does NOT help for every class:
|
…tuted placeholder
audit-pass's default `target` read "${CLAUDE_PROJECT_DIR} when set, else
git rev-parse --show-toplevel". That placeholder is substituted inline in skill
content before the file reaches the model, so the literal token is never visible
and "when set" is a test about a value already resolved. It is now prose — the
project root Claude Code resolved for this session, else the git toplevel — with
the prohibition written out so the shape does not return.
This was a contradiction inside one plugin, and one this batch created: #2250
landed exactly this prohibition in audit-prompting-postures while audit-pass
kept the shape, so two sibling skills disagreed about the same placeholder.
Both instances are fixed, not just the filed one. The report cited SKILL.md:42-43;
the same unevaluable condition also sat in the non-git refusal ("with no explicit
target and no ${CLAUDE_PROJECT_DIR}"), governing the diagnostic that refusal
exists to produce. Fixing only the cited line would have left the contradiction
half-standing while reading as closed.
Also states two facts the skill depended on and never carried: how `{id}` is
derived for ~/.claude/plugins/data/{id}/ (identifier with characters outside
a-z, A-Z, 0-9, _ and - replaced by -, so formatter@my-marketplace becomes
formatter-my-marketplace), and that ${CLAUDE_PLUGIN_DATA} is NOT in the Bash
tool's environment — the export is documented for hook processes and MCP/LSP
subprocesses only, so `echo "$CLAUDE_PLUGIN_DATA"` in a Bash call is empty even
though the token substitutes correctly in skill content.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-run the job to retry the review. A new push re-triggers this lane only if the caller's |
cursor
Bot
force-pushed
the
fix/2280-audit-pass-placeholder-conditions
branch
from
August 12, 2026 08:49
f5ab88f to
4bc51e8
Compare
kyle-sexton
added a commit
that referenced
this pull request
Aug 12, 2026
…ed-tools on both siblings Two defects found in self-review of the change itself. 1. `audited` summed only frontmatter blocks and allow rules, so a root whose only auditable surface was plugin settings.json files — all parsed, all clean — printed "NOTHING TO AUDIT: … this run has no denominator" two lines above the coverage line reporting `plugins: 2 manifest(s); 2 settings.json parsed`. It had a denominator; the code had not counted it. That is A5's own defect in a new spelling, inside the fix for A5. All three axes now count, the zero-denominator message enumerates all three, and case 10bb pins it — case 10a uses a genuinely empty root and could never have caught it. 2. Declaring `disallowed-tools` on `audit-prompting-postures` alone opened a fresh instance of the sibling divergence CC-F6 is about, in the release that fixes CC-F6. `audit-instructions` states the identical report-only contract and names neither Edit nor Write anywhere in its body, so it carries the same declaration now. `audit-pass` states the contract too and is NOT touched here — PR #2403 owns that file — so it is filed as #2415. Also qualifies the CHANGELOG's CC-F11 claim: only the uninstall half is actioned, `when_to_use` is still unused, and the description grew 1,290 -> 1,305 chars to carry `output-styles`, which is the opposite direction from that row's headroom note. Tests: 99 -> 102 checks, all passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 12, 2026
…make audit-prompting-postures' contract agree with itself (#2408) Closes #2281 Closes #2283 > **Scope note.** #2281's eight rows are all taken, with **CC-F11 partial and said so below**. > #2283 takes four of five — **A8 is declined on its rationale** and is filed as **#2406** with the > corrected mechanism, so neither closing keyword drops a reproducing defect. Two follow-ups filed: > **#2406** (A8) and **#2415** (`audit-pass`'s missing `disallowed-tools`, blocked on #2403). ## Summary Two `claude-config` skills whose contracts contradicted themselves. Released together as **0.37.0** (minor: new output on every detector run, new env-var surface). #2382 (0.35.3), #2403 (0.35.4) and #2396 (0.36.0) all merged while this was in flight; 0.37.0 sits above all three and the CHANGELOG order gate is green. > **Review round: 7 P2 threads, all real, all fixed, replied to individually and resolved** (verified > unresolved=0 via GraphQL, not inferred). Four were *this PR's own defect class turned on this PR*, > which is itself the finding — so they are named rather than folded in silently: > > | # | Finding | Instance of | > |---|---|---| > | 1 | P3 axis missing from `audited` | denominator counting only successes (2nd) — already fixed at HEAD; thread was outdated | > | 2, 5 | A candidate `find` can list but the process cannot **read** is counted nowhere | same shape (3rd) — two reviewers converged independently | > | 6 | `audited` counts "produced a finding" on two axes, "examined" on the third | same shape (4th) | > | 3 | `disallowed-tools` claimed to make read-only "a property of the tool set" | **a false assurance claim, mine** | > | 7 | Phase D "dropped **or** demoted" vs the new schema's "kept for the record" | contract disagreeing with itself, introduced by its own fix | > | 4 | P7 searched rules + hooks but not the **script gate** its own catalog blesses | same, introduced by its own fix | > > After three instances of one shape, the completeness property stopped being asserted per-site and > is **derived once**: four exhaustive buckets, reconciled against the enumeration on every run, with > a negative test that deletes a bucket increment and asserts the check fires. The denominator's unit > is stated once for all three axes — *an input successfully read and examined, never one that > produced something* — and printed on every run. ### `audit-permission-grants` — reports a clean bill with no denominator (#2283) **A5 — the headline.** `No fragile permission grants found.` printed identically whether the run parsed forty `allowed-tools` blocks and found them healthy or parsed none at all. Every run now ends with a coverage block, and a run whose denominator is zero prints `NOTHING TO AUDIT` instead. **The denominator counts what was *not* read, because one built only from successes is the same defect in a new spelling.** Establishing that turned up two fail-open paths that were not in the issue, both folded in: - A settings file present but **not valid JSON** was skipped by a silent `|| return 0`. Its rules were never read and the run still printed a clean bill — and an unparsable rules file is exactly where a fragile grant would sit unexamined. Now reported per scope as `NOT VALID JSON — its rules were not read`. - Both `find` walks discarded stderr. This script's own header already argues against that: *"a swallowed permission error was indistinguishable from a clean bill."* Unreadable paths are now captured and counted. The `vendor/` exclusion moved out of the `find` predicate into the loop so the run can report how many files it removed. Same predicate, same result set — but an exclusion whose count is printed cannot suppress silently. `--count` keeps the bare integer on stdout (the machine contract) and puts the block on stderr. **A11.** `$PERMISSION_HYGIENE_SCAN_ROOT` is now the sanctioned name for the one scoping lever, with `$PERMISSION_HYGIENE_FIXTURE_DIR` kept as a back-compatible alias (new name wins when both are set). #2249 made that variable the documented operator remedy for the exit-2 refusal while its name still told them it was a test seam; `reference/criteria.md`, which never mentioned it at all, now sanctions it explicitly. **A15.** Consumer-declared exemptions must disclose themselves, may **widen** the fragile set but never delete a finding, and a run where every finding is exempted says so instead of printing a clean bill. The audited repo authors those declarations — the threat model the docs name directly (*"Review project skills before trusting a repository, since a skill can grant itself broad tool access"*, fetched 2026-08-12). The report schema grows an `Exempt?` column to hold it. **A16 — the filed remedy declined, with the measurement.** The row says the four scope filters are "advertised but not implemented". They *are* implemented, as a presentation filter, and `SKILL.md` says so at `:70-71`. The real defect is that the argument hint reads like a scan-scope. The filed fix (detector flags) rests on a cost claim that no longer holds: since #2249 the root is a git toplevel, `$CLAUDE_PROJECT_DIR`, or an explicitly named directory — never an unbounded sweep — and I measured the two walks over this repository at **0.49 s** and **0.41 s**. Flags to skip half a second would buy nothing and add a second place for scope to be defined. Fixed the wording instead, adopting the formulation both sibling audit skills already ship. **A8 — declined, filed as #2406.** It reasons from the `vendor/` exclusion's own justification ("not loadable, so the grant never takes effect") to `node_modules/`, worktrees and marketplace mirrors. That step is false. <https://code.claude.com/docs/en/skills>, fetched 2026-08-12: > Skills also load from nested `.claude/skills/` directories below your working directory. When > Claude reads or edits a file in a subdirectory, skills from that subdirectory's `.claude/skills/` > become available. So `node_modules/<pkg>/.claude/skills/<name>/SKILL.md` **is** loadable, and the exclusion would make P2 — an `error`-tier check whose whole subject is a leaked username — silently blind to a live grant. Same failure shape as the `//` path exemption on #2382. The defensible half (every exclusion reports its own count) ships here; the rest needs a loadability model, which #2406 specifies. ### `audit-prompting-postures` — contract disagrees with itself in eight places (#2281) All eight rows taken (CC-F11 partial — see its bullet). Prose-only, so there is no behavioral test to write and I am not inventing one — same posture #2403 took on the sibling skill. - **CC-F3.** P7 blesses a deny-by-default hook or script gate as presence evidence "without any prose", while Phase B inventories instruction *text* — so the one evidence form P7 names was the one form Phase B could not see, on the posture whose false MISSING is most expensive. Split the two: the inventory bounds what may produce a finding, not what counts as evidence, and Phase C now looks for the gate before judging P7 — in all three places the catalog blesses: settings rules, hook configuration, and, **after review caught the procedure searching only the first two**, the script the component delegates the destructive step to, followed and read. A component whose destructive action runs through a gating script is gated and nothing in its own text says so. Tightened `destructive-capable` from "**can** delete, reset, force-push" — which matches every component with a shell — to what the body has the model DO, per the classification section's own opening line. - **CC-F5 + CC-F10, written as one edit** because both move the same seam and landing them separately would ship a Phase A that disagreed with itself. The best-practices page is fetched every run and its failure **aborts** (single non-negotiable input; ten `wording-unverified` postures is a report shaped like an audit that audited nothing). Model subpages are fetched lazily in Phase C per applicable row and fail locally — which is what the observed run already did and the wording forbade. The verdict schema was closed at three tokens while the body mandated two more; it now carries four verdicts including `info`, with `wording-unverified` / `(unverified)` named as markers that ride alongside a verdict rather than replacing one. - **CC-F6.** The surface set is named in this skill instead of inherited by reference from a sibling that versions independently — the coupling that let `output-styles` become inventoried here and unnameable by this skill's own filter. `output-styles` is now a scope token. - **CC-F7.** P8 carries the model condition the skill's own gotcha mandates. **Leg the issue marked unverified, re-fetched by me 2026-08-12:** the pointed-at section scopes context awareness to "Claude Sonnet 5, Claude Sonnet 4.6, Claude Sonnet 4.5, and Claude Haiku 4.5". - **CC-F4 — and the false assurance claim review caught in it.** `disallowed-tools: Edit, NotebookEdit` is declared, but an earlier draft of this PR claimed it made "never edits a component" *a property of the tool set*. **That was false and is removed rather than softened.** `Write` is retained for the mandated persist and Phase B has already read every audited component, so it can overwrite one; `Bash` is retained for the state key, and a shell mutates files too. The declaration narrows the accident surface, it does not enforce the contract — both skills now say so, and both forbid telling an operator the skill *cannot* edit their files. A skill whose subject is auditing assurance must not ship a false assurance claim about itself; the CHANGELOG records that the earlier claim was wrong instead of quietly shipping the corrected text. **Second unverified leg, re-fetched 2026-08-12** — the frontmatter reference's semantics, including *"The restriction clears when you send your next message"*, which is the right lifetime: whoever accepts a proposal can apply it. - **CC-F8 — the issue's stated mechanism is wrong at HEAD, and I fixed the real one.** The issue says `grep -c "audit-prompting-postures" .../audit-instructions/SKILL.md` → **0**, "the token appears nowhere in the sibling". At HEAD it returns **1**, at `audit-instructions/SKILL.md:392`, in a state-key aside. The grep claim is false; the substantive claim survives, because a mention in an aside is not a route-out — `audit-instructions`' Scope boundary section still never named this skill. Added that route-out line. Also added two evals: one whose prompt carries **no slash invocation**, so description-driven selection is exercised for the first time (all five existing cases invoke explicitly), and one pinning CC-F3's mechanical-gate rule. The description-drives-discovery claim was itself an unverified leg — **third one re-fetched 2026-08-12** (`skills.md:259`, `:425-426`). - **CC-F11 — partial, and the partial goes one way I should name.** The uninstall half is fixed: the state key stops overwrites, not reaping, with the sentence quoted and `--keep-data` named (re-fetched 2026-08-12). The row's other two observations are **recorded, not fixed** — and one moved the wrong way. `when_to_use` is still unused, and the description grew from **1,290 to 1,305** of its 1,536-char cap to carry `output-styles` for CC-F6, which is the opposite direction from the row's headroom note. That trade is deliberate (a scope token that is unnameable is the actual defect; 231 chars of headroom is not), but "all eight rows taken" would have been the wrong sentence to leave standing. CC-F9 is correctly **not** touched: the issue records it as falsified, and it is. ### One divergence this PR would otherwise have created CC-F4 declares `disallowed-tools: Edit, NotebookEdit` on `audit-prompting-postures`. `audit-instructions` states the **identical** report-only contract ("There is no `--fix`… never by this skill") and names neither `Edit` nor `Write` anywhere in its body — so declaring it on one of the pair and not the other would have opened a fresh instance of exactly the sibling divergence CC-F6 is about, in the release that fixes CC-F6. It is declared on both, in the file this PR was already editing. `audit-pass` states the contract too and is **not** touched: PR #2403 owns that file right now, so it is filed as **#2415** rather than collided with. ## Test plan **Fail-before / pass-after for A5.** Three roots that `origin/main` (`5ea4f87f`) describes with one identical string, run against the old and new detectors: ``` ==================== BEFORE — origin/main (5ea4f87) ==================== --- root with 0 grants --- No fragile permission grants found. --- root with 2 healthy grants --- No fragile permission grants found. --- root whose only rules file is invalid JSON --- No fragile permission grants found. --- --count --- empty=0 healthy=0 badjson=0 ==================== AFTER — this branch ==================== --- root with 0 grants --- NOTHING TO AUDIT: 0 allowed-tools block(s) and 0 allow rule(s) were read under this root, so this run has no denominator. That is a scan of nothing, not a clean bill — do not report it as one. Scan coverage (the denominator — what this run actually read): root: /tmp/tmp.EbQtjPHKzq/empty-root (resolved from $PERMISSION_HYGIENE_FIXTURE_DIR (alias of $PERMISSION_HYGIENE_SCAN_ROOT)) frontmatter: 0 allowed-tools block(s) parsed from 0 candidate file(s); 0 excluded under a vendor/ path segment as non-loadable settings: 0 allow rule(s) from 0 scope(s) read — project: absent; local: absent; user-global (...): absent plugins: 0 manifest(s); 0 settings.json parsed NOT read: 0 path(s) the walk could not open; 0 settings file(s) and 0 plugin settings.json present but not valid JSON never in scope here: managed-policy and enterprise settings, a --settings flag file, and the pre-v2.1.211 start-directory copy... --- root with 2 healthy grants --- No fragile permission grants found. Scan coverage (the denominator — what this run actually read): ... settings: 2 allow rule(s) from 1 scope(s) read — project: 2 rule(s); local: absent; user-global (...): absent ... --- root whose only rules file is invalid JSON --- NOTHING TO AUDIT: 0 allowed-tools block(s) and 0 allow rule(s) were read under this root... Scan coverage (the denominator — what this run actually read): ... settings: 0 allow rule(s) from 0 scope(s) read — project: NOT VALID JSON — its rules were not read; local: absent; ... NOT read: 0 path(s) the walk could not open; 1 settings file(s) and 0 plugin settings.json present but not valid JSON ... --- --count (stdout only) --- empty=0 healthy=0 ``` The `--count` line is the point restated: stdout is still `0` in both cases (the machine contract is unchanged), and stderr now separates them. **The denominator got the same defect wrong four times, and the fourth is why it is now structural.** Recorded in full because the pattern is more useful than any one instance: 1. `audited` omitted the P3 axis, so a root of clean plugin `settings.json` printed `NOTHING TO AUDIT` two lines above `plugins: 2 manifest(s); 2 settings.json parsed`. Caught in self-review. 2. A candidate `find` can *list* but the process cannot *read* — `find` needs only directory traversal to report `-type f` — reached `awk`, failed, wrote to the real stderr rather than `WALK_ERR`, and was counted in no bucket, while the coverage block promised to disclose exactly that input. Caught by two reviewers independently. 3. `audited` counted "produced a finding" on the frontmatter and settings axes but "examined successfully" on P3's, so a `SKILL.md` with no `allowed-tools` and a `settings.json` with an empty `allow` array contributed nothing despite being read and found to grant nothing. Three instances means the invariant was maintained *by convention at each `continue`*, so it is now derived once. Every enumerated candidate lands in exactly one of four buckets — vendor-excluded, unreadable, no `allowed-tools` block, parsed — and `reconcile_frontmatter` checks they sum to the enumeration on every run, printing `DENOMINATOR BUG` and naming **itself** as the defect when they do not. A check that cannot fail is not a check, so a negative test deletes a bucket increment from a copy of the script and asserts the reconciliation fires. And the unit is stated once for all three axes — *an input successfully read and examined, never one that produced something* — and printed: ``` DENOMINATOR = 3 input(s) successfully examined: 2 frontmatter file(s) + 1 settings scope(s) + 0 plugin settings.json. The unit on every axis is "read and examined", never "produced a finding". reconciled: 3 candidate(s) = 1 vendor-excluded + 0 unreadable + 1 without an allowed-tools block + 1 parsed ``` Extraction stderr now joins the walk's rather than escaping to the terminal, and a run that audited nothing **and** could not open its own inputs says so distinctly instead of reporting an empty tree. **Suite: 76 → 102 on this branch's own base, then 121 after the review round, all passing.** 26 of those are this branch's, across the denominator (including the P3 axis), the unparsable-settings skip, the `--count` stdout/stderr split, the exclusion disclosure, and the scan-root rename; the rest are #2382's, which landed mid-flight. **Merge note — and the trap it walked into.** #2382 (0.35.3), #2403 (0.35.4) and #2396 (0.36.0) all merged into `main` while this was in flight, twice leaving the PR `CONFLICTING`. **A conflicted merge ref suppresses the `pull_request`-triggered runs entirely**, and the PR then displayed **3 checks, all passing** instead of 33 — nothing distinguishes that from a real green except counting the rows. Caught by comparing against #2382's 34, not by reading the failure count. Resolved with merge commits rather than repeated rebases, and verified marker-free across the whole tree before each commit: - `plugin.json` — **0.37.0**, because #2396 took 0.36.0 (the version this PR originally claimed) and also rewrote the description to "Nine configuration-health skills". Theirs kept, version raised. - `CHANGELOG.md` — 0.37.0 / 0.36.0 / 0.35.4 / 0.35.3 / 0.35.2; `--check-order` green. - `permission-rule-check.test.sh` — both sides pure additions, both kept. - `permission-rule-check.sh` and `criteria.md` auto-merged with **no** conflict: #2382's hunks are the P2 pattern block and P2's criteria section, neither of which this PR touches. - #2396 also changed `lib/permission-patterns.sh`, which this detector sources — so the suite was re-run against the merged library, not just against this branch's own base. Still passing. ``` $ bash plugins/claude-config/skills/audit-permission-grants/scripts/permission-rule-check.test.sh PASS: empty root reports NOTHING TO AUDIT PASS: empty root does NOT print a clean bill PASS: empty root still prints the coverage block PASS: empty root denominator names zero blocks PASS: healthy root prints the clean bill PASS: healthy root is not NOTHING TO AUDIT PASS: clean bill carries a non-zero rule count PASS: a clean P3-only root is a clean bill PASS: a clean P3-only root is NOT a scan of nothing PASS: coverage counts the plugin settings it parsed PASS: coverage names the project scope it read PASS: coverage names an absent scope as absent PASS: unparsable settings file is named, not skipped in silence PASS: unparsable file is counted under NOT read PASS: a run whose only rules file will not parse is not a clean bill PASS: --count stdout is still the bare integer PASS: --count writes the coverage block to stderr PASS: --count coverage carries the denominator PASS: vendor exclusion discloses its count PASS: coverage names the candidate file total PASS: PERMISSION_HYGIENE_SCAN_ROOT resolves a root PASS: coverage names the rung that resolved the root PASS: the legacy alias still resolves a root PASS: the sanctioned name wins over the alias PASS: the alias did not win PASS: refusal names the sanctioned variable as the fix ... All 121 checks passed. ``` **The A16 measurement, since it is what declines the row:** ``` $ time (find . -type f \( -name 'SKILL.md' -o \( -name '*.md' -path '*/agents/*' \) \ -o \( -name '*.md' -path '*/commands/*' \) \) ! -path '*/vendor/*' | sort -u | wc -l) 214 real 0m0.491s $ time (find . -type f -path '*/.claude-plugin/plugin.json' | wc -l) 65 real 0m0.412s ``` **Repo gates:** ``` $ CHECK_SKILL_SKILLS_ROOT=plugins/claude-config/skills bash plugins/skill-quality/scripts/check-skill.sh audit-prompting-postures INFO: description length 1305/1536 chars INFO: all 6 base-ref trigger phrase(s) preserved INFO: SKILL.md 200/500 lines INFO: markdownlint clean CHECK-SKILL audit-prompting-postures: PASS — 0 errors, 0 warning(s) $ ... check-skill.sh audit-permission-grants INFO: script test passed: scripts/permission-rule-check.test.sh CHECK-SKILL audit-permission-grants: PASS — 0 errors, 1 warning(s) # no-Gotchas warning is pre-existing $ npx markdownlint-cli2 <the 6 changed markdown files> Summary: 0 issues in 0 files $ shellcheck -S warning .../permission-rule-check.sh .../permission-rule-check.test.sh (clean) $ bash scripts/check-shell-portability.sh <merge-base> No unexcused GNU-only constructs in 2 shell file(s). $ bash scripts/check-skill-portability.sh <merge-base> No unexcused coupling tokens in 6 skill file(s). $ bash scripts/check-changelog-parity.sh --check-order All 76 changelog(s) read newest-first with no duplicate versions. $ bash scripts/check-changelog-parity.sh --check-bump origin/main Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry. $ bash scripts/validate-plugins.sh All plugin manifests and the catalog validated. $ bash scripts/check-silent-skips.sh No silent prerequisite skips found in hook entry scripts. $ bash plugins/skill-quality/scripts/check-evals-quality.sh .../audit-prompting-postures/evals/evals.json check-evals-quality: PASS (0 warning(s) across 1 file(s)) $ npx ajv-cli@5 validate --spec=draft2020 -s plugins/skill-quality/reference/evals.schema.json -d .../evals.json .../evals.json valid ``` `check-listing-budget` was run and reports the aggregate **already** over budget on `main` (97537/8000) — advisory-only, and this PR's contribution is **+15 chars** (description 1290 → 1305 for `output-styles`). Not introduced here and not resolvable here. ## Related - **#2281** — closed here; all eight rows (CC-F3 … CC-F11) taken. CC-F9 deliberately untouched, as the issue's own "Not in this issue, on purpose" section requires. - **#2283** — closed here for A5, A11, A15, A16. **A8 split to #2406 before merge** so the auto-close drops nothing. - **#2406** — the A8 follow-up, with the falsified rationale and what a real loadability model must distinguish. - **#2382** (0.35.3), **#2403** (0.35.4) and **#2396** (0.36.0) — all merged during this PR's life; see the merge note in the test plan. #2396 took 0.36.0, so this PR is **0.37.0**. - **#2249** (closed) — removed the `$PWD` fallback and added the exit-2 refusal. This is the residue it named: a *resolved* root with nothing in it still reported clean. - **#2250** (closed) — keyed the report path per project; CC-F11 is the residual amplifier (uninstall still reaps the directory). - **#2259** — added the two evals and 67 lines to `audit-prompting-postures/SKILL.md`; every anchor in #2281 past `:78` was re-derived at this HEAD before editing. Inbox items: `20260811-021645-plugin-audit-four-components-and-guard-deadlock-ownership` (#2281), `20260811-024628-claude-config-audit-permission-grants-defects-and-fleet-grant-hygiene` (#2283). Ledgers: `.work/handoff-inbox-batch-4/ledgers/I9-021645-four-components.md` § Lane A; `.work/handoff-inbox-batch-4/ledgers/I10-permission-grants-fleet.md` § A5, A11, A15, A16. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
This was referenced Aug 12, 2026
kyle-sexton
added a commit
that referenced
this pull request
Aug 12, 2026
…it specified (#2441) ## Summary `audit-pass` specified a runtime and shipped no executable. `SKILL.md` plus ten `reference/*.md` spell out a lease — path, refresh discipline, a two-sided liveness window, a `released` tombstone, `owner_epoch` fencing — and an epoch-scoped append-only partial that `--resume` reads instead of the report. The skill shipped **no `scripts/` directory at all**, the only audit skill in `claude-config` without one. The gap has a sharper form than "no scripts", and it is fully verifiable from the tree: `lib/state-key.sh`'s own header records the keying scheme as *"`audit-pass`'s, reused rather than reinvented"* and points at `skills/audit-pass/reference/run-state-and-resumability.md` §3 — and `grep -rl state-key.sh plugins/` returns `audit-instructions`, `audit-prompting-postures` and `claude-memory:audit`. Every skill called it except the one that specified it. **What this PR adds.** `skills/audit-pass/scripts/run-state.sh`, with subcommands `paths`, `lease acquire|heartbeat|release|classify`, and `partial append`, plus `run-state.test.sh`. `paths` derives `<plugin-data>/runs/<state-key>/<run-id>` through `lib/state-key.sh`, so the skill now runs on its own scheme. No `jq`, no GNU-only constructs, no network, no background process. **What it deliberately does not add, and now says so in-contract.** Stale-lease adoption (the `owner_epoch` compare-and-set) and §7 assembly are **not** implemented. §3 and §7 now state which clauses the script enforces and which remain the run's own discipline. The standard this PR is held to is that a contract must not read as enforced while nothing enforces it — that is satisfied by implementing *or* by declaring, and each clause here does one of the two. **Two clauses were reduced rather than implemented, because a skill cannot keep them.** 1. The lease specified a **60-second wall-clock heartbeat**. A skill-driven run acts between tool calls and has no timer, so that cadence named a mechanism no run could provide — the same defect as specifying a lease and shipping no writer. Refresh is now **boundary-driven** (acquire, each lane's persistence point, release). Each lease records the `stale_after_s` and `skew_grace_s` its writer committed to, and `classify` reads them from the artifact — which is what the section's own "two implementations must reach it identically" concern actually needed. The default threshold moves 5 minutes to 30: with boundary-driven refresh a single delegated lane can outlast five minutes, and a threshold shorter than a lane classifies a *running* pass as abandoned, the unsafe direction because it lets `--resume` adopt a live run's artifact. 2. §5 specified a **run manifest** carrying per-lane input digest and completion state, beside the partial. §7 already required that `--resume` read the partial "so completion state is derivable from the artifact rather than tracked beside it and able to disagree with it" — and a manifest beside the partial is precisely what can disagree with it. §5 now states the manifest as the partial's own lane records. This closes the second link in F12's chain: making the partial real while leaving completion state in a file nothing writes would have moved the defect, not fixed it. **Evidence discipline.** F5 is `SELF_REPORTED` and its "the observed run wrote none" leg is not reproducible from this repository. Nothing in the code, the CHANGELOG, or this PR asserts it. The basis for every change here is the **specification-versus-implementation gap**, which is verifiable from the tree and was verified. **Trust surface.** The script writes only under the plugin data directory it is *given* — it discovers no directory — and never inside a target repository, so the report-only contract (`disallowed-tools: Edit, NotebookEdit`) is untouched. Both path segments it contributes are validated: `lib/state-key.sh` already refuses a remote URL that would become traversing directory components, and a `--run-id` outside `[A-Za-z0-9][A-Za-z0-9_.-]*`, or containing `..`, is refused here. Both refusals carry negative tests. Version: `claude-config` **0.37.1 to 0.38.0** (new executable surface), with a CHANGELOG entry. ## Test plan **Fail-before.** The script is new, so `main`'s state is the script absent. With `run-state.sh` moved aside, every one of the 52 checks fails: ``` $ mv run-state.sh ../run-state.sh.hidden && bash run-state.test.sh FAIL: paths exits 0 on a git target detail: expected exit 0, got 127 FAIL: paths echoes the plugin data dir it was given detail: expected to contain: plugin_data=/tmp/tmp.Z8gZvUvVaH/plugin-data FAIL: paths derives a state key through lib/state-key.sh detail: expected to contain: state_key=github.com/example/demo/ FAIL: the run dir is keyed under runs/<state-key>/<run-id> detail: expected to contain: run_dir=/tmp/tmp.Z8gZvUvVaH/plugin-data/runs/github.com/example/demo/ ... ``` **Pass-after.** ``` $ bash plugins/claude-config/skills/audit-pass/scripts/run-state.test.sh PASS: a run id containing '..' is refused PASS: the refusal names the traversal PASS: a run id that is not a plain segment is refused PASS: without the '..' arm the same id is accepted — the check discriminates PASS: and it would have assembled a traversing run dir PASS: without the segment check an absolute id is accepted — the check discriminates PASS: and it would have escaped the plugin namespace PASS: a heartbeat beyond the skew grace classifies stale, not live PASS: and the skew is reported rather than swallowed PASS: a heartbeat within the skew grace still classifies live PASS: without the lower bound a dead run pins itself live — the bound discriminates PASS: the partial is named for the epoch the lease holds PASS: appends accumulate rather than rewriting the document PASS: a partial is never written without a lease to classify it ... All 52 checks passed. ``` **The three negative tests are the point.** Each mutates a copy of the script to delete exactly one check and asserts the mutated copy reaches the outcome the real one refuses — a passing test that would still pass with the check deleted proves nothing: - delete the two-sided window's **lower** bound, and a `heartbeat_at` two hours in the future reads `live` instead of `stale`, so a dead run pins itself live and every `--resume` refuses an abandoned run indefinitely (assertion 3.9); - delete the `..` arm of `--run-id` validation, and `a..b` is accepted and a traversing run dir is assembled; - delete the segment-shape check, and `/etc/passwd` is accepted as a run id and escapes the plugin namespace. Each construction is verified before it is trusted: if the `sed` target no longer matches, the test `fail`s with "UNVERIFIED by this run" rather than passing vacuously. **Repo gates, run locally:** ``` $ shellcheck --rcfile .shellcheckrc -S info plugins/claude-config/skills/audit-pass/scripts/*.sh (clean) $ bash scripts/check-shell-portability.sh --paths .../run-state.sh .../run-state.test.sh No unexcused GNU-only constructs in 2 shell file(s). $ bash scripts/check-changed-skills.sh origin/main INFO: SKILL.md 485/500 lines INFO: script test passed: scripts/run-state.test.sh CHECK-SKILL audit-pass: PASS — 0 errors, 2 warning(s) $ bash scripts/check-changelog-parity.sh --check-bump origin/main Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry. $ bash scripts/check-changelog-parity.sh --check-preserved origin/main All 1 changed changelog(s) preserve every version heading they carried at da05706 (66 heading(s) compared). $ bash scripts/check-cross-plugin-source-drift.sh --check No unregistered or drifted cross-plugin source clusters found. $ npx markdownlint-cli2 "plugins/claude-config/skills/audit-pass/**/*.md" "plugins/claude-config/CHANGELOG.md" Summary: 0 issues in 0 files ``` Every other `plugins/claude-config/**/*.test.sh` still passes (`lib/state-key.test.sh` 23, `instruction-scan.test.sh` 70, `permission-rule-check.test.sh` 136, and the rest). Both `SKILL.md` warnings are pre-existing: the 200-line soft target, and a stale `fresh-eyes-exempt` directive in `## Self-check`, a section this PR does not touch. ## Related Closes #2280 Origin: handoff-inbox item `20260811-020411-claude-config-audit-pass-report-path-inside-scan-set` (ledger `.work/handoff-inbox-batch-4/ledgers/I8-audit-pass-report-path.md` § F3, F5, F12, F13). Row state, all six: | Row | State | |---|---| | **F3** — 1,547 lines of contract, zero scripts | fixed here: `scripts/run-state.sh` + `run-state.test.sh`, three negative tests | | **F5** — the lease is prose with no executable | fixed here: `lease acquire\|heartbeat\|release\|classify`; the unkeepable 60s cadence reduced to boundary-driven refresh | | **F12** — `--resume` reads a partial nothing writes | fixed here: `partial append`, Phase 4's `open` terminator written when the handoff is recorded, and §5's separate manifest folded into the partial's lane records | | **F13** — the disclaimed dimension's mitigation is unimplemented | fixed here: the mitigation now names `partial append`; the disclaimer itself is unchanged, since persistence bounds nothing | | **F8** — `{id}` mangling and the Bash-env fact unstated | **already fixed at HEAD** by ceaf013 (#2403); verified by `git log -S` and by reading `reference/report-location-and-schema.md:25-37` | | **F9** — `${CLAUDE_PROJECT_DIR}` "when set" conditional | **already fixed at HEAD** by ceaf013 (#2403); verified the same way at `SKILL.md:48-58` | Adjacent and deliberately not touched: #1430 (cross-epoch carry-forward), #1940 (`rerun-contract.md` design-doc drift), #1568 (`${CLAUDE_*}` substitution scope). F4's refuse-vs-specify fork is settled on `main` already and is not reopened here. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Refs #2280 — takes rows F9 and F8. Rows F3, F5, F12 and F13 are not taken; see
## Related.No linked issue closes here on purpose: #2280 carries six rows and this PR takes two, so a closing keyword would auto-close four live rows.
Summary
F9 —
audit-passasked the model to evaluate a placeholder it never sees. The defaulttargetread:${CLAUDE_PROJECT_DIR}substitutes inline in skill content before the file reaches the model (plugins reference: "Skill and agent content | Anywhere the placeholder appears"), so the literal token is never visible and "when set" is a test about a value that has already been resolved. It is now prose — the project root Claude Code resolved for this session; where no such root is available,git rev-parse --show-toplevel— with the prohibition itself written out so the shape does not come back.This is a contradiction inside one plugin, and one this batch created. #2250 landed exactly this prohibition in
audit-prompting-postures, where it still reads "Do not express the path as a condition over${CLAUDE_PROJECT_DIR}'when set'", whileaudit-passkept the shape. Two sibling skills disagreed about the same placeholder. Fixed rather than documented.Both instances, not just the filed one. #2280 cites
SKILL.md:42-43. The same unevaluable condition also sat in the non-git refusal at:66— "with no explicittargetand no${CLAUDE_PROJECT_DIR}" — where it governs the diagnostic path that refusal exists to produce. That second one was not in the issue; I found it during pre-flight and recorded it on #2280 before writing this. Fixing only the cited line would have left the contradiction half-standing while reading as closed.F8 — two facts the skill depended on and never stated.
{id}is derived. The skill said${CLAUDE_PLUGIN_DATA}resolves to~/.claude/plugins/data/{id}/and never said how{id}is formed. Now quoted verbatim: "the plugin identifier with characters outsidea-z,A-Z,0-9,_, and-replaced by-", with the page's own worked example —formatter@my-marketplace→~/.claude/plugins/data/formatter-my-marketplace/. A wrong derivation writes the report where the next run will not look, which is also how--resumeloses a partial.${CLAUDE_PLUGIN_DATA}is not in the Bash tool's environment. The export is scoped: "All three are exported as environment variables to hook processes and to MCP and LSP server subprocesses." The Bash tool is none of those, soecho "$CLAUDE_PLUGIN_DATA"in a Bash call returns an empty string even though the token substitutes correctly in skill content. Nothing in the skill said so, which invites exactly that shell expansion.Both doc quotes were re-fetched as raw markdown (
curl …/docs/en/plugins-reference.md) on 2026-08-12 and grepped — lines 672 and 709 — not recalled.Test plan
Prose-only change to model-facing instructions; there is no behavioral test to write, and I am not inventing one. What is verifiable is that the shape is gone and the surfaces agree:
One hit, and it is the prohibition itself — both conditional uses are gone. Before this change the same grep returned
:42and:66, the two instances.Repo gates:
Both
check-skill.shwarnings are pre-existing onaudit-pass(a stale fresh-eyes-exempt directive atSKILL.md:367, and nometadata.category); neither is introduced here.Related
scripts/resolve-run-paths.shandscripts/lease.sh. The issue itself marks this a "deferred shape".SELF_REPORTED: the non-write is a runtime observation from the one session that ran the component, and nothing in this repository can confirm or refute it. Building a lease implementation on an unreproducible observation is the mistake this batch's re-verification discipline exists to prevent.--resumepartial dependency and the unbounded intra-lane fan-out. Both re-verified present at HEAD; both are consequences of F3/F5's missing mechanism rather than independent prose fixes.${CLAUDE_PROJECT_DIR}-"when set" prohibition inaudit-prompting-postures. The drift this PR removes is the sibling it did not sweep.~/.claude/plugins/data/{id}/) landed in fix(claude-config): audit-pass claims CLAUDE_PLUGIN_DATA resolves outside any target repository — false by construction #2229/fix(claude-config): audit-pass gates its report-path safety machinery on the --report-to flag, not on containment #2230; the derivation and the Bash-environment fact are what they left standing.${CLAUDE_*}substitution-scope question empirically. This PR does not depend on it:SKILL.mdis unambiguously skill content, which both the plugins reference and the skills page agree substitutes, and no claim here rests on bundled spoke files.claude-config. It claims 0.35.3; this PR takes 0.35.4 to avoid a manifest collision, since fix(claude-config): tighten permission-rule-check P1/P2 precision #2382 is green and ready first. Whichever merges second needs no rebase for the version, only for the CHANGELOG ordering if both land.Inbox item:
20260811-020411-claude-config-audit-pass-report-path-inside-scan-set.Ledger:
.work/handoff-inbox-batch-4/ledgers/I8-audit-pass-report-path.md§ F9, F8.