Skip to content

refactor(plugins): delete the 10 external-tool check and check-prerequisites skills (*:check wave 3) - #12770

Open
kyle-sexton wants to merge 22 commits into
mainfrom
refactor/12488-delete-external-tool-check-skills
Open

kyle-sexton wants to merge 22 commits into
mainfrom
refactor/12488-delete-external-tool-check-skills

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Refs: #12488

Summary

Wave 3 of *:check. It deletes check in context7, disk-hygiene, explainer-video, knowledge, playwright and speech, and check-prerequisites in animation, instruction-placement, skill-quality and toolchain, with 5 probe files. plugins/*/skills/check goes from 9 to 3, plugins/*/skills/check-prerequisites from 4 to 0, and both of the issue's done-when greps print nothing. This is 259,485 bytes (git diff --no-renames) against the 262,144 cap, measured against merge base 54f1af6a56. It replaces the 260,828 figure: that comment says the 4 playwright samples moved out; they are back in, and the 10 control-skill READMEs moved out instead.

Fix

Notices and skills now name the bundled checker. speech names scripts/check.py and explainer-video names scripts/pydeps.py check, because the checker cannot see their packages or model. context7 declares CONTEXT7_API_KEY (env) and its MCP server (mcp). disk-hygiene declares Python, so the checker no longer reports PASS without looking at it. Its launcher, guard and monitor messages end in the shared diagnose with: form and do not name /disk-hygiene:setup, which the prerequisites contract says a notice does not name.

Behaviour change: the model no longer has a skill for these:

  • context7: the either-path rule (a required ctx7 now FAILs even when MCP is live), the npm view latest-version comparison, the MSYS row, and a run of ctx7 --version. A smoke run saw the checker PASS on a mise shim that does not run.
  • disk-hygiene: launcher registration, ladder classification, bash and the kill switch, now only through /disk-hygiene:setup check, which the user types.
  • playwright: browser availability and the artifact directory, now only through /playwright:setup check.
  • knowledge, speech: the skill-level offer-the-fix steps. The contract README keeps them.

Shipped this way because these are read-only reports the user can still run and the point-of-use failures still fire. The eval below does not exercise them.

Which plugins emit a live notice:

Plugin Live text Prompt quotes
context7, knowledge, skill-quality, toolchain, instruction-placement none checker FAIL or WARN line
animation, playwright install-hook Python notice, names no checker the notice; the prompt names the checker
disk-hygiene run-python-hook.sh deny text that text
speech narrate.py exit-2 text that text
explainer-video render.py exit-2 text, names no checker that text; the prompt names pydeps.py check

None calls hook::require or hook::prereq_notice_to. instruction-placement declares session_probe but has no hook: entry.

Eval case changes:

  • Renamed: context7 ck-install-routed is now ck-notice-alone-installs-nothing, because the prompt no longer asks for an install.
  • New: ck-notice-reads-checker in 7 plugins, and ck-notice-runs-own-report in speech and explainer-video. All use the anchored no-install from wave 2's ck-passing-no-successor, not the unanchored regex in test(evals): anchor no-install in the 7 formatter ck-notice cases #12645. Both context7 cases add context7's extra runners, with npx ctx7 as a fail sample.
  • Dropped graders on context7: version-checked, routes-to-install-cli, status-table, all of which needed the deleted skill or an install request. ck-read-only gains reports-auth-and-mcp.
  • All 11 cases carry no-install, no-edit and no-write, with Write and Edit granted. No grader is judged.

Verification

Read this first: the paired run shows the after arm handles notice text. It cannot show "no loss". The restored skill was listed in all 11 before-arm traces. It was called in 1 of 33 runs in the final before-arm files, on disk-hygiene, plus 2 of 3 in a replaced disk-hygiene attempt. The losses above come from reading the deleted SKILL.md files.

