feat(claude-config): make audit Category G executable and give the skill a real hook inventory - #2301
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 6m 1s —— View job Claude finished @kyle-sexton's taskTasks
SummaryReviewed for correctness, error handling, test coverage, and maintainability per this repo's This repo's Posted 3 inline findings, all on
Other observations (no action needed)
Not independently verified: I was unable to execute |
|
Claude finished @kyle-sexton's task in 2m 26s —— View job Security Review Complete
Scope reviewed: the 10 files changed in this PR — the new
One SUGGESTION worth flagging, not a vulnerability in the traditional sense:
No other findings. The remaining changed files are documentation/checklist prose only (budget constants, lever ordering, fragility caveats for |
![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: 348949333a
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ill a real hook inventory Category G was the one Phase 2 category a consumer could not run. Its only overflow detector was `/doctor`, which needs an interactive TTY, so the whole category yielded nothing in the harness's own headless mode while the documented `--debug` route went unmentioned. It stated no budget constant, so "overflowed, and by how much?" was unanswerable by hand. Its ordered "cheapest first" lever list put `skillOverrides` second while carrying the caveat that it does not reach plugin skills, and never named `/plugin`, the substitute upstream prescribes. And it was the only letter with no checklist table. Separately, `audit` asserted hook coverage it had no way to enumerate. Category D writes rules for plugin-root placeholders that appear only in a plugin-provided hook, and Category B's narrowing 3 turns on whether such a hook is live — while three of the skill's own surfaces said in prose that the enumeration was impossible. New `scripts/check-hook-coverage.sh` takes that inventory: enabled plugins resolved through the installed-plugin registry, all four documented hook-config shapes read, suppression levers reported, and an exit code that separates "complete" from "partial" from "no inventory". The fail-open posture survives, narrowed to what a partial run could not read. Writing it surfaced a live defect worth naming: on Git for Windows `jq` writes stdout in text mode and appends a CR to every line — its own stream, not the input file's line endings — so a plugin key read out of `jq` was `name@mkt\r` and every registry lookup missed. Deleting the CR strip fails 17 of the suite's 34 checks, including cases whose fixtures are pure LF. Also: the destructive-bash-deny baseline now ships with the fragility caveat the same file already applies to the Read deny table one section earlier, plus the prefix-anchoring hole (`Bash(git push --force *)` does not match `git push origin main --force`); the "speed bump" hook ranking is scoped to the threat model it was written for rather than governing destructive git too; and the read-it-verbatim guard covers all of Phase 3 instead of one checklist row. Closes #2274 Closes #2275 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hygiene job's exec-bit check failed: both new .sh files landed 100644 while every sibling script in the same directory is 100755. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
995c7d4 to
5a0f34e
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:
|
|
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 |
…on in hook inventory - Merge enabledPlugins with local > project > user precedence - Resolve installPath by scope and projectPath instead of array order - Read managed-settings JSON when present; surface managed caveat in output - Add regression tests for scope precedence and install-record selection Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
) Fixes #2282 (scoped rows A2, A3, A19). > **Scope note:** this PR takes three of #2282's five rows. **A7b** and **A12** both reproduce at HEAD and are untouched; they are now filed as **#2397**, so the closing keyword above no longer drops them. ## Summary Scoped fix for three of #2282's five rows in `audit-permission-grants`. - **A2 — direction reversed after review.** As filed, this row implies "make the `//` exemption real". The docs settle it the other way: `permissions.md` gives `//path` = "Absolute path from filesystem root" with `Read(//Users/alice/secrets/**)` → `/Users/alice/secrets/**`, and "Use `//Users/alice/file` for absolute paths." So `//Users/<name>/…` is the canonical *spelling* of a hardcoded user home, and exempting it would have made an `error`-tier username-leak check blind to the documentation's own example of the leak. **`criteria.md` moved; the detector keeps flagging `//`.** The inconsistency the row reports was real — the two shipped files disagreed — but the document was the wrong one. - **A3:** P2 findings report the full `Tool(…)` rule, not an eight-character path fragment. Two regressions this introduced are also fixed: the tool name was enumerated as `(Read|Edit|Write|Bash|PowerShell)` (silently dropping `WebFetch`/`Glob`/`NotebookEdit`/`mcp__*`/`Agent` rules — `Agent` indefensible, this script ships `scan_agent()`), and the `//` skip was a substring test that let `Read(//opt/data/../Users/kyle/secrets)` read clean. - **A19:** `Bash(npm view ctx7 version*)` is no longer treated as a bare package-manager wildcard — `*` must be preceded by a separator. Out of scope and **now tracked in #2397**: A7b (inert-grant check) and A12 (`~user` username leak). Both reproduce at HEAD, with evidence carried into that issue, so closing #2282 here drops nothing. ## Test plan A19's change verified against the shipped library at this branch, sourced verbatim — the false positive is gone and every true positive still matches: ``` $ . plugins/claude-config/lib/permission-patterns.sh $ for r in 'Bash(npm view ctx7 version*)' 'Bash(npm *)' 'Bash(npm:*)' 'Bash(npx *)' \ 'Bash(pnpm dlx *)' 'Bash(npm test)' 'Bash(npm run build *)'; do printf '%-32s -> ' "$r" out=$(printf '%s\n' "$r" | grep -oE "$CCPERM_P1_ERE"); [ -n "$out" ] && echo FLAGGED || echo clean done Bash(npm view ctx7 version*) -> clean # the false positive A19 filed Bash(npm *) -> FLAGGED Bash(npm:*) -> FLAGGED Bash(npx *) -> FLAGGED Bash(pnpm dlx *) -> FLAGGED Bash(npm test) -> clean Bash(npm run build *) -> FLAGGED ``` A2/A3's `P2_RULE_ERE` compared against the pattern it replaces, across tool names — this is where the two regressions below were found: ``` rule | old P2 | new P2 outcome Read(//Users/alice/secrets/**) | /Users/a | SKIPPED (//) Bash(/c/Users/kyle/x.sh:*) | /Users/k | FLAGGED (full rule now — A3 works) Read(/Users/kyle/.aws/credentials) | /Users/k | FLAGGED WebFetch(/Users/kyle/x) | /Users/k | NOT MATCHED <-- regression Agent(/Users/kyle/x) | /Users/k | NOT MATCHED <-- regression mcp__srv__tool(/Users/kyle/x) | /Users/k | NOT MATCHED <-- regression Read(//opt/data/../Users/kyle/secrets) | /Users/k | SKIPPED (//) <-- over-broad exemption ``` `permission-rule-check.test.sh` passes on this branch (72 + 16 new cases); none of the seven rows above is in it, which is why it stayed green. ## Related - **#2282** — the owning issue, closed here. Rows **A7b** and **A12** were split to **#2397** before merge so the auto-close drops nothing. - **#2397** — the follow-up carrying A7b + A12 with their HEAD reproductions, the A12-vs-`%USERPROFILE%` split, and A7b's branching-remedy constraint. - **#2260** — extracted the P1 rule vocabulary into `plugins/claude-config/lib/permission-patterns.sh`, which is why A19's fix lands in `lib/` rather than at the `scripts/permission-rule-check.sh:138,144` anchors #2282 names. That library now has a **second consumer** (`audit-permission-state`), so this change is no longer scoped to one detector. - **#2248 / #2249** (closed) — the previous `permission-rule-check` passes this builds on. #2248 rewrote P2's *rationale* without touching `P2_ERE`; #2249 removed the `$PWD` fallback and added the exit-2 refusal. - **#2283** — the sibling `audit-permission-grants` issue (clean bill with no denominator, `vendor/`-only exclusion, unimplemented scope filters). Not touched here. - **#2284** — the `criteria.md` staleness cluster. **Directly relevant:** the `//` exemption this PR implements comes from `criteria.md:64-65`, and #2284 is the issue about that file's doctrine being stale. See the review thread. - **#1398** (open) — faults P1's bare-name-on-PATH `Recommend`, the same remediation surface. - **#2301, #2335** — handoff-inbox batch 4 lane CC, the other `claude-config` waves. #2335 is open and bumps the same manifest, so this PR and that one will collide on `plugin.json` / `CHANGELOG.md`; whichever merges second needs a rebase. Inbox item: `20260811-024628-claude-config-audit-permission-grants-defects-and-fleet-grant-hygiene`. Ledger: `.work/handoff-inbox-batch-4/ledgers/I10-permission-grants-fleet.md` § A2, A3, A19. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
Two clusters in
claude-config:audit, both of the same shape: the skill claiming coverage it did not have.Category G was unexecutable (#2274). It is the one Phase 2 category a consumer could not run.
/doctor, which needs an interactive TTY, so the category yielded nothing in the harness's own headless mode. It now names the documented headless route — "When the listing exceeds its budget, Claude Code also writes a warning to the debug log, visible with--debug" — and reports which route it took./context's Skills row is labelled as what it is: a second interactive reading, not a headless substitute.skillListingBudgetFractiondefault0.01,SLASH_COMMAND_TOOL_CHAR_BUDGET's documented 8,000-character fallback,skillListingMaxDescCharsdefault1536, and the200,000 × 4 × 0.01 = 8,000arithmetic reconciling the first two.skillOverridessecond while carrying the caveat that it does not reach plugin skills, and never named the substitute upstream prescribes — so on a plugin-heavy roster the ordered list degenerated to the entry it labels "last resort". Levers are now split by roster origin,/pluginis named, and a roster-composition count is required before any lever is recommended. No per-skillname-onlystate is promised for/plugin; no page documents one.audit-checklist.md:5-7that said it had none is gone rather than left contradicting the same commit.Corrected remedy for the table row. The originating item asked for a new
check-skill-listing-budget.sh. That measurement already exists asplugins/skill-quality/scripts/check-listing-budget.sh(#1404), so the fix is wiring — and the wiring says out loud what the item did not: the two tools measure different populations. That script walks a repository's skills roots; Category G asks about the listing a consumer's session assembled. It is pointed at as an explicit in-repo proxy, never as the consumer's number. Its measured cost is stated too (below), because a marketplace-wide invocation cannot finish inside a default Bash tool timeout (#2216).auditasserted hook coverage it could not enumerate (#2275). Category D writes rules for${CLAUDE_PLUGIN_ROOT}/${CLAUDE_PLUGIN_DATA}placeholders that appear only in a plugin-provided hook, and Category B's third baseline narrowing turns on whether such a hook is live — while three of the skill's own surfaces disclosed in prose that the enumeration was impossible (context/procedures.md,reference/required-permissions.mdtwice). Gap disclosed in three places, closed in none.New
scripts/check-hook-coverage.sh(+.test.sh) closes it. It resolves each enabled plugin through the installed-plugin registry — every record carries a version-pinnedinstallPath, so no version-directory ordering is inferred — and reads its hook config in all four documented shapes (hooks/hooks.json; ahookspath, array of paths, or inline object inplugin.json; plugins-reference: "Location:hooks/hooks.jsonin plugin root, or inline in plugin.json", key typedstring|array|object). It also reportsdisableAllHooks/allowManagedHooksOnly/strictPluginOnlyCustomization, because a hook a setting has switched off is not coverage.The exit code is the contract:
0complete,1partial with the unenumerated sources named,2no inventory. The fail-open posture is kept and narrowed — it applies to what a partial run could not read, not to every run by default. This also removes the ordering problem the narrowing-3 precondition used to carry: Phase 1.0 runs before Category B, so the lever reading is available even on a scope-filtered/audit permissions. All three "no enumeration path exists" surfaces were rewritten in the same commit, and the eval that asserted "having no way to enumerate a plugin's hooks/hooks.json" was rewritten with them.A live defect found while writing it, with the mechanism stated correctly. On Git for Windows
jqwrites stdout in text mode and appends a CR to every line — a property of jq's own output stream, not of the input file's line endings. Untreated, a plugin key read out ofjqisname@marketplace\r, every registry lookup misses, and the plugin is reported as not installed on a machine where it is installed. My first comment blamed CRLF input files; the red run refuted that (LF-only fixtures failed too) and the comment was corrected to match. Pinned by a regression case.Also in this PR:
destructive-bash-denyships with its fragility. Eight argument-constraining globs, ratederrorwhen absent, with no caveat — in a file that quotes the permissions page's "Bash permission patterns that try to constrain command arguments are fragile" one section earlier, against a different table. The concrete hole is now stated: matching is prefix-anchored, soBash(git push --force *)does not matchgit push origin main --force. The patterns stay — they raise the cost of an accidental force push — but a finding no longer implies they bound a determined one. Deliberately not claimed: that upstream ranks the hook above the glob for destructive commands. That page's remedy recommendation is scoped to URL filtering; only the fragility half is doc-supported, and the PR says so.filesystem.*paths andnetwork.*hosts, with no expression for a command's arguments, so it cannot separategit pushfromgit push --forceto the same remote.settings, fetched in Phase 3.1, is the page a summarizing fetch already reported three present keys as absent on.Every doc quote above was re-fetched as raw markdown (
curl …/docs/en/<page>.md) on 2026-08-12 and grepped, not recalled.Test plan
New suite, red-then-green. Red proof: deleting the CR strip from
jqs():Green, with the fix restored:
Against the real machine (73 enabled plugins), the inventory the skill previously could not take:
The Category G proxy-cost figure quoted in the checklist, measured here:
Repo gates:
The two
check-skill.shwarnings (SKILL.md over the 200-line soft target; no Gotchas surface) are pre-existing on this skill, not introduced here.Security review note
check-hook-coverage.shis a new read-only surface. It widens what the skill reads — the user-scopesettings.json, the installed-plugin registry, and each enabled plugin's hook config — and adds no write, no network call, and no execution. It never runs a hook command; hook commands are printed as strings only. It grants nothing: no permission rule, noallowed-toolsentry, no new hook. The one judgment it deliberately does not make is whether an enumerated hook covers a permission family; that stays with Category B against the three preconditions inrequired-permissions.md, so the new data cannot by itself downgrade a security finding.plugin.jsonbumped 0.33.0 → 0.34.0 (new surface + behavior change) with a matching CHANGELOG entry.Related
Closes #2274
Closes #2275
Inbox items:
20260810-225906-claude-config-category-g-unmeasurable(#2274),20260810-225905-claude-config-audit-category-b-hook-blindness(#2275).Ledgers:
.work/handoff-inbox-batch-4/ledgers/I3-claude-config-category-g.md§ B1–B5 ·.../I2-claude-config-category-b.md§ A4, A2, MN1.Adjacent, not duplicated: #1404 (closed) shipped the measurement engine Category G now points at; #2216 owns its Windows runtime; #2023 / #2034 / #1271 own the marketplace-wide budget program, and no repo-wide aggregate is quoted here on purpose. #1598 is A2's precedent on the Read deny table in the same file. #2185 landed MN1's partial fix (Category F only).