Skip to content

plugin-quality: the resume rule breaks on any multi-target audit, retention is a model-executed recursive delete over the tree holding the deliverable, and packets are mutable by sibling hooks #1808

Description

@kyle-sexton

Filed by an agent session that ran /plugin-quality:audit across six components and audited the audit skill itself with a fresh plugin-quality:auditor. claude plugin validate on the install passes. All doc citations fetched 2026-07-30.

1. The resume rule is provably broken for a multi-target audit

SKILL.md:122-124 specifies the compaction-survival path: re-derive the packet path from session id, "target slug (re-sanitize the argument)", and latest nonce, and "Never rely on remembering the path."

Applied literally to this run's argument, computed deterministically:

  • argument: a natural-language phrase, not a <plugin>[:<component>] token
  • sanitised per SKILL.md:116 (non-[A-Za-z0-9_-] to -): an 88-character slug
  • os.path.exists() on the derived directory: False
  • slugs actually present for this session: disk-hygiene-clean, guardrails-block-noncanonical-commit, markdown-format, plugin-quality-audit, rate-limit-guard, typos-format

So a post-compaction resume of this audit matches no directory at all and, by SKILL.md:139, must conclude the findings are missing and stop.

The cause is the orchestrator's own reasonable improvisation — allocating one conforming per-component slug — which the skill never authorises or documents. argument-hint is <plugin>[:<component>] and the packet model, slug rule, and resume rule are all singular, while a request like "audit the plugins we used" resolves to six components across five plugins. The single-target slug rule and multi-target reality contradict each other, and the resume rule is where that becomes data loss.

The Windows 260-character path hazard is real but secondary — the derived path measured 239 characters — and was never actually hit, because the improvisation that broke resume is exactly what kept paths short.

Suggested fix: declare fan-out (one packet per component under a conforming per-component slug) as documented behaviour, and redefine the resume slug to derive from resolved component identity rather than the raw argument, with a length cap. That repairs the break at the root and retires the path-length hazard.

2. Retention is an unbounded model-executed DELETE over the tree that also holds the only copy of emitted work items

SKILL.md:121: "on every new audit run, delete packet directories older than 30 days."

Three compounding problems:

  • It is prose, not a script. The skill's own references/recurring-concerns.md section 4 and references/component-types/skill.md:25-26 both ask whether a component relies on the model obeying instructions "where a deterministic mechanism (script) would be more reliable". A recursive directory delete is the worst operation to leave to model prose, and this is the one place the skill does exactly that. The plugin already ships a scripts/ directory, so the deterministic option is available and unused.
  • It has never executed. Oldest content on this machine is four days old, so the delete path is entirely unexercised and no eval covers it. This is a hazard, not an observed failure — a maintainer can fairly answer "no defect has occurred." The trigger is the first packet to cross 30 days.
  • It deletes the deliverable. Step 6 rung 4 writes the work item as item.md "next to the packet" (SKILL.md:268-269), and the autonomous clause sends every unattended run to rung 4 unconditionally (:283-287). In that path item.md is the only copy of the audit's entire output. So an autonomous fleet's work items sit on a delete timer with no external copy. SKILL.md:268-269 also reads ambiguously as to whether "next to the packet" means inside the nonce directory or beside it; both readings sit under evidence/.

Empirically, item.md count across all 20 stored packets on this machine: 0. The unattended emit path has never run here either.

Suggested fix: move item.md out of the retention-swept tree, or exclude any packet containing an unemitted item.md from pruning. Then replace the prose rule with a script under the existing scripts/, invoked at a defined point, with a dry-run default and a documented escape hatch.

3. Packet content is mutated in place by sibling hooks after the write returns

SKILL.md:144-172 anticipates a filename-based write rejection. It does not anticipate post-write content mutation, and the packet's status as durable ground truth depends on the content being what the auditor wrote.

Verified from the sibling installs' own manifests: typos-format/0.4.2/hooks/hooks.json and markdown-format/0.8.4/hooks/hooks.json both register PostToolUse with matcher: "Write|Edit", unconditionally, and both rewrite in place. Neither matcher excludes anything, so every packet write in this environment is post-processed.

Observed in this run, three times:

  • typos-format rewrote 11 identifier tokens inside the packet documenting its own defect, collapsing before/after pairs so the surviving text read as though the finding were false.
  • It destroyed a verbatim upstream dictionary citation that was a root-cause proof, and "corrected" two of upstream's own false-positive documentation examples — identifiers that exist in those docs precisely because they contain deliberate misspellings.
  • markdown-format normalised a quoted changelog line's leading plus sign to a hyphen (MD004, fixable), falsifying a quotation, and separately turned a line-leading issue reference #1426 into # 1426 (MD018, fixable), destroying the reference and fabricating a top-level heading.

