feat(claude-config): complete the permission-state skill and add the auto-mode authoring lane - #2396
Conversation
Records the contract and implementation plan for two new claude-config skills: audit-permission-state, which computes the effective merged permission state across all five settings scopes with per-rule provenance, and draft-auto-mode-rules, the non-writing authoring lane. The plan is ten phases, sequential. Phase 0 discharges the fresh-docs mandate before any version-gate constant ships. Phase 9 moved ahead of the skill work because it owns two seams that do not exist yet: a fixture-home override so user-global scope is testable without reading the real home, and a side-effect-free extraction of the shared pattern vocabulary, which today lives in a script that self-executes at load. Two probes settled questions the Brief had parked. The debug channel narrates the permission merge per destination and names every allow rule auto mode drops, with its source path and reason; it becomes an opt-in, explicitly priced oracle cross-checked against the computed prediction, not a replacement for it. A non-strict JSON parser is genuinely required for one optional lane: the malformed CLI output carries a raw line feed inside a string value, so no line-oriented filter repairs it, which puts Python in the optional-feature class and leaves the portable core intact. A fresh-context reviewer returned nineteen findings against an earlier draft. All nineteen were re-verified against the repository and all held. Both critical ones were stale premises: PR #2089 had already merged with its OFFICIAL-DOCS row, the frontmatter-name refactor had landed on main, and this slice was untracked on a spent branch and absent from main. Two items are left open rather than guessed: which second existing check the Brief meant by widening two, and what a -p oracle session writes outside its scratch path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
Phase 0 re-confirms every upstream fact the two planned skills would ship, against the auto-mode-config, settings, permission-modes, and permissions pages fetched today, rather than the single local capture the Brief was resting on. Eleven facts are confirmed with quoted wording, including the scope precedence, the deny-then-ask-then-allow evaluation order, the four classes of allow rule auto mode drops on entry, and that permission rules from a pre-v2.1.211 start-directory settings.local.json stay in effect. The absence of allowManagedAutoModeRulesOnly is now supported by the governing page rather than by an unverified research slice. Five corrections land against the plan. The largest: managed policy is not two JSON files. It is a plist domain on macOS, a registry hive on Windows, a JSON file, and a managed-settings.d drop-in directory, per OS, so the managed read cannot be jq over a fixed pair of paths. The local settings file also resolves through worktrees to the main checkout, which this topic is itself being planned inside. And autoMode.classifyAllShell suspends every shell allow rule when set, inverting the drop-set answer that no criterion currently covers. Three claims move to not-stated and must not ship as fact: the v2.1.142 gate on project-scope defaultMode, the scope restriction on useAutoModeDuringPlan, and the Write(path) never-consulted mechanic, which the page describes differently and with a startup warning as its observable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
Phase 1's managed-policy reader is split into a portable core (the per-OS managed-settings.json and its managed-settings.d/ drop-in directory, always read) and two declared optional platform legs (the Windows registry policy keys and the macOS managed-preferences domain) that warn visibly and skip rather than failing the run. A Windows reader that checks only the Program Files JSON reports "no managed policy deployed" while a registry-deployed policy is in force, so dropping the registry buys a wrong finding rather than a documented gap. An unelevated reg query of HKLM\SOFTWARE\Policies was measured to succeed, so the leg costs no elevation. The Brief's long-unassigned second scope-widening is named: claude-config's audit skill gains the start-directory settings.local.json copy, whose permission rules stay in effect and which nothing audits today. The authoring lane drops its unnamed history input and drafts from the interview plus the computed merge, removing an unreviewable read surface and a second dependency on the priced debug-channel oracle. Phase 9 absorbs the shared managed-scope path enumeration, since approving the managed reader would otherwise make this repository's third copy of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…ared vocabulary Check P1 scanned project and local settings only, so an interpreter-wildcard rule in the user-global settings file was invisible to it — and that is the scope Claude Code's own "Always allow" path writes to, so it is where the broad rules auto mode drops actually accumulate. The user scope resolves CLAUDE_CONFIG_DIR before $HOME, matching the resolver claude-memory already cites to the .claude-directory doc; that resolution is also the test seam, so a fixture home replaces the operator's real one and no test reads it. The audit skill's structure check gains the pre-v2.1.211 start-directory settings.local.json, whose permission rules stay in effect alongside the repository-root copy. The row appears only where the two directories genuinely differ, so one file is never counted as two rule sources. Two libraries come out of the widening. The auto-mode drop vocabulary was inline in a detector that self-executes and cannot be sourced, leaving a second consumer no way to reuse it; the per-OS managed-policy locations were hand-kept in three places that had already disagreed about whether the drop-in directory exists. claude-memory carries a byte-identical copy of the managed-scope library, registered as a cross-plugin cluster so the two cannot drift — verified by perturbing a copy and watching the check fail. Both file-only readers now name the managed surfaces they do not read (the Windows policy registry keys, the macOS preferences domain) so an absent managed-settings.json cannot read as "no managed policy deployed". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…ipping it With neither CLAUDE_CONFIG_DIR nor HOME set there is no user-global settings file to resolve, and the detector scanned nothing without saying so — which would let "No fragile permission grants found." rest on a scope that was never opened. It now writes a named notice to stderr, with a regression test covering it. The plugin README carries the widened scope and the fact that a user-global finding's remediation is the operator's, and the plan records the two review findings this pass closed plus the P2 / standards-managed machine-path overlap it deliberately left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…ch holds Claude Code exposes no way to see the permission rules actually in effect — no `claude permissions` subcommand, no machine-readable export — so the honest answer to "where is this rule coming from" has been to read five files in five places and hope you knew all five. This reader finds them: managed policy, user-global, project, local, and any pre-v2.1.211 copy left in the session's start directory, each scope's allow/ask/deny rules attributed to the surface they came from. The status vocabulary is the substance. `absent` means looked and found nothing; `skipped` means could not look. Every scope and every managed surface emits a record on every OS, including the ones that do not apply here, so a surface that was never attempted can never be read as one that is genuinely empty. Server-managed settings arrive remotely and have no local path at all, which the reader discloses rather than letting silence imply completeness. Managed policy is four surfaces per OS, not one file. The JSON file and its drop-in directory are the portable core, read everywhere and merged in the documented order; the Windows policy registry keys and the macOS managed-preferences domain are optional platform integrations that announce a skip and leave the core result intact. Exercising it under a stub PATH caught two defects that each produced a confident wrong answer. MSYS rewrites any argument containing backslashes as a POSIX path, so every registry query died with "Invalid syntax" and scored as no policy deployed. And with plugin-root resolution broken, the shared library failed to source while the run still exited 0, reporting every managed surface absent; that path now exits 2, in this reader and in the audit skill's structure check, which had the same fall-through. The registry surface is verified against the real registry via a scratch key outside the policy tree, created and deleted within the check, so no policy was ever deployed to the machine. The macOS domain and the Linux paths are an honest manual-verification gap, stated in the skill. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…that exists A stray admin-level key could make user-level policy look like the managed policy. The search stopped at the first key whose Settings value parsed, so an HKLM key with a missing or malformed value fell through to HKCU — which the settings doc calls lowest policy priority, used only when no admin-level source exists. Reporting it while an admin-level key is in force is the precise failure that rule exists to prevent. `reg query <key> /v Settings` cannot distinguish a missing key from a present key with no such value: both return exit 1 and the same message. A bare `reg query <key>` does distinguish them, so key existence is now the probe that ends the search, and an existing key that yields nothing readable is reported unreadable with a note rather than as permission to consult the next key. The regression test uses HKCU\SOFTWARE, which exists on every Windows install and carries no Settings value, so it covers the case read-only and writes nothing to the registry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…ule's mechanic named audit-permission-state could say what every scope holds but not what any of it means together. This adds the merge: one effective allow/ask/deny set, each rule naming every scope that contributes it and the documented mechanic that put it there. The two governing doc sections were re-fetched before any code was written, and they overturned the planned model. Permission rules "merge across scopes rather than override", so a rule written at two scopes has no winner and electing one would have been a precedence claim no page supports. What a rule can lose is its kind: deny is evaluated before ask and ask before allow, from any scope, in both directions -- a user-level deny blocks a project-level allow just as a project-level deny blocks a user-level allow. That is the only election made here, and a scope-ranked implementation would get the low-scope-deny case exactly backwards. The beaten entry ships as an `inert` record naming what beat it, which is the answer to "why is my allow rule ignored". Every run states the two bounds on the claim. The command-line scope (--settings, --allowedTools, --disallowedTools) outranks the files and has no file to read. Rules are compared by exact text, so a narrow allow blocked only by a broader deny pattern is still reported effective -- the docs state the broad deny wins, so the error direction is known: it over-reports allow and never over-reports blocking. An empty merge cannot mean an empty machine. A pipeline that swallowed the reader's exit 2 would report "nothing in effect" on a machine full of rules, so zero scope records on input is exit 2 before any merge line is emitted. The skill also stops overclaiming against `/permissions`, which the permissions page documents as listing every rule and the file it came from. The Purpose now points readers there for "where is this rule written" and claims only the difference: outcome resolution, telling a scope that was empty from one that could not be read, and an output another tool can consume. reference/criteria.md finally has content that is not a restatement -- the precedence_basis vocabulary, each token mapped to the sentence it follows from. 40 new checks, the reader's 42 still pass, shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…eny removed Exact-text comparison treated `deny: ["Bash"]` and `allow: ["Bash(git status)"]` as two unrelated rules and reported both as effective. The permissions page says the bare name "removes the tool from Claude's context entirely, so Claude never sees it" -- so the report claimed prompt-free access to a tool the model cannot see, in managed policy, which is exactly where whole-tool denies get written. Not a case for the exact-text caveat. That caveat covers pattern subsumption, which needs a matcher; the tool token is the text before the first `(` and "contains no `(`" is the whole test, so this was decidable and therefore a defect. A whole-tool deny now makes every rule naming that tool inert -- other denies included, moot rather than weakened -- and a whole-tool ask outranks every scoped allow for its tool, since it matches every call and ask is evaluated first. `EndConversation` is exempt from removal per its documented carve-out. Both cases print a NOTE naming the tool rather than leaving a reader to infer it from a run of inert records. Found by fresh-context review after the merge had already passed 40 checks and its own author had signed off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
Version conflicts only, plus one comment hunk. main took claude-config to 0.31.0 and claude-memory to 0.8.0 while this branch held 0.30.0 and 0.7.1, so this branch's entries move up to 0.32.0 and 0.8.1 above them -- still one entry per plugin on this branch, not two. check-structure.sh resolves to this branch: main only re-dated the comment above the per-OS managed paths, and this branch had already replaced that block with a source of lib/managed-scope.sh, which carries the 2026-08-10 verification date itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…new scripts Four CI lanes were red on this branch, all on gates that had never run against these files before. Every shipped script carried index mode 100644 despite a shebang. The exec-bit gate names them individually; the two define-only libraries stay 644, correctly, since they have no shebang and are only ever sourced. The spell checker splits hyphenated compounds, so "mis-resolved" and "mis-cited" each read as the non-word "mis". Reworded rather than suppressed -- "resolved to the wrong place" and "cited the wrong section" are also plainer. Fixture records in the merge suite used /home/.claude/settings.json, which the machine-specific-path detector reads as a real Linux user path. They are synthetic strings in heredoc records, never touched on disk, so they move to an unmistakably fake /fx/home. The two places that write a real file under the temp fixture home are unchanged. The portability gate flagged Windows registry key literals as regex escapes. They are neither -- they are key paths compared as strings -- so each site takes the sanctioned per-site `portability-ok:` escape. The four inside multi-line `env` continuations could not carry a comment where they stood, so they are hoisted to named variables that can. The oracle fixture capture is a different case: it reproduces Claude Code's own [DEBUG] narration byte for byte, and that narration prints Windows paths in native form, so rewriting it to POSIX form would make the fixture stop matching what the parser must survive. That file takes a whole-file `portability-scope:` declaration stating exactly that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
Auto mode became the default for new sessions, and on entry it silently drops broad allow rules. Nothing told a consumer which of theirs survive. This classifies every effective allow rule as dropped -- with the documented reason named: blanket, wildcarded interpreter, package-manager run, or Agent -- or as carried over. The classes come from lib/permission-patterns.sh, the vocabulary audit-permission-grants check P1 already scans with. The library now exposes its four alternatives individually as well as the union, because a driver that must NAME the class a rule fell into has to test them one at a time; the union is composed from them rather than written twice. autoMode.classifyAllShell inverts the answer wholesale -- with it on, narrow rules do NOT carry over -- so the reader inventories it as a new `conf` record, emitted as JSON so a boolean true and the string "true" stay distinguishable. It is resolved only from the scopes the classifier actually reads: user, managed, and inline --settings/SDK JSON. A project- or local-scope copy is reported inert rather than obeyed. Both of the phase's open questions were measured rather than reasoned about, and one answer changed what ships. A -p oracle session does NOT modify any settings file, so criterion 9 holds -- but it DOES rewrite ~/.claude.json and add project, session-env, security, subagent and backup state under the config root. The cost notice said only "does not modify any settings file", which was true and incomplete in the way that matters for a flag whose whole job is being honest about what it costs. It now enumerates the measured writes. The second question is moot: a plain -p run with no mode flag emitted 216 drop lines, so the differential capture held in reserve was never needed. A third fact fell out of the probe -- an isolated run with CLAUDE_CONFIG_DIR pointed at scratch cannot authenticate, because credentials live in the real config root, so a probe of this shape necessarily touches it. Copying credentials into a scratch root to fake isolation was rejected rather than attempted. One defect caught by its own regression case: the oracle's rule-text parse claimed to cut at the LAST " from " but the sed was leftmost-matching, so `Bash(python3 import from x *)` parsed as `Bash(python3 import`. 49 checks, shellcheck clean, every sibling suite still green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
… read The permission plane accepts things it silently ignores, and the operator who wrote them has no way to know. Eight checks over every scope at once, before a session starts. The highest-consequence one is disableAutoMode typed as a boolean. The documented value is the STRING "disable"; a boolean is valid JSON, is accepted, and does nothing -- so an operator who set it believes auto mode is locked out when it is not. It is read at BOTH documented key paths, top-level and under `permissions`, in every scope, because it is not managed-only and a scan of one path would miss half the surface. The three dead-config gates stay three findings. They cover different scope sets and carry different version histories, so a merged count would let someone fix one, watch the number drop, and believe they had fixed all three. Criterion 6's fourth item shipped as two checks rather than one. A parameter-form rule on a primary content field (`Bash(command:rm *)`) is ignored with a startup warning; a path rule on Write/NotebookEdit/Glob/MultiEdit is accepted and never consulted. Different mechanics with different observables -- one merged check would have been wrong about at least one of them. Both were re-derived against the pages rather than shipped as the plan wrote them. The reader owns the config reads, not the lint: permission-state.sh's conf record grew from one key to six, emitted as JSON so a boolean true and the string "true" stay distinguishable. For the disableAutoMode check that distinction IS the finding. A lint that re-opened settings files itself would have duplicated scope resolution the reader already does correctly. False positives were designed against explicitly and each has a regression case asserting it does NOT fire: a bare tool-name rule (legitimate at the tool level, and the most common rule shape there is), `:*` at pattern end, a parameter rule on a non-content field, and a POSIX-form Windows path. SKILL.md was over its soft line target, so the scope tables and the per-check mechanic table moved to reference/criteria.md. 43 checks, shellcheck clean, siblings still green, check-skill PASS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…ading it
A different surface from everything else in this skill: four natural-language
sections an LLM classifier reads, not permission rules the harness matches. So
only the mechanical checks live here. `claude auto-mode critique` owns the
semantic judgment and is surfaced rather than reimplemented.
The headline finding is a section that omits "$defaults". Customizing a section
REPLACES the built-in list rather than adding to it, so the finding names how
many built-in entries are gone and the first one by label -- "you dropped 65
soft_deny rules" is actionable where "missing $defaults" is not. It fires only
when entries are actually missing, never merely when the token is absent: the
CLI expands "$defaults" in its own output, so an expanded section and an omitted
one differ only in what is gone, and the first draft fired on three sections the
fixture had never customized.
Contradiction and shadowing compare label SUBJECTS, never bodies. The bodies are
prose written for an LLM; no mechanical comparison of prose is defensible, and
attempting one would be exactly the judgment reserved for critique.
Reading this block at all required the whole measured defensive contract, every
item of it a way to have reported a confident wrong answer:
- `auto-mode config` emits raw control characters inside JSON string values.
jq rejects it outright, exit status still 0. The offending byte is a raw
line feed inside a string, so no line-oriented POSIX filter can tell it from
the pretty-printer's structural newlines -- which is why this lane needs a
real parser, and why pure POSIX was tested and rejected rather than assumed
inadequate.
- `defaults --label` OMITS a non-matching key rather than returning an empty
list, so a missing key is tolerated as "no entries", not raised.
- Labels carry a bracketed annotation before the colon
(`Git Destructive [named+specifics ...]: ...`), so the split is at the first
`[`, verified against the real captured payload rather than a description.
- Exit status is never consulted. A run that produced nothing is reported
status=unavailable with an explicit "NOT a clean bill", because "your block
is clean" and "the block was never read" are different claims and only one
of them is true.
The raw-control-character fixture is ASSERTED invalid, not assumed: the suite
runs `jq -e .` on it and fails if jq parses cleanly. That caught a real defect
during construction -- the fixture had been regenerated through json.dumps,
which escaped the line feed, so the headline regression case was testing
nothing. It is now authored as explicit bytes and pinned `-text` in
.gitattributes, because line-ending normalization would rewrite that byte and
silently make it valid JSON again.
The lane is optional by declaration: absent python3 or claude it prints a
visible skip notice and exits 0, and every other stage of the skill still runs.
44 checks, shellcheck clean, portability gate clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…ich are not An administrator deploys managed policy believing it is policy. Some of it is, some of it is not, and nothing in the product surfaces which. A managed `permissions.deny` is the strongest thing an administrator can write: no level, command line included, can override it, and a tool denied at any level cannot be allowed at another. A managed `autoMode` section is not that at all -- it is additive. A developer cannot remove entries it provides, but a developer-added allow can override an organization soft_deny, because permissions, hooks, MCP, sandbox-filesystem and sandbox-network each got an exclusivity lock and auto mode did not. The finding quotes that wording and names the page's own remedy rather than inventing one. That claim was supposed to ship caveated, on the grounds that it came from a research slice nobody had independently verified. It no longer needs the caveat: the governing page states it affirmatively, and Phase 0 recorded the wording. The caveat is discharged rather than narrowed, and nothing here rests on the slice. A second loosenable class fell out of the merge mechanics and was not in the plan. "Managed settings are highest" and "deny before ask before allow, from any scope" are both true, so a lower-scope deny beats a managed allow WITHOUT overriding it. An administrator reading only the precedence table would not expect that, which makes it precisely this report's remit. The report prescribes nothing and ships no security floor of its own. That property is asserted positively -- the suite walks every rule the report printed and fails if any string is absent from the input -- rather than by checking that some recommendation marker never appears, which would pass unconditionally. Completeness bounds every claim. Server-managed settings have no local path, and a managed surface that could not be read gets a note saying it is not evidence that no policy is deployed there. "No local policy" is a stated status, not an empty report: an administrator reading silence as "nothing to worry about" is the exact failure this exists to prevent. 28 checks, shellcheck clean, portability gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…part The autoMode block is a natural-language prompt for a classifier, not a rule list the harness matches, so it fails in ways a config file does not. This skill interviews for one entry at a time and prints a paste-ready block. The entry shape is not invented here. `claude auto-mode critique`, run against a real 66 KB hand-authored block, found the classifier is "an LLM doing a single pass under a 'default is ALLOW' instruction", so "buried conditions in paragraph position 40 will be missed at a materially higher rate than conditions in a bullet list", and recommended exactly this: a label, bulleted COVERED / NOT COVERED, one line of rationale, provenance stripped. The finding is acted on at authoring time rather than reported after the fact. The interview's fourth question is the load-bearing one. The same critique named uncheckable conditions the biggest weakness -- entries state preconditions the classifier cannot evaluate from command text, so it either allows blindly (the condition is decorative) or blocks (the grant is inoperable), with no stated disposition. The skill pushes back and asks for the observable form: "only when the deploy is approved" is not checkable; "only when the command names the staging endpoint" is. Every emitted section opens with "$defaults", by construction. Customizing a section REPLACES the built-in rule list rather than adding to it, so a drafter that omitted the token would bake in the exact defect the audit sibling reports. jq composes the JSON rather than shell string-building, because hand-rolled escaping is how a draft would end up shipping the raw-control-character defect this plugin reports elsewhere. A case feeds quotes, backslashes and a tab and asserts the output is strict-valid. It writes nothing, in any scope, under any flag. That contract is asserted by RUNNING the script and diffing the filesystem, including a fixture HOME whose settings file is checked byte-identical afterwards. Grepping the source for redirect syntax was tried and rejected: the script is mostly an embedded jq program where `>` is a comparison operator, so a text scan cannot tell a redirect from a comparison. Measuring the effect is sound where scanning is not. The history input the earlier design called for stays dropped: it named no actual location, and an unnamed read surface in a shipped skill is unreviewable. SKILL.md says so, so it is not silently reintroduced. 33 checks, shellcheck clean, check-skill PASS, catalog and cheat sheet regenerated for the ninth skill. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…rity review Criterion 1 reviewed hook commands as the code-execution surface and said nothing about a skill's frontmatter `allowed-tools`, which is also one. Measured on 2.1.225: a marketplace-installed skill's allowed-tools entry takes effect at USER scope in a NEVER-TRUSTED workspace, under -p where no trust dialog can appear. The covered command ran without a prompt, the uncovered one blocked with "This command requires approval", and a no-grant baseline confirmed that shape blocks. So the install-time plugin trust prompt is the ONLY gate in front of such a grant -- there is no second, per-workspace one. The criterion now says so, and denies by default anything broader than the specific command a skill's own scripts invoke. A wildcard interpreter grant is a deny outright: it is arbitrary code execution in a workspace the consumer never trusted. audit-permission-grants check P1 already detects those shapes, so the criterion names it as its mechanical half rather than leaving the check to memory. The measurement's bounds are stated with it: a local-directory marketplace, user scope only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
… fast enough to run Criterion 9 says these skills write nothing, in any scope, under any flag. That is the reason they are safe to point at a consumer's real configuration, so it is now proven by RUNNING all ten actions of both skills -- oracle explicitly ON against a stub `claude` -- and diffing a fixture tree and a fixture HOME either side. Reading the code would not have been evidence. Building that harness found a performance defect I introduced in Phase 4. The `conf` extension ran one `jq` invocation per key per settings surface: six spawns per file, thirty across five scopes. Process spawning dominates on Windows, so the reader took 65 seconds per run -- slow enough that every caller timed out and the harness looked like it was hanging rather than working. Collapsed to one invocation emitting all six keys: 65s to 7s, identical output. Worth fixing on its own terms; a reader nobody will wait for is a reader nobody will run. The harness itself needed four corrections, and the last is the one worth remembering. A helper returned its produced file path on STDOUT, which interleaved with the PASS lines the same function printed -- four stages silently never ran and the harness reported "All 10 checks passed". A green result over work it had not done. The path now comes back in a named variable, every stage asserts its output is non-empty, and a final assertion checks that all ten actions were attempted. A harness that can miscount its own cases is worse than no harness: it converts absence of verification into a passing claim. The three earlier corrections were a stub-only PATH that starved the scripts of tools they need (its `env` wrapper re-entered itself through the PATH it was setting up, deadlocking before the first assertion), pipelines composed inside `bash -c` strings that stalled on quoting, and a `while read` snapshot loop whose subshell deadlocked against its own feeding pipeline on Git Bash. Each failed silently -- no error, just no further output. Criterion 10 was NOT met and now is. The permissions.ask-under-auto-mode discrepancy (#83766, #42797) was recorded in the plan but appeared nowhere the skills ship, so a consumer acting on an `ask` finding had no way to know the reported behavior contradicts the documented one. It is now a named caveat with its retirement condition, and SKILL.md tells the reader to carry it. ADR 0009 records the two decisions that outlive this branch: report-never-write, and compute the merge locally bounded by decidability. Both had a defensible opposite answer, and enforcement was available rather than absent -- the posture is a choice, not a limitation. Also: audit-pass gains the permission lane, with its oracle path marked never-dispatched and its optional-lane skip required to reach the report as "unchecked with reason" rather than collapsed into "clean". OFFICIAL-DOCS dates advance for the four pages re-fetched during this work. Criterion 12 measured: 6737/8000 chars across nine listing-eligible skills. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
docs/topics/<slug>/ is Contract tier: committed on the task branch, pruned
before merge. The durable outcomes graduated first, so nothing is lost with it:
- The two decisions that outlive the branch -- report-never-write, and compute
the merge locally bounded by decidability -- are ADR 0009.
- The citation base moved into the skills' own reference/criteria.md, which is
plugin-owned and ships to consumers rather than living in a branch-local
planning file. Every load-bearing quotation is there with its mechanic:
scope merge semantics, evaluation order in both directions, the v2.1.142 and
useAutoModeDuringPlan gates, primary-content-field and uncovered-path rules,
the systemd drop-in merge order, managed autoMode additivity, and the open
permissions.ask discrepancy with its retirement condition.
- The plugin-acceptance consequence of the allowed-tools measurement went to
MIGRATION-PLAYBOOK.md, where the review that needs it lives.
- The researcher preload defect is issue #2338.
Verified before deleting: no shipped surface cites docs/topics/ -- a runtime
reference into a path that will not exist after merge would have been the one
unrecoverable mistake here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…atterns The portability gate flagged four `\s` / `\S` runs. Three were in the drafter's embedded jq program, where POSIX bracket expressions say the same thing and are what the gate asks for -- `[[:space:]]` and `[^[:space:]]`. Behavior unchanged; 33 checks still pass. The fourth is different in kind and takes the declared-scope escape: the suite deliberately feeds a literal backslash through the drafter to prove jq owns the JSON escaping rather than shell string-building. That backslash is adversarial INPUT inside a heredoc, not shell code, so rewriting it would delete the case rather than fix anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…fication An independent fresh-context verifier, given the acceptance criteria with the rationale withheld, found six defects in Phase 3. Five are fixed here; all were reproduced before being touched. A bare `Bash` allow was reported as SURVIVING auto mode. It is the whole-tool form -- strictly broader than `Bash(*)`, which the same run drops as blanket. The asymmetry was the evidence: `Agent`'s bare form was handled and the two shell tools' were not. Wrong in the direction that costs an operator: it says their widest shell grant is safe. `--merge-only` silently inverted criterion 4. The merge dropped `conf` records under that flag, so the entry diff lost `autoMode.classifyAllShell` and reported a narrow shell rule as kept when it is actually suspended. Two documented flags composing into a wrong answer with no warning. conf records now always pass through -- they are input a downstream stage needs to be correct, not presentation. The oracle's rule/path split was wrong in both directions, and the second was invisible to the first fix. The narration delimits neither field, and both sides may legitimately contain " from ": a rule (`Bash(python3 import from x *)`) and a directory (`notes from work`). Cutting at the first separator truncated the rule; cutting at the last truncated it when the PATH carried the word, yielding two false verdicts plus a phantom rule. No fixed choice is right for both, so the split is now resolved by which candidate leaves a well-formed tool token on the left. A line that resolves to zero or several candidates is ANNOUNCED as unresolvable rather than silently split -- a wrong rule name in a divergence verdict is worse than an admitted gap. `classifyAllShell: false` was described as a type error. It is the documented default. The code was right and the operator-facing sentence was wrong, which is its own kind of defect. Criterion 3 was only half met. The classifier reads autoMode from three scopes and this reader can open two; inline --settings and SDK JSON have no file, and classifyAllShell set there inverts every shell verdict. Both SKILL.md and criteria.md named that third scope while nothing disclosed the gap. Every run now states it. Also corrected: a comment asserted as MEASURED that drop lines "appear exactly when the session's permission mode is auto". That was never measured -- the recorded probe emitted 216 drop lines with no mode flag passed. The flag is valid and harmless, but the claim was mine and unproven, and the verifier was right to flag the artifact contradicting its own evidence file. D6 (an empty rule in the narration parsing to "" and being filtered) is left: the new tool-token resolution rejects it as unresolvable, so it now surfaces in the unparsed count rather than vanishing. 63 checks in the entry diff (was 49), 51 in the merge (was 49), shellcheck and portability clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…ng it in two The verifier flagged this as pipeline-wide rather than oracle-only, and asked whether it was reachable through a real settings file. It is -- measured: a settings file containing "Bash(echo hi\nthere *)" is ordinary valid JSON, and the reader emitted rule user settings allow Bash(echo hi rule user settings allow there *) Two records, neither of which is a rule in any file, both flowing downstream into the merge and the entry diff as if they were real grants. That is the same failure class as the phantom rule in the oracle parse, on the read path instead of the corroboration path -- and it outranks where I had it, because settings files are the input this tool exists to read. These records are line-oriented, so such a rule cannot be represented in one. Rather than pick a lossy encoding, the reader now detects it and says so: no rule record is emitted, and a NOTE names the scope, the kind, and the rule text rendered on a single line. Admitting one rule cannot be represented beats inventing two that do not exist. Applied to the registry read path as well as the file path. Managed policy is where a phantom rule would do the most damage, so it does not keep the older splitting read. 47 checks in the reader suite (was 42), shellcheck and portability clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
An in-string CARRIAGE RETURN was silently deleted, not announced. The newline fix landed last round covered LF only; `tr -d '\r'` then quietly turned "Bash(a\rb *)" into the rule string "Bash(ab *)" -- present in no settings file and flowing downstream as a real grant, verbatim the failure the newline NOTE exists to prevent. The strip is load-bearing (jq emits CRLF on Windows) so it stays; the in-string case is now caught before reaching it. C6-winPath was DEAD in the real pipeline. It tested for a doubled backslash -- the JSON SOURCE spelling -- but `jq -r` decodes the escape, so the reader emits `Read(C:\Users\alice\**)` and the check never fired. It passed its own suite only because every fixture there was a hand-written record that bypassed the reader. Now tested on the single backslash the reader actually emits, with an end-to-end case that drives the reader rather than standing in for it. The autoMode block lane exited 0 with a traceback and NO summary line on a payload that parsed but had the wrong shape (a JSON array, a string, a number). A caller grepping status= got nothing and a success exit -- in the one lane whose whole premise is that the CLI's output shape cannot be trusted. Shape is checked before use; status=unexpected-shape, exactly one summary line, always. --critique spawned a real session with no cost notice while its entry-diff sibling prices its spawn. An unpriced spawn is the surprise the opt-in flag exists to prevent. managed-conformance reported a CORRUPT managed policy as absence of policy: invalid-json raised no note and yielded status=no-local-policy, which an administrator reads as "nothing deployed". It now groups with skipped and unreadable exactly as the merge already did, and "no local policy" is claimed only when every surface was looked at and found genuinely empty -- otherwise status=incomplete. The drafter emitted section names Claude Code does not read: `section bogus` became a key, and an empty section name became "". An authoring skill manufacturing the "written where nothing reads it" defect its own Phase 4 sibling exists to catch is the worst possible source for it. Unknown sections are skipped with a warning on stderr; stdout stays valid JSON. Suites: reader 50, merge 51, entry diff 63, plane lint 45, block lint 62, conformance 32, drafter 39. shellcheck and portability clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
… round two
Parameter matching is documented for DENY AND ASK RULES ONLY. Re-fetching the
section to settle the ambiguity the verifier flagged turned up a defect nobody
had raised: "An allow rule for one parameter value wouldn't establish that the
call is safe overall, so allow rules continue to use each tool's own specifier
syntax." So `allow: ["Agent(model:opus)"]` is not a narrowed grant -- it is a
rule whose form the allow path does not use, and the operator believes they
scoped a permission and did not. New check C6-allowParam.
It fires only where the shape is unambiguously the parameter form: a top-level
parameter the page names, on a tool whose own syntax is a path or a command.
`WebFetch(domain:host)` is the documented WebFetch form and `Bash(npm:*)` is a
command prefix -- neither is distinguishable from a parameter by shape, so
neither fires. That was the verifier's actual complaint about the colonStar
message, and this is the honest resolution: narrow the check to what the page
supports rather than restate an unhedged claim in different words.
The content-field wording stays as it is. The page is unconditional there --
"Claude Code ignores it and emits a startup warning" -- so the certainty is the
page's, not mine.
Removed a stale comment block that described a superseded oracle design and
asserted a limitation the shipped code does not have ("a path containing a
space still defeats this" -- it does not; the tool-token resolution handles it).
A comment claiming a constraint that was removed outlives everyone who knew
better.
Moved the oracle cost measurement into reference/criteria.md. The verifier's
sharpest point: the artifact stopped contradicting its evidence because the
evidence was PRUNED with the contract slice, leaving a comment citing a capture
the repository no longer contains. The measurement was real and is now
somewhere a reader can actually check it, in a file that ships to consumers.
Plane lint 50 checks (was 45, now nine checks not eight), entry diff 63,
shellcheck and portability clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
PR #2260 was merged as a squash while this branch kept moving, so main now carries the walking skeleton and the merge stage while this branch carries those plus Phases 3-8, the contract-slice prune, and thirteen defect fixes from three independent verification rounds. Every code conflict resolves to this branch: same lineage, strictly further along. The squash landed the Phase 1-2 versions of permission-state.sh and permission-merge.sh; this branch has both plus the whole-tool deny handling, the conf-record passthrough, the newline and carriage-return reporting, and the 9x reader speedup. Versions move above what main reached rather than re-using the numbers this branch had claimed: claude-config 0.36.0 (main was at 0.35.2), claude-memory 0.9.1 (main was at 0.9.0). Both CHANGELOGs interleave so this branch's entry sits above main's history in descending order. Catalog and cheat sheet regenerated. validate-plugins and all four changelog-parity modes pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
Resolving eighteen conflicts with --ours was right for the files this branch advanced and wrong for three it did not, and the drift gate caught the first of them rather than my own review. Checking the rest of the --ours set against main found two more. permission-rule-check.sh is the one that mattered. Main had REMOVED a $PWD fallback for the scan root, because outside a git repository $PWD is typically the user profile and both scans walk the root with an unbounded `find` -- so an unresolved root became a silent sweep of the operator's home that still exited 0, indistinguishable from a clean bill. My side was simply the older file and added nothing; taking --ours reintroduced the fallback. Restored from main, with its SKILL.md and test suite. cross-plugin-source-registry.txt lost main's lib/state-key.sh entry the same way, which is what the drift gate failed on. managed-scope.test.sh differed only in the wording of two portability-ok comments; took main's to keep the diff small. permission-patterns.sh stays on this branch's version: it splits the P1 union into its four named alternatives so the entry diff can report WHICH class dropped a rule, and composes the union from them rather than restating it. Suites after restoring: permission-rule-check 76, managed-scope 16, permission-patterns 12, permission-state 50. Drift, portability and validate-plugins all clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
|
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 2m 0s —— View job Security Review
No security issues found. This PR contains no GitHub Actions workflow changes and no changes to Notes from the review (informational, not findings):
|
![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: f8abbafa2a
ℹ️ 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".
…s settle
C6-winPath went from dead to flooding. Round two fixed it to test a bare
backslash, and the verifier's follow-up caught what that costs: backslashes are
ordinary in shell rules -- a regex `sed 's/\./_/g'`, an escape `Bash(echo \n *)`,
a quoted string -- so every one became a severity-`error` finding carrying
Windows-path advice, and the one true finding drowned in them. Worse than the
dead check it replaced, because dead was at least quiet.
Now tested on SHAPE: a drive-letter or UNC prefix, the two forms that actually
are a Windows path. A backslash anywhere else is somebody escaping something.
UNC gets its own message, since "use //c/**" is wrong advice for it.
C6-colonStar fired on `WebFetch(domain:*.example.com)`, calling a documented
working rule broken. The page: "WebFetch rules use a `domain:` prefix and match
against the hostname… supports `*` wildcards." The colonStar mechanic it states
is about COMMAND PREFIX patterns specifically -- "In a pattern like
`Bash(git:* push)`, the colon is treated as a literal character". Documented
parameter forms are now exempt.
The same page settles the other half in the opposite direction, so nothing
changes there: `Bash(command:rm *)` is the page's OWN example of a rule Claude
Code ignores, so the unconditional wording in C6-contentField is the page's
certainty, not mine.
A section present but not a list (`{"allow": "not-an-array"}`) counted as a
present section and reported status=read -- a clean bill on something no check
could examine, the same class as the wrong-shape payload fixed last round, one
level down. Now status=partial with a note naming the section.
Header comments corrected: they still described the doubled-backslash test and
the unqualified colonStar rule.
Plane lint 58 checks (was 50), block lint 62, shellcheck and portability clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
|
Claude finished @kyle-sexton's task in 3m 39s —— View job Security Review
No injection, secret-handling, or access-control vulnerabilities found. All new/modified scripts ( This PR's changed files are a security-reporting tool, so I also verified the logic-flaw findings already surfaced by Codex against the current head ( Confirmed, still present at
None of the above are exploitable in the traditional sense (no attacker-controlled input reaches these paths — they operate on the operator's own local settings files), but all three cause this audit tool to under-report risk or over-state assurance to the administrator reading it, which is the specific failure mode this PR's design commentary repeatedly says it's trying to eliminate. Worth fixing before merge given the PR's own stated bar (three rounds of independent verification, "no silent false-clean reports"). Everything else reviewed — |
The changelog-parity gate caught this and it is a real defect, not a gate quirk: resolving the merge relabelled the `## [0.8.1]` heading to `## [0.9.1]` rather than adding a new one, so a released entry's heading vanished. The gate's own message names that exact failure mode -- "RELABELLING that heading to the new version; git leaves no conflict marker behind for either". The right fix turned out to be smaller than restoring the heading. PR #2260 already shipped 0.8.1 with this body, and diffing the two entries shows they are identical apart from the heading. This branch changes nothing in claude-memory: no code, no docs, only a version number and a relabelled heading it had no business touching. So both files revert to main exactly, and the plugin carries no bump at all. claude-config is unaffected -- 0.36.0 sits above main's history with every heading main shipped still present. All four parity modes and validate-plugins pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
|
Claude finished @kyle-sexton's task in 3m 10s —— View job Security Review
No injection, secret-handling, or GitHub Actions issues found. No workflow files changed. jq/awk calls consistently pass untrusted values via This PR's diff is a security-reporting tool, so the relevant trust boundary is: can a bug in it cause the tool to overstate assurance to the administrator reading its output (report something as safe/enforced/read when it isn't)? I verified against the current head ( 1. IMPORTANT (confidence: high) — a present-but-uninventoried managed surface is reported as a clean, fully-read policy. 2. IMPORTANT (confidence: high) — a schema-invalid settings file is silently treated as an empty, clean scope. 3. SUGGESTION (confidence: medium) — auto-mode drop classification isn't anchored to the rule's own tool. 4. SUGGESTION (confidence: medium) — Everything else in scope — None of the four findings above are remotely exploitable — no attacker-controlled input reaches these paths, they operate on the operator's own local settings. All four are about this audit tool giving false assurance to the administrator reading it, which is the specific failure mode this PR's design commentary and commit history repeatedly say it exists to eliminate. #1 and #2 are the ones worth fixing before merge; #3 and #4 are lower-stakes refinements. |
…osed on an unknown section Three fixes, all from round-four verification. The first two correct my own reasoning, not just my code. C6-colonStar exempted `WebFetch/domain` by name, and the verifier immediately found `Agent(model:*-haiku)` -- a documented parameter form whose wildcard lands mid-value -- still flagged. The allowlist was the wrong SHAPE, not just too short: the page says parameter matching works "on any tool" for "any scalar parameter the tool accepts", so enumerating names could only ever chase it. Now by grammar: in a deny or ask rule an `identifier:value` body IS the parameter form. In an allow rule it cannot be -- allow rules use the syntax each tool defines -- so a command-prefix reading is the only one available there and the check still applies. The CR strip narrows rather than disappears, and my reason for not fixing it was wrong. I had argued that catching an in-string CR meant deleting the strip that makes CRLF-on-Windows work. It does not: `tr -d '\r'` removes every CR, while `sed 's/\r$//'` removes only a line-ending one. A trailing CR is a line ending; a CR anywhere else is data. So both work at once, and the sharper argument is not the rule text at all -- the old path REPAIRED a payload before classifying it, so a file could be reported `status=present` on the strength of a repair the reader made itself. A status must never be kinder than the file it describes. The drafter now FAILS CLOSED on an unknown section: exit 2, nothing on stdout. Skipping it with a stderr warning was correct until a caller captured both streams, and agent tooling merges them by default -- so the merged stream was not parseable JSON. Failing loudly leaves no partial output to corrupt and matches the posture every script here already takes. A `_warnings` key inside the JSON was considered and rejected: it would land in the pasted autoMode block as a section the classifier does not read, manufacturing the exact defect this check exists to prevent. Plane lint 58, drafter 42 (was 39), reader 50, shellcheck and portability clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
|
Claude finished @kyle-sexton's task in 3m 52s —— View job Security Review
No injection, secret-handling, or GitHub Actions issues in this diff (still none — no workflow files changed; jq/awk calls throughout use New since the last review — introduced by 5. IMPORTANT (confidence: high) — the new param_form = (kind != "allow") && (prefix_of(body) ~ /^[A-Za-z_][A-Za-z0-9_]*$/)
if (cs > 0 && cs + 1 < length(body) && !param_form && !((tool SUBSEP prefix_of(body)) in documented_param))
Still present — confirmed unchanged at 1. IMPORTANT — a present-but-uninventoried managed plist is still reported as a clean 2. IMPORTANT — a schema-invalid (non-array) 3. SUGGESTION — auto-mode drop classification still isn't anchored to the rule's own parsed tool. 4. SUGGESTION — Everything else in this diff — Summary: 1 new IMPORTANT finding introduced in the latest commit ( |
|
Claude finished @kyle-sexton's task in 2m 47s —— View job Security Review
No injection, secret-handling, or GitHub Actions issues found. No workflow files changed in this PR. jq/awk calls continue to pass untrusted values via This diff is a security-reporting tool, so I re-checked whether the prior rounds' "overstates assurance to the administrator" findings are still live at Still present at
None of the four are exploitable in the traditional sense — no attacker-controlled input reaches these paths, they operate on the operator's own local settings/managed policy. All four are about this audit tool giving false assurance to the administrator reading it, the specific failure mode this PR's design commentary and three-rounds-of-verification history say it exists to eliminate. #1 and #2 are worth fixing before merge; #3 and #4 are lower-stakes refinements. These align with Codex's four P1/P2 inline comments on this PR ( Everything else in scope — |
…laiming managed enforcement
A second verifier found a FOURTH file my merge resolution reverted, and it is
the worst of the four. main's classify_json_file has a second validation stage
after `jq empty`: it checks `.permissions` is null or an object whose values are
null or arrays. Taking --ours deleted it, so `["Bash(x)"]` and
`{"permissions":{"allow":"Bash(*)"}}` -- valid JSON, not settings files --
reported `status=present`. A malformed scope read as a healthy empty one, which
is the exact thing the status vocabulary exists to prevent.
The suite could not catch it. Its malformed fixture is SYNTACTICALLY broken,
which `jq empty` rejects on its own, so the assertion passed identically with
the stage gone -- a green suite over a reverted fix. The new fixture is
structurally valid and malformed, and it fails without the stage.
That is now two independent verifiers disagreeing about the same merge: one
proved the resolution clean by `git apply --reverse --check` over the conflict
set, the other found this by asking what a consumer hits on day one. The second
method won because the first tests whether main's PATCH still applies, and a
patch whose surrounding lines we rewrote no longer applies either way.
managed-conformance over-claimed enforcement. `lower[]` was keyed on exact rule
text, so a lower-scope WHOLE-TOOL rule was invisible: managed
`allow Bash(npm test)` under user `deny Bash` reported `enforced allow` while
the merge, on the same records, reported `inert allow removed_by=deny@Bash`.
Two stages, opposite answers, and the administrator-facing one was wrong. It now
tests the tool token as well, using the same tool_of() definition the merge uses
so the two cannot drift apart again.
Two write claims were false as written. "Writes nothing, in any scope, under any
flag" is contradicted by the skill's own --oracle cost notice; the contract that
actually holds is that no SETTINGS FILE is written, including under --oracle.
Both sites now say that, and say what the spawn does leave behind.
The registry test seam used `:-`, which treats set-but-empty as unset -- so
every test passing "" was probing the operator's REAL registry rather than the
empty surface it asked for. Now `-`.
Reader 55 checks (was 50), conformance 37 (was 32), plane lint 58, entry diff
63, merge 51, block lint 62, drafter 42, no-writes 12. shellcheck, portability
clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…kind My grammar exemption opened a fail-open hole, and the verifier found it by asking the question I asked it to ask. `git` is a valid identifier, so `deny Bash(git:* push)` parsed as the parameter form and went silent -- the dead-rule example the permissions page itself gives, silenced in the kind where silence costs most. The direction is what makes this worse than the false positive it replaced. A dead ALLOW fails closed: the operator is denied something they thought they had, and finds out immediately. A dead DENY fails OPEN: they believe they blocked `git push`, they did not, and nothing tells them. Six command-prefix shapes were silent as denies. The discriminator is the SPACE, and it is grammar rather than a name list, so it does not reintroduce the allowlist the last fix removed. "Each rule names one parameter" and its value is a single scalar, so a parameter value never carries space-separated trailing words -- while a dead command prefix is precisely a prefix FOLLOWED BY more words. Verified over the verifier's eleven-rule split: all six space-carrying prefixes fire in deny, ask and allow; all five parameter forms stay exempt in every kind. The suite gap that let this through is closed: no case covered a mid-pattern `:*` in a DENY rule, so 58/58 passed while the docs' own example went unreported. Plane lint 62 checks, shellcheck and portability clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
…patterns main's PR #2382 tightened the P1 runner alternative (a `([\"' :])` before the `\*`, so `Bash(npmx *)` no longer matches the `npm` runner). This branch had restructured the same four patterns into named sub-EREs so the entry diff can report WHICH class dropped a rule. Composed rather than resolved away. Taking --ours here would have reverted a precision fix and been the fifth such clobber on this branch, so the resolution is checked the only way that proves it: the composed $CCPERM_P1_ERE is byte-identical to main's, which makes the restructure provably behaviour-preserving rather than merely test-passing. claude-config goes to 0.36.0, above main's 0.35.3, with every CHANGELOG heading main shipped still present -- verified by set difference, since relabelling one is exactly how this branch lost a released entry before. The manifest was rebuilt from main's text rather than patched, because an earlier Python edit of mine had corrupted its em-dashes to mojibake. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
|
Claude finished @kyle-sexton's task in 5m 0s —— View job Security Review
No injection, secret-handling, or GitHub Actions issues found. No workflow files changed. jq/awk calls throughout continue to pass untrusted values via This diff is a security-reporting tool, so the trust boundary that matters, as in every prior pass on this PR, is whether a logic bug lets it overstate assurance to the administrator reading its output. Since the last review ( Fixed since the last review:
Still open at
None of the four are attacker-reachable — no untrusted input reaches these paths, they operate on the operator's own local settings/managed policy. All four are about this audit/lint tool under-reporting risk or over-stating assurance to the administrator, the specific failure mode this PR's design commentary and its multiple rounds of independent verification say it exists to eliminate. #1 and #2 are the same root cause (a "present" status asserted before/without content validation) surfacing on the two platform-specific managed surfaces this reader touches; #3 and #4 are the lower-stakes refinements carried forward from earlier rounds, largely unchanged. Everything else in the current diff — |
…cidable residual `allow Agent(model:*-haiku)` drew two findings with two mechanics, and the colonStar one explained it as a dead Bash COMMAND PREFIX. Agent does not take command prefixes, so that explanation is simply wrong. The rule is dead either way -- allow plus parameter form is ignored -- and C6-allowParam already says so correctly, so the fix is to suppress the second, not to add a third. Suppression is scoped to the parameter shape rather than to the allow kind: a real dead command prefix in an allow rule (`allow Bash(git:* push)`) still fires colonStar, which the regression asserts. The remaining fail-open window is recorded as a known gap rather than quietly accepted. In a deny or ask rule, a mid-pattern `:*` with NO space after it -- `Bash(git:*push)` -- goes unreported, because it is structurally identical to the parameter form `Agent(model:*-haiku)`. Once the space is gone there is nothing in the rule text to tell them apart; the space was the only signal, so this is undecidable by grammar rather than merely unimplemented. The documented example is the space form, and the pages show no no-space mid-pattern rule anywhere. Saying that in criteria.md is the honest treatment: a reader who hits the case learns the check has a boundary and why, rather than trusting silence. Plane lint 66 checks, shellcheck and portability clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
|
Claude finished @kyle-sexton's task in 1m 40s —— View job Security Review
No new injection, secret-handling, or GitHub Actions issues. No workflow files changed. The only commit since the last-reviewed head ( Commit is_param_shape = (prefix_of(body) ~ /^[A-Za-z_][A-Za-z0-9_]*$/) && (value_of(body) !~ /[ \t]/)
param_form = (kind != "allow") ? is_param_shape \
: (is_param_shape && (tool SUBSEP prefix_of(body)) in param_only)I traced whether this could open a gap where a dead allow rule now gets neither Previously reported findings — unchanged since the last review, still present at
None of the four are attacker-reachable — all operate on the operator's own local settings/managed policy, not untrusted input. They're carried forward unchanged from the prior review round; #1 and #2 are the ones worth fixing before merge. |
|
Claude finished @kyle-sexton's task in 8m 13s —— View job Code Review (correctness / architecture / maintainability lane)
Per Two inline findings posted (both suggestion/nit level — nothing blocking):
One additional finding that predates this PR's diff (the arg-parsing block itself wasn't touched by this PR, so no changed line could carry it — noted here per REVIEW.md's cross-file/whole-PR routing):
Cross-file maintainability note (not inline-able — spans four files, none of which individually "owns" the fix): Confirmed clean against REVIEW.md's two standing code-review checks:
Test coverage for the new logic is otherwise thorough — the major branches (oracle ambiguous-line parsing, |
The typo gate flagged six occurrences across the block lint, its suite, and the drafter's SKILL.md. `status=unparsable` is an output string, so the script and the assertions that read it change together or the suite breaks. The one occurrence in source-control is main's and untouched here. Block lint 62 checks, shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
|
Claude finished @kyle-sexton's task in 42s —— View job Security Review
No new issues. The only commit since the last review round is Previously reported findings — unchanged, still present at
None of the four are attacker-reachable — all operate on the operator's own local settings/managed policy, not untrusted input. They're carried forward unchanged from prior rounds; #1 and #2 (both "present status asserted before/without content validation") are the ones worth fixing before merge; #3 and #4 are lower-stakes refinements. No GitHub Actions workflow files are part of this diff, so zizmor's advisory lane doesn't apply. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @kyle-sexton's task in 3m 1s —— View job Code Review (correctness / architecture / tests / maintainability)
Note: security-relevant findings are covered by the dedicated security-review lane (already posted above, several rounds) and are out of scope for this lane per REVIEW.md's lane split. Deep script review in progress. |
…-path gate The editorconfig failure was a real defect wearing a whitespace costume. Two fixtures had been written by a script that turned `\n` into a literal newline, so `Bash(echo \n *)` became two lines and `printf 'section \n...'` became four. Both suites still passed -- the fragments happened not to match anything -- so the trailing-whitespace error was the only signal that a test was asserting against input it was no longer feeding. Restored to single lines; 66 and 42 checks still pass, now over the fixtures they were written for. The machine-specific-path gate flags this skill for the reason the gate exists: C6-winPath's whole job is finding Windows paths in permission rules, so its finding message quotes one, criteria.md quotes the settings page quoting one, and its fixtures must carry them or the check has nothing to detect. The oracle fixtures reproduce Claude Code's own [DEBUG] narration, which prints native Windows settings paths -- rewriting those would make the fixture stop matching what the parser must survive on the plugin's primary platform. That is the same class as the two exclusions already in this lane (the guardrails path-detection lib self-matches the detector; the autonomy fixtures carry adversarial host paths on purpose), so it takes the same treatment: four named files, with the reason recorded beside them. The paths are invented -- `alice` and `x` -- not any operator's. actionlint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
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 |
…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>
2 participants
Summary
PR #2260 landed the walking skeleton and the merge stage. This is the rest: four more analysis
stages, the second skill, and thirteen defect fixes from three rounds of independent verification.
What this adds on top of #2260
wildcarded interpreter, package-manager run, or
Agent) or kept, from the same shared patternvocabulary
audit-permission-grantscheck P1 scans with. ReadsautoMode.classifyAllShell, whichinverts the answer wholesale, and resolves it only from the scopes the classifier actually reads.
highest-consequence one:
disableAutoModetyped as a boolean instead of the string"disable"isvalid JSON, accepted, and does nothing — so the operator believes auto mode is locked out and it is
not.
autoModeclassifier block —$defaultsomissions (which replace the built-in list,not extend it), contradictions, and entries an earlier
hard_denyforecloses.claude auto-mode critiqueis surfaced and wrapped, never reimplemented.autoModesection is additive, not a policy boundary; and a lower-scope deny beats a managedallow without overriding it, which the precedence table alone does not suggest.
claude-config:draft-auto-mode-rules— the authoring counterpart. Interviews, prints apaste-ready block, and writes nothing, in any scope, under any flag.
allowed-toolstrust measurement carried intoMIGRATION-PLAYBOOK.md's plugin-acceptance security review.Independent verification
Three rounds, fresh context, rationale withheld. Thirteen defects, all reproduced before being fixed.
The ones worth naming:
Bashallow was reported as surviving auto mode. It is the whole-tool form — broaderthan
Bash(*), which the same run drops.Agent's bare form was handled; the shell tools' was not.--merge-onlysilently inverted the entry diff. Two documented flags composing into a wronganswer:
confrecords were dropped, so a suspended rule was reported kept.Bash(a\rb *)intoBash(ab *)—a rule in no settings file, flowing downstream as a real grant. The newline fix from the previous
round had covered LF only.
C6-winPathwas dead in the real pipeline — it tested the JSON source spelling thatjq -rdecodes away, and passed its own suite only because those fixtures bypassed the reader.
give an administrator.
challenge: "allow rules continue to use each tool's own specifier syntax", so
allow: ["Agent(model:opus)"]is not a narrowed grant. New checkC6-allowParam.Two findings were about my own claims rather than the code: a comment asserting as measured
something never measured, and a comment describing a design that had been replaced.
Verification
Suites: reader 50, merge 51, entry diff 63, plane lint 50, block lint 62, conformance 32, drafter 39,
grants 76, managed-scope 16, permission-patterns 12, no-writes 12.
Criterion 9 (writes nothing, any scope, any flag) is proven by running all ten actions of both
skills with the oracle explicitly ON and diffing a fixture tree and fixture HOME either side — not
by reading the code.
shellcheck -x, portability, drift, contract-slice prune, changelog parity andvalidate-pluginsall clean. Listing budget 6737/8000.
Note on the merge resolution
Resolving this branch against post-#2260
mainproduced eighteen conflicts. Taking--ourswas rightfor the files this branch advanced and wrong for three it did not — most importantly
permission-rule-check.sh, where main had removed a$PWDscan-root fallback that could sweepthe operator's home directory. The drift gate caught one; checking the rest of the
--oursset foundthe other two. All three restored from main; see
f8abbafa.Related
No linked issue — this continues the research and planning thread that produced #2260.
🤖 Generated with Claude Code
https://claude.ai/code/session_01TgD5TknENb3ifk1a47eqUr