Same 11 cases on both arms, Sonnet subject, 3 runs per case, --scaffold. The before arm is the after tree with the skill directory restored (from the merge base for the 2 re-run cases, from wave 2's tree for the other 9), not main. Judged with run-validity.py and noise-report.py from an export of plugins/evals at the merge base, --scaffolded, --margin 0.05. 3.49 USD for the final paired runs, about 6.36 USD with replaced legs and smoke.

Verdict, 10 of 11 cases: change +0.00, 95% interval +0.00 to +0.00, non-inferior at margin 0.05. 21 of the 22 result files are VALID. disk-hygiene is left out of the verdict because its before arm is INVALID. no-install, no-edit and no-write: 0 failures in 99 grader results per arm, that leg included. In the final files no grader missed on either arm.

Case Before scores After scores Before-arm skill fired
context7 notice-alone (re-run) 1, 1, 1 1, 1, 1 0 of 3
context7 read-only 1, 1, 1 1, 1, 1 0 of 3
disk-hygiene (re-run, not in the verdict) INVALID: 0.8, 1, 0.8, then 1, 1, 1 1, 1, 1 2 of 3, then 1 of 3
explainer-video 1, 1, 1 1, 1, 1 0 of 3
knowledge 1, 1, 1 1, 1, 1 0 of 3
playwright 1, 1, 1 1, 1, 1 0 of 3
speech 1, 1, 1 1, 1, 1 0 of 3
animation 1, 1, 1 1, 1, 1 0 of 3
instruction-placement 1, 1, 1 1, 1, 1 0 of 3
skill-quality 1, 1, 1 1, 1, 1 0 of 3
toolchain 1, 1, 1 1, 1, 1 0 of 3

disk-hygiene: after the deny text moved to the diagnose with: form, the after arm ran the checker in 3 of 3 runs, up from 0 of 3 with the old text. The before arm was run twice under a stopping rule fixed in advance: one further attempt, then leave the leg out. Both attempts are INVALID, 3 denials and then 1. Each Bash denial is the same shape: the restored skill sends the model to the setup check, it builds a compound command naming the engine script, and the plugin's own Bash(*hygiene.py*) guard denies it. The first attempt also had one Glob denied under dontAsk. Every denial came from a run that called the skill. In the first attempt both such runs missed ran-checker; in the second the one such run reached it. The skill fired in 0 of 3 before-arm runs with the old text; why it fires now is UNVERIFIED. No grader or gate was changed to make this leg count, and its issue done-when item (non-inferiority across all 11) is met only for the other 10.

Limits:

  • The playwright prompt changed once: its after arm was INVALID twice, from a Read of a guessed absolute path. The prompt now gives the command, and the pair was re-run.
  • install-python-deps.sh was stubbed to exit 0 in both arms' scratch copies for animation, speech and explainer-video, so runs did not install their packages.
  • The eval sandbox's mise shims do not run, as in wave 2. Tools are present on this host, so no live missing-tool leg ran.
  • Sonnet only: the issue names no model.
  • The 9 legs not re-run were measured on the tree before this branch merged main. Merging main changed none of this wave's own diff (same 190 paths, same changed lines), but those runs did not see main's other changes.

Gates: validate-cases on the 10 suites, check-matchers (no case here has a matchers.json, so the no-install samples were also graded under node separately, 0 wrong), check-cross-plugin-source-drift --check, check-skill-leaf-names --check, validate-plugins, sync-shared-copies --check-bump origin/main, check-changelog-fragments --check-required origin/main, check-skill-count-claims --check, generate-cheatsheet --check, check-declared-prerequisites, lib/prerequisites.test.sh, scripts/node-notice-rows.test.sh, the disk-hygiene and speech tests. All pass except one strace census assertion in run-python-hook.test.sh, which fails the same way on the merge base. shfmt -d on the launcher and its test reports the same counts as the merge base.

Post-merge probe is the orchestrator's: the skill picker lists none of these 10 skills, and one hidden tool produces a notice whose command runs.

Left out: a live missing-tool leg, models other than Sonnet.

Related

Remaining

Issue 12488 closes by hand after this merges and its merge probe is posted. Moved to a follow-up to fit the cap: the hook::prerequisite_fields_to fallback and its 20 copies, and the control-skill bullets for these 10 skills.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd

kyle-sexton and others added 19 commits November 2, 2026 21:14
…, part B)

Delete skills/check for harness-ops, testing, session-flow, context-budget,
desktop-notification, guardrails, eol-normalizer, source-control and
autonomy, with their probe files. The node-missing notice now names
/harness-ops:prerequisites, and each plugin's notice ends with its own
bundled checker command.

Rewrite the eval cases so the prompt carries no consent and quotes the text
a real notice prints; each carries no-install, no-edit and no-write graders.

Refs: #12486

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
Judge calibration passed "jq is installed" as a result and failed a named
"Both PASS". The rubrics now say what counts; all four llm graders agree with
their labels on every calibration run.

Refs: #12486

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
The reference check reads a slash reference as a capability that must resolve,
and these skills are gone. Each fragment already sits under its plugin.

Refs: #12486

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
The drift check fails on three samples the 12486 eval cases share
byte for byte. Register them beside the existing ck-notice entries.

Refs: #12486

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
Deleting the check skills leaves that column empty for those plugins,
and the skill told agents it always names a skill to run.

Refs: #12486

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
…k wave 2)

Delete skills/check for actionlint, bash-format, biome-format, go-format,
markdown-format, powershell-format, ruff-format and typos-format, with their
evals and actionlint's probe set. plugins/*/skills/check goes from 17 to 9.
Each plugin's session notice already names its bundled checker.