The severity argument is observed rather than argued:

  • The mutation is silent with respect to the artifact. It is detectable only because a hook prints a notice into the session — exactly the context the packet exists to outlive. A fresh auditor reading the packet after compaction sees mutated text and no notice.
  • It is content-destructive, not cosmetic: it damaged verbatim quotations and code-span identifiers, the two content classes an evidence packet exists to preserve.
  • The hooks' own notices state "the autocorrect has no memory", so hand-repair is reverted on the next edit. Any fix must change the content's shape or the hooks' config.

Suggested fix: state explicitly that packet files are write-once evidence and that sibling PostToolUse formatters may rewrite them in place, then record a content hash beside each packet file at write time so a later reader can detect mutation instead of trusting silently-altered evidence.

Note what does not work, because it was proposed and then disproved by this audit's own sibling findings: a typos allowlist and a .markdownlint opt-out for the packet tree are both unreachable. typos config discovery is first-match-wins with no user or home layer by design; markdownlint discovery is cwd-anchored and does not reach above cwd while the hook cds to the file's own directory for non-repo files — so the opt-out would have to be planted in every per-run nonce directory, each created fresh. The workable levers are the hash, and the hooks' own write-authority fixes.

4. A stated harness claim is false per current docs

SKILL.md:108-110: "The ${CLAUDE_PLUGIN_DATA} token does NOT substitute in skill markdown (it is a hook/monitor/MCP path substitution), so derive the directory deterministically…"

