Coordinator Running Review Cycles

---
name: coordinator-running-review-cycles
description:
  "Use when an implementer reports DONE, when consolidating review findings,
  when handling implementer pushback on a finding, or when arriving at a review
  gate. Loads coordinator-specific discipline for running review cycles cleanly."
# source: claude-plugin/skills/coordinator-running-review-cycles/SKILL.md
---

## Coordinator: Running Review Cycles

### Run code-quality + spec-compliance review in parallel

**Why:** Reviewers serve different purposes. `feature-dev:code-reviewer` catches
security, error-handling, idiom, and dead-code issues. `verify-against-plan`
catches missing scope, unmet acceptance criteria, and silent deviations from the
plan. Running them sequentially doubles review-cycle latency for no reason —
they're independent reads of the same diff.

**How to apply:** When an implementer reports DONE on a branch, dispatch both
reviewers as parallel sub-agents in the same message. Pass each a clear scope
("review the diff at <BASE>...<HEAD> for <criteria>").

> **Model tiers:** pass an explicit `model:` on every dispatch; choose model and
> effort per the `choosing-subagent-models` skill (read `runtime.role_models` at
> the moment of use). Never let a sub-agent inherit your own model.

### Wait for both reviewers before sending anything to the implementer

**Why:** Sending findings from reviewer A while reviewer B is still running
causes the implementer to fix batch 1 and miss batch 2 entirely, extending the
review cycle by a full extra round.

**How to apply:** Block until both review sub-agents return their full output.
Do not send partial findings, even if the user is asking for an update. If one
reviewer takes substantially longer, the right move is a status update to the
implementer ("review running, hold"), not a partial batch.

### Consolidate findings into ONE numbered list across all severities

**Why:** A single numbered list with mixed severities lets the implementer work
top-to-bottom without context-switching across "minor batch" and "critical
batch" calls. Severity labels live next to each finding; ordering is by
severity, then by file path. Two separate lists invite the implementer to skip
one.

**How to apply:** Merge both reviewers' output. Number sequentially from

1. Each item: `<severity> · <file:line> · <one-line summary>` followed by the
   detail and suggested fix. Keep the numbering stable across review rounds —
   when re-reviewing fixes, refer to "finding #4" and the implementer can locate
   it instantly.

**Route mechanical bookkeeping findings to the gate, not the implementer.** If a
consolidated finding is itself mechanical bookkeeping of the kind the gate is
authorized to self-fix (see `coordinator-merging-code`: census/shard regen,
line- number or key refreshes, ledger totals/ceilings, per-entry waivers or
citations recording a classification already established elsewhere — never a new
safety judgment of your own — nosec baseline hashes, comment wording, stale test
expectations that merely follow a landed change), do not route it to the
implementer as a normal finding. Dual review runs BEFORE the merge gate, so at
this point there is no gate run yet to fix it: tag the finding as
gate-deferred-bookkeeping in the consolidated list and carry it forward — the
gate self-fixes it (a `GATE-AUTHORED` commit plus a narrow re-run of only the
affected checks) when it runs the merge gates on this branch. Reserve immediate
implementer round-trips for real defects and design judgment calls.

### Verify reviewer claims against actual code before forwarding

**Why:** Reviewers reading a large diff sometimes cite wrong line numbers or
describe behavior that doesn't match the current code. Forwarding a misread
finding causes the implementer to waste time investigating a non-issue and
erodes trust in the review process.

**How to apply:** For any finding citing a specific file and line, read that
file at that range before forwarding. For API-shape claims, grep for the symbol
and read the signature. For behavior claims, trace the call path. Drop or
correct findings that don't survive verification — silently forwarding wrong
findings poisons the cycle.

### Treat implementer pushback as a signal to verify, not dismiss

**Why:** When an implementer disputes a finding ("you said X, but the actual
shape is Y — here's the trace"), that is the correct behavior, not
insubordination. The coordinator may have stated expectations that drift from
runtime reality. Treating pushback as a challenge breaks the feedback loop and
trains implementers to swallow corrections.

**How to apply:** When an implementer pushes back: read the cited file at the
cited lines, run the cited command, or `bd show <id>` for beads state. If
they're right, acknowledge the correction explicitly and update session context.
If the finding was based on a misread, drop it from the batch and tell them so.
If you're still right after verification, restate with the new evidence — but
never just reassert without verification.

### Re-review after fixes land — repeat until both pass

**Why:** A single review pass rarely catches everything, and fix commits can
introduce new regressions. Skipping the re-review accepts an unknown risk that
scales with diff size.

**How to apply:** When the implementer reports fixes complete, re-dispatch both
reviewers (in parallel) on the new diff. Most of the time, round 2 returns clean
or with one minor finding. If round 3 still has Critical or Important findings,
escalate to the user — repeated failure to land fixes signals a deeper scope or
design issue, not just sloppy review.

### Use structural evidence when a green test is blocked upstream

**Why:** A test that cannot run (blocked upstream build, unrelated flake, CI
environment issue) cannot validate behavior. Saying "no errors seen" is weaker
than enumerating what a regression would produce and showing none of those
signatures occurred.

**How to apply:** When a green-test check is blocked, prove absence of
regression by: (1) listing specific failure modes a regression would produce
(error types, wrong status codes, missing state changes); (2) showing none of
those modes occurred (wire-level status codes, specific log assertions, expected
state present); (3) documenting the upstream blocker in a tracking issue so the
green-test check can be re-run when it clears.

### Run the philosophy merge gate before the actual merge

**Why:** Dual review (code-quality + spec-compliance) is general-purpose and
does not carry the project-specific structural lenses that
`.thrum/philosophy.md` requires — folding them in as dual-review findings would
dilute both the general review and the specialized gate.

**How to apply:** After dual review passes clean, before running `git merge`,
invoke `coordinator-philosophy-merge-gate` for any branch that introduces or
materially changes a service, handler, background job, adapter, or RPC boundary.
Skip it for small/mechanical fixes. The gate walks all anti-patterns and red
flags in `.thrum/philosophy.md`, with single-responsibility (SRP) as one lens
(Lens 10) among the full set — not a separate pass.

### Prose-pre vs code-post: what this review cycle covers

**Why:** Once the planning-skill review loop ships, there are two distinct
review surfaces and conflating them double-reviews one artifact. The planning
loop reviews PROSE artifacts (brainstorm, plan, impl-prompt) BEFORE
implementation; the review cycle in THIS skill reviews the implementer's CODE
AFTER they report DONE. They target different artifacts and both are needed.

**How to apply:**

- The planning loop's prompt gate already reviewed the impl-prompt; its
  `THRUM-REVIEW: stage=prompt verdict=Ready:Yes` stamp satisfies the
  pre-dispatch review (see `coordinator-dispatching-work`). Do not re-review the
  prompt here.
- This skill's dual review (code-quality + spec-compliance, in parallel) runs on
  the CODE the implementer produced — the diff on their branch. That always runs
  at DONE regardless of any planning-loop stamps, because the code is a new
  artifact the planning loop never saw.
- Net: the prompt is reviewed once (in the planning loop, pre-dispatch); the
  code is reviewed once (here, post-DONE). No artifact is reviewed twice.

### See also

- `coordinator-branch-split-on-block` — when a merge gate or this skill's dual
  review BLOCKS on one concern of a branch that bundles multiple independent
  concerns, while the other concern(s) are clean.

### Project-specific rules (already loaded)

Load the $thrum:project-rules-protocol skill and follow it.

If you accumulate a new rule mid-session (the user corrects you), capture it via
the Memory section of your role preamble — it references the `thrum-memory`
skill for the canonical `thrum memory create` shape.