From d21d0a12ae236025d7bb074a6fb0692a068418a9 Mon Sep 17 00:00:00 2001 From: Mikhail Novikov Date: Thu, 17 Sep 2026 07:06:49 +0000 Subject: [PATCH 1/4] fix(skill-review): stop requiring client-specific TodoWrite and Task Preflight aborted the review when TodoWrite or Task was missing. Newer models ship without a built-in task-tracking tool, so the skill could not start at all. File read is now the only hard requirement; task tracking and sub-agent delegation are optional capabilities with declared fallbacks. WF12/WF13 and Bingo #21 catch the same lock-in in reviewed skills. Co-Authored-By: Claude Opus 5 (1M context) --- .../skill-review/.claude-plugin/plugin.json | 2 +- plugins/skill-review/CHANGELOG.md | 19 +++++++ .../skill-review/skills/skill-review/SKILL.md | 51 +++++++++++++------ .../references/antipattern-bingo.md | 3 ++ .../references/checklist-workflow.md | 18 +++++-- .../references/instruction-singlepass.md | 21 +++++--- 6 files changed, 85 insertions(+), 29 deletions(-) diff --git a/plugins/skill-review/.claude-plugin/plugin.json b/plugins/skill-review/.claude-plugin/plugin.json index 261e8de..c2e10ba 100644 --- a/plugins/skill-review/.claude-plugin/plugin.json +++ b/plugins/skill-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "skill-review", "description": "Quick AI reviewer for Agent Skills: checks structure, workflow, references, and links. Clear report without jargon, with a summary from an exhausted data scientist.", - "version": "1.0.0", + "version": "1.1.0", "author": { "name": "mindbox.cloud" } diff --git a/plugins/skill-review/CHANGELOG.md b/plugins/skill-review/CHANGELOG.md index f49758f..37b9c16 100644 --- a/plugins/skill-review/CHANGELOG.md +++ b/plugins/skill-review/CHANGELOG.md @@ -1,5 +1,24 @@ # Changelog — skill-review +## [1.1.0] — 2026-09-17 + +### Changed + +- Planning no longer depends on a client-specific tool. The preflight used to stop the + review when `TodoWrite` or `Task` was missing — newer models ship without a built-in + task-tracking tool, so the skill could not start at all. Now only file read is a hard + requirement; task tracking and sub-agent delegation are optional capabilities with + declared fallbacks (plan as a markdown checklist in chat; single-pass regardless of + volume when there is no sub-agent tool). +- Review plan gained an explicit completion gate: no final report while plan items are open. +- WF12 extended with a portability check of the planning mechanism, WF13 reworded in terms + of open plan items instead of one tool's status names. + +### Added + +- Antipattern Bingo #21 "Client tool lock-in" (stage 2, WF12, owner Workflow) — a skill tied + to a concrete client tool with no fallback. + ## [1.0.0] — 2026-05-26 ### Added diff --git a/plugins/skill-review/skills/skill-review/SKILL.md b/plugins/skill-review/skills/skill-review/SKILL.md index 3fc1acf..e596f0b 100644 --- a/plugins/skill-review/skills/skill-review/SKILL.md +++ b/plugins/skill-review/skills/skill-review/SKILL.md @@ -12,7 +12,7 @@ description: > user asks to create a new skill, to review code, user asks to review a prompt that is not a skill. metadata: - version: 1.6.0 + version: 1.7.0 --- # Skill Review Novice — Autonomous Instruction for the Chat Agent @@ -25,23 +25,32 @@ metadata: ## Preconditions -1. Before Step 0, check `Task`, `TodoWrite`, and file access for the skill. If anything is missing — stop and report what is lacking. -2. If the user selects logging **ON** in Step 0, verify file write capability. If write is unavailable — stop and suggest disabling logging. +1. Before Step 0, check that you can **read the files** of the skill under review. If read access is unavailable — stop and report what is lacking. +2. Check which optional client capabilities you have and record the result — **neither is a hard requirement**, each has a fallback: + +| Capability | Tool names across clients | If unavailable | +|:---|:---|:---| +| Sub-agent delegation | `Agent`, `Task` or equivalent | Run the review in **single-pass** mode regardless of volume and note this in `Review Limitations` | +| Task tracking | `TodoWrite` or equivalent | Keep the review plan as a markdown checklist in chat (see `## Mandatory Review Plan`) | + +> Tool availability differs between clients and models — newer models are shipped without a built-in task-tracking tool. A missing optional tool changes **how** the review is executed; it never cancels the review. + +3. If the user selects logging **ON** in Step 0, verify file write capability. If write is unavailable — stop and suggest disabling logging. --- ## CRITICAL — Mandatory Rules - **Questions to the user — only in Step 0**. Mandatory question: context of use + logging. Ask for log path confirmation **only if** the user enabled logging. -- **A TODO plan is MANDATORY** from the very start of the review. -- The TODO plan must cover the full review scope. For every 3–4 check items there should be a separate TODO item. Do not create one giant TODO for the entire review. -- Update statuses as the review progresses: `pending` → `in_progress` → `completed`. +- **A review plan is MANDATORY** from the very start of the review. Keep it in the client's task-tracking tool if there is one; otherwise as a markdown checklist in chat. The absence of such a tool is never a reason to skip planning. +- The plan must cover the full review scope. For every 3–4 check items there should be a separate plan item. Do not create one giant item for the entire review. +- Update statuses as the review progresses: `pending` → `in_progress` → `completed` in the tool, or `[ ]` → `[~]` → `[x]` in the chat checklist. - If the folder is not a valid skill directory (no `SKILL.md`) or contains more than one skill — see `## Troubleshooting`, problem 1. Do not abort the check silently. - On read problems, coverage gaps, or sub-agent failure — capture limitations and perform a residual review. On write problems with `logs=on` — notify the user and stop the review. -- In sub-agent mode (`>= 500` lines), the orchestrator must delegate checklist checks to sub-agents via `Task`. Running a full review in the main context instead of launching sub-agents is a workflow violation. Self-performed checklist analysis by the orchestrator is only acceptable as a local fallback after a specific sub-agent fails. +- In sub-agent mode (`>= 500` lines), the orchestrator must delegate checklist checks to sub-agents via the client's sub-agent tool (`Agent`, `Task` or equivalent). Running a full review in the main context instead of launching sub-agents is a workflow violation — unless the client has no sub-agent tool at all (see `## Preconditions`). Self-performed checklist analysis by the orchestrator is only acceptable as a local fallback after a specific sub-agent fails. - **Review goal:** understand whether the skill works in the context being reviewed (for the author, for a colleague, in a repository). - **Novice does not compute a maturity stage.** Scope is determined by the user's choice. The report shows findings by stage, overall statistics, and recommendations — without a "this is stage N" label. -- **Language:** conduct the entire review (report, logs, TODO, sub-agent briefs) in the language the user started the conversation in. +- **Language:** conduct the entire review (report, logs, review plan, sub-agent briefs) in the language the user started the conversation in. - **Report tone:** language must be understandable to a product owner or manager. Avoid technical checklist jargon. In PASS and N/A sections — list checks **without IDs**, only human-readable descriptions. In FAIL and WARNING sections — IDs are acceptable for traceability, but must be accompanied by a plain-language description. --- @@ -103,9 +112,19 @@ The orchestrator captures **two distinct entities**: --- -## Mandatory TODO Plan +## Mandatory Review Plan + +Create a preliminary plan immediately after the user's response and refine it after collecting the manifest and choosing the mode. Break the review into blocks so that one plan item covers approximately 3–4 checks, not the entire document. Update statuses as work proceeds. -Create a preliminary TODO immediately after the user's response and refine it after collecting the manifest and choosing the mode. Break the review into blocks so that one TODO covers approximately 3–4 checks, not the entire document. Update statuses as work proceeds. +**Where the plan lives:** + +| Client capability | Where the plan lives | How statuses are updated | +|:---|:---|:---| +| Task-tracking tool available | In the tool | `pending` → `in_progress` → `completed` | +| No task-tracking tool | Markdown checklist in chat: publish it before the first check | `[ ]` → `[~]` → `[x]`, repost the updated checklist at every phase boundary | +| No task-tracking tool, logging ON | Same chat checklist, additionally saved as `review-plan.md` in the results folder | Rewrite the file after every completed block | + +**Completion gate:** do not produce the final report while any plan item is still open. An item is closed either by a verdict or by an explicit entry in `Review Limitations`. **Example of a good breakdown:** ```text @@ -124,7 +143,7 @@ Create a preliminary TODO immediately after the user's response and refine it af 3a. Check file write 3b. Get timestamp from command line (date +%Y%m%d-%H%M or equivalent) 3c. Show user the full path to the results folder, await confirmation -4. Create the preliminary TODO plan for the review +4. Create the preliminary review plan (see "Mandatory Review Plan") 5. Collect the manifest of the skill under review: file list, sizes, line counts, frontmatter, presence of references/, scripts/, assets/. The goal of this step is routing and passing to sub-agents; do not perform checklist content analysis at this step. @@ -132,18 +151,18 @@ Create a preliminary TODO immediately after the user's response and refine it af 7. Count the total lines of all readable files in the skill under review 8. Choose the mode based on the threshold (see "Execution Mode Selection") 9. IF SINGLE-PASS (< 500 lines): - 9a. Refine the TODO plan for compact single-pass review + 9a. Refine the review plan for compact single-pass review 9b. Read references/instruction-singlepass.md 9c. Execute the entire review sequentially in the main context 9d. If logging ON — write report.md to the confirmed folder 10. IF SUB-AGENT (>= 500 lines): - 10a. Refine the TODO plan for sub-agent review + 10a. Refine the review plan for sub-agent review 10b. If logging ON — create the results folder: {skill-name}-review-{YYYYMMDD-HHMM}/ 10c. Using the "scope → sub-agents" matrix, determine which sub-agents to launch 10d. Launch the required sub-agents; pass scope and checklist mapping to each. - Each selected sub-agent is launched as a separate Task call. + Each selected sub-agent is launched as a separate call of the sub-agent tool. Do not combine multiple sub-agents in one prompt. - Parallel launch of multiple separate Task calls is allowed. + Parallel launch of multiple separate sub-agent calls is allowed. 10e. Collect condensed summaries (and temp_log_path if direct log write to the output folder failed) 10f. Run the verification gate: - Count total FAIL / WARNING across all summaries. @@ -212,6 +231,8 @@ After collecting the manifest, count the total number of lines **in all files of | **< 500 lines** | **Single-pass** | Read `references/instruction-singlepass.md` and run the entire review in the main context, without sub-agents | | **>= 500 lines** | **Sub-agent** | Launch sub-agents per the scope matrix (logic below) | +> If the client provides no sub-agent tool, run single-pass regardless of volume: read `references/instruction-singlepass.md`, execute the checks sequentially in the main context, and record in `Review Limitations` that the review ran without delegation. + --- ## "Scope → Sub-agents" Matrix (sub-agent mode only) diff --git a/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md b/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md index a0694a8..10e5d1b 100644 --- a/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md +++ b/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md @@ -60,6 +60,7 @@ For **each** antipattern, choose one of four verdicts: | 18 | **Lifecycle hygiene gap (rot risk)** | 4 | LC03 | **Lifecycle** | | 19 | **Silent chain failures** | 2 | WF11 | **Workflow** | | 20 | **Monolithic reference dump** | 3 | RF06, RF12 | **References** | +| 21 | **Client tool lock-in** | 2 | WF12 | **Workflow** | --- @@ -67,6 +68,7 @@ For **each** antipattern, choose one of four verdicts: - **#15 Schema drift risk** vs **#13 Mirroring MCP schema**: both use WF22, but differently. #13 is a structural fact (a schema copy lives in SKILL.md). #15 is a lifecycle risk (the schema version is not pinned, drift is not tracked). - **#16 Context overfitting** vs **#12 Hardcoded paths**: #12 is a concrete mechanical signal (absolute paths). #16 is a broader pattern (coupling to OS, permissions, environment, implicit requirements). If the only signal is hardcoded paths, do not duplicate the verdict. +- **#21 Client tool lock-in**: the skill is tied to a concrete tool of one client (`TodoWrite`, `Task` and the like) with no fallback for a client that does not provide it. `CRITICAL` if a preflight stops the skill over the missing tool; `MINOR` if the skill merely assumes the tool without a fallback. Tool availability differs between clients and models — a tool present today may be absent in the next model. - **#16 in novice mode:** checked **partially** — only by mechanical portability signals (WF15: preconditions, OS, permissions, packages). Deep overfitting analysis (IN09–IN14: hidden assumptions, self-sufficiency, implicit knowledge) is only available in skill-review-nightmare with a full Intern walkthrough. --- @@ -95,3 +97,4 @@ For **each** antipattern, choose one of four verdicts: | 18 | Lifecycle hygiene gap (rot risk) | 4 | [NONE / MINOR / CRITICAL / NOT_CHECKED] | [if MINOR: 3–4 words, otherwise `—`] | | 19 | Silent chain failures | 2 | [NONE / MINOR / CRITICAL / NOT_CHECKED] | [if MINOR: 3–4 words, otherwise `—`] | | 20 | Monolithic reference dump | 3 | [NONE / MINOR / CRITICAL / NOT_CHECKED] | [if MINOR: 3–4 words, otherwise `—`] | +| 21 | Client tool lock-in | 2 | [NONE / MINOR / CRITICAL / NOT_CHECKED] | [if MINOR: 3–4 words, otherwise `—`] | diff --git a/plugins/skill-review/skills/skill-review/references/checklist-workflow.md b/plugins/skill-review/skills/skill-review/references/checklist-workflow.md index 76b35e6..b6c068a 100644 --- a/plugins/skill-review/skills/skill-review/references/checklist-workflow.md +++ b/plugins/skill-review/skills/skill-review/references/checklist-workflow.md @@ -51,13 +51,21 @@ *Context:* Without checkpoints, the agent continues the workflow on a silent step failure — this is "silent chain failures". A good skill does not just list steps — it sets conditions: what must be true before proceeding. -**WF12** If the workflow is longer than **4 steps** — do the instructions explicitly declare a **planning tool**, task list, or external planning artifact? +**WF12** If the workflow is longer than **4 steps** — do the instructions explicitly declare a **planning mechanism**: a task-tracking tool, a task list, or an external planning artifact? *Context:* In long sessions, the agent easily loses its plan and starts jumping between tasks. Anthropic recommends structured note-taking / agentic memory: an explicit task list that maintains state between tool calls. -**WF13** If planning is used, is there an **enforcement gate**: completion is not allowed while there are `pending` or `in_progress` tasks? +**Additional check — portability of the planning mechanism.** If the skill names a concrete client tool (`TodoWrite` and the like), does it survive a client that does not provide that tool? Tool availability differs between clients and models: a tool present today may be absent in the next model. -*Context:* The most effective way to make planning mandatory is to prohibit completion with unclosed tasks. Otherwise the TODO list remains decorative and does not prevent context loss. +| Signal | Verdict | +|---|---| +| Preflight stops the skill because a client-specific tool is missing | FAIL — the skill does not start at all | +| Planning is tied to one tool, no fallback described | WARNING — planning silently disappears | +| Planning is described as a capability with a fallback (tool, or checklist in chat, or a file) | PASS | + +**WF13** If planning is used, is there an **enforcement gate**: completion is not allowed while plan items remain open? + +*Context:* The most effective way to make planning mandatory is to prohibit completion with unclosed tasks. Otherwise the plan remains decorative and does not prevent context loss. The gate must be worded in terms of open plan items, not in terms of the status names of one specific tool — otherwise it evaporates in a client without that tool. **WF14** Are critical rules, prohibitions, and stop conditions at the beginning of the skill or under explicit `CRITICAL` headers, not buried in the middle of a long text? @@ -149,7 +157,7 @@ > **Stage: 2** | Axis: personal use (if workflow is multi-step with multiple tools / co-skills) -**WF27** If the workflow passes through several tools / co-skills, is there an external planning artifact or TODO list that survives the handoff between steps? +**WF27** If the workflow passes through several tools / co-skills, is there an external planning artifact or task list that survives the handoff between steps? *Context:* In a multi-skill environment, task state is most often lost precisely at transitions between steps and tools. An external planning artifact maintains dependent steps, execution status, and blockers that would otherwise dissolve into the thread history. @@ -173,7 +181,7 @@ | Context-refresh: re-reading the plan before each new phase | WARNING | | Strategy for context overflow (compaction, sub-agent, respawn) | INFO (if < 10 steps), WARNING (if >= 10) | | Checkpoints with state capture | WARNING (for critical phases) | -| TODO list as attention management (updated during the work) | INFO | +| Task list as attention management (updated during the work) | INFO | **Scale:** - **PASS:** there is an explicit context management strategy (files, checkpoints, refresh) diff --git a/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md b/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md index a0538fe..8157b6b 100644 --- a/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md +++ b/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md @@ -8,16 +8,16 @@ > This mode assumes the orchestrator has already run the preflight of the parent skill. -- `TodoWrite` and file access are expected to be available. +- **File read access is required.** If it is unexpectedly unavailable, do not start the review and immediately notify the user that single-pass cannot start in the current client. - If the parent orchestrator enabled logging, file write is additionally required. -- If any of these capabilities is unexpectedly unavailable, do not start the review and immediately notify the user that single-pass cannot start in the current client. +- A task-tracking tool (`TodoWrite` or equivalent) is **optional**. If the client does not provide one — keep the review plan as a markdown checklist in chat. This is not a reason to stop. --- ## Mandatory Rules - **No questions to the user.** Review parameters (scope, declared target, logging) were already determined by the orchestrator in Step 0. -- Create a **TODO plan** for the review: one TODO item per 3–4 checks. +- Create a **review plan**: one item per 3–4 checks. Use the client's task-tracking tool if there is one; otherwise publish the plan as a markdown checklist in chat and repost it updated at every phase boundary. Do not produce the final report while any plan item is still open. - Evaluate **only by observable artifacts** — do not infer what is not present in the files. - Do not count as PASS any runs, stability, or lifecycle maturity without file confirmation. - If a section is not applicable (no `references/`, `scripts/`, MCP, sub-agents) — mark **N/A**, not FAIL. @@ -49,7 +49,7 @@ Checks outside scope are marked **"not checked for selected scope"** (not N/A an ## Algorithm ```text -1. Create the TODO plan for the review (compact, no sub-agents) +1. Create the review plan (compact, no sub-agents) 2. Sequentially run all checks by scope: Part A → Part B → Part C → Part D → [Part E if scope is full] 3. Fill Antipattern Bingo (Part F) @@ -211,13 +211,15 @@ Checks outside scope are marked **"not checked for selected scope"** (not N/A an *Context:* Without checkpoints, the agent continues the workflow on a silent step failure — "silent chain failures". A good skill does not just list steps — it sets conditions: what must be true before proceeding. -**WF12.** If the workflow has > 4 steps — is there a **planning tool**, task list, or external planning artifact? +**WF12.** If the workflow has > 4 steps — is there a **planning mechanism**: a task-tracking tool, task list, or external planning artifact? *Context:* In long sessions, the agent easily loses its plan. Anthropic recommends structured note-taking / agentic memory: an explicit task list that maintains state between tool calls. -**WF13.** If planning is used — is there an **enforcement gate**: completion is not allowed while there are `pending`/`in_progress` tasks? +**Portability of the planning mechanism:** if the skill names a specific client tool (`TodoWrite` and the like) as a hard requirement — is there a fallback for a client that does not provide it? Tool availability differs between clients and models. A preflight that stops the skill over a missing client-specific tool — **FAIL** (the skill does not start at all); a planning instruction with no fallback — **WARNING** (planning silently disappears). -*Context:* The most effective way to make planning mandatory is to prohibit completion with unclosed tasks. Otherwise the TODO list remains decorative and does not prevent context loss. +**WF13.** If planning is used — is there an **enforcement gate**: completion is not allowed while plan items remain open? + +*Context:* The most effective way to make planning mandatory is to prohibit completion with unclosed tasks. Otherwise the plan remains decorative and does not prevent context loss. The gate must be worded in terms of open plan items, not in terms of one tool's status names — otherwise it evaporates in a client without that tool. **WF14.** Are critical rules and prohibitions at the beginning of the skill or under `CRITICAL` headers, not buried in the middle? @@ -289,7 +291,7 @@ Checks outside scope are marked **"not checked for selected scope"** (not N/A an *Context:* Checkpoints matter not only within a single skill (WF11), but also at handoffs between tools and co-skills. Task state is most often lost at these transitions. -**WF27.** If the workflow passes through several tools / co-skills — is there an **external planning artifact / TODO** that survives the handoff? +**WF27.** If the workflow passes through several tools / co-skills — is there an **external planning artifact / task list** that survives the handoff? *Context:* An external planning artifact maintains dependent steps, execution status, and blockers that would otherwise dissolve into the thread history. @@ -528,12 +530,14 @@ Evaluate **only by observable artifacts**. For `MINOR`, the note is short: `trig | 18 | **Lifecycle hygiene gap (rot risk)** | 4 | LC03 | | 19 | **Silent chain failures** | 2 | WF11 | | 20 | **Monolithic reference dump** | 3 | RF06, RF12 | +| 21 | **Client tool lock-in** | 2 | WF12 | ### Notes - **#15 vs #13:** both use WF22, but #13 is a structural fact (schema copy), #15 is a lifecycle risk (drift). - **#16 vs #12:** #12 is mechanical (absolute paths), #16 is broader (coupling to OS, permissions, environment). Do not duplicate the verdict. - **#16 in novice mode:** checked partially — only by mechanical portability signals (WF15: preconditions, OS, permissions). +- **#21 Client tool lock-in:** the skill is tied to a concrete tool of one client (`TodoWrite`, `Task` and the like) with no fallback. `CRITICAL` if a preflight stops the skill over the missing tool; `MINOR` if the tool is merely assumed without a fallback. ### Bingo Table for Report @@ -559,6 +563,7 @@ Evaluate **only by observable artifacts**. For `MINOR`, the note is short: `trig | 18 | Lifecycle hygiene gap (rot risk) | 4 | | | | 19 | Silent chain failures | 2 | | | | 20 | Monolithic reference dump | 3 | | | +| 21 | Client tool lock-in | 2 | | | --- From bbf9afcade083314ae3dbd40303b6585d351c49b Mon Sep 17 00:00:00 2001 From: Mikhail Novikov Date: Thu, 17 Sep 2026 07:23:20 +0000 Subject: [PATCH 2/4] fix(skill-review): align single-pass mode with the new fallbacks Self-review of the branch found the new fallbacks half-landed: instruction-singlepass.md still claimed it runs only below 500 lines, its algorithm never wrote review-plan.md, and WF12 lost the PASS row its checklist twin has. Shell execution, required for the log timestamp, was missing from the capability table that claims to enumerate what the review needs. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/skill-review/CHANGELOG.md | 3 +++ .../skill-review/skills/skill-review/SKILL.md | 1 + .../references/instruction-singlepass.md | 18 ++++++++++++++---- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/plugins/skill-review/CHANGELOG.md b/plugins/skill-review/CHANGELOG.md index 37b9c16..e3cca80 100644 --- a/plugins/skill-review/CHANGELOG.md +++ b/plugins/skill-review/CHANGELOG.md @@ -11,6 +11,9 @@ declared fallbacks (plan as a markdown checklist in chat; single-pass regardless of volume when there is no sub-agent tool). - Review plan gained an explicit completion gate: no final report while plan items are open. +- Single-pass mode now knows it can be activated by the absence of a sub-agent tool, not only by + volume: it gets a reading strategy for a large skill in one context and an obligation to record + the missing delegation in `Review Limitations`. - WF12 extended with a portability check of the planning mechanism, WF13 reworded in terms of open plan items instead of one tool's status names. diff --git a/plugins/skill-review/skills/skill-review/SKILL.md b/plugins/skill-review/skills/skill-review/SKILL.md index e596f0b..8bbc677 100644 --- a/plugins/skill-review/skills/skill-review/SKILL.md +++ b/plugins/skill-review/skills/skill-review/SKILL.md @@ -32,6 +32,7 @@ metadata: |:---|:---|:---| | Sub-agent delegation | `Agent`, `Task` or equivalent | Run the review in **single-pass** mode regardless of volume and note this in `Review Limitations` | | Task tracking | `TodoWrite` or equivalent | Keep the review plan as a markdown checklist in chat (see `## Mandatory Review Plan`) | +| Shell command execution | `Bash` or equivalent | Needed only with logging ON, for the timestamp. Ask the user for the current timestamp together with the log path in Step 0 | > Tool availability differs between clients and models — newer models are shipped without a built-in task-tracking tool. A missing optional tool changes **how** the review is executed; it never cancels the review. diff --git a/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md b/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md index 8157b6b..ce3f8ab 100644 --- a/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md +++ b/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md @@ -1,6 +1,8 @@ # Skill Review — Single-pass Mode -> **Activation context:** This mode was activated by the orchestrator because the total volume of the skill under review is < 500 lines. All checks are executed in one context without sub-agents. The set of checks, report format, and Bingo are **identical** to sub-agent mode. +> **Activation context:** This mode was activated by the orchestrator for one of two reasons: the total volume of the skill under review is < 500 lines, **or** the client provides no sub-agent tool and delegation is impossible at any volume. All checks are executed in one context without sub-agents. The set of checks, report format, and Bingo are **identical** to sub-agent mode. +> +> **If the mode was activated for the second reason** (volume `>= 500` lines, no delegation): read the files of the skill under review in parts, by check group, instead of pulling everything into context at once; after each completed Part, keep only the verdicts and drop the raw text; and record in `Review Limitations` that the review ran without delegation because the client has no sub-agent tool. --- @@ -54,7 +56,8 @@ Checks outside scope are marked **"not checked for selected scope"** (not N/A an Part A → Part B → Part C → Part D → [Part E if scope is full] 3. Fill Antipattern Bingo (Part F) 4. Generate the final report (Part G) -5. If logging ON — write report.md; if logging OFF — do not write any files +5. If logging ON — write report.md (and review-plan.md, if the plan is kept as a chat checklist); + if logging OFF — do not write any files ``` --- @@ -215,7 +218,13 @@ Checks outside scope are marked **"not checked for selected scope"** (not N/A an *Context:* In long sessions, the agent easily loses its plan. Anthropic recommends structured note-taking / agentic memory: an explicit task list that maintains state between tool calls. -**Portability of the planning mechanism:** if the skill names a specific client tool (`TodoWrite` and the like) as a hard requirement — is there a fallback for a client that does not provide it? Tool availability differs between clients and models. A preflight that stops the skill over a missing client-specific tool — **FAIL** (the skill does not start at all); a planning instruction with no fallback — **WARNING** (planning silently disappears). +**Portability of the planning mechanism:** if the skill names a specific client tool (`TodoWrite` and the like), does it survive a client that does not provide it? Tool availability differs between clients and models. + +| Signal | Verdict | +|---|---| +| Preflight stops the skill because a client-specific tool is missing | FAIL — the skill does not start at all | +| Planning is tied to one tool, no fallback described | WARNING — planning silently disappears | +| Planning is described as a capability with a fallback (tool, or checklist in chat, or a file) | PASS | **WF13.** If planning is used — is there an **enforcement gate**: completion is not allowed while plan items remain open? @@ -309,6 +318,7 @@ Checks outside scope are marked **"not checked for selected scope"** (not N/A an | Context-refresh: re-reading plan before each phase | WARNING | | Strategy for context overflow | INFO (< 10 steps), WARNING (>= 10) | | Checkpoints with state capture | WARNING (for critical phases) | +| Task list as attention management (updated during the work) | INFO | - **PASS:** explicit context management strategy is present - **WARNING:** workflow is long (>= 5 steps), none of the signals above are present @@ -686,7 +696,7 @@ Evaluate **only by observable artifacts**. For `MINOR`, the note is short: `trig ### Formation Rules - Do not invent confirmations; do not inflate severity. -- If the review had limitations (unreadable files, invalid artifact) — the `Review Limitations` section is mandatory. +- If the review had limitations (unreadable files, invalid artifact, review run without delegation) — the `Review Limitations` section is mandatory. - Group findings by stages 2/3/4. Each section has three states: has findings / no issues / not checked. - Evidence: `file § section`. Line numbers — only a hint. - Top 3 — concrete actions, not abstractions. From 9237e7625890ca3ab1d8be6b1ea42d97bb71390b Mon Sep 17 00:00:00 2001 From: Aleksandr Korchak Date: Thu, 17 Sep 2026 22:25:30 +0300 Subject: [PATCH 3/4] refactor(skill-review): make the review plan a copyable chat checklist Replaces the capability/fallback tables with a single planning mechanism: a markdown checklist copied into the response, the pattern the Anthropic skill authoring guide recommends. No client tool is named anywhere in the skill now, so nothing goes stale when a client renames or gates a tool. - Preconditions keep file read as the only hard requirement - SKILL.md and instruction-singlepass.md each ship a ready-made plan skeleton to copy, with an explicit instruction to check items off - WF12 asks for a planning mechanism that survives a client without a given tool; its signal/verdict table is folded into the question so the check still yields a single verdict - Bingo #21 note reworded without tool names and disambiguated against #16 - Drop the review-plan.md artifact and the invented [~] status Fixed along the way: with logging on, the results folder is created right after the user confirms its path, so single-pass mode has somewhere to write report.md. Previously it was created only on the sub-agent branch. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/skill-review/CHANGELOG.md | 34 +++++----- .../skill-review/skills/skill-review/SKILL.md | 63 +++++++++---------- .../references/antipattern-bingo.md | 2 +- .../references/checklist-workflow.md | 14 +---- .../references/instruction-singlepass.md | 43 +++++++------ 5 files changed, 77 insertions(+), 79 deletions(-) diff --git a/plugins/skill-review/CHANGELOG.md b/plugins/skill-review/CHANGELOG.md index e3cca80..d053efc 100644 --- a/plugins/skill-review/CHANGELOG.md +++ b/plugins/skill-review/CHANGELOG.md @@ -4,23 +4,29 @@ ### Changed -- Planning no longer depends on a client-specific tool. The preflight used to stop the - review when `TodoWrite` or `Task` was missing — newer models ship without a built-in - task-tracking tool, so the skill could not start at all. Now only file read is a hard - requirement; task tracking and sub-agent delegation are optional capabilities with - declared fallbacks (plan as a markdown checklist in chat; single-pass regardless of - volume when there is no sub-agent tool). -- Review plan gained an explicit completion gate: no final report while plan items are open. -- Single-pass mode now knows it can be activated by the absence of a sub-agent tool, not only by - volume: it gets a reading strategy for a large skill in one context and an obligation to record - the missing delegation in `Review Limitations`. -- WF12 extended with a portability check of the planning mechanism, WF13 reworded in terms - of open plan items instead of one tool's status names. +- The review no longer depends on a client-specific tool. The preflight used to stop the + review when `TodoWrite` or `Task` was missing, so the skill could not start at all in a + client that does not offer them. File read is the only hard requirement now. +- The review plan is always a markdown checklist in the chat. Both execution modes now ship a + ready-made skeleton to copy into the response and check off as the review proceeds — the + pattern Anthropic's skill authoring guide recommends. It works in any client and names no tool. +- The review plan gained an explicit completion gate: no final report while plan items are open. +- Single-pass mode can now also be activated by the absence of a sub-agent tool, not only by + volume: it gets a reading strategy for a large skill in one context and an obligation to + record the missing delegation in `Review Limitations`. +- WF12 now asks for a planning mechanism that survives a client without a given tool. + WF13 is worded in terms of open plan items instead of one tool's status names. ### Added -- Antipattern Bingo #21 "Client tool lock-in" (stage 2, WF12, owner Workflow) — a skill tied - to a concrete client tool with no fallback. +- Antipattern Bingo #21 "Client tool lock-in" (stage 2, WF12, owner Workflow) — the skill names + a tool of one client and describes no way to work without it. + +### Fixed + +- With logging on, the results folder is now created right after the user confirms its path, + so single-pass mode has somewhere to write `report.md`. Previously the folder was created + only on the sub-agent branch. ## [1.0.0] — 2026-05-26 diff --git a/plugins/skill-review/skills/skill-review/SKILL.md b/plugins/skill-review/skills/skill-review/SKILL.md index 8bbc677..f251898 100644 --- a/plugins/skill-review/skills/skill-review/SKILL.md +++ b/plugins/skill-review/skills/skill-review/SKILL.md @@ -26,29 +26,22 @@ metadata: ## Preconditions 1. Before Step 0, check that you can **read the files** of the skill under review. If read access is unavailable — stop and report what is lacking. -2. Check which optional client capabilities you have and record the result — **neither is a hard requirement**, each has a fallback: +2. If the user selects logging **ON** in Step 0, verify file write capability. If write is unavailable — stop and suggest disabling logging. -| Capability | Tool names across clients | If unavailable | -|:---|:---|:---| -| Sub-agent delegation | `Agent`, `Task` or equivalent | Run the review in **single-pass** mode regardless of volume and note this in `Review Limitations` | -| Task tracking | `TodoWrite` or equivalent | Keep the review plan as a markdown checklist in chat (see `## Mandatory Review Plan`) | -| Shell command execution | `Bash` or equivalent | Needed only with logging ON, for the timestamp. Ask the user for the current timestamp together with the log path in Step 0 | - -> Tool availability differs between clients and models — newer models are shipped without a built-in task-tracking tool. A missing optional tool changes **how** the review is executed; it never cancels the review. - -3. If the user selects logging **ON** in Step 0, verify file write capability. If write is unavailable — stop and suggest disabling logging. +> Do not gate the review on the presence of a named client tool. Planning always works (see `## Mandatory Review Plan`); a missing sub-agent tool only changes the execution mode (see `## Execution Mode Selection`). --- ## CRITICAL — Mandatory Rules - **Questions to the user — only in Step 0**. Mandatory question: context of use + logging. Ask for log path confirmation **only if** the user enabled logging. -- **A review plan is MANDATORY** from the very start of the review. Keep it in the client's task-tracking tool if there is one; otherwise as a markdown checklist in chat. The absence of such a tool is never a reason to skip planning. +- **A review plan is MANDATORY** from the very start of the review — as a markdown checklist in the chat. - The plan must cover the full review scope. For every 3–4 check items there should be a separate plan item. Do not create one giant item for the entire review. -- Update statuses as the review progresses: `pending` → `in_progress` → `completed` in the tool, or `[ ]` → `[~]` → `[x]` in the chat checklist. +- Check items off (`[ ]` → `[x]`) as they close and repost the updated checklist at every phase boundary. +- **Do not produce the final report while any plan item is still open.** - If the folder is not a valid skill directory (no `SKILL.md`) or contains more than one skill — see `## Troubleshooting`, problem 1. Do not abort the check silently. - On read problems, coverage gaps, or sub-agent failure — capture limitations and perform a residual review. On write problems with `logs=on` — notify the user and stop the review. -- In sub-agent mode (`>= 500` lines), the orchestrator must delegate checklist checks to sub-agents via the client's sub-agent tool (`Agent`, `Task` or equivalent). Running a full review in the main context instead of launching sub-agents is a workflow violation — unless the client has no sub-agent tool at all (see `## Preconditions`). Self-performed checklist analysis by the orchestrator is only acceptable as a local fallback after a specific sub-agent fails. +- In sub-agent mode (`>= 500` lines), the orchestrator must delegate checklist checks to sub-agents. Running a full review in the main context instead of launching sub-agents is a workflow violation — unless the client provides no sub-agent tool at all (see `## Execution Mode Selection`). Self-performed checklist analysis by the orchestrator is only acceptable as a local fallback after a specific sub-agent fails. - **Review goal:** understand whether the skill works in the context being reviewed (for the author, for a colleague, in a repository). - **Novice does not compute a maturity stage.** Scope is determined by the user's choice. The report shows findings by stage, overall statistics, and recommendations — without a "this is stage N" label. - **Language:** conduct the entire review (report, logs, review plan, sub-agent briefs) in the language the user started the conversation in. @@ -115,24 +108,26 @@ The orchestrator captures **two distinct entities**: ## Mandatory Review Plan -Create a preliminary plan immediately after the user's response and refine it after collecting the manifest and choosing the mode. Break the review into blocks so that one plan item covers approximately 3–4 checks, not the entire document. Update statuses as work proceeds. - -**Where the plan lives:** - -| Client capability | Where the plan lives | How statuses are updated | -|:---|:---|:---| -| Task-tracking tool available | In the tool | `pending` → `in_progress` → `completed` | -| No task-tracking tool | Markdown checklist in chat: publish it before the first check | `[ ]` → `[~]` → `[x]`, repost the updated checklist at every phase boundary | -| No task-tracking tool, logging ON | Same chat checklist, additionally saved as `review-plan.md` in the results folder | Rewrite the file after every completed block | +Create a preliminary plan immediately after the user's response, before the first check. -**Completion gate:** do not produce the final report while any plan item is still open. An item is closed either by a verdict or by an explicit entry in `Review Limitations`. +**Copy this checklist into your response and check items off as you complete them:** -**Example of a good breakdown:** ```text -- Check folder structure, SKILL.md presence, directory validity -- Check frontmatter: name, trigger phrases, safety +Review plan: +- [ ] Collect the manifest, count the total volume, choose the execution mode +- [ ] Structure checks (ST01–ST16) +- [ ] Workflow checks (WF01–WF29) +- [ ] References checks (RF01–RF15) +- [ ] Link checks (LK01–LK07) +- [ ] Lifecycle checks (LC01–LC05) +- [ ] Antipattern Bingo +- [ ] Final report ``` +Refine it once the manifest is collected and the mode is chosen: drop the groups that lie outside the selected scope, and split any group covering more than 3–4 checks into separate items. Repost the updated checklist at every phase boundary. + +**Completion gate:** do not produce the final report while any plan item is still open. An item is closed either by a verdict or by an explicit entry in `Review Limitations`. + --- ## Orchestrator Algorithm @@ -144,6 +139,7 @@ Create a preliminary plan immediately after the user's response and refine it af 3a. Check file write 3b. Get timestamp from command line (date +%Y%m%d-%H%M or equivalent) 3c. Show user the full path to the results folder, await confirmation + 3d. Create the confirmed results folder: {skill-name}-review-{YYYYMMDD-HHMM}/ 4. Create the preliminary review plan (see "Mandatory Review Plan") 5. Collect the manifest of the skill under review: file list, sizes, line counts, frontmatter, presence of references/, scripts/, assets/. The goal of this step is routing and passing to @@ -158,19 +154,18 @@ Create a preliminary plan immediately after the user's response and refine it af 9d. If logging ON — write report.md to the confirmed folder 10. IF SUB-AGENT (>= 500 lines): 10a. Refine the review plan for sub-agent review - 10b. If logging ON — create the results folder: {skill-name}-review-{YYYYMMDD-HHMM}/ - 10c. Using the "scope → sub-agents" matrix, determine which sub-agents to launch - 10d. Launch the required sub-agents; pass scope and checklist mapping to each. - Each selected sub-agent is launched as a separate call of the sub-agent tool. + 10b. Using the "scope → sub-agents" matrix, determine which sub-agents to launch + 10c. Launch the required sub-agents; pass scope and checklist mapping to each. + Each selected sub-agent is launched as a separate sub-agent call. Do not combine multiple sub-agents in one prompt. Parallel launch of multiple separate sub-agent calls is allowed. - 10e. Collect condensed summaries (and temp_log_path if direct log write to the output folder failed) - 10f. Run the verification gate: + 10d. Collect condensed summaries (and temp_log_path if direct log write to the output folder failed) + 10e. Run the verification gate: - Count total FAIL / WARNING across all summaries. - Verify that every FAIL / WARNING from every summary entered the working findings list. - Log sub-agents not launched due to scope: "[sub-agent] — not launched: all checks outside selected scope". - Capture cross-signals between summaries if they require a note in the final report. - 10g. If logging ON — move fallback logs to the output folder + 10f. If logging ON — move fallback logs to the output folder 11. Fill in "Antipattern Bingo" per references/antipattern-bingo.md. This file may be read early to distribute Bingo assignments, but fill the final table only after receiving summaries. 12. After receiving summaries, generate the final report with findings grouped by stage @@ -307,7 +302,7 @@ Findings counters are maintained **in total across all checked stages**: total F > - Antipatterns above the current scope — mark `NOT_CHECKED`. > - After reading, work strictly according to the checklist, scope, base rules, and bingo file. -3. **Logging (if enabled in Step 0):** The orchestrator creates the folder `{skill-name}-review-{YYYYMMDD-HHMM}/`. The sub-agent first tries to write `log-{subagent}.md` directly to the output folder. If that fails — writes the log to a temporary file and returns `temp_log_path`, and the orchestrator moves such a log to the output folder. Do not request additional permissions from the user. **If logging is off:** sub-agents return only a condensed summary. No files or folders are created. File write capability is checked before the review starts. +3. **Logging (if enabled in Step 0):** The orchestrator has already created the folder `{skill-name}-review-{YYYYMMDD-HHMM}/` in Step 0. The sub-agent first tries to write `log-{subagent}.md` directly to the output folder. If that fails — writes the log to a temporary file and returns `temp_log_path`, and the orchestrator moves such a log to the output folder. Do not request additional permissions from the user. **If logging is off:** sub-agents return only a condensed summary. No files or folders are created. File write capability is checked before the review starts. 4. **Sub-agents return condensed summaries** to the orchestrator. A summary is the distillation of the review; when log fallback applies, `temp_log_path` is added. --- diff --git a/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md b/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md index 10e5d1b..ed7fe11 100644 --- a/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md +++ b/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md @@ -68,7 +68,7 @@ For **each** antipattern, choose one of four verdicts: - **#15 Schema drift risk** vs **#13 Mirroring MCP schema**: both use WF22, but differently. #13 is a structural fact (a schema copy lives in SKILL.md). #15 is a lifecycle risk (the schema version is not pinned, drift is not tracked). - **#16 Context overfitting** vs **#12 Hardcoded paths**: #12 is a concrete mechanical signal (absolute paths). #16 is a broader pattern (coupling to OS, permissions, environment, implicit requirements). If the only signal is hardcoded paths, do not duplicate the verdict. -- **#21 Client tool lock-in**: the skill is tied to a concrete tool of one client (`TodoWrite`, `Task` and the like) with no fallback for a client that does not provide it. `CRITICAL` if a preflight stops the skill over the missing tool; `MINOR` if the skill merely assumes the tool without a fallback. Tool availability differs between clients and models — a tool present today may be absent in the next model. +- **#21 Client tool lock-in** vs **#16 Context overfitting**: #21 is a mechanical signal — the skill names a tool of one client (for planning, delegation, or anything else) and describes no way to work without it. `CRITICAL` if a precondition stops the skill over the missing tool; `MINOR` if the tool is merely assumed. #16 is the broader pattern. If the only signal is a named tool, do not duplicate the verdict. - **#16 in novice mode:** checked **partially** — only by mechanical portability signals (WF15: preconditions, OS, permissions, packages). Deep overfitting analysis (IN09–IN14: hidden assumptions, self-sufficiency, implicit knowledge) is only available in skill-review-nightmare with a full Intern walkthrough. --- diff --git a/plugins/skill-review/skills/skill-review/references/checklist-workflow.md b/plugins/skill-review/skills/skill-review/references/checklist-workflow.md index b6c068a..64f50be 100644 --- a/plugins/skill-review/skills/skill-review/references/checklist-workflow.md +++ b/plugins/skill-review/skills/skill-review/references/checklist-workflow.md @@ -51,21 +51,13 @@ *Context:* Without checkpoints, the agent continues the workflow on a silent step failure — this is "silent chain failures". A good skill does not just list steps — it sets conditions: what must be true before proceeding. -**WF12** If the workflow is longer than **4 steps** — do the instructions explicitly declare a **planning mechanism**: a task-tracking tool, a task list, or an external planning artifact? +**WF12** If the workflow is longer than **4 steps** — do the instructions explicitly declare a **planning mechanism** — a task list, a checklist in the response, or an external planning artifact — that does not depend on a tool being present in a particular client? -*Context:* In long sessions, the agent easily loses its plan and starts jumping between tasks. Anthropic recommends structured note-taking / agentic memory: an explicit task list that maintains state between tool calls. - -**Additional check — portability of the planning mechanism.** If the skill names a concrete client tool (`TodoWrite` and the like), does it survive a client that does not provide that tool? Tool availability differs between clients and models: a tool present today may be absent in the next model. - -| Signal | Verdict | -|---|---| -| Preflight stops the skill because a client-specific tool is missing | FAIL — the skill does not start at all | -| Planning is tied to one tool, no fallback described | WARNING — planning silently disappears | -| Planning is described as a capability with a fallback (tool, or checklist in chat, or a file) | PASS | +*Context:* In long sessions, the agent easily loses its plan and starts jumping between tasks. Anthropic recommends structured note-taking / agentic memory: an explicit task list that maintains state between tool calls. Tool availability differs between clients and model versions, so a plan that exists only inside one named tool disappears where that tool is not offered — and a precondition that stops the skill over a missing tool is worse than having no plan at all. **WF13** If planning is used, is there an **enforcement gate**: completion is not allowed while plan items remain open? -*Context:* The most effective way to make planning mandatory is to prohibit completion with unclosed tasks. Otherwise the plan remains decorative and does not prevent context loss. The gate must be worded in terms of open plan items, not in terms of the status names of one specific tool — otherwise it evaporates in a client without that tool. +*Context:* The most effective way to make planning mandatory is to prohibit completion with unclosed tasks. Otherwise the plan remains decorative and does not prevent context loss. The gate must be worded in terms of open plan items, not in terms of the status names of one specific tool. **WF14** Are critical rules, prohibitions, and stop conditions at the beginning of the skill or under explicit `CRITICAL` headers, not buried in the middle of a long text? diff --git a/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md b/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md index ce3f8ab..0dc2683 100644 --- a/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md +++ b/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md @@ -2,7 +2,7 @@ > **Activation context:** This mode was activated by the orchestrator for one of two reasons: the total volume of the skill under review is < 500 lines, **or** the client provides no sub-agent tool and delegation is impossible at any volume. All checks are executed in one context without sub-agents. The set of checks, report format, and Bingo are **identical** to sub-agent mode. > -> **If the mode was activated for the second reason** (volume `>= 500` lines, no delegation): read the files of the skill under review in parts, by check group, instead of pulling everything into context at once; after each completed Part, keep only the verdicts and drop the raw text; and record in `Review Limitations` that the review ran without delegation because the client has no sub-agent tool. +> **If the mode was activated for the second reason** (volume `>= 500` lines): read the files of the skill under review per check group rather than all at once, do not re-read a file once its group is closed, write each verdict down as soon as you reach it, and record in `Review Limitations` that the review ran in one context without delegation. --- @@ -11,15 +11,14 @@ > This mode assumes the orchestrator has already run the preflight of the parent skill. - **File read access is required.** If it is unexpectedly unavailable, do not start the review and immediately notify the user that single-pass cannot start in the current client. -- If the parent orchestrator enabled logging, file write is additionally required. -- A task-tracking tool (`TodoWrite` or equivalent) is **optional**. If the client does not provide one — keep the review plan as a markdown checklist in chat. This is not a reason to stop. +- If the parent orchestrator enabled logging, file write is additionally required. If it is unavailable — notify the user and stop the review. --- ## Mandatory Rules - **No questions to the user.** Review parameters (scope, declared target, logging) were already determined by the orchestrator in Step 0. -- Create a **review plan**: one item per 3–4 checks. Use the client's task-tracking tool if there is one; otherwise publish the plan as a markdown checklist in chat and repost it updated at every phase boundary. Do not produce the final report while any plan item is still open. +- Create a **review plan** as a markdown checklist in the chat (see `## Algorithm`): publish it before the first check and repost it updated at every phase boundary. Do not produce the final report while any plan item is still open. - Evaluate **only by observable artifacts** — do not infer what is not present in the files. - Do not count as PASS any runs, stability, or lifecycle maturity without file confirmation. - If a section is not applicable (no `references/`, `scripts/`, MCP, sub-agents) — mark **N/A**, not FAIL. @@ -56,10 +55,24 @@ Checks outside scope are marked **"not checked for selected scope"** (not N/A an Part A → Part B → Part C → Part D → [Part E if scope is full] 3. Fill Antipattern Bingo (Part F) 4. Generate the final report (Part G) -5. If logging ON — write report.md (and review-plan.md, if the plan is kept as a chat checklist); - if logging OFF — do not write any files +5. If logging ON — write report.md; if logging OFF — do not write any files ``` +**Step 1 — copy this checklist into your response and check items off as you complete them:** + +```text +Review plan: +- [ ] Part A — Structure and Form (ST01–ST16) +- [ ] Part B — Workflow (WF01–WF29) +- [ ] Part C — References and Progressive Disclosure (RF01–RF15) +- [ ] Part D — Link Integrity (LK01–LK07) +- [ ] Part E — Ownership and Lifecycle (LC01–LC05) +- [ ] Part F — Antipattern Bingo +- [ ] Part G — Report Format +``` + +Drop the Parts that lie outside the selected scope, and split any Part covering more than 3–4 checks into separate items. + --- ## Troubleshooting @@ -214,21 +227,13 @@ Checks outside scope are marked **"not checked for selected scope"** (not N/A an *Context:* Without checkpoints, the agent continues the workflow on a silent step failure — "silent chain failures". A good skill does not just list steps — it sets conditions: what must be true before proceeding. -**WF12.** If the workflow has > 4 steps — is there a **planning mechanism**: a task-tracking tool, task list, or external planning artifact? - -*Context:* In long sessions, the agent easily loses its plan. Anthropic recommends structured note-taking / agentic memory: an explicit task list that maintains state between tool calls. +**WF12.** If the workflow has > 4 steps — is there a **planning mechanism** — a task list, a checklist in the response, or an external planning artifact — that does not depend on a tool being present in a particular client? -**Portability of the planning mechanism:** if the skill names a specific client tool (`TodoWrite` and the like), does it survive a client that does not provide it? Tool availability differs between clients and models. - -| Signal | Verdict | -|---|---| -| Preflight stops the skill because a client-specific tool is missing | FAIL — the skill does not start at all | -| Planning is tied to one tool, no fallback described | WARNING — planning silently disappears | -| Planning is described as a capability with a fallback (tool, or checklist in chat, or a file) | PASS | +*Context:* In long sessions, the agent easily loses its plan. Anthropic recommends structured note-taking / agentic memory: an explicit task list that maintains state between tool calls. Tool availability differs between clients and model versions, so a plan that exists only inside one named tool disappears where that tool is not offered — and a precondition that stops the skill over a missing tool is worse than having no plan at all. **WF13.** If planning is used — is there an **enforcement gate**: completion is not allowed while plan items remain open? -*Context:* The most effective way to make planning mandatory is to prohibit completion with unclosed tasks. Otherwise the plan remains decorative and does not prevent context loss. The gate must be worded in terms of open plan items, not in terms of one tool's status names — otherwise it evaporates in a client without that tool. +*Context:* The most effective way to make planning mandatory is to prohibit completion with unclosed tasks. Otherwise the plan remains decorative and does not prevent context loss. The gate must be worded in terms of open plan items, not in terms of one tool's status names. **WF14.** Are critical rules and prohibitions at the beginning of the skill or under `CRITICAL` headers, not buried in the middle? @@ -547,7 +552,7 @@ Evaluate **only by observable artifacts**. For `MINOR`, the note is short: `trig - **#15 vs #13:** both use WF22, but #13 is a structural fact (schema copy), #15 is a lifecycle risk (drift). - **#16 vs #12:** #12 is mechanical (absolute paths), #16 is broader (coupling to OS, permissions, environment). Do not duplicate the verdict. - **#16 in novice mode:** checked partially — only by mechanical portability signals (WF15: preconditions, OS, permissions). -- **#21 Client tool lock-in:** the skill is tied to a concrete tool of one client (`TodoWrite`, `Task` and the like) with no fallback. `CRITICAL` if a preflight stops the skill over the missing tool; `MINOR` if the tool is merely assumed without a fallback. +- **#21 vs #16:** #21 is mechanical — the skill names a tool of one client and describes no way to work without it. `CRITICAL` if a precondition stops the skill over the missing tool; `MINOR` if the tool is merely assumed. #16 is the broader pattern; do not duplicate the verdict. ### Bingo Table for Report @@ -603,7 +608,7 @@ Evaluate **only by observable artifacts**. For `MINOR`, the note is short: `trig ## Review Limitations -[Optional section. Show only if there were execution degradations: unreadable files, log write failure, context overflow, partially restricted scope.] +[Optional section. Show only if there were execution degradations: unreadable files, log write failure, context overflow, review run without delegation, partially restricted scope.] > - [What went wrong] > - [How it affected completeness] From 6afddb226f215ff99b7f522202ea378cd3855c34 Mon Sep 17 00:00:00 2001 From: Aleksandr Korchak Date: Thu, 17 Sep 2026 22:35:48 +0300 Subject: [PATCH 4/4] fix(skill-review): anchor Bingo #21 to WF15 so it cannot report a false NONE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #21 hung on WF12 alone, and WF12 is gated on "if the workflow is longer than 4 steps". A short skill whose preconditions hard-require a client tool made WF12 N/A, and the row then had to be filled as NONE — a clean verdict for a skill that has exactly this defect. The file's own Scope Fill Rule exists to avoid that false impression of cleanliness. WF15 (preconditions) carries no such gate, so it becomes the second anchor and the row fires whenever the antipattern is applicable at all. This is the same shape #20 already uses: RF12 is gated on "many files", RF06 is not, and the row survives on RF06. Stage 2 -> 3, by the maximum of the anchors, as #15 does with WF22+LC02. It also puts #21 next to #12 and #16 — the other two portability rows, both stage 3, both owned by Workflow. The #16 disambiguation note now splits by signal instead of by breadth: both rows use WF15, so a named client tool goes to #21 and everything else (OS, permissions, packages, paths) to #16. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/skill-review/CHANGELOG.md | 5 +++-- .../skills/skill-review/references/antipattern-bingo.md | 6 +++--- .../skill-review/references/instruction-singlepass.md | 6 +++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/plugins/skill-review/CHANGELOG.md b/plugins/skill-review/CHANGELOG.md index d053efc..1c5e03c 100644 --- a/plugins/skill-review/CHANGELOG.md +++ b/plugins/skill-review/CHANGELOG.md @@ -19,8 +19,9 @@ ### Added -- Antipattern Bingo #21 "Client tool lock-in" (stage 2, WF12, owner Workflow) — the skill names - a tool of one client and describes no way to work without it. +- Antipattern Bingo #21 "Client tool lock-in" (stage 3, WF12 + WF15, owner Workflow) — the skill + names a tool of one client and describes no way to work without it. WF15 is the second anchor + so the row still fires on a skill too short for WF12 to apply. ### Fixed diff --git a/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md b/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md index ed7fe11..de6a114 100644 --- a/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md +++ b/plugins/skill-review/skills/skill-review/references/antipattern-bingo.md @@ -60,7 +60,7 @@ For **each** antipattern, choose one of four verdicts: | 18 | **Lifecycle hygiene gap (rot risk)** | 4 | LC03 | **Lifecycle** | | 19 | **Silent chain failures** | 2 | WF11 | **Workflow** | | 20 | **Monolithic reference dump** | 3 | RF06, RF12 | **References** | -| 21 | **Client tool lock-in** | 2 | WF12 | **Workflow** | +| 21 | **Client tool lock-in** | 3 | WF12, WF15 | **Workflow** | --- @@ -68,7 +68,7 @@ For **each** antipattern, choose one of four verdicts: - **#15 Schema drift risk** vs **#13 Mirroring MCP schema**: both use WF22, but differently. #13 is a structural fact (a schema copy lives in SKILL.md). #15 is a lifecycle risk (the schema version is not pinned, drift is not tracked). - **#16 Context overfitting** vs **#12 Hardcoded paths**: #12 is a concrete mechanical signal (absolute paths). #16 is a broader pattern (coupling to OS, permissions, environment, implicit requirements). If the only signal is hardcoded paths, do not duplicate the verdict. -- **#21 Client tool lock-in** vs **#16 Context overfitting**: #21 is a mechanical signal — the skill names a tool of one client (for planning, delegation, or anything else) and describes no way to work without it. `CRITICAL` if a precondition stops the skill over the missing tool; `MINOR` if the tool is merely assumed. #16 is the broader pattern. If the only signal is a named tool, do not duplicate the verdict. +- **#21 Client tool lock-in** vs **#16 Context overfitting**: #21 is a mechanical signal — the skill names a tool of one client (for planning, delegation, or anything else) and describes no way to work without it. `CRITICAL` if a precondition stops the skill over the missing tool; `MINOR` if the tool is merely assumed. Both use WF15, so split by signal: a named client tool goes to #21, everything else (OS, permissions, packages, paths) to #16. Do not duplicate the verdict. - **#16 in novice mode:** checked **partially** — only by mechanical portability signals (WF15: preconditions, OS, permissions, packages). Deep overfitting analysis (IN09–IN14: hidden assumptions, self-sufficiency, implicit knowledge) is only available in skill-review-nightmare with a full Intern walkthrough. --- @@ -97,4 +97,4 @@ For **each** antipattern, choose one of four verdicts: | 18 | Lifecycle hygiene gap (rot risk) | 4 | [NONE / MINOR / CRITICAL / NOT_CHECKED] | [if MINOR: 3–4 words, otherwise `—`] | | 19 | Silent chain failures | 2 | [NONE / MINOR / CRITICAL / NOT_CHECKED] | [if MINOR: 3–4 words, otherwise `—`] | | 20 | Monolithic reference dump | 3 | [NONE / MINOR / CRITICAL / NOT_CHECKED] | [if MINOR: 3–4 words, otherwise `—`] | -| 21 | Client tool lock-in | 2 | [NONE / MINOR / CRITICAL / NOT_CHECKED] | [if MINOR: 3–4 words, otherwise `—`] | +| 21 | Client tool lock-in | 3 | [NONE / MINOR / CRITICAL / NOT_CHECKED] | [if MINOR: 3–4 words, otherwise `—`] | diff --git a/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md b/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md index 0dc2683..7a360c7 100644 --- a/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md +++ b/plugins/skill-review/skills/skill-review/references/instruction-singlepass.md @@ -545,14 +545,14 @@ Evaluate **only by observable artifacts**. For `MINOR`, the note is short: `trig | 18 | **Lifecycle hygiene gap (rot risk)** | 4 | LC03 | | 19 | **Silent chain failures** | 2 | WF11 | | 20 | **Monolithic reference dump** | 3 | RF06, RF12 | -| 21 | **Client tool lock-in** | 2 | WF12 | +| 21 | **Client tool lock-in** | 3 | WF12, WF15 | ### Notes - **#15 vs #13:** both use WF22, but #13 is a structural fact (schema copy), #15 is a lifecycle risk (drift). - **#16 vs #12:** #12 is mechanical (absolute paths), #16 is broader (coupling to OS, permissions, environment). Do not duplicate the verdict. - **#16 in novice mode:** checked partially — only by mechanical portability signals (WF15: preconditions, OS, permissions). -- **#21 vs #16:** #21 is mechanical — the skill names a tool of one client and describes no way to work without it. `CRITICAL` if a precondition stops the skill over the missing tool; `MINOR` if the tool is merely assumed. #16 is the broader pattern; do not duplicate the verdict. +- **#21 vs #16:** #21 is mechanical — the skill names a tool of one client and describes no way to work without it. `CRITICAL` if a precondition stops the skill over the missing tool; `MINOR` if the tool is merely assumed. Both use WF15, so split by signal: a named client tool goes to #21, everything else to #16. ### Bingo Table for Report @@ -578,7 +578,7 @@ Evaluate **only by observable artifacts**. For `MINOR`, the note is short: `trig | 18 | Lifecycle hygiene gap (rot risk) | 4 | | | | 19 | Silent chain failures | 2 | | | | 20 | Monolithic reference dump | 3 | | | -| 21 | Client tool lock-in | 2 | | | +| 21 | Client tool lock-in | 3 | | | ---