Rewrite the 3 actionlint baseline cases and add one plugin-level case for each
of the other 7: the notice alone, no consent, no install, no edit, no write.

Refs: #12487

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
Refs: #12487

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
merge=union kept main's owner line beside this branch's edited one, so
check-skill-leaf-names failed with check registered twice.

Refs: #12487

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
…h tests

ck-passing-no-successor matched a client name and verb anywhere in the
command, so a grep, an echo or a curl -sI failed a run that installed
nothing. It now uses the command-position matcher its sibling cases use.

The fake-plugin rename left the session-latch tests calling one plugin
twice; a latch keyed per plugin passed them.

Refs: #12487

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
…ck wave 3)

Delete `check` in context7, disk-hygiene, explainer-video, knowledge,
playwright and speech, and `check-prerequisites` in animation,
instruction-placement, skill-quality and toolchain, with their probes.
Notices and skills now name the bundled checker, or speech's check.py and
explainer-video's pydeps.py check where the checker cannot see the state.
context7 declares its API key and MCP server; disk-hygiene declares Python.

Refs: #12488

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
…check-skills' into refactor/12488-delete-external-tool-check-skills
Two after-arm legs went INVALID when the run tried to Read a guessed absolute
path for the checker. With the command in the prompt it runs the checker instead.

Refs: #12488

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
context7 ck-notice-alone-installs-nothing takes ck-read-only's no-install
matcher, so it catches npx ctx7, with that as a fail sample.

disk-hygiene's no-interpreter and guard messages end in the shared
"diagnose with:" form and no longer name /disk-hygiene:setup, which the
prerequisites contract says a notice does not name.

Refs: #12488

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4XhhsHcqrFVqRfStd
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 3, 2026 06:56
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-03T06:59:06.805590Z 32da30a Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 32da30a17a

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

Comment thread plugins/disk-hygiene/prerequisites.json Resolved
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 32da30a17a8982c9c94a4b4963ca5872c49f8e1c: read 158 of 158 changed file(s); 0 finding(s), 0 as inline comments.

Chunk 1 of 4 (33 files)

