From a9f8d032f3a05c742d3b9d2106bd0aa2679957cd Mon Sep 17 00:00:00 2001 From: Dmitry Dementiev Date: Fri, 15 May 2026 15:53:05 +0300 Subject: [PATCH] =?UTF-8?q?feat(skill):=20=D1=8F=D0=B7=D1=8B=D0=BA=20?= =?UTF-8?q?=D0=BE=D1=82=D0=B2=D0=B5=D1=82=D0=B0=20=D1=80=D0=B5=D0=B2=D1=8C?= =?UTF-8?q?=D1=8E=D0=B5=D1=80=D0=B0,=20=D0=B1=D0=B0=D1=82=D1=87-=D0=BF?= =?UTF-8?q?=D0=B0=D1=83=D0=B7=D0=B0,=20=D1=84=D0=B8=D0=BA=D1=81=D1=8B=20?= =?UTF-8?q?=D0=B8=D0=B7=20=D1=80=D0=B5=D0=B2=D1=8C=D1=8E?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Зачем: - Ревьюер отвечал на английском вне зависимости от языка сессии — оператор тратил усилия на чтение и перевод. - Скилл паузил оператора непредсказуемо (то спрашивал, то применял молча) — нужен явный критерий, что требует мнения оператора, а что не требует. - Накопились фактические неточности из двух раундов self-review (детекция мёртвого ревьюера, disallowedTools, /agent, грамматика инвокации). - Что: - Step 2: новый блок Language — лид определяет язык сессии и добавляет директиву в брифинг; парсимые литералы (VERDICT, severity tags, заголовки секций) остаются английскими. - Step 4: новый блок «structural vs non-structural» с батч-паузой — одна пауза на раунд только при наличии structural-фиксов; non-structural применяются молча. - reviewer-prompt.md: добавлен плейсхолдер {LANGUAGE_BLOCK} и описание в таблице placeholders. - Фиксы из ревью: детекция мёртвого ревьюера переписана под async-модель Agent Teams; disallowedTools — путь через кастомный subagent; /agent описан как CLI оператора, а не tool лида; bare резолвится в mode=plan; добавлены формы code-vs-plan и code-vs-plan ; xhigh — orthogonal effort flag; «plan in context» получило конкретные критерии. - Проверка: - git diff main..HEAD -- SKILL.md reviewer-prompt.md — посмотреть итоговый набор правок. - Запустить /claude-team-review на самой ветке и убедиться, что директива Language вставляется и батч-пауза срабатывает только на structural. Co-Authored-By: Claude Opus 4.7 (1M context) --- SKILL.md | 165 +++++++++++++++++++++++++++++++++++++++------ reviewer-prompt.md | 15 ++++- 2 files changed, 157 insertions(+), 23 deletions(-) diff --git a/SKILL.md b/SKILL.md index 466b564..09c5c7e 100644 --- a/SKILL.md +++ b/SKILL.md @@ -23,20 +23,33 @@ platform-agnostic; the platform decides how to spawn. ## When to invoke +Invocation grammar: `/claude-team-review [] [xhigh]` — +both pieces are independently optional. `xhigh` is an effort flag, not +a mode, and can be combined with any of the mode arguments below. + - `/claude-team-review` — auto-detect what to review - `/claude-team-review plan` — force plan review - `/claude-team-review code` — force code review -- `/claude-team-review ` — review a specific file (argument contains `/` or `.`) -- `/claude-team-review xhigh` — use max reasoning effort for the reviewer +- `/claude-team-review code-vs-plan` — force code-vs-plan review (uses the plan most recently shown in this conversation) +- `/claude-team-review code-vs-plan ` — force code-vs-plan review against an explicit plan file +- `/claude-team-review ` — review a specific file as a **plan** (argument contains `/` or `.`; the file is treated as the plan artifact). For plan-vs-code comparison use `code-vs-plan ` explicitly. +- `xhigh` (anywhere in args) — use max reasoning effort for the reviewer --- ## Step 1: Determine review mode -Check in priority order: +First, strip an `xhigh` token from the args if present (it does not +consume the mode slot — see Step 2 "Effort override"). Then resolve mode +in priority order: -**1. Explicit argument** (`plan`, `code`, file path) → use it. For `plan`, -skip all git checks and proceed to Step 2. +**1. Explicit argument** (`plan`, `code`, `code-vs-plan`, +`code-vs-plan `, or a file path) → use it. For `plan` and +`code-vs-plan` without an explicit path, skip git for the plan side and +proceed to Step 2. A bare file path (no preceding `code-vs-plan` token) +resolves to **plan** mode with that file as the plan artifact; if the +user wants plan-vs-code, they must invoke `code-vs-plan ` +explicitly. **2. Plan Mode active** (Claude Code) — if context contains the system message "Plan mode is active" → mode = `plan`, skip git. @@ -46,7 +59,18 @@ message "Plan mode is active" → mode = `plan`, skip git. 1. Check for code changes (any non-empty output means changes exist): - `git diff --name-only` — unstaged - `git diff --cached --name-only` — staged -2. Check if a plan exists in the current conversation context. +2. Check if a **structured plan artifact** exists in the current + conversation context. This means one of: + - The user pasted or referenced a markdown plan document with + section headers like `Goals`, `Steps`, `Verification`, `Tasks`, + or similar + - The user explicitly labeled content as a "plan", "design", "spec", + or "RFC" + - A previous turn produced a written plan (e.g. via + `superpowers:writing-plans` or a saved plan file) + + A casual sentence of intent ("I'll refactor auth next") is **not** a + plan in context. | Code changes? | Plan in context? | Mode | |---------------|------------------|---------------------| @@ -55,6 +79,11 @@ message "Plan mode is active" → mode = `plan`, skip git. | Yes | No | **code** | | No | No | Ask the user | +If the "Plan in context?" call is genuinely ambiguous (something looks +plan-ish but doesn't match the criteria above), ask the user rather +than guessing — silently misclassifying into `code-vs-plan` produces +contorted reviews. + --- ## Step 2: Spawn the reviewer @@ -69,9 +98,11 @@ pass it the briefing assembled from `reviewer-prompt.md`. Task tool spawn is one-shot — Round 2+ has to go through the operator- gated fresh-spawn path in Step 6. - **Codex:** native subagent spawn (host orchestrates spawn/wait/consolidate). - Codex supports intra-session continuation — the lead can send follow-up - instructions to a running subagent thread, and `/agent` switches between - active threads. + Codex supports intra-session continuation — the lead can address a + running subagent thread by name in its next instruction and Codex's + orchestration routes the message there. (`/agent` is a human-facing + CLI command for inspecting/switching threads from the terminal, not a + tool the lead invokes.) - **Other hosts:** equivalent subagent mechanism. The skill works as long as the host can spawn a subagent and (optionally) route follow-up messages to it. @@ -91,10 +122,42 @@ scope exclusions, and required output format. **Effort override:** if the user passed `xhigh`, route the spawn through a maximum-reasoning configuration if the host supports it. -**Tool restrictions:** the briefing explicitly forbids file modification. -If the host supports enforced read-only sandboxing (Claude Code -`disallowedTools: Write, Edit`, Codex `sandbox_mode = "read-only"`), apply -it on top — it's defense in depth, not the primary control. +**Language.** The skill template is intentionally English (stable, host- +agnostic), but the reviewer's *output* should match the operator's +session language so verbatim show-back to the operator works without +translation. Detect the language from recent operator messages (the last +few human turns in the conversation). If it's English — no extra block +needed (default). Otherwise, append a `## Language` section to the +briefing with this content: + +``` +## Language + +Respond in . The following stay in English (they are +literals parsed by the lead and must not be translated): section headers +(Summary, Findings, Verdict), severity tags +(`[severity: critical|high|medium]`), and the final verdict line +(`VERDICT: APPROVED` or `VERDICT: REVISE`). All other content — +prose, reasoning, descriptions, recommendations — in +. +``` + +The verbatim reply from the reviewer is shown to the operator as-is — do +not translate it back; it should already be in the operator's language. + +**Tool restrictions:** the briefing's "must NOT modify files" rule is +the primary control on the read path. The built-in `general-purpose` +subagent on Claude Code has no tool-restriction knob the lead can pass +at invocation time, so the briefing rule is the only control there. If +you want enforced sandboxing on top, that requires going off the +default path: + +- **Claude Code:** define a custom subagent (e.g. + `~/.claude/agents/adversarial-reviewer.md`) with frontmatter + `disallowedTools: Write, Edit`, and dispatch that agent type instead + of `general-purpose`. +- **Codex:** the host's `sandbox_mode = "read-only"` applies to the + spawned subagent natively. --- @@ -178,6 +241,51 @@ If an operator is present, show the matrix before applying. In headless or autonomous runs, proceed but be ready to explain each decision in the final summary. +### Decide which fixes need operator sign-off + +After the matrix is built and every finding has an action (`accept` / +`reject with reasoning` / `re-scope`), classify each `accept` / `re-scope` +fix as **structural** or **non-structural** — this controls whether the +operator is paused. + +**Structural** — pause operator before applying: + +- Changes the invocation grammar (new/removed args, renamed mode, changed + argument semantics) +- Changes an external contract: output format, parsed literals + (e.g. the `VERDICT: APPROVED|REVISE` line), section header names, + named workflow states, public-facing option semantics +- Adds or removes a workflow step or a terminal state +- Changes the meaning of a publicly documented option (e.g. what `xhigh` + does, what `code-vs-plan` means) +- Any fix the lead is uncertain how to scope — when in doubt, classify + as structural + +**Non-structural** — apply without pausing: + +- Wording / phrasing changes that don't change semantics +- Correcting factual inaccuracies in descriptions (e.g. wrong API name, + wrong tool mechanic, wrong attribution) +- Removing outdated comments or examples +- Adding clarifying sentences or examples that don't change behavior +- Internal heuristic refinements that don't change observable behavior + +**Batch-pause rule.** Do not pause once per fix. Instead: + +1. Walk the entire matrix first, classify every accepted/re-scoped fix. +2. If structural count is **zero** → apply everything, no pause. +3. If structural count is **≥ 1** → make exactly **one** pause showing + the operator: + - The list of structural fixes (one-line per fix; ask for go/no-go on + each, or as a batch) + - The list of non-structural fixes (informational only — auto-applied) + - The list of rejects with reasoning (informational only) + +**Headless / no operator.** If no operator is reachable, apply structural +fixes anyway but flag them explicitly in the final summary as +"structural changes applied without operator sign-off" — the operator +reviews them after the fact rather than gating them up-front. + --- ## Step 5: Apply or push back @@ -284,7 +392,7 @@ relying on this):** |---|---|---| | Claude Code + Agent Teams (`CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1`) | yes | `SendMessage` to the teammate inbox | | Claude Code without Agent Teams | **no** — Task spawn is one-shot | — | -| Codex | yes — intra-session | follow-up message to a running thread; `/agent` to switch | +| Codex | yes — intra-session | address the running subagent thread by name in the next instruction; Codex orchestration routes it (operators can use `/agent` from the CLI to inspect/switch threads — not a lead-side tool) | Two routes lead to the "ask operator" step: - The host has no continuation mechanism for our use case (Claude Code @@ -292,12 +400,29 @@ Two routes lead to the "ask operator" step: - The host has continuation but the previous reviewer is no longer reachable (process ended, thread closed, inbox dead) -**Detecting a dead reviewer (continuation-supporting hosts).** When the -send call returns only a routing acknowledgment (e.g. `{"success": true, -"message": "Message sent to reviewer's inbox"}`) without review content — -the reviewer process has ended; the message went to a dead inbox. Do not -wait. Treat the reviewer as unreachable, fall through to the operator -question. +**Detecting a dead reviewer (continuation-supporting hosts).** The send +itself is fire-and-forget — `SendMessage` returns immediately with a +routing acknowledgment regardless of teammate liveness, and the reply +arrives asynchronously in a later turn via the host's automatic message +delivery. **Do not treat the ack as the reply.** After sending, wait for +the reply to be delivered. Treat the reviewer as unreachable only when +one of the following is true: + +- The host surfaces an explicit shutdown / "teammate exited" / "no such + teammate" event. On Claude Code + Agent Teams this includes the + documented case where a resumed session no longer has the previous + teammates alive, and idle notifications (`TeammateIdle` / + message-delivery events) — when present, these are the authoritative + liveness signal. +- Send fails outright (error response, not a success ack). +- The reply is overdue (no `TeammateIdle` / message-delivery notification + in a reasonable window) **and** an operator is present to confirm — + surface the wait and let the operator decide whether to abandon. If + no operator is present, keep waiting; auto-abandoning on a wall-clock + guess is worse than waiting (LLMs don't track wall time reliably and + the host's notification channel is the documented signal). + +When unreachable is confirmed, fall through to the operator question. **Detecting an operator.** Treat the run as **interactive** (operator available) only if both conditions hold: diff --git a/reviewer-prompt.md b/reviewer-prompt.md index 0e97535..ab5c5d1 100644 --- a/reviewer-prompt.md +++ b/reviewer-prompt.md @@ -131,6 +131,8 @@ VERDICT: REVISE ``` Approve if no findings or all low severity. Revise if any high or critical. + +{LANGUAGE_BLOCK} ``` --- @@ -143,15 +145,22 @@ Approve if no findings or all low severity. Revise if any high or critical. | `{ARTIFACT}` | Path to plan, inline plan text, or instruction to use git for code | | `{ROUND}` | `1` for initial, `N` for re-review | | `{PREVIOUS_FINDINGS_BLOCK}` | Empty on round 1 or when the same teammate continues. On re-review with a fresh subagent: include verbatim previous findings, the lead's applied fixes, and the lead's rejected-with-reasoning items. | +| `{LANGUAGE_BLOCK}` | Empty when operator's session language is English (template default). Otherwise a `## Language` section instructing the reviewer to respond in the operator's language while keeping section headers, severity tags, and the `VERDICT: APPROVED\|REVISE` line as English literals. See SKILL.md Step 2 "Language" for the exact block. | ## Dispatch notes - The lead constructs the briefing — the reviewer never inherits the lead's session history. Pass only what the reviewer needs. - Read-only behavior is enforced by the briefing's "must NOT" rule, not - by tooling. If the host platform supports an enforced read-only sandbox - (Claude Code `disallowedTools`, Codex `sandbox_mode = "read-only"`), - feel free to use it on top. + by tooling. Enforced sandboxing is platform-specific and is off the + default path: + - **Claude Code** with the built-in `general-purpose` agent has no + invocation-time tool-restriction parameter — the briefing is the + only control. To enforce, define a custom subagent (e.g. + `~/.claude/agents/adversarial-reviewer.md`) with frontmatter + `disallowedTools: Write, Edit` and dispatch that agent type instead. + - **Codex**: the host's `sandbox_mode = "read-only"` applies to the + spawned subagent natively. - For plan-mode reviews where the plan is inline, include the full plan text in `{ARTIFACT}` rather than a summary — the reviewer needs the actual artifact, not paraphrase.