fix(claude-config): drop P2's unsupported no-expansion claim, refuse an unresolvable scan root, and key the postures report per project - #2259
Conversation
…an unresolvable scan root, and key the postures report per project
P2 told authors "Bash rules match literally with no ~/$HOME/env expansion".
Grepping the complete raw markdown of both the permissions and skills pages finds
no such sentence on either — the claim is unsupported, not merely over-broad —
and two documented behaviors contradict it: ${CLAUDE_SKILL_DIR} and
${CLAUDE_PROJECT_DIR} are substituted in allowed-tools Bash rules, which the docs
present as the way to run a bundled script without a prompt, and a leading
assignment of known-safe env vars is stripped.
The same string is emitted on Read and Edit findings, where it is false twice
over. Probing the shipped detector confirms P2 fires on Read and Edit rules
carrying a Bash-scoped message, while those classes use gitignore syntax and DO
resolve ~/. One message serves every class, so it now carries only the
portability break, which is true of all of them, and names the portable form per
class. The mechanism moves into criteria.md as a per-class table synced down from
the permission-rule-hygiene convention, which already held the corrected
doctrine. The wrong text was pinned by a passing assertion; the test moved too.
Root resolution fell through to $PWD, which outside a repo is usually the user
profile — then walked it with find, no depth bound, stderr discarded, exit 0, on
a model-invocable skill. A timeout and a clean bill were indistinguishable. The
ladder now ends at $CLAUDE_PROJECT_DIR and an unresolvable root exits 2, reusing
the environment-gap channel already documented for missing jq rather than minting
a code. --count refuses too, since a 0 from a scan that resolved nothing reads
exactly like a clean bill. All five always-exits-0 statements moved with it;
SKILL.md carried none, contrary to the ledger's citation.
audit-prompting-postures persisted to one fixed last-audit.md under
CLAUDE_PLUGIN_DATA, which resolves per PLUGIN, never per project — so its only
durable deliverable was overwritten by the next run from any other root. The path
now carries a state key reusing audit-pass's <repo-identity>/<worktree-
discriminator> rather than minting a second scheme, plus one added non-repo rung
that audit-pass does not need because it refuses non-git targets. The derivation
is written as commands to run, never as a condition over ${CLAUDE_PROJECT_DIR} —
that placeholder substitutes inline, so the literal token never reaches the model
and the filed fix sketch would have introduced that defect while removing this
one. A mandated three-line header makes a surviving report attributable.
Suite 50 -> 62 checks; 8 of the new assertions fail against the pre-fix script.
Closes #2248
Closes #2249
Closes #2250
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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 3m 9s —— View job Security review complete
Scope: only the 8 files changed in this PR ( IMPORTANT — Unsanitized
|
![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: 06f9f8f3fd
ℹ️ 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".
… and placeholder the doc quotes CI's machine-specific-paths detector flagged four sites I added: the new Read/Edit test fixture, the docs quote in criteria.md's rule-class table, and its two echoes in the changelog. The test file already had the idiom and I did not follow it — it assembles POSIX_MP and WIN_MP from a SL variable precisely so no contiguous path literal appears in source. The new Read/Edit fixtures now do the same, and the prose uses <home> rather than reproducing the permissions page's example username verbatim. Verified locally against the same path classes the CI detector reports on, over every file this branch touches, before pushing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 13s —— View job Security review complete
Scope: the 8 files changed in this PR ( IMPORTANT — Unsanitized
|
|
Claude finished @kyle-sexton's task in 5m 26s —— View job Code review complete
Scope: the 8 files changed in this PR, per Two findings posted inline on
Everything else held up under review:
Not independently verified — flagging per instructions rather than restating as confirmed: the PR body's |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
… before it becomes a directory path Three review findings on the state-key derivation, all reproduced before fixing. The snippet read remote.origin.url while claiming to reuse audit-pass's scheme verbatim — and that scheme says "the first configured remote URL", not one named origin. Reproduced: a repo whose only remote is `upstream` has a remote, yet fell through to the local/ rung. That is the same defect class this batch keeps finding: prose asserting something the code does not do, and this time in a sentence claiming faithful reuse. It now reads `git remote | head -1`. A remote URL is arbitrary text and becomes DIRECTORY COMPONENTS here. Reproduced with `git remote add origin ../central.git`: identity `../central`, and the report path normalizes to /…/central/…, outside this skill's namespace entirely. Absolute-local and Windows-path remotes break the same way. The identity is now accepted only in the shape the scheme means — segments of [a-z0-9._-] each starting alphanumeric — and anything else keys by hash, still deterministically. Verified after the fix across seven remote shapes: relative, absolute-local, Windows, https, scp-style ssh, no-remote, and non-repo. All stay contained; https and ssh normalize to the same identity; the key is stable across runs and picks the first remote when several exist. And root now strips CRLF, matching the sibling script this same PR touches, which does it because Windows Git can return a CRLF-terminated path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 44s —— View job Security review complete
Scope: the 8 files changed in this PR. The "first configured remote" gap and the raw IMPORTANT — The new sanitization check validates a line, not the string: an embedded newline in the remote URL still bypasses itFile: remote_name=$(git remote 2>/dev/null | head -1)
remote=$(git config --get "remote.${remote_name}.url" 2>/dev/null || true)
...
identity=$(printf '%s' "$remote" | sed ... | tr '[:upper:]' '[:lower:]')
if ! printf '%s' "$identity" | grep -qE '^[a-z0-9][a-z0-9._-]*(/[a-z0-9][a-z0-9._-]*)*$'; then
identity="remote/$(printf '%s' "$remote" | sha256 | cut -c1-12)"
fi
Git's own config-file format supports this precondition directly: a double-quoted config value may contain a literal Verified the bypass mechanically (not just reasoned about it): feeding a two-line value ( Suggested fix: reject on embedded newlines before or as part of the shape check, e.g. require the whole (possibly multi-line) value to fail if it contains Severity: IMPORTANT — same root class and impact as the traversal this commit fixes (report write escapes No other new security issues found
|
|
Claude finished @kyle-sexton's task in 4m 33s —— View job Code review complete
Scope: this lane owns correctness/design/conventions/error-handling/tests/docs. Path-traversal / injection concerns already raised by the security lane and by Codex's inline comment are left to that lane, not re-litigated here — except to note, as context for this review, that
|
…guard I added a `-d "$ROOT"` refusal in this PR and shipped it untested, while writing four assertions for the sibling never-resolved branch — every existing run() passes a fixture dir the test had just created, so nothing exercised it. Five cases now do: a fixture root that does not exist, one that is a regular file, and --count on a nonexistent root, each asserting exit 2, the reason in the message, and that it is not reported as a clean bill. Without the guard `find` would print nothing to a discarded stderr and the run would report "No fragile permission grants found" for a root it never read. Suite 62 -> 68 checks; 14 now fail against the pre-fix script, up from 8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 49s —— View job Security review complete
Scope: the 8 files changed in this PR. IMPORTANT — Shape-check on the derived
|
|
Claude finished @kyle-sexton's task in 2m 8s —— View job Code review complete
Scope: this lane owns correctness/design/conventions/error-handling/tests/docs. Security concerns (the remote-URL-derived path validation in The new commit (
|
…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>
1 participant
Summary
Three findings, one plugin. Every claim below was executed or grepped this pass, not derived from a
model of the code — the standard this batch adopted after PR 2's review found three defects of the shape
"a comment asserting something the code does not do".
P2's rationale is unsupported by the docs, and false on rule classes it fires on (#2248)
reference/criteria.md:63read "Bash rules match literally with no~/$HOME/env expansion".Grepped, not assumed. Both pages pulled with
curlto a file (skills.md87,211 bytes;permissions.md61,351 bytes) and searched:grep -in "no ~/\$HOME|match literally|literally with no|no expansion|does not expand"returns zerohits across both. The Bash section (
permissions.md:162-176) specifies wildcard glob matching andstates no no-expansion rule. So the claim is unsupported, not merely over-broad.
Two documented behaviors contradict it:
skills.md:333— "Claude Code substitutes${CLAUDE_SKILL_DIR}and${CLAUDE_PROJECT_DIR}in twoplaces: the skill's markdown content, and Bash rules in the
allowed-toolsfrontmatter." Thecanonical example at
:339isallowed-tools: Bash(${CLAUDE_SKILL_DIR}/scripts/render.sh *). Theskill was telling authors to remove the documented zero-prompt pattern.
permissions.md:190— known-safe leading env-assignment stripping, and scoped: an allow rulewon't match past an assignment of any other variable, while deny/ask match past any. Stated flatly it
would over-generalize, so the new text carries the scoping.
And the message is emitted on rule classes where it is false twice over. The single string at
permission-rule-check.sh:139serves every class. Probed against the shipped script:Bash(/c/Users/kyle/x.sh:*)Read(/c/Users/kyle/notes.md)Edit(/Users/alice/src/**)Read(~/Documents/notes.md)Bash(${CLAUDE_SKILL_DIR}/scripts/x.sh *)Bash(${CLAUDE_PROJECT_DIR}/scripts/lint.sh *)Read/Editrules use gitignore pattern syntax and do resolve~/(permissions.md:280:Read(~/Documents/*.pdf)→/Users/alice/Documents/*.pdf). So on aReadfinding the old message namedthe wrong rule class and asserted a mechanism false for that class.
The emitted message now carries only what is true of every class — the portability break — and names the
portable form per class. The mechanism detail moves into
criteria.mdas a per-rule-class table, syncingdown from
docs/conventions/permission-rule-hygiene/README.md:106-134, which already held thecorrected doctrine and two limits the ledger's sketch omitted:
${CLAUDE_PROJECT_DIR}substitutionrequires v2.1.196+, and
${CLAUDE_PLUGIN_ROOT}is not substituted at all, so a rule using it isinert. No convention edit needed — this is the sync direction.
The scan fell through to
$PWDand swept the user profile, exiting 0 (#2249)permission-rule-check.sh:71-76endedROOT="${CLAUDE_PROJECT_DIR:-$PWD}". Outside a repository$PWDis whatever directory the session stands in — on a developer machine, usually the user profile — and
both scans walk it with
find, no-maxdepth, no-prune, stderr discarded, thenexit 0. A timeoutor a swallowed permission error was indistinguishable from a clean bill, on a skill that is
model-invocable (
disable-model-invocation: falseatSKILL.md:5).The ladder now ends at
${CLAUDE_PROJECT_DIR}; an unresolvable root exits 2, reusing theenvironment-gap channel the contract already documents for a missing
jqrather than minting a code, sothe advisory exit-0-for-findings contract is untouched.
--countrefuses too — a0from a scanthat never resolved a root reads exactly like a clean bill, and that path exited 0 separately at
:264.Ledger correction, verified: the ledger lists
SKILL.md:56as a third "always exits 0" site toupdate.
grep -n "exits 0\|exit 0"overSKILL.mdreturns no match;:56is the jq-exits-2 line.SKILL.mdcarries no always-exits-0 claim. The real count is five, in two files:reference/criteria.md:20plus the script's header, usage block, and--helptext. All moved together.The postures report had no project dimension (#2250)
audit-prompting-postures/SKILL.md:78persisted to one fixed${CLAUDE_PLUGIN_DATA}/audit-prompting-postures/last-audit.md.${CLAUDE_PLUGIN_DATA}resolves to~/.claude/plugins/data/{id}/where{id}is the plugin identifier, never the project — so theskill's only durable deliverable was silently overwritten by the next run from any other root.
The filed fix sketch is not what shipped, deliberately. It proposed
${CLAUDE_PROJECT_DIR}with a"when set, else" fallback. That placeholder substitutes inline in skill content, so the model never
sees the literal token and cannot evaluate "when set" — the defect filed separately against
audit-passas
F9. Implementing it as sketched would have introduced that defect while removing this one. Thederivation is therefore written as commands to run.
The scheme is
audit-pass's, reused rather than reinvented —<repo-identity>/<worktree-discriminator>fromrun-state-and-resumability.md§3 — because a secondscheme for one concern is the drift this batch exists to remove. One rung added: that ladder has
git-with-remote and git-without-remote and no non-repo rung.
audit-passdoes not need one (PR #2234made it refuse non-git targets); this skill is report-only and legitimately audits them — the run that
produced the finding was rooted at a non-repo home directory.
Plus the mandated three-line header (resolved root, scope filter, UTC timestamp), so a surviving report
is self-describing rather than merely un-overwritten.
What review caught — including the same defect class, in my own work
Five threads, all resolved, four follow-up commits. Two of the findings were the exact shape this batch
keeps hitting, and both were mine:
remote.origin.urlwhile the sentence above it claimed verbatim reuse of a schemethat says "the first configured remote URL". Reproduced: a repo whose only remote is
upstreamreturns empty and fell to the
local/rung despite having a remote, so one repository keyeddifferently depending on what someone named their remote. Now
git remote | head -1.Reproduced with
git remote add origin ../central.git: state key../central/2a8fd283, and thereport path normalizes to
/…/central/…— outside this skill's namespace entirely. Absolute-localand Windows-path remotes break the same way. The identity is now accepted only in the shape the scheme
means, and anything else keys by hash, still deterministically.
Plus three smaller ones:
rootnow strips CRLF to match the sibling script this same PR touches; thenew
-d "$ROOT"guard shipped untested next to four assertions for its sibling branch, and now has fivecases; and CI's machine-path detector caught four literals I added, where the test file already had a
runtime-assembly idiom I had failed to follow.
Verified after the remote fix across seven shapes — relative, absolute-local, Windows, https, scp-style
ssh, no-remote, non-repo:
Trust surface
Narrows. The scan refuses an unresolved root instead of walking the user's home, and the report path can
no longer escape the skill's own directory via a crafted remote. Nothing widens. No new grant, hook, or
network read.
Test plan
Fail-before / pass-after, both rows. The new assertions run against the pre-fix script
(
git checkout origin/main -- permission-rule-check.sh, test file kept):Restored, and after the fix:
Baseline for reference was
All 50 checks passed.at the merge-base.Two of the new assertions are guards, not failing tests, and are presented as such: the
${CLAUDE_SKILL_DIR}positive case and "explicit fixture root still scans" both pass before the changetoo. The A4 refusal test unsets
CLAUDE_PROJECT_DIRas well asPERMISSION_HYGIENE_FIXTURE_DIRand setsGIT_CEILING_DIRECTORIES, so it fails for the intended reason rather than inheriting a root from theouter session.
The CC-F1 derivation was executed, not just written. All three identity rungs, in real contexts:
And the worktree discriminator does what it exists for — two worktrees of this repository:
Portability caught by executing rather than assuming:
sha256sumis absent on stock macOS, so thesnippet carries a
shasum -a 256fallback (both verified present here).Static gates:
CHANGELOG verified as additions only:
git diff --cached CHANGELOG.md | grep -c '^-[^-]'= 0, so theshipped
0.30.0and0.31.0entries are untouched.Related
Closes #2248
Closes #2249
Closes #2250
Inbox items: batch-4 ledger
I10rowsA1andA4; ledgerI9rowCC-F1, whose mechanism wasreplaced per reconciliation
OR-3(rationale_falsifiedin effect, caught cross-ledger).Bump:
claude-config0.31.0 → 0.32.0 — minor, because two graded behaviors move (an unresolvableroot now refuses; the report path changes).
Reproduced but deliberately not fixed:
criteria.md:60-61claims//…forms are not flagged, andRead(//Users/alice/secrets/**)— the docs' own literal example atpermissions.md:278— is flagged.That is ledger row
A2(MED,implement_now: false), a different hunk in the same file. Thereproduction is recorded on #2248 as corroborating evidence for it; the new text asserts nothing about
//exemption. Also left:A3(P2 prints an 8-char fragment),A11,A14, andCC-F2–CC-F11.Filing for those is held pending the operator's call on batch-4 volume (
OR-4).