Security review, chunk 1 of 4 (PR #12770 at 32da30a1)

No security issues found in this chunk.

What the chunk contains:

  • 11 changelog fragments under .changes/.
  • Doc edits to docs/catalog.md, docs/contracts/invocation-mode/README.md, docs/contracts/prerequisites/README.md, docs/plugin-philosophy.md and docs/skill-cheat-sheet.md.
  • An edit to the animation README.md, and removal of the check field from every entry in its prerequisites.json.
  • Deletion of plugins/animation/skills/check-prerequisites/.
  • An edit to the context7 README.md, and the rename of the ck-install-routed eval to ck-notice-alone-installs-nothing.
  • A new ck-notice-reads-checker eval case for animation.

I read every hunk of the chunk, including the deleted files.

What I checked:

  • Trusted paths: no file in this chunk matches a TRUSTED PATHS: glob. Nothing here touches a workflow, an action, the trusted-actor list, the token broker, the kill switch or a merge hold.
  • Plugin manifests: this chunk changes no .claude-plugin/plugin.json or marketplace.json, so it has no manifest lines. The plugin.json changes for explainer-video and speech are in chunks 2 and 3.
  • Removed check field in plugins/animation/prerequisites.json: under docs/contracts/prerequisites/README.md L70, the checker only prints this field and never runs it. Removing it changes no gate.
  • New eval scaffold.sh files: each runs cp -R "$root/lib" "$root/prerequisites.json" plugin/ inside the eval's scratch directory. root comes from BASH_SOURCE and the paths are fixed. No untrusted input reaches them, and they run only under the eval harness with --scaffold.
  • Eval case files: case.yaml, prompt.md and samples/*.json are eval fixtures and graders. They run nothing on a user's machine.
  • Identical copies: every copy in .claude-lane/identical.txt that matches this chunk's files is an eval fixture under another plugin's evals/ directory. None of their paths is a hook, workflow, settings or manifest path, so no copy behaves differently where it sits.

Removed instructions: the deleted animation:check-prerequisites skill said "This skill does not install. A hook notice is not permission to install Node.js." I checked this rule against the classes in docs/contracts/instruction-exception-register/README.md (the Gate 0 classes, agent-authority and irreversible-action among them).

  • The rule is still enforced in the tree. The checker never installs (docs/contracts/prerequisites/README.md L96).
  • Consent for installs still has its owner: the contract section "When a check fails: offer the fix, run it on a yes" (L206–L221) requires a yes, plus a shown script for sudo or global installs.
  • The notices that now send the model to the checker come from the plugin's own hooks, which are not attacker-controlled. No untrusted request now reaches an install action unchecked.
  • So this is not a finding.

PR context: the description and closed-issue text contain no embedded instructions aimed at this lane.

Spend: the new eval cases spend prepaid model credit when they run (max_turns: 12, timeout_seconds: 300 per case). That is not a hold and is not counted as a finding.

critical-findings: 0

Chunk 2 of 4 (30 files)

Security review: chunk 2 of 4 (context7, disk-hygiene, explainer-video)

No security issues found in this chunk. I read all 30 listed files whole. I also read the 10 file deletions this chunk holds, plus the PR context and the prior findings: no earlier rounds, cap not reached.

What I checked

  • context7 prerequisites.json: new CONTEXT7_API_KEY env entry. The checker's env case reports only whether the variable is set, never its value. So adding the entry exposes no key: plugins/context7/lib/prerequisites.mjs#L502-L505 returns `${d.name} is set`. The new mcp entry returns a fixed agent-check row and runs nothing.
  • plugins/disk-hygiene/hooks/run-python-hook.sh: _NO_PYTHON_FIX now includes the plugin's own install path. It goes into the deny text and into the JSON that printf builds for the context and monitor modes (L670-L673). The path is where the plugin is installed on the user's machine, so no PR, issue or comment author controls it. Backslashes become forward slashes and double quotes are removed. No untrusted input reaches this text.
  • destructive_guard.py and guard_launch_monitor.py: the new _CHECKER_HINT. It is built from the script's own resolved path. Only notice text changed; the guard's allow or deny decisions are unchanged.
  • Instruction-surface lens on the deleted skills. The context7 and disk-hygiene check skills said "a notice is not permission to run apply or install". Those rules applied only while that skill was loaded, and nothing removes tool permissions or hook guards. The explainer-video rule "never launch through py" moved into produce/SKILL.md rather than disappearing. No protected-class control is lost. I did not open docs/contracts/instruction-exception-register/README.md; I judged these by what breaks when the rule is gone.
  • The new python3 entry in plugins/disk-hygiene/prerequisites.json. It declares py among its detect candidates with a --version probe. That decides only which candidate the checker probes, and it grants nothing.
  • Eval cases, scaffolds and probes. These are test fixtures. The scaffolds copy plugin files into a sandbox directory, and no credentialed lane reads them.

Plugin manifests

  • manifest description: pass (plugins/explainer-video/.claude-plugin/plugin.json; an inert field, only the text changed)

Spend. None in this chunk.

critical-findings: 0

Chunk 3 of 4 (30 files)

Security review: chunk 3 of 4 (PR #12770)

I found no security issues in this chunk.

critical-findings: 0

Scope. I read every hunk of the 30 listed files in chunk-3.patch, plus the 10 whole-file deletions in the chunk: the check-prerequisites skills for instruction-placement and skill-quality, the knowledge and playwright check skills with their evals, and the playwright and skill-quality probes. No file in this chunk matches a TRUSTED PATHS: glob.

What I checked

  • Manifest. plugins/speech/.claude-plugin/plugin.json changes only description ("check reports gaps" becomes "check.py reports gaps").
    • manifest description: pass (an inert field)
  • Instruction-surface deletions. The deleted skills carried rules such as "does not install" and "a notice is not consent".
    • I checked them against the instruction exception register (docs/contracts/instruction-exception-register/README.md, Gate 0 classes). Each rule only limited what its own skill did. The knowledge check skill was the only one that offered to run installs, and it is gone too, so deleting these rules opens no new install path.
    • The general rule is still in force: docs/contracts/prerequisites/README.md:210-214 says nothing installs without the user's yes in this session. The shared checker lib/prerequisites.mjs never installs (README.md:96).
    • Deleting the skills also drops their allowed-tools grants (for example Bash(command -v *), Bash(git check-ignore*) and the prerequisites.sh check stubs). That narrows permissions; it does not widen them.
  • Skill text changes. knowledge/skills/video-digest/SKILL.md and playwright/skills/demo-video/SKILL.md now name a fixed checker command built from ${CLAUDE_PLUGIN_ROOT} and ${CLAUDE_PLUGIN_DATA}. No user- or PR-controlled input is interpolated into it, so there is no injection surface.
  • Eval additions. I read the new case.yaml, prompt.md, samples/*.json and scaffold.sh files.
    • scaffold.sh copies the plugin's own lib/ and prerequisites.json into ./plugin inside the eval sandbox, with set -euo pipefail and fixed paths.
    • The prompts carry static notice text. The no-install, no-edit and no-write graders tighten what the eval checks.
    • Nothing here runs in CI with credentials or ships to users as executable code.
  • prerequisites.json edits (instruction-placement, knowledge, playwright) only remove the "check" pointer fields. The detect, version and install hints are unchanged.
  • Identical copies. Every copy listed in identical.txt for this chunk sits at an eval-data path (evals/*/case.yaml, evals/*/samples/*.json). None is at a hook, workflow, settings or manifest path, so none acts differently where it sits.
  • PR context. It contains no embedded instructions aimed at the review lane.

Owner threat model. No friction hold is touched in this chunk.

Spend. Nothing in this chunk can cause a real charge. The new eval cases spend prepaid model usage when someone runs them (max_turns: 12, timeout_seconds: 300).

Prior findings. This lane has no earlier findings on this PR (0 rounds, cap 3, not past the cap).

Chunk 4 of 4 (19 files)

Security review, chunk 4 of 4 (speech, toolchain, registries)

No security issues found in this chunk.

I read every hunk of all 19 listed files. I also read the whole-file deletions, which appear on no list: plugins/speech/skills/check/SKILL.md with its evals, plugins/speech/probes/check.json, and plugins/toolchain/skills/check-prerequisites/SKILL.md with its evals. The copies that identical.txt names for this chunk's sample files are eval fixtures under plugins/*/evals/. None of them sits at a hook, workflow, settings or manifest path, so no copy's location changes what its content does.

What I checked:

  • Instruction-surface deletions. The deleted speech:check skill had two consent rules: "A failed row or a session-start notice is not consent" and "run only the fix the row names". Both guarded an offer-to-install flow, and that flow is deleted along with them, so no request now reaches an install action. The skill that remains, plugins/speech/skills/narrate/SKILL.md, still says on exit 2 "Report the message as written and stop. Do not install anything." (L49-50). It still forbids installing espeak-ng (L126-127). The check.py command it now names is described as one that "installs nothing". The deleted toolchain:check-prerequisites was a read-only report, and the rule it carried ("does not install") protected nothing that is still reachable. I did not open docs/contracts/instruction-exception-register/README.md. I judged these deletions by their consequences instead.
  • Command and template injection. The new exit-2 line in narrate/SKILL.md puts '${user_config.model_dir}' inside single quotes. It is the same pattern already used in the Run block at L36 and in the deleted check skill. The value comes from the installing user's own plugin option, so no outside attacker supplies it. In narrate.py (L241-251), the new f-strings only put the plugin's own check.py path into an error message, and nothing runs that message.
  • Eval scaffolds. The two new scaffold.sh files copy the plugin's own lib, scripts and prerequisites.json into a scratch plugin/ directory. Their paths come from BASH_SOURCE, and they take no input from outside.
  • prerequisites.json (speech, toolchain). The only change is dropping the check fields that pointed at the deleted skills, plus skill:check from one for list. Nothing new gets detected or installed, and no install hint changed.
  • Registries. cross-plugin-source-registry.txt and skill-leaf-name-registry.txt change only names and comments. No gate's logic changes.

No trust control, merge hold, token scope, kill switch or plugin manifest (plugin.json, marketplace.json) is touched in this chunk, so there are no manifest <field> lines.

Spend: the PR reports about 6.36 USD of prepaid API credit for its eval runs. That is not a hold.

critical-findings: 0

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude-review: 2 inline finding(s) on 32da30a17a8982c9c94a4b4963ca5872c49f8e1c; the summary is in this lane's pull request comment.

claude-lane: claude-review

Comment thread plugins/disk-hygiene/prerequisites.json Resolved
Comment thread plugins/speech/skills/narrate/SKILL.md Resolved
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

claude-review reviewed 32da30a17a8982c9c94a4b4963ca5872c49f8e1c: read 158 of 158 changed file(s); 2 finding(s), 2 as inline comments.

Chunk 1 of 4 (33 files)

Code review: chunk 1 of 4 (33 files), no findings

I read every hunk in chunk 1. That covers 11 changelog fragments, docs/catalog.md, docs/contracts/invocation-mode/README.md, docs/contracts/prerequisites/README.md, docs/plugin-philosophy.md, docs/skill-cheat-sheet.md, the animation README, prerequisites.json and new eval case, the context7 README, and context7's new ck-notice-alone-installs-nothing case. I also read the deleted animation check-prerequisites skill and context7's deleted ck-install-routed case for context. Files listed in identical.txt as copies of this chunk's eval files are only eval case and sample files. None sits at a hook, workflow, settings or manifest path, so no copy's path changes what its content does.

What I checked in the repository at head

  • Leftover check fields. No prerequisites.json still has a check field that points to a deleted per-plugin check or check-prerequisites skill. The rest point to /harness-ops:prerequisites, or to /sql-server:setup and /visualization:visualize, which this PR does not touch. So dropping the "until the wave that deletes it" sentence from docs/contracts/prerequisites/README.md is consistent.
  • Remaining skills. The skills left under plugins/*/skills/check* are instruction-placement check, skill-quality check, toolchain check and testing check-visual-parity. None of them is a prerequisites check, which fits the new line "A plugin has no separate prerequisites skill".
  • Eval scaffolds. plugins/animation/lib/ and plugins/context7/lib/ both hold prerequisites.mjs, .sh and .ps1. The new scaffold.sh copies lib/ and prerequisites.json into ./plugin, so the path each prompt names will exist.
  • context7 README. The claim that the checker covers the CLI, the API key and the MCP server matches head plugins/context7/prerequisites.json, which declares ctx7 (cli), context7-api-key (env) and context7-mcp (mcp).
  • Catalog text. The explainer-video and speech lines in docs/catalog.md match the head plugin.json descriptions word for word.
  • Changelog bumps. Removing a user-invocable skill is a major bump in each affected plugin's fragment. harness-ops gets a patch for its probe change. Both levels fit the changes.
  • Grader samples. In the new ran-checker and no-install samples, the regexes match the escaped JSON command form (prerequisites.mjs\" check). context7's no-install extends wave 2's anchored pattern with npm/pnpm/yarn/bun install verbs and npx/bunx/pnpx. I checked this by reading only; I did not run any grader.

Closing issue. pr-context.md has no ## Closing issue section; the PR body only refs #12488 and says it will be closed by hand. So there are no acceptance criteria to check here.

Author-claimed, not verified by me. The PR's paired eval verdict, the list of gates it says pass (validate-cases, check-matchers, generate-cheatsheet --check and so on), and the byte count against the 262,144-byte limit. I had no shell, so I ran none of them.

Spend, not a hold. The new and renamed eval cases use prepaid API credits whenever someone runs them (the PR reports about 6.36 USD for its own runs). No billing setting or billed resource changes.

There are no prior findings from this lane, so the round cap does not apply. I saw no embedded instructions in the PR context.

Chunk 2 of 4 (30 files)

Code review: chunk 2 of 4 (context7, disk-hygiene, explainer-video)

I read all 30 files in this chunk in full. For context I also read the bundled checker (lib/prerequisites.mjs), the interpreter ladder in run-python-hook.sh, and identical.txt.

1 finding (advisory, inline): the new python3 entry in plugins/disk-hygiene/prerequisites.json does not choose an interpreter the way the launcher does.

  • The checker reads the version of the first python3/python/py it finds.
  • resolve_python3 skips any interpreter that fails the version-floor probe and moves on to the next.
  • So on a machine where python3 is below 3.11 but python meets the floor, the checker reports FAIL … outdated while the guard works. The new deny text sends the model to that checker.

Checked, no issue:

  • _CHECKER_ROOT in run-python-hook.sh gives the plugin root for both registered spellings: absolute POSIX paths, and C:\…/hooks paths. It turns backslashes into forward slashes and drops double quotes, so the text stays valid inside the JSON that the context and monitor modes print.
  • Path(__file__).resolve().parents[3] in destructive_guard.py and guard_launch_monitor.py resolves to plugins/disk-hygiene. Both messages are printed through json.dumps, so the double quotes in the hint are safe.
  • The test update monitor._CHECKER_HINT builds the expected string the same way the monitor builds the message.
  • The new run-python-hook.test.sh assertion ties prerequisites.json's min to the engine's MIN_PYTHON.
  • env, mcp and reject_store_alias are kinds and options the bundled checker already supports. An optional missing CONTEXT7_API_KEY reports WARN, not FAIL, and the MCP entry reports INFO through the agent-check path.
  • I found no remaining live reference to /disk-hygiene:check, /explainer-video:check or /context7:check under plugins/; the only hits are historical CHANGELOG entries.
  • Probe competitor lists and ranking notes no longer name the deleted skills.
  • Identical copies: the disk-hygiene ck-notice-reads-checker/case.yaml and the context7 ck-read-only sample copies sit at eval-sample paths. Their path does not change what the content does.

Manifest (plugins/explainer-video/.claude-plugin/plugin.json):

  • manifest description: pass. It now names scripts/pydeps.py check instead of the deleted check skill, and the marketplace entry is updated in chunk 1.
  • manifest version: pass. Releases come from changelog fragments, and .changes/explainer-video/…-14b5d1f1.md carries bump: major for the removed skill.

Closing issue: the PR refs #12488 but closes no issue in the PR context file, so I had no acceptance criteria to check. Verification in the PR body (paired eval runs, gate results) is author-claimed; I did not run any of it.

Spend (not a hold): the new and changed eval cases spend subscription/API usage when run (up to 12 turns and 300 s per case). No billable change.

Prior findings: none (round 0 of a cap of 3).

Chunk 3 of 4 (30 files)

Code review: chunk 3 of 4 (30 files)

No findings in this chunk.

I read every hunk of the chunk's 30 files: harness-ops probes, instruction-placement, knowledge, playwright, skill-quality, and the first part of speech. That count includes the deleted check / check-prerequisites skills and their evals and probes.

What I checked

  • Dangling references. I searched the head tree for /knowledge:check, /playwright:check, /speech:check, and {skill-quality,instruction-placement,toolchain,animation}:check-prerequisites. The only remaining matches are historical CHANGELOG.md entries. Probe competitor lists drop the deleted skills: harness-ops/probes/prerequisites.json, playwright/probes/{demo-video,playwright}.json and skill-quality/probes/check.json.

  • Replacement commands exist. lib/prerequisites.{mjs,sh,ps1} is present in instruction-placement, knowledge, playwright, skill-quality and speech. So the commands now named in knowledge/skills/video-digest/SKILL.md, playwright/skills/demo-video/SKILL.md and playwright/README.md resolve. Each new scaffold.sh copies lib and prerequisites.json into ./plugin, the path the prompts name.

  • Quoted notice text matches the source. The FAIL lines in the new prompts match the degrade strings in prerequisites.json:

    • instruction-placement jq: the remove-shims text.
    • knowledge yt-dlp: the 2026.6 floor.
    • skill-quality jq: ends Then run /harness-ops:prerequisites, which is that entry's unchanged check field.

    The speech prompt matches the exit-2 text that narrate.py raises (plugins/speech/scripts/narrate.py:46-48).

  • Eval graders. ran-checker matches the JSON-escaped form prerequisites.mjs\" check. The speech variant also accepts check.py, and its no-install adds a pydeps.py install alternative. Each reports-state sample passes on a PASS row and fails on a non-answer.

  • EVAL_PLAYWRIGHT_SKIP_PYDEPS in the playwright prompt is read by plugins/playwright/hooks/install-python-deps.sh:14, as in the existing playwright cases.

  • Identical copies. The copies whose first path is in this chunk are:

    • instruction-placement case.yaml and reports-state.json, copied to skill-quality. Both cases test jq, which fits.
    • the playwright skill-fired.json copies, all at eval sample paths.

    None sits at a path where it acts differently.

Plugin manifests (plugins/speech/.claude-plugin/plugin.json)

  • manifest description: pass. The description now says check.py reports gaps rather than naming the deleted check skill. scripts/check.py exists and is the probe behind /speech:setup check (plugins/speech/skills/setup/SKILL.md:25-29). The same text is in docs/catalog.md, and the marketplace entry has no description that would override it.
  • manifest version: pass. The repository releases from changelog fragments, and .changes/speech/…-7deecd99.md declares bump: major for removing a skill. The knowledge, playwright, skill-quality and instruction-placement fragments also declare bump: major.
  • manifest keywords: pass. None of them names the removed skill.

Closing issue and context

The PR context names no closing-issue section; it only says "Refs #12488". So no acceptance criteria were checked here. The behaviour losses the PR body lists (browser and artifact-directory checks now only through /playwright:setup check, and the knowledge offer-the-fix steps) are disclosed and intentional. I don't count them as findings.

The verification figures in the PR body (paired eval runs, gate results) are claimed by the author. I did not verify them; I had no shell.

Spend

The eval cases spend prepaid API credit when run (12 turns, 300 s each); this change adds no billing.

No embedded instructions were found in the PR context or the prior-findings file. Prior findings: 0 rounds, cap 3, not past the cap.

Chunk 4 of 4 (19 files)

Code review, chunk 4 of 4 (speech, toolchain and the two registries)

I read every hunk of this chunk's 19 listed files and its 5 whole-file deletions. I found 1 advisory finding and no blocking ones.

Finding (inline):

  • plugins/speech/skills/narrate/SKILL.md:117-120: the ## Next section no longer names a skill. That breaks the successor format in .claude/rules/skill-bodies-state-current-rules.md.

What I checked and found consistent:

  • narrate.py messages: the exit-2 messages in plugins/speech/scripts/narrate.py now point at check.py by absolute path. check.py accepts being run without --data-dir, because pydeps.data_dir falls back to <config>/plugins/data/speech-*.
  • Test update: test_narrate.py now asserts on check.py and still catches a regression of that message.
  • Removed check field: dropping it from both prerequisites.json files is safe. prerequisites.mjs falls back to "Then run this check again", so the checker never names a skill that no longer exists.
  • hook-utils fallback: hook-utils.sh:549 would still produce /speech:check if it were called. The PR says nothing calls it, and the PR body defers that fallback to a follow-up.
  • Leftover references: I searched the repository for /speech:check and /toolchain:check-prerequisites. Only CHANGELOG history entries remain.
  • Skill-name registry: scripts/skill-leaf-name-registry.txt now reads check instruction-placement,skill-quality,toolchain. That matches the 3 plugins/*/skills/check/SKILL.md that remain, and no check-prerequisites skill is left.
  • toolchain README: "Four skills: three for mechanical verification" matches the 4 skill directories that remain (check, lint, ratchet, setup).
  • Eval scaffolds: the toolchain and speech scaffold.sh files copy the paths their prompts name. lib/prerequisites.mjs and scripts/check.py exist under both plugins.
  • Speech eval graders: the ran-checker and reports-state samples match the graders in the speech ck-notice-runs-own-report/case.yaml. That file is in chunk 3; I read it from the tree as context.
  • Cross-plugin source registry: the copies it lists have identical cp -R lines across plugins (checked with a search).
  • Copies left out of the chunks: the 11 copy groups in identical.txt are eval samples and case.yaml files under plugins/*/evals/. None sits at a hook, workflow, settings or manifest path, so no copy changes what its content does.

Not verified: the gate results and the paired eval run in the PR body (check-cross-plugin-source-drift, check-skill-leaf-names, check-next-graph, the speech tests, and the 10 of 11 non-inferiority verdict) are the author's claims. I had no shell and ran none of them.

Prior findings: none; this is round 1 of 3, so the round cap does not apply.

Spend: nothing in this chunk can cause a real charge. The PR's paired eval runs drew on prepaid API credit, which the author reports as about 6.36 USD.

kyle-sexton and others added 2 commits November 3, 2026 02:59
…ment

The registry policy test reads each comment block as belonging to the one
entry that follows it, so the eight ck-notice entries after the first had no
"No dedicated check" / "Canonical copy:" annotation and failed
check-cross-plugin-source-drift.test.sh. Give each entry its own block.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vcupom3BKF6QuC5U3sRJPj
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KeBVgcHpe4YpFuL5b24Ft2
@kyle-sexton

Copy link
Copy Markdown
Member Author

CI fix pushed by PR Lane Automation, on top of 32da30a (no commits of yours were rewritten).

  • bef394a fix(scripts): annotate each ck-notice registry entry with its enforcement: test-bash (3) failed in scripts/check-cross-plugin-source-drift.test.sh ("registry policy gaps"). The policy test reads each comment block as applying only to the one entry right after it, so the eight evals/ck-notice-* entries after the first had no No dedicated check / Canonical copy: annotation. Each entry now has its own block; the canonical copies are unchanged (toolchain for ck-notice-reads-checker, speech for ck-notice-runs-own-report). Failed run
  • f566bbb Merge origin/main: the pre-push stale-base-overlap check blocked the first push, so fresh-push.sh merged main in.

Local: the drift test passes 15/15 and check-cross-plugin-source-drift.sh --check reports no unregistered or drifted clusters. ci-status and ci-lanes failed only because of test-bash (3). The 0s test-bash/test-python failures in run 41592945254 were a cancelled, superseded run.

I did not touch the five open review threads (the py launcher restatement, the ## Next sections in explainer-video produce and speech narrate, and the Python ladder mismatch in disk-hygiene/prerequisites.json). They are still yours to settle.

- explainer-video produce: keep the decision not to launch through py and
  point at the Python docs section instead of restating what a py launch does.
- explainer-video produce and speech narrate: name /harness-ops:prerequisites,
  the fleet prerequisites report, as the successor in ## Next now that the
  per-plugin check skills are gone.
- disk-hygiene prerequisites.json: the checker judges only the first
  interpreter found (detect.any is first-match-wins in the prerequisites
  contract), while the guard tries python3, python, then py -3 against its
  floor. The degrade text now says so.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vcupom3BKF6QuC5U3sRJPj
@kyle-sexton

Copy link
Copy Markdown
Member Author

Review threads settled by PR Lane Automation in fde3192, pushed on top of f566bbb (no force push).

  • py launcher restatement (explainer-video produce): the body keeps the decision never to start through py and links the Python docs section for why, without restating what a py launch does.
  • ## Next sections (explainer-video produce, speech narrate): both now name /harness-ops:prerequisites, the fleet prerequisites report that replaces the deleted per-plugin check skills. The readiness commands stay in the recovery sections.
  • Python check in disk-hygiene prerequisites.json: the hook is unchanged, per the hub's decision. The prerequisites contract defines detect.any as first match wins, so the checker cannot try each interpreter until one passes. The entry keeps the hook's order (python3, python, py), and its degrade text now says the check judges only the first interpreter on PATH while the guard tries each in turn.

Local checks: check-changed-skills.sh passed 5 skills with 0 errors, check-next-graph.py passed, check-declared-prerequisites and its 48 tests passed, the disk-hygiene prerequisites check passed, and markdownlint found 0 issues. All five threads have a reply and are resolved.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant