Skip to content

Commit 182b2fc

Browse files
bradygasterCopilot
andcommitted
fix: replace eager fan-out with minimum-sufficient dispatch
Squad's coordinator doctrine told every surface to spawn eagerly: fan out on "Team, ...", pre-spawn anticipatory downstream agents, and let Ralph open an agent per actionable issue. That burned agent budget on speculative work and made real parallelism indistinguishable from noise. Coordinator doctrine - `squad.agent.md` and `routing.md` now encode one primary agent by default, a second only for a genuinely independent concern or a required reviewer, a hard cap of 2 domain agents per request and 3 in flight, no speculative or anticipatory agents, sync-vs-background chosen from the actual dependency, and explicit stop conditions. - Genuine parallelism for provably independent work is preserved. Reviewer protocol - The embedded Reviewer Rejection Protocol no longer mandates lockout on every rejection or a third-agent revision cycle. Lockout applies only to substantive rejections; nits (<5 changed LOC, non-blocking, no logic/security/API change) are fixed in the same PR. Review is capped at two passes; Pass-2 disagreement routes to a single Flight arbitration; deadlock is only reachable after arbitration. - The shipped `reviewer-protocol` skill templates were stale mirrors of the retired rule and now match the authoritative skill. Ralph runtime caps - `squad watch --execute` and `ralph-instructions.md` stop telling Ralph to spawn an agent per actionable issue. Ralph works the highest priority items within the dispatch caps and queues the rest. Ralph is exempt from the caps; the domain agents it spawns are not. Team state - Agent histories and `decisions.md` were condensed to reduce hot context; every removed line is preserved verbatim in the matching `history-archive-*.md` / `decisions-archive.md`. - Charters, `team.md`, `routing.md`, `ceremonies.md`, and the superseded `.squad/scribe-charter.md` scaffolding copy were reconciled with the authoritative sources. Template mirrors under `templates/`, `packages/squad-cli/templates/`, `packages/squad-sdk/templates/`, and `.github/agents/` are byte-identical copies required by `scripts/sync-templates.mjs`, `scripts/sync-skill-templates.mjs`, and `test/template-sync.test.ts`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1d252323-965f-4d27-b332-890801ac0d23
1 parent 5ab85b4 commit 182b2fc

82 files changed

Lines changed: 3685 additions & 2476 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@bradygaster/squad-cli": patch
3+
"@bradygaster/squad-sdk": patch
4+
---
5+
6+
Replace the coordinator's eager fan-out doctrine with minimum-sufficient dispatch. `squad.agent.md` and `routing.md` now encode one primary agent by default, a second agent only for an independent concern or a required reviewer, a hard cap of 2 domain agents per request and 3 in flight, no speculative or anticipatory agents, sync-vs-background chosen from the actual dependency rather than a background default, and explicit stop conditions. Fast dispatch and true parallelism for provably independent work are preserved.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
"@bradygaster/squad-cli": patch
3+
"@bradygaster/squad-sdk": patch
4+
---
5+
6+
Align the shipped reviewer-protocol and Ralph dispatch behavior with minimum-sufficient dispatch.
7+
8+
The coordinator template's embedded Reviewer Rejection Protocol no longer mandates lockout on every rejection or a third-agent revision cycle. It now encodes the current authority: lockout applies only to substantive rejections; nits (<5 changed LOC, non-blocking, no logic/security/API change) are fixed in the same PR with no lockout; review is capped at two passes; Pass-2 disagreement routes to a single Flight arbitration; deadlock is only reachable after arbitration. The shipped `reviewer-protocol` skill templates were stale mirrors of the retired rule and now match the authoritative skill.
9+
10+
`squad watch --execute` and the shipped `ralph-instructions.md` no longer tell Ralph to spawn an agent for every actionable issue. Ralph now works the highest-priority actionable items within the team's dispatch caps — max 2 active domain agents per request, max 3 tasks in flight, max 1 in-flight task per agent, no speculative work — and queues the rest. Ralph itself remains exempt from the caps; the domain agents it spawns are not.
11+
12+
Published docs on parallel execution and the reviewer protocol were rewritten from eager fan-out to minimum-sufficient dispatch, preserving genuine parallelism for provably independent work.

