subagent-review-cycles
Structure a multi-round Leader / Developer / Code-Reviewer review of a document or code change. Use when planning how many review rounds to run, dispatching a Code Reviewer subagent and deciding which tools and review criteria it carries, adjudicating a reviewer's BLOCKER / MAJOR
Install
npx skills add https://github.com/wei18/apple-dev-skills/tree/main/collaboration-skills/skills/subagent-review-cycles
claude plugin marketplace add https://llmmart.ai/marketplace.json && claude plugin install wei18-apple-dev-skills@llmmart
git clone https://github.com/wei18/apple-dev-skills.git
The skills CLI installs just this skill, for any of its supported agents. Claude Code installs the whole wei18/apple-dev-skills collection as a plugin from our marketplace. Git is the plain clone.
Skill manifest
Subagent Review Cycles
When to invoke
- About to dispatch a sub-agent to draft a technical document / design section.
- The first version of a document is ready and a Code Reviewer should audit technical correctness.
- User asks "how many review rounds", "should Code Reviewer run CLI", "what counts as rejection".
The triad
| Role | Job |
|---|---|
| Leader (main agent) | Dispatch, integrate review results, accept / reject, communicate with the user |
| Developer (subagent) | Draft / revise document sections; works after Leader replies with rejection. (May be dispatched as any implementer-type subagent when the task is architectural in nature; the role name in this skill remains "Developer".) |
| Code Reviewer (subagent) | Audits Developer output for technical / API correctness and logic gaps; CLI is forbidden for probing API/runtime behavior (build, run, simctl, trial-and-error); read-only search (grep, rg, git log, git show) is allowed |
The Leader never writes the implementation / drafts a section directly — that's the Developer's job. The Leader never does the review either — that's the Code Reviewer's job.
Round structure
round N:
Leader → Developer: dispatch (scope + skills + inputs + return format + criteria)
Developer → Leader: draft
Leader → Code Reviewer: dispatch (review criteria + must use WebSearch, no CLI)
Code Reviewer → Leader: BLOCKER / MAJOR / MINOR list
Leader: ACCEPT / REJECT each item with explicit reason
if accepted_count == total: done
if N == limit: pause, report to user
else: round N+1 with feedback
limit(N) is typically 3–5. If the limit is reached without convergence: pause, report to the user, wait for direction.
The "round-1 cosmetic" pragmatic rule
| Signal | Leader inline-edits | Dispatch round N+1 to Developer |
|---|---|---|
| Fix scope | Spelling / formatting / paragraph order / string typo only | Any semantic or content change |
| Time to fix | ≤ 5 minutes | > 5 minutes |
| New decisions required | None | Any |
Still record inline fixes in the meeting log. Reasoning: "limit is an upper bound, not a requirement"; burning a whole round just for typos is uneconomical.
Dispatch contract for Code Reviewer
Every Code Reviewer dispatch prompt must include:
- Target file / section scope (explicit file + section)
- Forbidden tools: CLI is forbidden for probing API/runtime behavior (build, run, simctl, trial-and-error); read-only search (grep, rg, git log, git show) is allowed
- Allowed tools: WebSearch / WebFetch (for verifying Apple APIs, library behaviour)
- Review criteria (4 dimensions + domain-specific checklist):
- Technical correctness (API name, behaviour, version)
- Logical consistency (internal contradictions, cross-section conflicts)
- Completeness (missing edge case, error handling, prerequisite)
- Efficiency (algorithm, CI / build / runtime cost)
- Return format: BLOCKER / MAJOR / MINOR three-level classification; each item with location (file + section) + suggestion. An absence claim ("the spec doesn't define this", "nothing covers this case") must include the grep/search command run and its zero-hit output — an absence claim with no evidence attached doesn't count as a finding.
Accept / Reject reply style
For each review finding the Leader gives:
- ACCEPT + reason: accepted; specify who fixes it this round
- REJECT + reason: rejected with a technical reason (not just "no")
- DEFER: acknowledged but deferred (goes to backlog / open items)
REJECT must cite specific evidence (API doc, prior decision, design constraint); pure preference is not acceptable.
Anti-patterns
- Using CLI to probe Apple API behaviour: forbidden. Use official docs / WebSearch instead.
- Repeatedly rejecting the same point in the same section: more than 2 identical rejections counts as a communication failure; pause and clarify with the user.
- ACCEPT without a reason: every ACCEPT should still have a one-line note of why it adds value.
- Leader drafting sections themselves: violates the role separation; only allowed for cosmetic-grade fixes.
Verification checklist
- Every absence claim ("not defined", "no coverage") in a review finding cites the grep/search command and its zero-hit output — otherwise it doesn't count as a finding.
- Each round has an explicit dispatch prompt (all 5 items of §Dispatch contract for Code Reviewer present, on top of the 6 elements from
leader-developer-handoff-contract). - Each review finding has an explicit accept / reject label + reason.
- When limit(N) is reached without convergence, pause; don't keep iterating indefinitely.
- Cosmetic fixes are inline-edited by the Leader; don't burn a round on them.
- The round-summary is recorded in the meeting log (not a verbatim copy of review content).
Phase TODO sweep checklist
A separate close-the-loop activity that fires once per phase (not once per review round), so it's not part of the round structure above. Before the Leader signs off on a phase-completion PR, run a sweep against the phase's diff scope to catch deferred-and-forgotten debt.
For the full sweep procedure, command, disposition rule, and anti-pattern, read references/phase-todo-sweep.md.
Related skills
leader-developer-handoff-contract: details the 6 required elements of every dispatch prompt.spec-phase-orchestration: review cycles are usually embedded in the spec phase.methodology-pattern-extractor: "round-1 cosmetic inline edit" is a codifiable pattern.- Official sources: when verifying or updating a factual or version-sensitive claim, read
references/official-docs.md.
Files (apple-dev-skills)
-
references
-
official-docs.md 843 B
Official pages backing this skill's claims; read when verifying or updating a factual or version-sensitive claim. | Page | URL | Backs | |---|---|---| | Configure permissions | https://code.claude.com/docs/en/permissions#bash | Dispatch contract item 2 "Forbidden tools": a layered CLI ban can be enforced with Bash deny rules (which apply to subagents); the rule stays in prose where no rule is configured | | Create custom subagents | https://code.claude.com/docs/en/sub-agents#available-tools | Same item, caveat: `disallowedTools: Bash(...)` removes the whole Bash tool, including the allowed read-only `git log` / `git show` | | Git - git-diff Documentation | https://git-scm.com/docs/git-diff | references/phase-todo-sweep.md: `<base>...HEAD` (from merge-base) vs `..` (two tips); `--diff-filter=d` (lowercase excludes deleted); `-z` | -
phase-todo-sweep.md 3.1 KB
# Phase TODO sweep checklist A separate close-the-loop activity that fires **once per phase** (not once per review round): before the Leader signs off on a phase-completion PR, run a sweep against the phase's diff scope to catch deferred-and-forgotten debt. **Command** (Leader-run; against the phase's diff scope, not the full repo — `<base>` is the commit/branch the phase started from, e.g. `main` or the phase's starting SHA): ``` git diff -z --name-only --diff-filter=d <base>...HEAD | xargs -0 -r rg -n --no-heading -e 'TODO|FIXME|XXX|HACK|stub|placeholder' ``` If `rg` isn't installed (it isn't pinned in this repo's `.mise.toml`), use the `grep` fallback: ``` git diff -z --name-only --diff-filter=d <base>...HEAD | xargs -0 -r grep -rnE 'TODO|FIXME|XXX|HACK|stub|placeholder' ``` `<base>...HEAD` (three dots) diffs against the merge base, so commits landed on `<base>` after the phase branched off don't leak into the sweep. `--diff-filter=d` excludes deleted paths (a deleted file has nothing left to grep). `-z` / `xargs -0` keep filenames containing spaces intact. Add your own project's phase-marker string to the pattern if it has one (e.g. `|Phase [0-9]+ Part` for a project that labels work-in-progress chunks that way). Both forms scope to the files the diff actually touched — a file outside the phase's diff is out of scope for this sweep even if it contains a match. **Disposition rule** — every match must fall into exactly one bucket; otherwise the phase is not complete: 1. **Resolved this phase** — fixed or implemented before merge. 2. **Moved to §Backlog** — routed to the topic-appropriate document's §Backlog section (product → `design.md`, engineering → `foundations.md`, implementation step → `plan.md`, collaboration → `methodology.md`), and the §Backlog entry **cites the source `file:line`** so the debt is traceable. 3. **Intentionally left** — documented in the phase meeting log as "intentionally left, see <follow-up issue / phase reference>". **Stub / placeholder code without a literal TODO comment still counts.** Identifier names like `xxxPlaceholder`, `xxxStub`, or scaffolding values that are not real implementations must be flagged in the phase log even if the regex didn't catch them via comment text. **Ownership**: the Leader runs the sweep. Subagents may flag debt during implementation (in their impl-notes open-questions section — see `agent-impl-notes-log`), but closing the loop before phase merge is non-delegable. A PR reviewer subagent dispatched for the phase-completion PR should also run the sweep against the diff and report findings as MAJOR (unless already documented per the disposition rule above). **When to invoke**: - Before declaring a phase complete. - When dispatching a Code Reviewer subagent on a phase-completion PR — include this command in the review brief's checklist. - When auditing whether a previously-declared "complete" phase actually was. **Anti-pattern**: marking a regex match as "obviously fine, ignore" without writing the disposition down. The point of the sweep is the paper trail; an undocumented justification is indistinguishable from forgetting.
-
-
SKILL.md 6.2 KB
--- name: subagent-review-cycles description: Structure a multi-round Leader / Developer / Code-Reviewer review of a document or code change. Use when planning how many review rounds to run, dispatching a Code Reviewer subagent and deciding which tools and review criteria it carries, adjudicating a reviewer's BLOCKER / MAJOR / MINOR findings, or when asked "how many review rounds", "may the Code Reviewer run CLI", "what counts as a rejection". Does NOT own the dispatch prompt's required elements (leader-developer-handoff-contract) nor pre-dispatch worktree conflict checks (subagent-conflict-detection). --- # Subagent Review Cycles ## When to invoke - About to dispatch a sub-agent to draft a technical document / design section. - The first version of a document is ready and a Code Reviewer should audit technical correctness. - User asks "how many review rounds", "should Code Reviewer run CLI", "what counts as rejection". ## The triad | Role | Job | |---|---| | **Leader** (main agent) | Dispatch, integrate review results, accept / reject, communicate with the user | | **Developer** (subagent) | Draft / revise document sections; works after Leader replies with rejection. (May be dispatched as any implementer-type subagent when the task is architectural in nature; the role name in this skill remains "Developer".) | | **Code Reviewer** (subagent) | Audits Developer output for technical / API correctness and logic gaps; CLI is forbidden for probing API/runtime behavior (build, run, simctl, trial-and-error); read-only search (grep, rg, git log, git show) is allowed | The Leader **never** writes the implementation / drafts a section directly — that's the Developer's job. The Leader **never** does the review either — that's the Code Reviewer's job. ## Round structure ``` round N: Leader → Developer: dispatch (scope + skills + inputs + return format + criteria) Developer → Leader: draft Leader → Code Reviewer: dispatch (review criteria + must use WebSearch, no CLI) Code Reviewer → Leader: BLOCKER / MAJOR / MINOR list Leader: ACCEPT / REJECT each item with explicit reason if accepted_count == total: done if N == limit: pause, report to user else: round N+1 with feedback ``` `limit(N)` is typically 3–5. **If the limit is reached without convergence**: pause, report to the user, wait for direction. ## The "round-1 cosmetic" pragmatic rule | Signal | Leader inline-edits | Dispatch round N+1 to Developer | |---|---|---| | Fix scope | Spelling / formatting / paragraph order / string typo only | Any semantic or content change | | Time to fix | ≤ 5 minutes | > 5 minutes | | New decisions required | None | Any | Still record inline fixes in the meeting log. Reasoning: "limit is an upper bound, not a requirement"; burning a whole round just for typos is uneconomical. ## Dispatch contract for Code Reviewer Every Code Reviewer dispatch prompt must include: 1. **Target file / section scope** (explicit file + section) 2. **Forbidden tools**: CLI is forbidden for probing API/runtime behavior (build, run, simctl, trial-and-error); read-only search (grep, rg, git log, git show) is allowed 3. **Allowed tools**: WebSearch / WebFetch (for verifying Apple APIs, library behaviour) 4. **Review criteria** (4 dimensions + domain-specific checklist): - Technical correctness (API name, behaviour, version) - Logical consistency (internal contradictions, cross-section conflicts) - Completeness (missing edge case, error handling, prerequisite) - Efficiency (algorithm, CI / build / runtime cost) 5. **Return format**: BLOCKER / MAJOR / MINOR three-level classification; each item with location (file + section) + suggestion. An **absence claim** ("the spec doesn't define this", "nothing covers this case") must include the grep/search command run and its zero-hit output — an absence claim with no evidence attached doesn't count as a finding. ## Accept / Reject reply style For each review finding the Leader gives: - **ACCEPT + reason**: accepted; specify who fixes it this round - **REJECT + reason**: rejected with a technical reason (not just "no") - **DEFER**: acknowledged but deferred (goes to backlog / open items) REJECT must cite specific evidence (API doc, prior decision, design constraint); pure preference is not acceptable. ## Anti-patterns - **Using CLI to probe Apple API behaviour**: forbidden. Use official docs / WebSearch instead. - **Repeatedly rejecting the same point in the same section**: more than 2 identical rejections counts as a communication failure; pause and clarify with the user. - **ACCEPT without a reason**: every ACCEPT should still have a one-line note of why it adds value. - **Leader drafting sections themselves**: violates the role separation; only allowed for cosmetic-grade fixes. ## Verification checklist - Every absence claim ("not defined", "no coverage") in a review finding cites the grep/search command and its zero-hit output — otherwise it doesn't count as a finding. - Each round has an explicit dispatch prompt (all 5 items of §Dispatch contract for Code Reviewer present, on top of the 6 elements from `leader-developer-handoff-contract`). - Each review finding has an explicit accept / reject label + reason. - When limit(N) is reached without convergence, pause; don't keep iterating indefinitely. - Cosmetic fixes are inline-edited by the Leader; don't burn a round on them. - The round-summary is recorded in the meeting log (not a verbatim copy of review content). ## Phase TODO sweep checklist A separate close-the-loop activity that fires **once per phase** (not once per review round), so it's not part of the round structure above. Before the Leader signs off on a phase-completion PR, run a sweep against the phase's diff scope to catch deferred-and-forgotten debt. For the full sweep procedure, command, disposition rule, and anti-pattern, read `references/phase-todo-sweep.md`. ## Related skills - `leader-developer-handoff-contract`: details the 6 required elements of every dispatch prompt. - `spec-phase-orchestration`: review cycles are usually embedded in the spec phase. - `methodology-pattern-extractor`: "round-1 cosmetic inline edit" is a codifiable pattern. - Official sources: when verifying or updating a factual or version-sensitive claim, read `references/official-docs.md`.
Comments (0)
Sign in to join the conversation.
Reviews (0)
No reviews yet.
No comments yet.