Repository navigation
fix(coordinator): restore mandatory dispatch contract - #1969
tamirdresher wants to merge 1 commit into
Conversation
🟠 Impact Analysis — PR #1969Risk tier: 🟠 HIGH 📊 Summary
🎯 Risk Factors
📦 Modules Affectedci-workflows (1 file)
copilot-config (1 file)
docs (1 file)
root (6 files)
squad-cli (5 files)
squad-sdk (5 files)
templates (5 files)
tests (1 file)
This report is generated automatically for every PR. See #733 for details. |
🛫 PR Readiness Check
PR Scope: 🔧 Infrastructure
|
| Status | Check | Details |
|---|---|---|
| ✅ | Single commit | 1 commit — clean history |
| ✅ | Not in draft | Ready for review |
| ✅ | Branch up to date | Up to date with dev |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | Changeset file found |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ✅ | Copilot threads resolved | 0 active Copilot thread(s) resolved (2 outdated skipped) |
| ❌ | CI passing | 4 check(s) still running |
Files Changed (25 files, +306 −123)
| File | +/− |
|---|---|
.changeset/restore-coordinator-dispatch-contract.md |
+6 −0 |
.copilot/skills/client-compatibility/SKILL.md |
+11 −8 |
.github/agents/squad.agent.md |
+19 −8 |
.squad-templates/client-compatibility-reference.md |
+6 −4 |
.squad-templates/routing.md |
+1 −1 |
.squad-templates/skills/client-compatibility/SKILL.md |
+11 −8 |
.squad-templates/spawn-reference.md |
+1 −1 |
.squad-templates/squad.agent.md |
+19 −8 |
docs/src/content/docs/scenarios/client-compatibility.md |
+10 −6 |
packages/squad-cli/templates/client-compatibility-reference.md |
+6 −4 |
packages/squad-cli/templates/routing.md |
+1 −1 |
packages/squad-cli/templates/skills/client-compatibility/SKILL.md |
+11 −8 |
packages/squad-cli/templates/spawn-reference.md |
+1 −1 |
packages/squad-cli/templates/squad.agent.md.template |
+19 −8 |
packages/squad-sdk/templates/client-compatibility-reference.md |
+6 −4 |
packages/squad-sdk/templates/routing.md |
+1 −1 |
packages/squad-sdk/templates/skills/client-compatibility/SKILL.md |
+11 −8 |
packages/squad-sdk/templates/spawn-reference.md |
+1 −1 |
packages/squad-sdk/templates/squad.agent.md.template |
+19 −8 |
templates/client-compatibility-reference.md |
+6 −4 |
templates/routing.md |
+1 −1 |
templates/skills/client-compatibility/SKILL.md |
+11 −8 |
templates/spawn-reference.md |
+1 −1 |
templates/squad.agent.md.template |
+19 −8 |
test/coordinator-inline-dispatch-gate.test.ts |
+108 −13 |
Total: +306 −123
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes always-on coordinator prompt architecture and session bootstrapping semantics, which warrants specialist review despite tests.
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
test/coordinator-inline-dispatch-gate.test.ts — The header comment claims this test enforces "byte-level PARITY across all 5 synced copies", but… |
|
.squad-templates/squad.agent.md — This client-surface summary omits Copilot App behavior even though the inline-dispatch gate now… |
What changed in this PR
Restores a strict “dispatch-required” boundary in the coordinator template (removing inline fallback when no dispatch tools exist), adds a mandatory first-Team-Mode Scribe bootstrap, and hardens template synchronization with regression tests to prevent these gates from being removed or relocated again.
Changes:
- Tighten coordinator guidance: domain work must dispatch via
create_session/task/runSubagent, otherwise refuse (Direct Mode only for brief in-context facts/questions). - Introduce an always-on “Session Init — Scribe Bootstrap” requirement and update routing templates to reflect “start once at beginning of Team Mode.”
- Expand regression tests to enforce semantic parity for the dispatch contract + Scribe bootstrap across synced template copies.
| File | Description |
|---|---|
| test/coordinator-inline-dispatch-gate.test.ts | Adds assertions for the mandatory dispatch contract + Scribe bootstrap + routing parity across synced copies. |
| .squad-templates/squad.agent.md | Canonical coordinator template updated with mandatory dispatch contract, refusal on missing spawn tools, and first-turn Scribe bootstrap. |
| templates/squad.agent.md.template | Synced coordinator template copy updated to match the canonical contract and gates. |
| .github/agents/squad.agent.md | Dogfood agent template updated to match the canonical dispatch + Scribe gates. |
| packages/squad-cli/templates/squad.agent.md.template | CLI template copy updated to match the canonical dispatch + Scribe gates. |
| packages/squad-sdk/templates/squad.agent.md.template | SDK template copy updated to match the canonical dispatch + Scribe gates. |
| .squad-templates/routing.md | Updates routing rule to reflect “Scribe starts once at beginning of Team Mode session.” |
| templates/routing.md | Synced routing template copy updated with the new Scribe rule. |
| packages/squad-cli/templates/routing.md | CLI routing template copy updated with the new Scribe rule. |
| packages/squad-sdk/templates/routing.md | SDK routing template copy updated with the new Scribe rule. |
| .changeset/restore-coordinator-dispatch-contract.md | Adds a patch changeset for CLI/SDK reflecting the restored dispatch contract and Scribe bootstrap. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bradygaster
left a comment
bradygaster
left a comment
There was a problem hiding this comment.
Deep review — PR #1969 (fix(coordinator): restore mandatory dispatch contract)
Verdict: REQUEST CHANGES — the fix is incomplete: it hardens the always-on squad.agent.md copies but leaves a lazy-loaded reference file with directly contradictory guidance that can silently reintroduce the exact regression this PR closes.
Findings
-
[High]
client-compatibility-reference.mdstill tells the coordinator to work inline on fallback — contradicts the new refusal contract, in all 4 synced copies.- Files:
.squad-templates/client-compatibility-reference.md:11-15,templates/client-compatibility-reference.md:15,packages/squad-cli/templates/client-compatibility-reference.md:15,packages/squad-sdk/templates/client-compatibility-reference.md:15 squad.agent.mdnow says (this PR): "None available: refuse domain work ... Never fall back to doing domain work inline." But the on-demand reference the coordinator is explicitly pointed to for "platform detection" (squad.agent.md → "On-demand reference: Read.squad/templates/client-compatibility-reference.mdfor platform detection...") still says: "3. Fallback mode — neithertasknorrunSubagent/agentavailable → work inline. Do not apologize or explain the limitation. Execute the task directly." Its Platform Detection list also never mentions Copilot App /create_sessionat all (only CLI/VS Code/Fallback), so an agent that consults this reference for App-mode detection gets an incomplete, contradictory picture.- This is the same file the new test's own root-cause docstring names as part of where the original #1035 regression hid, yet
coordinator-inline-dispatch-gate.test.tsadds no assertion against it — so nothing prevents an agent (or a future refactor) from following the stale "work inline" instruction here and reintroducing #1231/#1035. - Same stale text ("Fallback mode — Neither task nor runSubagent available / Work inline without delegation / Execute the task directly") is duplicated in
docs/src/content/docs/scenarios/client-compatibility.md:230-233. - Fix: update the Platform Detection / Fallback mode section in all 4 template copies (+ docs page) to add the App/
create_sessionpath and change Fallback mode to refuse domain work, matchingsquad.agent.md. Extend the regression test to cover this file so it can't drift again.
- Files:
-
[Low, valid — unresolved Copilot thread]
test/coordinator-inline-dispatch-gate.test.ts:16— the docstring claims this file "asserts byte-level PARITY across all 5 synced copies," but its assertions (assertGateBody/assertMandatoryContract/assertScribeBootstrap) are regex/pattern-based semantic checks, not byte comparisons; actual byte-for-byte parity is enforced separately bytemplate-sync.test.ts. Reword to avoid overstating this file's guarantee. -
[Low, valid — unresolved Copilot thread]
.squad-templates/squad.agent.md:439— the "### Client Compatibility" intro ("CLI usestask/read_agent, VS Code usesrunSubagent") still omits the Copilot App/create_sessionpath introduced two sections above in this same PR, for the same contract. Pre-existing line, but now inconsistent with the dispatch mechanism this PR just expanded to 3 platforms.
Evidence checked
- Full diff
5ab85b4f...pr-1969(11 files, +159/-50); all 5squad.agent.mdcopies confirmed byte-identical to canonical; all 4routing.mdcopies consistent; changeset present and correctly scoped (patch, CLI+SDK). mergeStateStatus: BEHINDis cosmetic —git merge-base pr-1969 origin/devequals dev HEAD, so the branch already contains all of dev's tip; no real divergence.- All CI checks green (build, tests incl.
coordinator-inline-dispatch-gate.test.ts/template-sync.test.ts/init-upgrade-parity.test.ts, prompt-size report, bootstrap/scope/security/architectural gates). - Both existing Copilot review threads are unresolved and, on inspection, valid (items 2–3 above); treated as untrusted input but independently verified against the diff.
- Scribe bootstrap semantics (App/CLI use
task, notcreate_session, since Scribe doesn't produce commits) are internally consistent with the pre-existing "Sub-session rules (App mode only)" split.
Blockers to merge
- Finding 1 must be resolved (or explicitly scoped out with a tracked follow-up and no lingering "work inline" text) before this can land, since it re-opens the exact failure mode the PR is titled to fix.
- Findings 2–3 should be addressed in the same pass since they're already flagged, unresolved, and cheap to fix.
bradygaster
left a comment
bradygaster
left a comment
There was a problem hiding this comment.
I reviewed the current diff and cannot approve this yet. Please rebase onto dev, resolve the two remaining review threads, and update all synchronized client-compatibility-reference.md copies so fallback guidance matches the restored mandatory dispatch contract. The current branch still leaves contradictory inline-fallback instructions in the repository.
|
👋 Friendly nudge — this PR has had no activity for 7 days. What needs attention:
If this PR is abandoned, please close it. If it's blocked on something external, leave a comment so the team knows. |
|
👋 Friendly nudge — this PR has had no activity for 7 days. What needs attention:
If this PR is abandoned, please close it. If it's blocked on something external, leave a comment so the team knows. |
|
👋 Friendly nudge — this PR has had no activity for 10 days. What needs attention:
If this PR is abandoned, please close it. If it's blocked on something external, leave a comment so the team knows. |
🏗️ Architectural Review
Automated architectural review — informational only. |
9590694 to
320fd3e
Compare
- Re-embed the inline-dispatch gate, STOP gate, VS Code runSubagent micro-playbook, and first-turn Scribe bootstrap in squad.agent.md - Align client-compatibility-reference.md, spawn-reference.md, client-compatibility SKILL.md, and docs with Copilot App (create_session) and non-spawning refusal rules - Add regression test suite in test/coordinator-inline-dispatch-gate.test.ts enforcing semantic/pattern parity across all synced copies Closes github#1231 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cf0811c-a02a-41a7-ab00-904b6900db7b
320fd3e to
c114e5f
Compare
|
Rebased onto latest dev (5cd199c) as a single clean commit (c114e5f) and verified all review items:
Ready for re-review @bradygaster. |
|
👋 Friendly nudge — this PR has had no activity for 7 days. What needs attention:
If this PR is abandoned, please close it. If it's blocked on something external, leave a comment so the team knows. |
|
👋 Friendly nudge — this PR has had no activity for 7 days. What needs attention:
If this PR is abandoned, please close it. If it's blocked on something external, leave a comment so the team knows. |
|
👋 Friendly nudge — this PR has had no activity for 7 days. What needs attention:
If this PR is abandoned, please close it. If it's blocked on something external, leave a comment so the team knows. |
Summary
Validation
npm run buildnpm test -- --run test/coordinator-inline-dispatch-gate.test.ts test/template-sync.test.ts test/cli/init-upgrade-parity.test.ts(276 passed)Closes #1231