‎.copilot/skills/reviewer-protocol/SKILL.md‎

Lines changed: 113 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
---
22
name: "reviewer-protocol"
3-
description: "Reviewer rejection workflow and strict lockout semantics"
3+
description: "Reviewer rejection workflow, strict lockout semantics scoped to substantive rejections, 2-pass review cap with Flight arbitration, and no duplicate post-approval verification outside security/data-loss/release/migration classes"
44
domain: "orchestration"
55
confidence: "high"
66
source: "extracted"
77
---
88

99
## Context
1010

11-
When a team member has a **Reviewer** role (e.g., Tester, Code Reviewer, Lead), they may approve or reject work from other agents. On rejection, the coordinator enforces strict lockout rules to ensure the original author does NOT self-revise. This prevents defensive feedback loops and ensures independent review.
11+
When a team member has a **Reviewer** role (e.g., Tester, Code Reviewer, Lead), they may approve or reject work from other agents. On a **substantive** rejection, the coordinator enforces strict lockout rules to ensure the original author does NOT self-revise. This prevents defensive feedback loops and ensures independent review — without turning every one-line nit into a lockout, a second review pass, or a second independent verification.
1212

1313
## Patterns
1414

@@ -17,23 +17,92 @@ When a team member has a **Reviewer** role (e.g., Tester, Code Reviewer, Lead),
1717
When a team member has a **Reviewer** role:
1818

1919
- Reviewers may **approve** or **reject** work from other agents.
20-
- On **rejection**, the Reviewer may choose ONE of:
20+
- On a **substantive rejection** (see Scope below), the Reviewer may choose ONE of:
2121
1. **Reassign:** Require a *different* agent to do the revision (not the original author).
2222
2. **Escalate:** Require a *new* agent be spawned with specific expertise.
2323
- The Coordinator MUST enforce this. If the Reviewer says "someone else should fix this," the original agent does NOT get to self-revise.
24+
- On a **nit** (see Scope below), the original author fixes it in the same PR — no reject, no lockout, no revision cycle.
2425
- If the Reviewer approves, work proceeds normally.
2526

27+
### Scope of Lockout — Substantive Rejections Only
28+
29+
Lockout applies **only** to a rejection for a substantive reason:
30+
31+
- A logic defect, regression, or missing coverage for a real bug.
32+
- A security issue.
33+
- An unintended change to API surface or observable behavior.
34+
35+
Lockout does **NOT** apply — the finding is a **nit**, not a rejection — when ALL of the
36+
following hold:
37+
38+
- Non-blocking: no behavior, security, or API change results from the finding itself.
39+
- Fewer than **5 changed lines** to fix it.
40+
- No logic, security, or API/behavior change involved (e.g., typo, comment, wording, dead
41+
import, formatting).
42+
43+
**Objective test:** if the finding needs its own explanation of a behavior change, it's
44+
substantive. If it's a one-line diff a linter could plausibly have flagged, it's a nit.
45+
46+
**Nits close in the same PR.** The original author fixes them before merge — never filed as
47+
a follow-up issue, never treated as a rejection, never triggering lockout or another review
48+
pass. (Evidence this needs saying: PR #1837/#1812 was "approved with nits" but one real nit
49+
was deferred to a follow-up commit by a different agent instead of fixed in the reviewed PR.)
50+
2651
### Strict Lockout Semantics
2752

28-
When an artifact is **rejected** by a Reviewer:
53+
When an artifact is **rejected for a substantive reason** (see Scope above):
2954