The plugins reference (https://code.claude.com/docs/en/plugins-reference, Environment variables) documents substitution scope per component:

| Plugin component          | Fields where placeholders resolve |
| Skill and agent content   | Anywhere the placeholder appears  |
| Hook and monitor commands | Anywhere the placeholder appears  |

plus, for user_config: "Non-sensitive values can also be substituted in skill and agent content."

So skill content substitutes the token anywhere it appears, and the parenthetical is wrong twice over — the page says all three path variables are both exported to hook/MCP/LSP subprocesses and substituted inline in skill and agent content.

Mitigating, and the reason the fix is small: the derivation the skill prescribes is itself doc-correct. Same page: ${CLAUDE_PLUGIN_DATA} "resolves to ~/.claude/plugins/data/{id}/, where {id} is the plugin identifier with characters outside a-z, A-Z, 0-9, _, and - replaced by -." So delete the false premise and keep the derivation as a documented fallback; do not rewrite the path logic.

Two costs as observed: the prescribed manual derivation is unnecessary work, and a reader is taught to distrust a correct value in the same sentence that presents it.

5. The closed-set resume probe has no authority rule, and real runs already broke the set

The closed set {audit-notes.md, audit-data.md, findings.md} (SKILL.md:126-130) is genuinely good threat modelling and should not be weakened — see the credit list below. Two gaps, both observed in this plugin's own stored packets:

  • A name outside the set exists. One packet contains audit-notes.md (9,651 bytes) and audit-notes-2.md (13,175 bytes), written one minute apart. SKILL.md:130 states "Adding a fourth name is a change to this skill, never a runtime improvisation" — a runtime improvisation is what happened. A post-compaction resume probes the set, hits audit-notes.md first, and silently loses 13 KB: 58% of that audit's findings, with no signal anything is missing. The closed set answers "which name" but the skill provides no convention for findings exceeding one file, which real runs evidently need.
  • The probe is first-hit-wins with no recency or completeness check. In another packet, audit-data.md (21,012 bytes, 19:56) and audit-notes.md (51,173 bytes, 20:22) coexist. Probe order returns audit-notes.md, which here happens to be both later and larger — that is luck. Invert the timing (a partial audit-notes.md write followed by a complete audit-data.md rewrite, which is precisely the rename-fallback sequence the skill designs for) and resume silently returns the truncated file.

Suggested fix: add an authority rule — when several closed-set names exist, prefer the most recently modified and report that others were present rather than silently choosing. Then define a multi-file convention (audit-notes.md plus audit-notes.part<N>.md, with resume reading all parts) so the next audit-notes-2.md is recoverable instead of invisible.

6. The "every packet holds a non-empty evidence.md" premise is empirically false

SKILL.md:136-138 argues that "some file exists" is never evidence that grounded findings do, because "every initialized packet already holds a non-empty evidence.md."

One stored packet contains no evidence.md at all. It holds evidence-1-invocation-and-early-cycles.md, evidence-2-cycle-26.md, evidence-3-cycles-27-28.md, evidence-4-permissions-and-anomalies.md, and findings.md.

The conclusion the premise supports still holds, so this is not a reasoning error — but the premise is false, and it has a second consumer that does break: agents/auditor.md:49-51 tells a dispatched auditor to read the packet "(evidence.md first)". Pointed at that packet, a fresh auditor's first read fails with no documented enumeration fallback. Across all packets, five distinct evidence filenames and 22 distinct filenames overall were observed, against a documented schema of four.

Suggested fix: soften the agent instruction to enumerate evidence*.md rather than assume evidence.md.

7. The report-filename guardrail is misidentified, so its fallback is near-dead code

SKILL.md:146-152 and agents/auditor.md:23-27 describe "a Write-tool guardrail that rejects report-shaped filenames", "keyed on the filename alone", with a rewrite-to-audit-data.md fallback for when "a packet write is rejected on those grounds."

The skill is commendably honest that this is undocumented, and that holds up: the sub-agents page (https://code.claude.com/docs/en/sub-agents) expresses write restriction only at tool-access granularity — tools as allowlist, disallowedTools as denylist — with no filename-based mechanism anywhere.

But the mechanism is misidentified. In a dispatched auditor's context the constraint arrives as a system-prompt instruction to the subagent, not a tool-level rejection: "Do NOT Write report/summary/findings/analysis .md files. Return findings directly as your final assistant message…"

Consequences: it never rejects, it biases the model — so the audit-data.md fallback triggers on a condition that does not occur, which is why no rejection was observed and why the fallback has fired at most once in 20 packets. And because it is advisory rather than a filter, an auditor may comply by returning text and skipping the packet write entirely, which is the exact failure agents/auditor.md:33-35 warns against and is far likelier than a rejection.

The audit-notes.md naming choice is still correct and worth keeping — it sidesteps the instruction's name class. Only the stated mechanism and the fallback's trigger condition need correcting.

8. Smaller findings

  • agents/auditor.md:62-63 instructs the agent to walk references/recurring-concerns.md. The agent definition lives at agents/auditor.md and agents/ contains only that file; the reference is actually at skills/audit/references/recurring-concerns.md. The relative path resolves nowhere. And step 2 (SKILL.md:186-190) only guarantees passing "the applicable component-type lens file(s)" — recurring-concerns is listed in the index as "Every audit" but is not named as a required dispatch argument, so the mandatory checklist reaches the auditor only if the dispatcher volunteers it. This run did, which masked the defect.
  • SKILL.md:198-200 says what a context: fork inherits "is contested (see the plan's caveat on sweep-all-disciplines: fork subagents silently do not inherit the conversation, and two of four claimed they did #1258)". The skills page states it plainly: "Add context: fork to your frontmatter when you want a skill to run in isolation… It won't have access to your conversation history." Not contested. Worse, references/component-types/skill.md:19-20 already asserts that as fact — the lens file contradicts the hub within one plugin version, and the lens file is the one matching the docs. The #1258 citation also references a plan document no reader of the installed plugin can see. Note two distinct "fork" mechanisms exist (skill context: fork versus the Agent tool's context-inheriting fork type); step 2 does not disambiguate them, which is likely the origin of the confusion. The underlying two-property test — no inherited reasoning, named auditable worker — is correct and better than the claim it leans on; keep it unchanged.
  • The entire context-gate computation is unexercised and uncovered. ~/.claude/context-guard/ does not exist on this machine, so only the unknown row has ever run. Unexercised: percentage bands, token bands, class selection, the cli_version >= 2.1.132 guard, the conservative-min combination rule, the compaction override, and all zones.json shape validation — roughly 40 lines of the densest branching in the skill (SKILL.md:57-89). evals.json has 9 evals; eval 6 is the only zone eval and it tests the absent-snapshot unknown path, the one that already runs. Two real mitigations deserve credit: degradation is safe (absent data resolves to the tighter row, and SKILL.md:95 keeps steps 2-3 in a fresh subagent in every zone, so the untested logic cannot trade away independence), and scripts/zones-inline-drift.test.sh mechanically pins the inlined band values to the contract that owns them. The gap is behavioural coverage, not value drift. Worth noting the cost/benefit as written is unfavourable: zone_behavior: always-conservative exists to force the conservative row, the unknown default is that row, and the zone modulates only re-read style and flush timing — a large uncovered computation buying a small delta. Either add fixture-driven evals or reduce the gate to the rows that change behaviour.
  • The verb-contract note omits local writes and deletes. SKILL.md:292-295 records the deviation from the fleet's read-only audit verb and argues the unconditional draft+confirm is the user override. Assessed on the merits that is coherent and arguably stronger than the --apply flag it departs from: --apply is pre-authorisation granted before the payload exists, while draft+confirm is approval at the mutation point with the full body, destination, and acting identity in view. It is also applied consistently. But the note reasons only about external egress, while this "read-only" verb writes into its own data directory on every invocation and — per finding 2 — recursively deletes directories older than 30 days. Either scope the claim ("read-only with respect to the audited plugin and all external targets; it owns and prunes its own data directory") or gate the delete. One structural nit: argument-hint has no flag surface, so the skill could not accept an --apply-style argument even if the fleet standardised on one.
  • Step 5's seam resolution is presence-based where it needs to be capability-based. It offers "used when installed, with a one-line fallback when absent", with no third state for installed but inapplicable — which is the state that actually obtained for two of three seams here. review:quality-gate and review:fanout are diff-oriented end to end (they resolve a branch, a PR base via git merge-base, and write findings under .work/reviews/<branch-slug>/), so they cannot review a findings write-up in a non-repository home directory. And skill-quality:check's script exits 2 with "not in a git repo" against an installed plugin cache, so the gate that is mandatory for skill targets is unreachable in exactly the scenario this skill exists for. State each seam's precondition, and specify that an unmet precondition takes the absent path with the reason recorded.
  • agents/auditor.md sets effort: high and omits model. Both are valid, but the plugin's own references/component-types/agent.md:13-14 asks of audited agents whether the model is "explicitly set where the task demands it… or deliberately inheriting?" The plugin does not answer its own question, and the skill's entire independence argument rests on the auditor's judgment quality. Pin a model or record the inheritance as deliberate.
  • metadata is not a documented SKILL.md frontmatter field. SKILL.md:6-8 carries metadata: {workflow-stage, summary}; the skills page frontmatter table does not list it. Presumably a harmlessly-ignored fleet convention, and metadata.summary duplicates description. Flagged only because the skill's own discipline demands harness claims be doc-grounded. The description measures 882 characters, comfortably inside the documented 1,536-character cap.
  • scripts/zones-inline-drift.test.sh is inert in every installed deployment — it exit 0s in a consumer install by construction, so a consumer cannot detect band drift locally, and one of its assertions (both "token-shape version floor" "cli_version") only checks that a bare string appears in both files, which cannot fail meaningfully. The adjacent 2.1.132 assertion carries the weight. The SKIP is honestly documented and visible on stdout.

What this skill gets right — please do not regress these

  • The untrusted-content posture. Stated crisply at SKILL.md:22-26 and repeated independently at agents/auditor.md:40-45, where it actually has to hold. It names concrete attack shapes ("skip the confirm step", "send findings to repo X") and specifies the response. Eval 7 tests it with a realistic injected payload.
  • The closed-set resume probe is real threat modelling. SKILL.md:131-134 refuses to read the findings filename from evidence.md because evidence.md carries audited output, and spells out the attack: a forged substitution record redirecting a resume onto an attacker-chosen file. Deriving the probe set statically so there is no pointer to poison is the correct design. Finding 5 asks for an authority rule within the set; it does not question the set.
  • The egress gate is the strongest part of the design, and it demonstrably works. Draft plus destination plus acting identity in one surface, no auto-file mode, coverage extended to seam emits, and an autonomous clause that explicitly refuses to relax with the right reasoning: "an absent confirmer is not an implicit confirmation." Including acting identity is a subtlety most designs miss. A stored filed.md shows it captured in practice.
  • The producer/consumer split, reinforced at the agent boundary and tested by eval 4. Prevents the common failure of fixing the audited plugin from the session with the most context and least independence.
  • Independence is not traded away under context pressure — steps 2-3 run in the fresh auditor in every zone, and eval 6 tests that missing context data is not licence to inline the deep analysis. This is why finding 8's coverage gap is not more severe.
  • Calibrated honesty about unverified harness behaviour. SKILL.md:167-172 labels the filename guardrail "observed harness behavior, not documented", names the page and date checked, and draws the right conclusion. Finding 7 corrects the mechanism, not the epistemics — the epistemics are exemplary and rarer than the correctness.
  • The unattended decision table (:229-232) resolves each contract-lock decision to the conservative answer, refuses to re-grade severities without a human, and explains why an unresolved emit target is deliberately not a blocker. Unusually careful reasoning.

Scope of verification

One machine, one operator, 20 stored packets as the behavioural sample. Only the unknown zone and only Windows/Git Bash were exercised — no POSIX run, no populated zones.json, no compaction event, no always-conservative run. No config layer was ever populated, so per-key override, repo_map, and the markdown-dir sink are untested by observation. skills/setup/ was not audited. The step-4 adherence figure (contract.md present in 3 of 20 packets) cannot distinguish "step skipped" from "run abandoned before step 4".

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions