fix(claude-config): let installed hook coverage narrow a baseline deny finding, and stop calling the addition mechanical by kyle-sexton · Pull Request #2206 · melodic-software/claude-code-plugins

kyle-sexton · 2026-08-11T20:55:13Z

Summary

claude-config's audit skill had a two-link defect chain in Category B. Batch-4 reconciliation
AD-13 requires both links ship as one change, and they do.

Link 1 — no channel for hook coverage to retract a baseline finding (#2201). Category B iterates the
baseline permission patterns and states flatly that each "must appear" in permissions.deny, rated
error. Its only two off-ramps are keyed on the consuming repo writing prose: a documented exemption
in its own rules files (required-permissions.md:138-142), or its own documented hook conventions
(:157-160). Neither is keyed on a hook that is actually installed and enabled, and grep -rn "hook"
across the whole skill returns no hooks.json read, no plugin-hook enumeration, and no coverage concept
at all. So a repo that blocks git push --force with a PreToolUse hook exiting 2 — which
permissions.md says "stops the tool call before
permission rules are evaluated, so the block applies even when an allow rule would otherwise let the call
proceed" (raw fetch, 2026-08-11) — was told its security floor was missing.

"Narrowing the baseline" now carries a third narrowing: a family already blocked by an installed,
enabled PreToolUse hook is info, not error, whether the hook came from the repo or from a plugin.
It obliges the report to name the residual — plugin disable/uninstall, per-guard opt-out, and
allowManagedHooksOnly / strictPluginOnlyCustomization — because hook coverage is contingent in ways a
deny rule is not, and an info that hides that is worse than the error it replaced. And because the
skill genuinely cannot enumerate a plugin's hooks/hooks.json, a run that took no inventory now states
the finding conditionally ("if a PreToolUse hook on Bash already blocks this family, this finding
is void") rather than asserting it. Fail open, not fail silent.

Verification sharpened the wiring half. The two existing off-ramps were not in Category B's
instruction path at all — the category delegates by pattern ("iterate the patterns in
required-permissions.md") and audit-checklist.md:21-24 says "assert presence per sub-category". Neither
names the off-ramp sections. A third off-ramp added to required-permissions.md alone would inherit that
same weak wiring, so Category B and the checklist's B.1–B.3 severity table now both point at "Narrowing
the baseline", and the table says its ratings are the unnarrowed ones. (audit-checklist.md is not in
the ledger's fix sketch; it is added deliberately under AD-13's own logic — leaving a flat error rating
there would have left a third file asserting the opposite.)

Link 2 — the false positive was routed into --fix as mechanical (#2202). procedures.md:49 graded
"Add missing baseline deny rules" Auto-fixable: Yes (from checklist) / Requires judgment: No, and
SKILL.md:212-214 restated that in prose. It is now judgment-required with the judgment written out —
is the family already covered, and would the addition suppress a gate the project built deliberately.
Moving a deny rule from local to project stays mechanical (that is #8961 placement, not a policy
change), and SKILL.md splits the two the same way.

Corrected rationale, stated so it is not shipped wrong. The filed claim was "no human judgment gate",
which is accurate as written and must not be read as "no human gate" — a confirmation gate exists at
SKILL.md:187-193 and is already pinned by eval #2. Adding a confirmation prompt is therefore not the
fix. The residual harm is narrower: the matrix told the user this class of edit needed no thought, next to
an offered reply 'all' that applies the lot in one keystroke. Two more scope statements kept honest —
the applied change was more deny rules, which is fail-closed rather than a security regression; and the
ask-suppression harm requires a hook that returns an ask decision, since a hook blocking by exit 2
short-circuits before permission rules and suppresses nothing.

What review added, and it materially improved the change. Seven threads, all substantive, all
addressed:

The narrowing was too generous, three ways. It is now fenced by three preconditions. The hook must
be live: disableAllHooks, allowManagedHooksOnly, and strictPluginOnlyCustomization were named
only as a future residual, and a hook one of them has already switched off blocks nothing, so under
any of them the finding stands unnarrowed. The hook must be on the tool surface the pattern defends:
sensitive-file-deny is a Read-pattern family, so a Bash-only hook leaves Read/Grep/Glob
open and retires nothing there. And it must block that family — force-push coverage says nothing
about git clean -fd, nor a long flag about its short spelling.
The liveness precondition cited a check that did not exist. It told the auditor to take the lever
reading from Category D; Category D checks paths, readability, timeouts, matchers, exec-form
resolution, duplicates, and events, and never asked whether the hooks it inventoried could run. All
three lever names appeared nowhere in the skill. That is the same false-assurance shape this PR set
out to remove, so Category D now reads and reports all three (info — a repo may set them
deliberately, and the reading is state, not a defect), and where the reading was not taken the
narrowing is unavailable rather than assumed clear.
And the reading is sequenced. Category B owns the narrowing and runs before Category D. A–I is
presentation order, not a dependency ban, so Category B says to pull the reading forward or defer the
downgrade and revise the severity after Category D runs. On a scope-filtered run like
/audit permissions, which never reaches Category D, the narrowing is unavailable unless the operator
supplies the state.
Three pointer sites had gone stale against my own tightening, still saying "installed, enabled".
Fixed, then swept.
The changelog shape misrepresented the bump. Everything sat under ### Fixed while the bump is
minor. Two graded outputs genuinely move, so the behavior changes are now under ### Changed with the
reason up front, the wiring repairs under ### Fixed, and the new material under ### Added.

Three evals, not one, and they grade the discriminations rather than the happy path:

#	Case	Grades
8	`baseline-deny-narrowed-by-installed-hook`	per-family narrowing — force-push and hard-reset drop to `info` under a live Bash hook, while `git clean` patterns it does not match and `sensitive-file-deny` Read patterns it cannot reach hold at `error`
9	`no-hook-inventory-states-the-finding-conditionally`	the fail-open half — no inventory means a conditional finding, not an assertion
10	`suppressed-hook-does-not-narrow-the-baseline`	the negative case the liveness fence exists for — a declared hook under `disableAllHooks: true` narrows nothing

No trust surface widened: no new hook, no new grant, no new external read or write. The permission
guidance moves in the direction of fewer manufactured deny-rule additions, and the new Category D row is
a read-and-report of settings the audit already has in hand.

Test plan

There is no executable test for this change — it is skill content plus evals.json, and this skill's
scripts/ cover plugin-drift and settings structure, not Category B prose. The honest gates are the
static ones, all run against the working tree:

$ jq empty plugins/claude-config/skills/audit/evals/evals.json && echo "evals.json: valid JSON"
evals.json: valid JSON
$ jq empty plugins/claude-config/.claude-plugin/plugin.json && echo "plugin.json: valid JSON"
plugin.json: valid JSON

$ bash plugins/skill-quality/scripts/check-evals-quality.sh plugins/claude-config/skills/audit/evals/evals.json
check-evals-quality: PASS (0 warning(s) across 1 file(s))
# (this one earned its keep — it caught eval 9 sharing eval 1's prompt verbatim,
#  which would have made the two gradeable only by out-of-band context)

$ CHECK_SKILL_SKILLS_ROOT="$PWD/plugins/claude-config/skills" bash plugins/skill-quality/scripts/check-skill.sh audit
INFO: description length 444/1536 chars
INFO: all 3 base-ref trigger phrase(s) preserved
INFO: SKILL.md 256/500 lines
INFO: markdownlint clean
INFO: script test passed: scripts/check-plugin-drift.test.sh
INFO: script test passed: scripts/check-structure.test.sh
INFO: script test passed: scripts/fix-plugin-drift.test.sh
WARN: SKILL.md is 256 lines (soft target 200 — consider pushing detail to progressive-disclosure spokes)
WARN: no Gotchas surface (inline '## Gotchas' or context/gotchas.md) — confirm the skill has no observed failure history
INFO: no metadata.category in frontmatter (optional — category not machine-readable)
INFO: shell: declared — dynamic-context injection portability is the author's explicit choice
INFO: summary 90/100 codepoints

CHECK-SKILL audit: PASS — 0 errors, 2 warning(s)

$ npx --no-install markdownlint-cli2 --config .markdownlint-cli2.jsonc plugins/claude-config/CHANGELOG.md
markdownlint-cli2 v0.23.2 (markdownlint v0.41.1)
Summary: 0 issues in 0 files

Both check-skill warnings pre-date this change (the SKILL.md soft target and the absent Gotchas
surface); this diff adds 4 lines to SKILL.md. Every gate above was re-run after each review round;
the output shown is from the final tree.

Ledger rows in I2 left filed-not-fixed by design: A2 (baseline recommends the pattern class upstream
calls fragile — MED, adjacent section of the same file), A4 (nothing reads a plugin's hooks/hooks.json
— MED, and the reason the new off-ramp has to fail open rather than enumerate), B5's Category G half
(belongs to item …-category-g-unmeasurable), MN1 (LOW). Issue filing for those rows is held pending
the operator's call on batch-4 filing volume (RECONCILE OR-4).

…y finding, and stop calling the addition mechanical Category B iterates the baseline permission patterns and states flatly that each "must appear" in permissions.deny. The only two ways out were prose the consuming repo writes — a documented exemption in its own rules files, or its own documented hook conventions. Neither is keyed on a hook that is actually installed and enabled, and grep -rn "hook" across the whole skill returns no hooks.json read, no plugin-hook enumeration, and no coverage concept. A repo that blocks git push --force with a PreToolUse hook exiting 2 — which the permissions reference says stops the call before permission rules are evaluated, ahead even of an allow rule — was told its security floor was missing. "Narrowing the baseline" now carries a third narrowing: a family already blocked by an installed, enabled PreToolUse hook is info, not error, whether the hook came from the repo or a plugin — with the residual named (plugin disable/uninstall, per-guard opt-out, allowManagedHooksOnly / strictPluginOnlyCustomization). Where no hook inventory was taken the finding is stated conditionally rather than asserted, because the skill has no enumeration path over a plugin's hooks/hooks.json. The off-ramps were also unreachable from where the check runs — Category B delegates by pattern and the checklist asserts presence per sub-category, neither naming the off-ramp sections. Both now point at them, and the B.1-B.3 severity table says its ratings are the unnarrowed ones. Phase 5 graded "Add missing baseline deny rules" auto-fixable / judgment No, which is the column that routes this false positive into --fix as mechanical and which contradicted this skill's own baseline reference (a deny added over a family a project hook escalates to an ask suppresses that prompt). It is now judgment-required with the judgment written out; moving a deny rule from local to project stays mechanical, and SKILL.md's prose restatement splits the two the same way instead of asserting the opposite. Eval 8 grades the chain. Closes #2201 Closes #2202 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

cursor · 2026-08-11T20:55:18Z

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 · 2026-08-11T20:55:36Z

Claude finished @kyle-sexton's task in 3m 15s —— View job

Review complete — PR #2206

Todo list

Gather context (diff, CLAUDE.md, REVIEW.md)
Read changed files in full
Review Category B / hook-coverage wiring changes (procedures.md, validation-categories.md, audit-checklist.md, required-permissions.md)
Review SKILL.md and evals.json changes
Check CHANGELOG.md and plugin.json version bump
Post findings as inline comments + summary

Summary

Repo-root CLAUDE.md is empty, so I reviewed against REVIEW.md's code-review lane scope (correctness, design, conventions, error handling, tests, docs — security is out of scope for this lane since this repo has no claude-security-review workflow gate configured for it, but nothing here reads as security-relevant regardless).

This is skill-content-as-code (Markdown + evals.json), so I traced the actual wiring the PR claims to have closed — Category B (validation-categories.md) → "Narrowing the baseline" (required-permissions.md) → severity table (audit-checklist.md) → Phase 5 fix matrix (procedures.md/SKILL.md) → eval #8 — rather than treating the prose as self-evidently correct. The chain checks out: Category B and the checklist's B.1–B.3 table both now point at "Narrowing the baseline," the Phase 5 matrix and SKILL.md's restatement agree (add=judgment, move=mechanical), and eval #8's expectations match what the new prose in required-permissions.md actually asserts. I independently ran jq empty on both changed JSON files — both valid (I did not independently run the PR's check-skill.sh/check-evals-quality.sh/markdownlint commands quoted in the Test Plan; those are author-claimed and unverified by me).

Two things worth the author's judgment, posted inline:

plugin.json/CHANGELOG.md — the version bump is minor (0.29.2 → 0.30.0) but the changelog section is ### Fixed-only. Every prior ### Fixed-only release in this changelog bumped the patch digit instead; minor bumps were reserved for ### Added/### Removed sections. Likely should be 0.29.3.
required-permissions.md "Narrowing 3" — its Bash/PowerShell tool-surface scoping appears to implicitly exclude sensitive-file-deny (a Read-pattern family) from hook-coverage narrowing, while audit-checklist.md's pointer applies the narrowing generically across the whole B.1–B.3 table. Possible residual wiring ambiguity of the exact kind this PR set out to close — worth either scoping it explicitly or extending the tool-surface list.

No correctness bugs or missing test coverage found beyond those two documentation/consistency points — both are Suggestion-level, not blocking.

claude · 2026-08-11T20:55:45Z

Claude finished @kyle-sexton's task in 39s —— View job

Security review complete

Gather context (PR diff, changed files)
Review changed files for security issues
Post findings

Scope: All 8 changed files reviewed via git diff origin/main...HEAD (plugin.json version bump, CHANGELOG.md, and five files under plugins/claude-config/skills/audit/ — SKILL.md, context/procedures.md, context/validation-categories.md, evals/evals.json, reference/audit-checklist.md, reference/required-permissions.md).

Finding: none.

This PR is entirely prose/documentation content plus a JSON evals fixture and a manifest version bump — no executable code, no scripts, no GitHub Actions workflow files, no dependency or action pins, and no settings/permissions files that are themselves consumed as live config. The changes teach the audit skill's guidance text to recognize installed PreToolUse hooks as a narrowing condition and to stop grading a deny-rule addition as mechanical — that's a change to what the skill recommends a human or future audit run do, not a change to any code path that executes commands, handles secrets/tokens, or touches trust boundaries.

Specifically checked and ruled out as inapplicable to this diff:

Injection surfaces — no shell, SQL, path, or template execution anywhere in the diff; evals/evals.json's new prompt/expected_output fields are inert test fixture strings, not executed.
GitHub Actions risk (pull_request_target/workflow_run with untrusted code, run: script injection via github context, permission widening, unpinned actions) — no workflow files changed in this PR.
Secrets/token handling — none touched.
Authorization/access-control logic — the diff narrows an audit finding's severity under specific, well-sconed conditions (documented exemption, documented hook convention, or an installed+enabled PreToolUse hook with the residual explicitly named); it does not touch any enforcement path (permissions.deny/ask/hook execution) itself. The plugin.json change is a version bump only.

No security concerns to flag on this PR.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a9b6b6d20

ℹ️ 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".

github-actions · 2026-08-11T20:59:07Z

Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

…e, and the specific family Review found three ways the new off-ramp was too generous, and one changelog shape that misrepresented the bump. Liveness: installed and enabled is not sufficient. disableAllHooks turns every hook off, and managed allowManagedHooksOnly / strictPluginOnlyCustomization suppress non-exempt hooks outright — a hook a setting has already disabled blocks nothing, so under any of those the finding stands unnarrowed. Those settings were named only as a future residual; they are now a precondition too. Tool surface: sensitive-file-deny is a Read-pattern family and a Read deny covers the built-in file tools as well as recognized Bash file commands, so a hook matching only Bash leaves Read/Grep/Glob open and retires nothing there. Family precision: coverage of git push --force says nothing about git clean -fd, nor a long flag about its short spelling. Eval 8 now grades exactly that discrimination — force-push and hard-reset drop to info under a live Bash hook while git clean and the Read patterns hold — and the fail-open half moves to its own eval 9 with a distinguishable prompt. The changelog was entirely ### Fixed while the bump is minor. Two graded outputs really do move, so the behavior-changing entries now sit under ### Changed with the reason stated up front, and the new evals under ### Added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude · 2026-08-11T21:09:19Z

Claude finished @kyle-sexton's task in 3m 41s —— View job

Review complete — PR #2206

Gather context (diff, CLAUDE.md, REVIEW.md)
Read changed files in full
Review Category B / hook-coverage wiring changes
Review SKILL.md, evals.json, procedures.md
Cross-check prior review comments (Codex, earlier Claude passes) against current HEAD
Post findings

Summary

Repo-root CLAUDE.md is empty, so I reviewed against REVIEW.md's code-review lane scope (correctness, design, conventions, error handling, tests, docs). This repo has a .github/workflows/claude-security-review.yml gate, so security is out of scope for this lane per REVIEW.md's split — none of this diff reads as security-relevant regardless (skill-content prose + an evals fixture + a manifest version bump, no executable paths touched).

I diffed the two commits separately rather than treating the final diff as one unit, since the second commit (4b16a55, "fence the hook narrowing on liveness, tool surface, and the specific family") was written specifically in response to prior review. That check paid off: both Codex P1 findings on the first commit are already resolved by 4b16a55 — the liveness preconditions (disableAllHooks / allowManagedHooksOnly / strictPluginOnlyCustomization) were added to required-permissions.md §3, and eval #8 now explicitly covers both git push --force and git push -f forms while keeping git clean at its unnarrowed severity. The earlier Claude-review round's two findings are also resolved: the changelog now carries ### Changed/### Fixed/### Added sections justifying the minor bump (0.30.0) with two behavior-changing entries stated up front, and the sensitive-file-deny/Read tool-surface gap is now explicit in required-permissions.md's new "tool surface" precondition. I independently ran jq empty on both changed JSON files (valid) and read every changed file in full; I did not independently re-run check-skill.sh/markdownlint/check-changelog-parity.sh — those remain author-claimed and unverified by me (sandboxed shell in this environment declined to run them here).

One new finding, posted inline at all three locations (Suggestion-level, not blocking): the second commit corrected required-permissions.md §3 to require a hook be live — not merely "installed, enabled" — before narrowing applies (it's inert under disableAllHooks, and non-exempt under allowManagedHooksOnly/strictPluginOnlyCustomization). That correction didn't propagate to the three places that gloss narrowing 3 inline while pointing at it: context/procedures.md:62, reference/audit-checklist.md:41, and context/validation-categories.md:24 all still say "installed, enabled PreToolUse hook" — the exact phrase required-permissions.md:159 now explicitly calls insufficient. Each links through to the fenced version, so the risk is bounded, but it's the same category of drift this PR's own Category B/checklist wiring fix was written to close (a summary that can diverge from its own SSOT). Worth a one-word swap ("installed, enabled" → "live") in each, matching the terminology required-permissions.md and the CHANGELOG now use.

No correctness bugs found in the core wiring change itself — the Category B → "Narrowing the baseline" → checklist severity table → Phase 5 fix matrix → eval #8/#9 chain is internally consistent and each file's severity/judgment split agrees with the others.

claude · 2026-08-11T21:09:28Z

Claude finished @kyle-sexton's task in 2m 14s —— View job

Security review complete

Gather context (gh/git diff origin/main...HEAD, changed files)
Review changed files for security issues
Post findings

Scope: All 8 changed files in this PR (plugin.json version bump, CHANGELOG.md, and five files under plugins/claude-config/skills/audit/). This diff is prose (Markdown) plus an eval fixture (evals.json) — no executable code, no workflow files, no dependency pins, and nothing that itself enforces permissions. It teaches the audit skill's advisory text — what severity a human/future-audit-run should report for a missing baseline permissions.deny rule. Reviewed with that framing: the security-relevant risk in prose like this is a logic gap that causes the skill to assert a security control holds when nothing actually verified it — the same failure class the PR itself is fixing for Category B's older off-ramps.

Two prior review passes on this PR (Codex, at commit 6a9b6b6) flagged exactly that failure class twice — a disableAllHooks/allowManagedHooksOnly liveness gap and an incomplete command-family eval. The latest commit (4b16a55, "fence the hook narrowing on liveness, tool surface, and the specific family") added a "Three preconditions" block to required-permissions.md that appears to close both: it now requires the hook be live (not merely installed+enabled), scoped to the matching tool surface (Bash doesn't cover sensitive-file-deny's Read family), and matching the specific pattern family — and eval #8 in evals.json now exercises the split (force-push/hard-reset → info, git clean and Read patterns stay error). I re-verified the full baseline pattern table in required-permissions.md (4 force-push + 2 hard-reset + 2 git clean patterns) against eval #8's expectations — they line up correctly.

One finding survives that fix, introduced by the fix itself:

[IMPORTANT] The new liveness precondition cites a check that doesn't exist, reopening the same false-assurance gap one layer down
required-permissions.md#L159-L164

The new "hook is live, not merely present" precondition — the one meant to stop the audit from downgrading a missing baseline deny rule to info when the covering hook is actually inert — tells the auditor: "Check the settings-declared layer for these before downgrading — Category D already reads it."

Category D does not read it. I checked both validation-categories.md's Category D section and audit-checklist.md's ## D. Hooks table (neither touched by this PR): both cover script-path existence, readability, timeout units, matcher syntax, shell-quoting, and Windows exec resolution — none check disableAllHooks, allowManagedHooksOnly, or strictPluginOnlyCustomization. A repo-wide grep confirms those three setting names appear nowhere in the audit skill except in this new required-permissions.md text and the new eval prompt/expectations:

$ grep -rn "disableAllHooks\|allowManagedHooksOnly\|strictPluginOnlyCustomization" plugins/claude-config/skills/audit/
reference/required-permissions.md:159-160  (this PR's new precondition text)
reference/required-permissions.md:178-179  (this PR's "name the residual" text)
evals/evals.json:96,103                    (this PR's new eval fixture)

So the precondition that's supposed to gate narrowing 3 — confirm the hook can actually run before trusting it as coverage — delegates its own verification to a check that isn't wired anywhere in the skill's instruction path. Followed literally, an auditor reads "Category D already reads it," takes that as license to skip an independent settings.json check, confirms only "installed, enabled," and downgrades a missing destructive-bash-deny finding from error to info. If disableAllHooks: true (or a managed allowManagedHooksOnly) is actually set, the hook is inert, no deny rule exists, and the audit now reports info on a force-push/hard-reset guardrail that is fully absent — worse than the pre-PR state, where the same repo would have gotten a flat error.

This is the identical two-link defect pattern (Category B claims a narrowing that isn't backed by wiring elsewhere) that this PR's own summary calls out and fixes for the original off-ramps — just reintroduced one layer down, inside the fix commit that closed the first two instances. Suggested resolution: either add the three settings to Category D's checklist (and have narrowing 3 cite it correctly), or have narrowing 3 state the settings.json check directly inline rather than pointing at a category that doesn't perform it.

Confidence: High — verified by reading the current Category D content in both files and a full-skill grep for the three setting names; not a matter of interpretation.

Ruled out / out of scope for this lane: No GitHub Actions files changed (dangerous trigger patterns, permission-widening, unpinned actions N/A). No secrets/token handling touched. No injection surfaces — evals.json's new prompt/expected_output are inert fixture strings for a skill-quality checker, not executed code. plugin.json's change is a version bump only.

…to narrowing 3 The previous commit tightened "Narrowing the baseline" §3 but left the three sites that point at it — Category B, the B.1-B.3 severity note, and the Phase 5 judgment gloss — still saying "installed, enabled". That is the exact shape of drift this PR exists to remove: a reader who stops at the entry point, which for Category B is the common case, would apply the unfenced version. All three now say "live", name the tool-surface constraint, and send the reader to the three preconditions rather than restating a stale summary of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude · 2026-08-11T21:17:49Z

Claude finished @kyle-sexton's task in 2m 8s —— View job

Review complete — PR #2206

Todo list

Gather context (diff, CLAUDE.md, REVIEW.md)
Read changed files in full at current HEAD (6837804)
Verify whether the latest commit resolves the prior "stale shorthand" finding across the three pointer files
Check for any other correctness/wiring issues introduced by this commit
Post findings as inline comments + summary

Summary

Repo-root CLAUDE.md is empty, so I reviewed against REVIEW.md's code-review lane scope (correctness, design, conventions, error handling, tests, docs), leaving security-relevant findings to the dedicated security lane per REVIEW.md's split.

This sync added one commit, 6837804 ("carry the liveness fence into all three pointers to narrowing 3"), on top of the two already reviewed. It's a direct response to the previous code-review pass's finding: context/procedures.md:62, reference/audit-checklist.md:41, and context/validation-categories.md:24 all still said "installed, enabled PreToolUse hook" after required-permissions.md §3 was tightened to require liveness. I diffed 6837804 in isolation and confirmed all three now say "live", name the tool-surface constraint, and point at the three preconditions rather than restating a stale gloss — that finding is resolved.

One correctness issue remains, not touched by this sync (posted inline at required-permissions.md:163-164): the "hook is live" precondition tells the auditor "Category D already reads it" for the disableAllHooks / allowManagedHooksOnly / strictPluginOnlyCustomization settings. I independently read Category D in both places it's actually specified — validation-categories.md's "Category D: Hooks" and audit-checklist.md's "D. Hooks" — and neither file (untouched by this PR) checks those three settings; Category D covers script-path existence, readability, timeout units, matcher syntax, shell-quoting, and Windows exec resolution only. The citation is false, and it's the same defect class this whole PR exists to close (a claim about cross-file coverage that isn't backed by wiring) — reintroduced one layer down, inside the precondition meant to prevent exactly that failure mode. Full detail in the inline comment.

I re-verified the core wiring chain independently rather than trusting the PR's own description of it: Category B (validation-categories.md:21-29) and the checklist's B.1–B.3 table (audit-checklist.md:38-42) both point at "Narrowing the baseline"; the Phase 5 matrix (procedures.md:59-71) and SKILL.md's restatement (SKILL.md:212-216) agree add=judgment / move=mechanical; evals.json's eval #8 expectations match the per-family/per-tool-surface split in required-permissions.md (force-push and hard-reset → info, git clean and sensitive-file-deny Read patterns stay unnarrowed); and the CHANGELOG.md 0.30.0 entry's ### Changed/### Fixed/### Added split matches the two behavior changes it claims. I ran jq empty on both changed JSON files (valid). I did not independently run check-skill.sh / check-evals-quality.sh / markdownlint — those remain author-claimed and unverified by me.

No other correctness bugs or missing test coverage found. The one inline finding is a documentation/wiring accuracy issue (Suggestion/Important-adjacent, not blocking) — everything else raised in earlier review rounds on this PR has been addressed.

claude · 2026-08-11T21:18:07Z

Claude finished @kyle-sexton's task in 1m 52s —— View job

Security review complete

Gather context (git diff origin/main...HEAD, changed files)
Review changed files for security issues
Post findings

Scope: All 8 files changed in this PR at HEAD (6837804) — plugin.json version bump, CHANGELOG.md, and five files under plugins/claude-config/skills/audit/ (SKILL.md, context/procedures.md, context/validation-categories.md, evals/evals.json, reference/audit-checklist.md, reference/required-permissions.md). This diff is entirely prose (Markdown) plus an inert eval fixture (evals.json) and a manifest version bump — no executable code, no workflow files, no dependency pins, and nothing that itself enforces permissions. It teaches the audit skill's advisory text what severity to report for a missing baseline permissions.deny rule. Reviewed with the framing this repo's own PR description uses: the risk in prose like this is a logic gap where the skill asserts a security control holds when nothing actually verified it — exactly the failure class this PR sets out to fix for Category B's older off-ramps.

One finding, posted inline — still live at HEAD.

[IMPORTANT] Liveness precondition in required-permissions.md cites a Category D check that doesn't exist — reference/required-permissions.md#L159-L164

Narrowing 3's "hook is live, not merely present" precondition tells the auditor "Check the settings-declared layer for these before downgrading — Category D already reads it." It doesn't: I re-read Category D in both validation-categories.md (untouched by this PR) and audit-checklist.md's ## D. Hooks table (also untouched) and confirmed neither checks disableAllHooks, allowManagedHooksOnly, or strictPluginOnlyCustomization. A repo-wide grep shows those three setting names appear only in this PR's own new prose and its new eval fixture — nowhere in the skill's actual instruction path. Followed literally, an auditor confirms only "installed, enabled," skips an independent settings check on the strength of a citation that doesn't hold, and can downgrade a missing destructive-bash-deny finding to info even when disableAllHooks: true makes the covering hook fully inert — worse than the pre-PR flat error, because the guardrail is now both absent and unreported.

This is a repeat finding: a prior automated security-review pass on this PR flagged the identical line at commit 4b16a55. The next commit (6837804, "carry the liveness fence into all three pointers to narrowing 3") fixed the terminology drift ("installed, enabled" → "live") in the three files that point at narrowing 3, but did not touch required-permissions.md itself — the SSOT where the false "Category D already reads it" claim lives — so the gap survived into the current HEAD. Eval #8 also doesn't exercise it: its prompt states up front that disableAllHooks is not set, so no eval covers the case this precondition is supposed to catch.

Confidence: High — verified directly by reading current file content and a full-skill grep, not inferential.

Ruled out / not re-reported (already covered by prior review rounds and confirmed fixed at this HEAD): the earlier disableAllHooks/liveness gap in narrowing 3 itself (fenced by 4b16a55), the eval-8 partial-family-coverage gap (fixed by 4b16a55's per-family expectations), and the sensitive-file-deny/Read tool-surface scope gap (fixed by 4b16a55's tool-surface precondition). Out of scope for this lane: no GitHub Actions files changed (dangerous triggers, permission-widening, unpinned actions N/A); no secrets/token handling touched; no injection surfaces (evals.json's prompt/expected_output fields are inert fixture strings for a skill-quality checker, never executed); plugin.json's change is a version bump only.

…n levers the narrowing depends on The liveness precondition told the auditor to take the disableAllHooks / allowManagedHooksOnly / strictPluginOnlyCustomization reading from Category D. Category D does not take it: it checks script paths, readability, timeout units, matchers, exec-form resolution, duplicates, and events, and never asks whether the hooks it inventoried can run at all. Grep confirms all three names appeared nowhere in the skill before this PR. That is the same false-assurance shape this PR set out to remove — a precondition that cannot be evaluated is one the reader will quietly skip, and skipping it means downgrading a real finding on the strength of an inert hook. So the check is now real. Category D reads all three levers and reports each as set or unset with the hooks it switches off, rated info because a repo may set any of them deliberately and the reading is state rather than a defect. And the precondition no longer assumes the reading was taken: where it was not, the narrowing is unavailable rather than assumed clear, because an unread lever is not an unset one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude · 2026-08-11T21:26:32Z

Claude finished @kyle-sexton's task in 1m 3s —— View job

Security review complete

Gather context (git log/git diff origin/main...HEAD, changed files at new HEAD 7b501ef)
Review latest commit 7b501ef — verify it fixes the repeat "Category D already reads it" finding
Re-check rest of diff for new/unresolved security issues
Post findings

Scope: All 8 files changed in this PR at HEAD (7b501ef) — plugin.json version bump, CHANGELOG.md, and five files under plugins/claude-config/skills/audit/. This diff remains entirely prose (Markdown) plus an inert eval fixture (evals.json) and a manifest version bump — no executable code, no workflow files, no dependency pins, nothing that itself enforces permissions.

Prior finding — resolved at this HEAD.

Two prior automated security-review passes on this PR (at 4b16a55 and again at 6837804) flagged the same [IMPORTANT] finding: the "hook is live, not merely present" precondition in required-permissions.md told the auditor "Category D already reads it" for disableAllHooks / allowManagedHooksOnly / strictPluginOnlyCustomization, but Category D — in both validation-categories.md and audit-checklist.md — never actually checked those three settings. Followed literally, an auditor would skip an independent settings check and could downgrade a real missing-deny finding to info under an inert hook.

Commit 7b501ef closes this correctly, and does so with the right failure mode:

Category D now has a real check in both files: it reads disableAllHooks (settings-declared layer) and allowManagedHooksOnly/strictPluginOnlyCustomization (managed layer), reports each as set/unset with the hooks it disables, rated info (state, not a defect) — validation-categories.md:59-64, audit-checklist.md:102.
The narrowing-3 precondition in required-permissions.md no longer just cites Category D — it now specifies what happens when the reading wasn't taken: "where that reading was not taken, the narrowing is unavailable rather than assumed clear — an unread lever is not an unset one" (required-permissions.md:163-166).

That second piece is what actually closes the false-assurance gap rather than relocating it: even if a future run doesn't wire Category D's new check correctly, the precondition's default (unread → narrowing unavailable → finding stays at unnarrowed severity) fails closed, not open. This is the same "fail open on no inventory" pattern already used elsewhere in this PR for the plugin-hook enumeration gap.

No new security issues found in the rest of the diff at this HEAD — the remainder is unchanged from what prior passes already reviewed (no executable paths, no injection surfaces, no secrets/token handling, no GitHub Actions files touched, plugin.json is a version bump only).

Confidence: High — verified by reading the current content of all three files (required-permissions.md, validation-categories.md, audit-checklist.md) at HEAD 7b501ef, not inferential.
· branch fix/2201-audit-hook-coverage-off-ramp

claude · 2026-08-11T21:29:24Z

Claude finished @kyle-sexton's task in 3m 40s —— View job

Review complete — PR #2206

Todo list

Gather context (diff at current HEAD 7b501ef, CLAUDE.md, REVIEW.md)
Check whether the latest commit closes the repeat "Category D already reads it" finding
Re-verify the rest of the wiring chain for regressions
Post findings as inline comments + summary

Summary

Repo-root CLAUDE.md is empty, so I reviewed against REVIEW.md's code-review lane scope (correctness, design, conventions, error handling, test coverage, docs), leaving security-relevant findings to the dedicated security lane per REVIEW.md's split.

The latest commit, 7b501ef ("make Category D actually read the hook-suppression levers the narrowing depends on"), responds to the repeat security-lane finding on this PR: required-permissions.md's liveness precondition claimed "Category D already reads" disableAllHooks/allowManagedHooksOnly/strictPluginOnlyCustomization, when Category D (untouched by prior commits) checked none of them. I verified this is now actually true — validation-categories.md's Category D section and audit-checklist.md's ## D. Hooks table both gained a new row for the three levers (rated info, since the reading is state not a defect), and required-permissions.md's precondition was reworded so an unread lever leaves the narrowing unavailable rather than assumed clear. A full-skill grep confirms no stale "Category D already reads it" phrasing survives anywhere.

Two new findings, posted inline (both Suggestion-level, not blocking):

required-permissions.md:162-164 — the fix creates a new cross-category ordering question it doesn't resolve. Category B (Permissions) is processed before Category D (Hooks) in the A→I checklist order in validation-categories.md, yet Category B's narrowing now depends on a reading owned by Category D. Nothing sequences D before B, or says to revisit B's severity once D's reading exists.
evals/evals.json:96-103 — eval build(dependabot): fast-track first-party melodic-software/* actions #8's prompt explicitly states disableAllHooks is not set, so no eval exercises the case the new Category D check exists to catch: a lever actually set, where a live-looking hook is inert and the finding must stay error rather than drop to info.

Re-verified, no regressions found: the rest of the wiring chain (Category B → "Narrowing the baseline" → checklist B.1–B.3 table → Phase 5 fix matrix in procedures.md/SKILL.md → eval #8/#9) is internally consistent — I read every changed file at HEAD in full and independently ran jq empty on both changed JSON files (valid). I did not independently run check-skill.sh/check-evals-quality.sh/markdownlint — those remain author-claimed and unverified by me. All findings from earlier review rounds on this PR (changelog version-bump shape, sensitive-file-deny tool-surface scope, the "installed, enabled" → "live" terminology drift across three pointer files) are confirmed resolved at this HEAD.

…, and grade the suppressed-hook case Two gaps in the liveness fence, both real. Category B owns the narrowing and runs before Category D, which owns the lever reading it depends on. A-I is presentation order, not a dependency ban, so Category B now says so: pull the reading forward, or defer the downgrade until Category D has run and revise the severity then. What it may not do is take the narrowing on an unread lever — and on a scope-filtered run like /audit permissions, which never reaches Category D, the narrowing is simply unavailable unless the operator supplies the state. And every eval so far graded the clean case. Eval 10 grades the case the precondition exists for: a PreToolUse hook declared on Bash under disableAllHooks: true narrows nothing, the destructive-git patterns hold at error, and the report says why the narrowing did not apply instead of omitting it silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude · 2026-08-11T21:36:20Z

Claude finished @kyle-sexton's task in 1m 31s —— View job

Security review complete

Gather context (git diff origin/main...HEAD, changed files at new HEAD 8f93991)
Verify latest commit 8f93991 resolves the two open findings (ordering gap, eval coverage gap)
Re-check rest of diff for new/unresolved security issues
Post findings

Scope: All 8 files changed in this PR at HEAD (8f93991) — plugin.json version bump, CHANGELOG.md, and five files under plugins/claude-config/skills/audit/. This diff remains entirely prose (Markdown) plus an inert eval fixture (evals.json) and a manifest version bump — no executable code, no workflow files, no dependency pins, nothing that itself enforces permissions.

Both open findings — resolved at this HEAD.

Ordering gap (flagged on the prior sync, 7b501ef): Category B owns the third narrowing but is processed before Category D, which owns the disableAllHooks/allowManagedHooksOnly/strictPluginOnlyCustomization reading the narrowing depends on, and nothing sequenced the two or said what to do about an unread lever taken out of order. Commit 8f93991 closes this in validation-categories.md:30-35: it states plainly that A–I is presentation order, not a dependency ban, instructs the auditor to either pull Category D's reading forward or defer the downgrade until Category D has run and revise the severity then, and — critically — forecloses the failure mode a literal top-to-bottom read would hit: an unread lever leaves the narrowing unavailable, and a scope-filtered run like /audit permissions (which never reaches Category D) can't take the narrowing at all unless the operator supplies the state out of band. This fails closed (finding stays at unnarrowed error) in every branch, including the one where the auditor does nothing special.
Eval coverage gap: every eval up to this point only exercised the clean case (no suppression lever set), so nothing graded whether an audit run correctly holds error when a hook looks live but is actually suppressed. New eval build(deps): bump ci-workflows pin to b6431a1 (PSSA RULE_ERROR retry fix) #10 suppressed-hook-does-not-narrow-the-baseline (evals.json:118-131) closes it directly: a PreToolUse hook declared on Bash under disableAllHooks: true must not narrow the destructive-git patterns, they must hold at error, Category D must report the lever as set and name the hook it switches off, and the report must say why the narrowing didn't apply rather than omitting it silently. I checked the expectations against the current baseline pattern table and the Category D/narrowing-3 prose — they match what's actually specified.

No new security issues found in this commit or the rest of the diff at this HEAD. The remainder is unchanged from what prior review passes already covered: no executable paths, no injection surfaces, no secrets/token handling, no GitHub Actions files touched, plugin.json is a version bump only. I independently ran jq empty on both changed JSON files (valid).

Confidence: High — verified by reading the current content of validation-categories.md, required-permissions.md, audit-checklist.md, and evals.json at HEAD 8f93991, not inferential.
· branch fix/2201-audit-hook-coverage-off-ramp

claude · 2026-08-11T21:47:37Z

Claude finished @kyle-sexton's task in 2m 43s —— View job

Review complete — PR #2206

Todo list

Gather context (diff at current HEAD 8f93991, CLAUDE.md, REVIEW.md)
Verify latest commit 8f93991 resolves the two open findings from the prior review round (Category B/D ordering gap; eval coverage gap for disableAllHooks: true)
Re-check rest of diff for new/unresolved correctness, design, or test-coverage issues
Post findings as inline comments + summary

Summary

Repo-root CLAUDE.md is empty, so I reviewed against REVIEW.md's code-review lane scope (correctness, design, conventions, error handling, test coverage, docs), leaving security-relevant findings to the dedicated security lane per REVIEW.md's split.

The latest commit, 8f93991 ("sequence the lever reading against category order, and grade the suppressed-hook case"), is a direct response to the two open findings from the prior review round (both posted against required-permissions.md/evals.json at HEAD 7b501ef):

Ordering gap — Category B (which owns narrowing 3) runs before Category D (which now owns the disableAllHooks/allowManagedHooksOnly/strictPluginOnlyCustomization lever reading the narrowing depends on), and nothing sequenced the two. This commit adds a new bullet to validation-categories.md's Category B — line 30 — that states A–I is presentation order, not a dependency ban, and gives the auditor two explicit resolutions (pull Category D's reading forward, or defer the downgrade until Category D has run and revise the severity then), plus the scope-filtered case: on /audit permissions, which never reaches Category D, the narrowing is unavailable unless the operator supplies the state out of band. I checked this against SKILL.md's Phase 2 section (unchanged) — it lists categories A–I but never mandates strict sequential execution, so the new "presentation order, not a dependency ban" framing doesn't contradict it. Resolved.
Eval coverage gap — no eval exercised a lever actually being set. New eval build(deps): bump ci-workflows pin to b6431a1 (PSSA RULE_ERROR retry fix) #10 (suppressed-hook-does-not-narrow-the-baseline, evals.json lines 119-131) runs /audit all (so Category D does execute) with disableAllHooks: true alongside a declared hook that would otherwise cover force-push/hard-reset, and expects the patterns to stay error with the report stating why the narrowing didn't apply. I checked the expectations against the new Category B/D prose — they line up. Resolved. I ran jq empty on evals.json independently — valid JSON.

Two new findings, posted inline (both Suggestion-level, not blocking):

CHANGELOG.md:81 — this commit's changelog edit only names eval build(deps): bump ci-workflows pin to b6431a1 (PSSA RULE_ERROR retry fix) #10 under ### Added; the new Category B sequencing bullet itself (a real procedural/behavioral addition) isn't documented anywhere in the 0.30.0 entry, breaking the meticulous one-bullet-per-change discipline the rest of this PR's changelog entries follow.
validation-categories.md:30 — the new sequencing detail (pull-forward-or-defer, the /audit permissions example) lives only in this pointer file, not in required-permissions.md's "Narrowing the baseline" §3 that this same section directs readers to for the full preconditions — a minor instance of the pointer/SSOT asymmetry this PR's own 6837804 commit fixed for the "installed, enabled" → "live" wording drift.

No other changes since the last review round — git diff 7b501ef..8f93991 touches only CHANGELOG.md, validation-categories.md, and evals.json, all covered above. All findings from earlier rounds (changelog version-bump shape, sensitive-file-deny tool-surface scope, "installed, enabled"→"live" terminology drift, the "Category D already reads it" false citation) remain resolved at this HEAD. I did not independently run check-skill.sh/check-evals-quality.sh/markdownlint — those remain author-claimed and unverified by me.
· branch fix/2201-audit-hook-coverage-off-ramp

… it a changelog bullet The pull-forward-or-defer rule and the scope-filtered /audit permissions case lived only in Category B, while Category B tells the reader to go read the three preconditions in required-permissions.md. A reader who lands on precondition 1 directly got "take the reading from Category D" with nothing about Category D running later. Both files now carry it. The same commit's normative addition also went unrecorded in the changelog while every other prose change in this PR got a bullet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude · 2026-08-11T21:52:31Z

Claude finished @kyle-sexton's task in 48s —— View job

Security review complete

Gather context (git diff origin/main...HEAD, changed files at new HEAD fcb02a2)
Review latest commit fcb02a2 for security issues
Re-check rest of diff for unresolved security issues
Post findings

Scope: All 8 files changed in this PR at HEAD (fcb02a2) — plugin.json version bump, CHANGELOG.md, and five files under plugins/claude-config/skills/audit/. This diff remains entirely prose (Markdown) plus an inert eval fixture (evals.json) and a manifest version bump — no executable code, no workflow files, no dependency pins, nothing that itself enforces permissions.

Latest commit (fcb02a2, "mirror the sequencing into narrowing 3, and give it a changelog bullet") — no security-relevant content.

git diff 8f93991..fcb02a2 touches exactly two files:

required-permissions.md:162-167 — adds two sentences mirroring the Category B/D sequencing rule (pull-forward-or-defer, /audit permissions scope-filtered case) that already existed in validation-categories.md. This is documentation duplication for a single source of truth, not new logic — the underlying fail-closed behavior (unread lever → narrowing unavailable → finding stays at unnarrowed severity) is unchanged.
CHANGELOG.md:77-82 — adds the changelog bullet documenting the sequencing rule added in the prior commit (8f93991). Text only.

Both changes close code-review-lane suggestions from the prior round (cross-reference gap and missing changelog bullet) — not security findings. No new trust surface, no new setting, no new hook, no change to any enforcement path.

No new security issues found, and no regressions in previously-resolved findings. Everything else in the diff is unchanged from what five prior review passes on this PR already covered and confirmed resolved:

The "hook is live, not merely present" precondition and its three preconditions (liveness, tool-surface, family-match) in required-permissions.md §3.
Category D now actually reads and reports disableAllHooks/allowManagedHooksOnly/strictPluginOnlyCustomization (closing the earlier "Category D already reads it" false-citation gap).
The Category B/D ordering gap, now fail-closed via "unavailable if unread."
Eval build(deps): bump ci-workflows pin to b6431a1 (PSSA RULE_ERROR retry fix) #10 (suppressed-hook-does-not-narrow-the-baseline) exercising the negative case.
No GitHub Actions files touched (dangerous triggers, permission-widening, unpinned actions N/A). No secrets/token handling. No injection surfaces — evals.json's prompt/expected_output fields are inert fixture strings for a skill-quality checker, never executed. plugin.json's change is a version bump only.

Confidence: High — verified by diffing the exact commit range and reading the changed content directly, not inferential.
· branch fix/2201-audit-hook-coverage-off-ramp

kyle-sexton · 2026-08-12T01:40:28Z

Batch-4 filing note: #2275 files what this PR left standing in the same skill, and one of its rows is sharper because of this change.

A4 — nothing in Phase 1, Category D, or Category E ever reads a plugin's hooks/hooks.json. This PR added a third baseline narrowing that turns on whether a PreToolUse hook is live, and then correctly disclosed in three places that the audit cannot take that inventory (context/procedures.md:68-69, reference/required-permissions.md:184-186, and eval 9). So the gap is now documented in three surfaces and closed in none, and on most runs the new off-ramp resolves to "state the finding conditionally" rather than to an answer. The remedy is an enumeration path, not more disclosure.

Also in #2275: A2 (the destructive-bash-deny table is eight argument-constraining globs rated error with no fragility caveat — while the same file quotes that upstream warning at :57-60 against the Read table) and MN1 (the verbatim-fetch guard sits on one checklist row while Phase 3.1 and 3.4 fetch settings and permissions without it).

chatgpt-codex-connector Bot reviewed Aug 11, 2026

View reviewed changes

Comment thread plugins/claude-config/skills/audit/reference/required-permissions.md Outdated

Comment thread plugins/claude-config/skills/audit/evals/evals.json Outdated