3055
1. **The original author is locked out.** They may NOT produce the next version of that artifact. No exceptions.
3156
2. **A different agent MUST own the revision.** The Coordinator selects the revision author based on the Reviewer's recommendation (reassign or escalate).
3257
3. **The Coordinator enforces this mechanically.** Before spawning a revision agent, the Coordinator MUST verify that the selected agent is NOT the original author. If the Reviewer names the original author as the fix agent, the Coordinator MUST refuse and ask the Reviewer to name a different agent.
3358
4. **The locked-out author may NOT contribute to the revision** in any form — not as a co-author, advisor, or pair. The revision must be independently produced.
3459
5. **Lockout scope:** The lockout applies to the specific artifact that was rejected. The original author may still work on other unrelated artifacts.
35-
6. **Lockout duration:** The lockout persists for that revision cycle. If the revision is also rejected, the same rule applies again — the revision author is now also locked out, and a third agent must revise.
36-
7. **Deadlock handling:** If all eligible agents have been locked out of an artifact, the Coordinator MUST escalate to the user rather than re-admitting a locked-out author.
60+
6. **Lockout duration:** The lockout persists for that revision cycle. If the revision (Pass 2) is **also** rejected for a substantive reason, do NOT spawn a third agent to revise again — route to **Flight arbitration** (see Review Pass Cap below). The revision author remains locked out pending Flight's ruling.
61+
7. **Deadlock handling:** If Flight's arbitration ruling itself cannot be executed because every eligible agent is already locked out, the Coordinator MUST escalate to the user rather than re-admitting a locked-out author.
62+
63+
### Review Pass Cap and Arbitration
64+
65+
Ordinary review is capped at **2 passes** per artifact:
66+
67+
1. **Pass 1** — initial review.
68+
2. **Pass 2** — review of the revision produced under lockout.
69+
70+
If Pass 2 disagreement persists (rejects again), the Coordinator does **NOT** spawn a third
71+
full review pass or a third revision cycle. Instead:
72+
73+
- Route the disagreement to **one Flight arbitration** — a single ruling, not a re-review.
74+
- Flight reads both passes' findings and the artifact, then rules: merge, revise (naming the
75+
revision owner, still excluding all locked-out agents), or reject. Flight's ruling is
76+
final and recorded in the PR or decision log.
77+
- Arbitration ends the review; it does not restart the pass count or spawn a Pass 3.
78+
79+
(Evidence: FIDO history, 2026-08-22, PR #1832 — three full review passes ran on one PR before
80+
merge. Pass 3 there would now be a single Flight arbitration instead.)
81+
82+
### No Duplicate Independent Verification After Approval
83+
84+
Once a Reviewer **approves** an artifact and CI is **green**, no second agent independently
85+
re-verifies it from scratch. Approval + green CI is sufficient to merge.
86+
87+
**Exceptions — independent duplicate verification is still required:**
88+
89+
- **Security-sensitive surfaces** — anything RETRO owns or co-owns per `routing.md`
90+
(e.g. `packages/squad-sdk/src/hooks/`, `packages/squad-sdk/src/memory/`), or anything
91+
matching a trigger in `.copilot/skills/security-review/SKILL.md`.
92+
- **Data-loss-risk changes** — deletions, force operations, destructive migrations.
93+
- **Release/publish pipeline changes** — anything gated by `.squad/skills/release-process/SKILL.md`.
94+
- **Irreversible migrations** — one-way schema or data transforms that can't be undone by
95+
re-running the change.
96+
97+
Outside these four classes, do not add a second reviewer "just to be safe" — that is
98+
duplicate verification, not risk-based review.
99+
100+
### Default Reviewer Count
101+
102+
Default is **one reviewer** per PR — the module's advisory secondary per `routing.md`, or
103+
Flight if the module has none listed. Add **RETRO** as an additional reviewer only when the
104+
PR touches an actual security-sensitive surface (see exceptions above). Do not co-assign
105+
RETRO to review changes outside that surface.
37106

38107
## Examples
39108

@@ -54,12 +123,18 @@ When an artifact is **rejected** by a Reviewer:
54123
5. New agent produces v2
55124
6. Keaton reviews v2
56125

57-
**Example 3: Deadlock handling**
58-
1. Fenster writes module → rejected
59-
2. Verbal revises → rejected
60-
3. Hockney revises → rejected
61-
4. All 3 eligible agents are now locked out
62-
5. Coordinator: "All eligible agents have been locked out. Escalating to user: [artifact details]"
126+
**Example 3: Third-round disagreement → Flight arbitration, not deadlock**
127+
1. Fenster writes module → Hockney rejects (Pass 1, substantive)
128+
2. Verbal revises → Hockney rejects again (Pass 2, substantive)
129+
3. Coordinator does NOT spawn a third revision cycle — routes to Flight arbitration
130+
4. Flight reviews both passes, rules "merge with Verbal's revision; the remaining nit is
131+
non-blocking" — ruling recorded, review ends
132+
133+
**Example 3b: Genuine deadlock (rare)**
134+
1. Fenster writes module → rejected; Verbal revises → rejected (Pass 2)
135+
2. Flight arbitrates: "needs another revision, but by someone who hasn't touched this yet"
136+
3. No third eligible agent exists (small team, both already locked out)
137+
4. Coordinator: "All eligible agents have been locked out. Escalating to user: [artifact details]"
63138

64139
**Example 4: Reviewer accidentally names original author**
65140
1. Fenster writes module → rejected
@@ -68,18 +143,43 @@ When an artifact is **rejected** by a Reviewer:
68143
4. Hockney: "Verbal, then"
69144
5. Coordinator spawns Verbal
70145

146+
**Example 5: Nit — no lockout, fixed in the same PR**
147+
1. Fenster's PR renames a local variable for clarity in one extra line while fixing the
148+
requested change (4 lines total, no logic/security/API change)
149+
2. Hockney: "Approve with nit — the variable rename is fine, no objection"
150+
3. Coordinator: this is a nit (< 5 lines, no behavior change) — no rejection, no lockout
151+
4. Fenster fixes any remaining nit text directly in the same PR before merge; Hockney
152+
approves; PR merges — no second review pass, no follow-up issue
153+
71154
## Related Review Skills
72155

73156
For domain-specific review checklists, see:
74157
- **Architectural Review:** `.copilot/skills/architectural-review/SKILL.md` — module boundaries, dependency direction, export surface, sweeping refactor safety
75158
- **Security Review:** `.copilot/skills/security-review/SKILL.md` — credentials, injection, workflow permissions, supply chain
76159

160+
## What Stays Unchanged
161+
162+
This recovery narrows *lockout scope* and *review-pass* overhead. It does NOT touch:
163+
164+
- **Mutation testing** — still required wherever it already applies; unaffected by pass caps.
165+
- **Adversarial repro** — still required for the four exception classes in "No Duplicate
166+
Independent Verification," and whenever a reviewer chooses it as their method on Pass 1 or 2.
167+
- **CI gates** — unchanged; approval still requires green CI, always.
168+
- **Risk-based deep review** — a reviewer may go as deep as the change warrants within their
169+
2 passes; the cap bounds *unresolved disagreement*, not diligence on a single pass.
170+
77171
## Anti-Patterns
78172

79-
- ❌ Allowing the original author to self-revise after rejection
173+
- ❌ Allowing the original author to self-revise after a substantive rejection
80174
- ❌ Treating the locked-out author as an "advisor" or "co-author" on the revision
81175
- ❌ Re-admitting a locked-out author when deadlock occurs (must escalate to user)
82176
- ❌ Applying lockout across unrelated artifacts (scope is per-artifact)
83177
- ❌ Accepting the Reviewer's assignment when they name the original author (must refuse and ask for a different agent)
84178
- ❌ Clearing lockout before the revision is approved (lockout persists through revision cycle)
85179
- ❌ Skipping verification that the revision agent is not the original author
180+
- ❌ Treating a nit (< 5 changed lines, no logic/security/API change) as a rejection that triggers lockout
181+
- ❌ Deferring a nit to a follow-up issue instead of fixing it in the same PR before merge
182+
- ❌ Running a third full review pass instead of routing Pass-2 disagreement to Flight arbitration
183+
- ❌ Spawning a second independent reviewer after approval + green CI outside the security/data-loss/release/migration exceptions
184+
- ❌ Adding RETRO to a review that doesn't touch a security-sensitive surface
185+
- ❌ Skipping mutation testing, adversarial repro, or CI gates on the theory that the pass cap also caps diligence

0 commit comments

Comments
 (0)