Skip to content

fix(a2a): isolate Peer Agent workspaces by default - #396

Merged
bryantderosier merged 40 commits into
j5/mainfrom
j5/spawn-agent-workspace
Oct 8, 2026
Merged

bryantderosier merged 40 commits into
j5/mainfrom
j5/spawn-agent-workspace

Conversation

@bryantderosier

@bryantderosier bryantderosier commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Closes #274. spawn_agent used to copy the caller's branch and worktree into every Peer Agent, so parallel agents edited the same checkout. Now every spawn and every Crew seat has to say where it works. There's no default, per @Jacksondr5's direction on 2026-10-03.

 spawn_agent / propose_crew seat / request_crew_member
-  workspace?: shared | worktree        # default picked from the caller's checkout
+  workspace:  { type: "shared" }                                       # caller's checkout
+            | { type: "worktree", base_ref, branch?, start_from_origin? }  # new worktree via upstream ThreadLaunch
+            | { type: "existing_worktree", worktree_path }             # one of this project's git worktrees

How a spawn starts, by type:

spawn_agent / CrewLaunch
  withSpawnStart(threadId)            # refuses a second start in flight for the same thread
    resolveSpawnWorkspace
      shared            -> caller's checkout, no git read
      worktree          -> base_ref must resolve (hasCommit), branch must not exist
      existing_worktree -> path must match git's worktree list (main checkout excluded)
                           branch read live: invalidateLocalStatus + localStatus on that path
    create thread (create id encodes the type, so a retry can't switch it)
    record home / placement / provenance
    shared | existing_worktree -> brief starts now
    worktree                   -> upstream ThreadLaunch prepares the worktree, then releases the brief

What else changed, in short:

  • Roster card: a workspace select (Captain's checkout / New worktree / Existing worktree). A new worktree uses upstream's WorktreeBaseBranchPicker: searchable, pages past 100 branches, prefilled with the Captain's branch, with a start-from-origin toggle. An existing worktree gets a select of the project's worktrees. No branch list travels with the preview. A seat I add by hand starts at Captain's checkout.
  • Launch report: a seat whose first run is still preparing counts as pending, not started. ThreadLaunch's own "Preparing worktree" item used to read as provider activity.
  • A failed worktree preparation is left as upstream leaves it. The thread shows the failure; it isn't retried, retired, or guarded, and the spawner isn't told. Jackson's call on 2026-10-06, after three rounds of guards each grew a new edge case. The turn guard and the <j5_spawn_workspace_failed> notice are gone.
  • Older servers: if a preview comes from a server that doesn't send workspace fields, the card still shows the seats, just without the workspace control.
  • Deleted: the Captain-aware default, door, inWorktree, the git read for shared, the current-branch fallback, the "git unreadable, so share" fallback (it refuses now), the FIFO start permit, and the general worktree advice in tool text.
  • Docs: I rewrote the Workspace sections of agent-tools.md, crews.md and docs/user/personas.md, and the crews.md and agent-tools.md logs credit both decisions to Jackson.

No upstream files touched. Everything lives under apps/server/src/j5, apps/web/src/j5 and packages/contracts/src/j5.ts.

Evidence

The roster card, at d39c916 against the merge-base, same data, viewport and theme (captured by the review panel; the card doesn't change in the final head):

Before After
Roster before Roster after
Seat editor before Seat editor after

After only: base-branch search and the existing-worktree select.

  • Before: on the merge-base, a default spawn_agent wrote into the parent's checkout. The review panel reproduced it live.
    After: every input requires workspace. A worktree spawn runs in its own worktree, an existing_worktree spawn binds that worktree's live branch, and shared stays in the caller's checkout. The panel verified all three on Codex and Claude in round 3.
  • Focused tests on the final head: apps/server/src/j5/a2a, apps/server/src/j5/agents and apps/web/src/j5/crew, 654 passed, 1 env-gated skip. Server typecheck and lint are clean.
  • Regressions that each fail with their fix removed:
    seat run preparing, with a preparation item -> report still waits      (was: "started")
    two concurrent opposite-type starts         -> second refused, exactly one create
    listRefs says branch A, live status says B  -> binds B, stale preview's approval refused
    preview from an older server                -> seats shown, no workspace control
    

Merge Danger

Door: two-way

It's all J5-side code with no migrations, so reverting the squash commit restores the old behavior.

Blast Radius: agents

  • workspace is now required on spawn_agent, propose_crew and request_crew_member. An agent that leaves it out gets a schema refusal and has to pick one. A Crew proposal pending across the upgrade is refused until each seat has a workspace. Choose one in the seat's editor on the card; the editor's own preview offers the options, so there's no need to decline and re-propose.
  • A peer whose worktree fails to prepare is unbound, which upstream reads as the project's checkout, so a later message to it runs there. That's where every peer worked before this PR. Jackson's install has never hit it (3 of 61 preparations failed, all on day one, none messaged again).
  • Membership and the card's worktree list come through upstream's refreshed ref snapshot (about 5s coalescing), so a worktree created a moment ago can be refused once. The branch that actually gets bound is read live.
  • existing_worktree can put a peer into a worktree another spawn is still setting up. That's inherent to "no preparation step".
  • Stop and archive leave worktrees in place. Cleanup is a follow-up.
  • Not verified: remote or tunnel connections, mobile (it has no Crew gate, so nothing changed there), and a live re-run of the reduced version.

Models and harness: J5 Code Crew development playbook. Captain, planner and builder are Claude Code Opus 5.5. The reviewers are Claude Fable 5.1 and OpenAI GPT-6-Astra. The plan reviewer and publisher are OpenAI GPT-6-Sol. Review fixes in later rounds by Claude Code Opus 5.5.

🤖 Generated with Claude Code

bryantderosier and others added 8 commits October 2, 2026 11:14
spawn_agent and Crew seat launch copied the caller's branch and worktree
onto every new thread, so parallel Peer Agents edited one checkout.

Both doors now take an optional workspace: the caller's checkout
(shared) or a fresh worktree. A worktree spawn creates the thread
unbound, commits its home and placement, then hands the thread to
upstream's ThreadLaunch (reuseExistingThread), which provisions the
worktree, names the branch, runs setup, and releases the brief.

Defaults: spawn_agent gets a worktree wherever git can make one; a
seat shares a Captain's worktree and gets its own off the root
checkout. Seat workspaces resolve at preview and the approval token
binds them. A client_request_id stays bound to its first workspace
(choice-specific create command id, receipt recheck under a per-thread
start permit). ThreadLaunch's Squadron registration returns a home the
spawn already recorded instead of joining a second time.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Each seat row names where it will work, and the seat editor offers
Captain's checkout or New worktree, defaulting to the server's resolved
choice. The choice survives a persona change and the save.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Crew seat creates and briefs now run under the same per-thread start
permit as spawn_agent, and the permit documents its invariant: every
door that creates a spawn thread id goes through it, because the
orchestrator has no existing-thread guard and the projection upserts.

Tests cover a latch-held concurrent opposite-type start, a partial
failure followed by a restart that still refuses the other type, and
worktree options taking effect from the first accepted launch. The
spawn_agent description says a retry must repeat the same workspace.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A worktree spawn now has a test on the real orchestrator and
ThreadLaunch, with git and setup faked: the brief is held as preparing
and released once the worktree is bound, and the launch registers into
the spawn's Squadron. A failed worktree creation fails the run with
the detail the launch report and seat-finish notices read.

The J5 copy of upstream's ThreadLaunch test harness moves from the
persona launch test into j5/test-support so both tests share it.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The spawn_agent and propose_crew descriptions, the seat field, and the
docs promised root-checkout Captains a worktree per seat without
saying it needs a git repository on a branch. Outside one, or when git
cannot be read, the default shares the checkout; the wording now says so.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The spawn start permit is in-process, so it holds only while the MCP
handlers and CrewLaunch resolve the same SpawnWorkspaceService. The
production runtime topology test now counts its builds through the
shared HTTP and MCP graph and expects exactly one; a second build fails
it.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The concurrency test released the winner after a fixed number of
scheduler yields, which only allowed time for the loser to reach the
lock. The spawn start permit is now a small per-thread FIFO whose
queued state is observable: a start that finds the permit held queues
a ticket, signals (test hook only), and waits for the holder to hand
it over; an interrupted waiter withdraws its ticket.

The test now waits for that signal, racing it against a second create
so a missing permit fails deterministically. A new test covers an
interrupted waiter handing its place to the next start.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
base_ref reaches `git worktree add` as a bare positional, so a value
starting with "-" would be parsed as an option. Refs named for a
worktree must not start with "-" or contain whitespace or control
characters. The MCP and stored seat schemas check this, and so does
the resolver every door goes through, since the person's card edits
arrive through the unconstrained client contract.

Also hoists the SquadronId decode in SquadronThreadCreationService.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@bryantderosier bryantderosier added the bug Something isn't working label Oct 2, 2026
@bryantderosier bryantderosier self-assigned this Oct 2, 2026
@vercel

vercel Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
j5-code Ready Ready Preview Oct 7, 2026 10:17pm UTC

Request Review

@github-actions github-actions Bot added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Oct 2, 2026
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Repository: Jacksondr5/j5code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: aa4e8767-6264-4a84-865a-9fae19c291f8
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the size:XXL 1,000+ effective changed lines (test files excluded in mixed PRs). label Oct 2, 2026
@Jacksondr5

Jacksondr5 commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

Review panel: Claude Opus 5.5 (lead) + GPT-6.1-Sol

Reviewed at 9570521 against merge-base e1621b5. Sol tested live on an isolated dev server with scratch repos and real Codex and Claude agents; both of us read the diff independently. Everything below is agreed by both reviewers. Design direction on the defaults is with Jackson and will follow separately.

The fix works on the ordinary paths. On the merge-base the same spawn_agent call wrote into the parent's checkout. On the head, a default peer got its own worktree from the parent's committed HEAD and wrote only there. Explicit shared, detached HEAD, non-Git projects, a parent already in a worktree, persona spawns, both Crew defaults, roster-card overrides, request_crew_member, a Playbook Crew, same-key replay, opposite-type refusal, and the ref guard all behaved as the PR describes. The four screenshots match the head.

1. A peer whose worktree fails later runs in the project root (blocking)

spawnWorkspace.ts:170-176 creates a worktree peer's thread with branch and worktreePath null and relies on ThreadLaunch to bind it (:375-396). When preparation fails, the thread stays registered, addressable and unbound. Upstream's runtime policy reads a null worktreePath as the project's workspace root, so the next ordinary message starts a turn there.

Reproduced live:

  1. Parent calls spawn_agent with workspace: {type: "worktree", base_ref: "no-such-ref"}. It returns a participant and thread id.
  2. The first run fails at provision-worktree. The thread has branch=null, worktreePath=null. The parent gets no notice.
  3. Parent sends the peer a normal send_message. The peer runs in the parent's project root, on the parent's branch, and writes a file there. The thread ends with runs [failed, completed] and is still unbound.

That is the collision #274 is about, reached silently. A missing ref is only the easy trigger; any preparation failure leaves the same state. Two things are needed, and neither needs recovery machinery:

  • a peer that asked for a worktree and has none must not take ordinary turns (refuse the turn, with the reason);
  • the spawner must be told the preparation failed.

Related: the PR's invariants say an explicit worktree request Git cannot resolve is refused, but a nonexistent base_ref is accepted and fails after spawn_agent returns. Checking the ref up front is cheap and worth doing, but it does not replace the fix above.

2. Docs and tool text say setup finishes before the brief starts; it doesn't

docs/j5/product/a2a/agent-tools.md:92, docs/user/personas.md:49, the comment at spawnWorkspace.ts:211-216, and the spawn_agent description in mcp/tools.ts ("setup runs first") all say or imply the setup script completes before the agent begins. Upstream's setup scripts are asynchronous unless async=false (ProjectSetupScriptRunner.ts:437), and ThreadLaunchService.ts:488-517 releases the prepared run before awaiting them.

Reproduced live: with a setup script blocked on a gate, the child completed its turn while setup was still running. Upstream's behaviour should stay; the text should say what happens. In this repo that means a peer can start before vp i finishes.

3. The FIFO start permit can be a refusal

The race it closes is real while the two workspace types have different create command ids for one deterministic thread id. But waiting in line is not needed to close it: refusing a second start for a thread that already has one in flight, with a retry instruction, does the same job in a few lines (the existing worktree-handoff guard has that shape). makeStartPermits, the onStartQueued test hook and the tests built on it could go. The Crew path takes the permit too, and its own comment says the type check "never refuses here".

4. Merge conflict

Against current j5/main (f99c9f1) only apps/server/src/j5/a2a/mcp/tools.ts and docs/j5/product/a2a/agent-tools.md conflict, both in the propose_crew description: main reworded one phrase, this PR adds the workspace guidance. Mechanical. handlers.ts auto-merges with #423/#426/#428.

Confirmed, no action

  • Messages that arrive while a worktree is still being prepared are safe. Tested live with the checkout and a synchronous setup script each held open, from a spawner and from a sibling Crew seat: the messages queued behind the preparing run, nothing ran early, and every turn ran in the peer's own worktree once released. Finding 1 is about a preparation that fails, not one that is slow.
  • No upstream-owned file is touched and no FORK.md entry is missing; packages/contracts/src/j5.ts and docs/user/personas.md are J5's. The upstream.md and upstream-convergence.md edits are accurate.
  • The registerAtDurableLaunch early return is coherent, and we found no regression in ordinary launches.
  • Stop and archive leave worktrees in place, as the PR says. Sol's 25 minutes of testing left 13 in the scratch repo, so the follow-up for removal matters.

Not tested

Remote and tunnel connections, mobile, a live scheduled task, a real restart with the same caller session, and worktree failures other than a missing ref.

One added screenshot, for a state the PR's images don't cover: a preview from an older server (simulated by stripping workspace from the response) hides the Workspace control.
Older-server preview hides the Workspace control

bryantderosier and others added 5 commits October 3, 2026 13:53
# Conflicts:
#	apps/server/src/j5/a2a/mcp/tools.ts
#	docs/j5/product/a2a/agent-tools.md
The spawn start guard queued a second start for the same thread behind
the first in a bespoke FIFO permit. Queueing isn't needed to close the
opposite-type race: refusing a second start while one is in flight, with
a retry instruction, keeps the receipt check from racing the create it
guards. The guard now has the worktree-handoff guard's shape, and the
FIFO, its test hook, and the tests built on them are gone.

The concurrency test holds the first start inside its create and
proves the second returns refused; without the guard it reaches a
create of its own and the test fails.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… hears why

A Peer Agent spawned for a worktree of its own is created unbound and
bound by ThreadLaunch. When preparation failed it stayed registered and
unbound, and upstream reads an unbound thread as the project root, so
its next ordinary message ran in the caller's checkout.

- The worktree create command id now derives from the thread, so the J5
  message guard the orchestrator already calls (`modelMismatch`) finds
  the request by one receipt lookup. A turn for such a thread that has
  no worktree is refused with the reason, from a person or an agent;
  the brief ThreadLaunch holds as a preparing run is exempt.
- A plain spawner gets a <j5_spawn_workspace_failed> notice with the
  run's failure detail, posted by the finish notifier; Crew Captains
  already hear it from the launch report or the finish notice.
- An explicit base_ref must resolve to a commit (from origin, its
  fetched copy counts) before anything is created.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The spawn_agent description, the agent-tools Workspace rule, the Crews
user guide, and the startBrief comment said the setup script finishes
before the brief starts. Upstream's ThreadLaunch releases the brief once
the worktree is bound and awaits the setup script first only when it is
set to finish first (async: false); otherwise the agent may begin while
setup, such as installing dependencies, is still running.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The Workspace rule and two comments still described spawn starts as
queued one at a time. A second start for a thread with one in flight is
now refused with a retry instruction.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@Jacksondr5

Copy link
Copy Markdown
Owner

Design direction from Jackson (follows the panel findings above)

Jackson reviewed the product side of this PR with the panel on 2026-10-03. These are his decisions. They change the tool's shape, so they come before the fixes in the earlier comment; finding 1 there (a failed worktree leaves a peer that runs in the project root) still has to be fixed either way.

1. workspace is required on spawn_agent. There is no default.

The spawning agent chooses every time. It knows whether it is starting a reviewer that must see uncommitted work, a builder that must not collide, or a scout that can't collide at all; any default we pick is wrong for one of those, and the person can steer the choice with project instructions. Three choices:

type Where the peer works Fields
shared The spawner's own checkout none
worktree A new worktree base_ref required; branch and start_from_origin optional as now
existing_worktree A worktree that already exists worktree_path required
  • base_ref is required, with no fallback to the spawner's branch. No assumptions.
  • existing_worktree is new. The case: a director hears its builder is finished and spawns a reviewer into the builder's worktree. Use upstream's name and shape (its launch strategy already has existing_worktree with a path). It should be cheap: it is the shared path with a different path and branch, with no preparation step and none of the failure window from finding 1. Check that the path is one of this project's Git worktrees and read its branch from Git; refuse otherwise.

On upstream's "Default workspace" setting: we looked at whether the default should follow it. Upstream's own agent launch tool does not follow that setting (an omitted strategy means the project root), so J5 isn't bound by it, and with no default there is nothing to align.

2. The same on Crew seats

propose_crew and request_crew_member require each seat's workspace, with the same three choices. The Captain-aware default goes, including one worktree per seat from the root checkout. A seat the person adds by hand on the roster card starts at "Captain's checkout".

3. The roster card can edit all three

Workspace is editable on the card like harness, model, reasoning and access:

  • "New worktree" shows a base-branch select, prefilled with the Captain's current branch.
  • "Existing worktree" shows a select of the project's worktrees.
  • The server sends the local branch list and the worktree list with the preview.

4. Tool descriptions don't explain worktrees

Agents know how worktrees work. Keep only what is specific to J5: that spawn_agent returns before a new worktree is ready, and what each choice binds the peer to. Drop "commit first", "your uncommitted changes are not copied", and the advice to hand off into a task worktree before proposing.

5. Not handled, on purpose

Crew proposals already pending when a server is updated to this version have no workspace recorded. Don't build for that: updating a server with work pending accepts that risk.

What this lets the PR delete

  • The undefined-choice branch of resolveSpawnWorkspace, the door argument and inWorktree.
  • The Git inspection for shared (it reads Git today for no reason) and the current-branch lookup for worktree, since base_ref is given.
  • The fallback that turns a failed Git read into "not a repository, so share". A failed read should refuse.
  • The default-related tests, docs and tool text, and the client's hiding of the control when the server sends no workspace.

Two things it does not remove:

  • The retry guard. The thread id comes from the request key alone, so the same key retried with a different choice could still overwrite the first thread's binding. With three types the receipt check covers more cases, which is one more reason to take finding 3's route: refuse a second start that is in flight for the same thread, instead of queueing it.
  • Finding 1. An explicit choice doesn't help a worktree that fails to prepare.

Docs

docs/j5/product/features/crews.md logs this PR's Crew default as "2026-10-02 — AC3" with no decider. Replace it with the decisions above, attributed to Jackson, 2026-10-03, and rewrite the Workspace sections of agent-tools.md, crews.md and docs/user/personas.md to match.

Posted by the review panel lead (Claude Opus 5.5) at Jackson's direction.

The unbound-worktree guard refused every ordinary turn while a spawned
peer had no worktree, which includes the normal window between
spawn_agent returning and ThreadLaunch binding the worktree. A message
sent then was rejected for good (the rejected receipt replays, and an
A2A delivery exhausts into an alarm) instead of queueing behind the
brief as before. The guard now refuses only once preparation is over:
while the thread has a preparing run, a message queues and runs in the
worktree once it is bound.

The spawner notice reads its spawner with getThreadProjectionIfPresent
and catches its own failures, so a spawner that is gone or unreadable
is skipped quietly.

Refs #274

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@bryantderosier

Copy link
Copy Markdown
Collaborator Author

@Jacksondr5 thanks, this was a really useful pass. All four findings are fixed and pushed:

  1. Failed worktree peer running in the project root. Fixed in 5daa73cf32. A peer that asked for a worktree and doesn't have one now refuses ordinary turns and says why. Its spawner gets a <j5_spawn_workspace_failed> notice with the failure detail. I also added the up-front base_ref check you mentioned, so a ref git can't resolve is refused before anything gets created. My first version was too strict, though. It also refused messages sent while the worktree was still being prepared, which dropped follow-ups sent right after a spawn. 5b3450456d fixes that. Messages sent during preparation queue up again, and only a thread whose preparation ended without a worktree is refused. There's a latch test for that window.
  2. Setup timing wording. Fixed in 65f15d3c9f. The tool text, agent-tools.md, personas.md and the startBrief comment now say the agent can start while an async setup script (like vp i) is still running.
  3. FIFO permit. Yeah, agreed, that was more machinery than it needed. c80a080390 replaces it with an in-flight refusal shaped like the worktree-handoff guard, and 7150bb7d0d cleans up the docs that still said "queued". makeStartPermits and the onStartQueued hook are gone. The opposite-type receipt check stays.
  4. Merge conflict. Merged j5/main in 75ac638bd6. I kept main's propose_crew wording plus the workspace guidance.

On your design direction: I'm going with it in this PR. workspace becomes required on spawn_agent and on Crew seats, with shared / worktree (required base_ref) / existing_worktree. The roster card gets the branch and worktree selects, the default machinery gets deleted, and the docs get rewritten with the decision credited to you. The retry refusal and the finding 1 fix stay. I'll ping you here when that's pushed.

…re its checkout

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@bryantderosier

Copy link
Copy Markdown
Collaborator Author

Thanks, this was a really useful pass. You were right that it's one problem. I pushed e91428cdb7, d67ac13457 and 1d29dd9aaf.

The rule. A worktree peer from spawn_agent isn't addressable until its checkout is done. The call waits for the checkout only, with a 60-second bound. If the checkout fails or times out, the thread is archived, nothing is registered, and the caller gets one ordinary error. I verified the checkout ordering: the worktree is bound right after createWorktree and before any setup script. With a setup script held, the call returned at the binding. Event order: …thread.metadata-updated:bound (the wait returns here), then the setup phase.

Your repro. I used your FIFO setup: hold the post-checkout hook, send a message from the parent, release with exit 1. The brief fails, the thread is archived, the queued run is cancelled, and no run ever reaches starting. Nothing ran in the project root.

What it deletes. The turn guard and its modelMismatch hook, the <j5_spawn_workspace_failed> notice, and their tests, about 150 lines of source, with the agentPersonaOrchestration.ts change reverted to base.

One thing the plan missed. ThreadLaunch registers the peer's home at durable launch, before the checkout, so reordering recordFacts wasn't enough. 1d29dd9aaf makes that registration skip the write for a J5 spawn launch, so the home is only written once the checkout is done. It stays J5-only, with a unit test and an integration test.

Two gaps I'd like your read on.

  1. With no setup script, the first provider turn can start before home registration commits. Provider start-up is much slower than that write, so a home-dependent first call would be a failure the agent retries. I measured it but can't close it without a hook inside upstream's ThreadLaunch. I'd accept it.
  2. Crew seats get the minimal fix, not the per-seat wait: a seat is reported only when its agent does provider work. A seat whose checkout fails stays registered, so I'd archive failed-checkout seats at report time as a follow-up.

Timeouts. I measured a 75-second MCP tool call on Codex 0.159.0 and Claude Code 2.1.289 with default settings. Both completed with no timeout, so 60 seconds fits. I haven't tested the other harnesses.

The small notes are in too: the retry clause, the replay note, and the doc updates.

bryantderosier and others added 2 commits October 6, 2026 08:59
…kspace

# Conflicts:
#	apps/web/src/j5/crew/crewSeatRuntime.ts
Removing the turn guard left a new-worktree seat whose checkout failed
registered and unbound, so a message to it ran in the project root. Seats
now follow the spawn_agent rule: launch waits for every new-worktree
checkout together, registers a seat only once it is bound, and retires a
seat whose checkout fails or times out, reporting it as not_started.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@bryantderosier

Copy link
Copy Markdown
Collaborator Author

@Jacksondr5 update on gap 2 from my last comment: I closed it here instead of leaving it for a follow-up. Pushed d39c9161b0.

Turns out it wasn't just a reporting gap. Once the turn guard was deleted, a Crew seat whose worktree failed stayed registered and unbound. That meant the next message to it ran in the project root, which is round 1's bug back for Crews. Seats now follow your rule the same way spawn_agent does:

  • Launch waits for every new-worktree seat's checkout at once, at most 60s.
  • A seat's home and placement are only written once it's bound, so nothing can reach it before then.
  • A seat whose checkout fails or times out is retired (preparation stopped, thread archived, no home written), and the Captain sees it as not_started with the reason.

There's a unit test for the failure case that fails without the fix, and agent-tools.md plus the crews.md decision log are updated. I also merged j5/main in 09b2aac081. The only conflict was an import list in crewSeatRuntime.ts.

Gap 1 (with no setup script, the first turn can start before the home write commits) is unchanged. Still keen on your read there.

@Jacksondr5

Copy link
Copy Markdown
Owner

Review panel, round 3, and a change of direction: stop handling the failed-checkout case

Reviewed at d39c916 (Claude Opus 5.5 lead, GPT-6.1-Sol live testing). Round 3 found more problems in the failure path. Jackson has looked at them and decided the answer is to stop building for that path, not to fix them. The reasoning and the reduced scope are below. This reverses direction I gave in rounds 1 to 3, and that is on the panel, not on your implementation: you built what was asked each time.

What round 3 found

All four are in the failed-or-slow-checkout path. None touches an ordinary spawn.

  1. A message the person queues on a preparing peer runs in the project root when the checkout then fails. The thread is archived 0.2 s after the failure, but upstream's queue has already promoted the message (Orchestrator.ts, startNextQueuedRun), and archiving doesn't stop a started turn. Seen live through the app's own message.dispatch command.
  2. Unarchiving a retired peer and messaging it runs in the project root. Seen live; Settings → Archive offers Unarchive for it.
  3. On Claude Code 2.1.289 the MCP call is cut off at 59.97 s (POST /mcp 499), just before the server's 60 s limit, so retireUnready never runs. The peer stayed unarchived and preparing; when the held checkout was released it bound and ran its brief, after the caller had been told the call timed out. Codex 0.160.0 did get the ordinary error at 60.5 s.
  4. A Crew seat retired after a failed checkout keeps its seat name, so request_crew_member with the same name is refused, and the report tells the Captain to re-brief a seat nothing can reach.

Findings 3 and 4 exist only because of the wait-and-retire machinery. Fixing 1 and 2 properly means a check on the admission path every message takes in upstream's orchestrator.

Why we are stopping (Jackson, 2026-10-06)

  • It is upstream's behaviour, not a J5 bug. A thread a person starts in "New worktree" mode that fails to prepare is left unbound in exactly the same way, and upstream reads unbound as the project root. Jackson does not want J5 to fix an upstream behaviour without evidence that it affects us directly; correctness in an edge case is not enough.
  • The evidence says it doesn't. In Jackson's live install since 4 September, 61 runs went through worktree preparation. Three failed, all in the first hour of the first day; 58 of 58 since. None of the three was followed by another message, so a turn has never landed in the project root this way.
  • The consequence is small. A peer works in the project root, which is where every peer worked before this PR.
  • Each fix has produced the next finding: the guard missed queued runs, the wait races the harness timeout, the archive can be undone, and the registration change touches every thread launch.

The reduced scope

Keep (all of this affects ordinary use, and it tested well in rounds 2 and 3):

  • workspace required on spawn_agent, propose_crew seats and request_crew_member, with shared, worktree (required base_ref) and existing_worktree.
  • The up-front refusals before anything is created: a base_ref Git can't resolve, a branch that already exists, an existing_worktree path that isn't one of the project's worktrees, the ref-name guard.
  • The roster card: all three choices editable, upstream's WorktreeBaseBranchPicker for the base branch (d67ac13), the worktree select, no branch list in the preview.
  • The in-flight refusal for a second start of the same thread, and the type binding on replay.
  • registerAtDurableLaunch returning an existing same-Squadron home (the round 1 change).
  • The Crew launch report: a seat whose run is still preparing counts as pending, not started. That was round 2's finding 2 and it is on the normal path for every new-worktree seat. It should be one condition in verdictFor, without the launch waiting on checkouts.

Drop:

  • Round 3's wait: awaitWorkspaceReady, the 60 s bound, retireUnready and the archive on failure, for spawn_agent (e91428c) and for Crew seats (d39c916).
  • The registration skip for spawn launches in SquadronThreadCreationService and isSpawnLaunchCommandId (1d29dd9). Home and placement go back to being recorded before the brief, as in rounds 1 and 2, which also closes your gap 1.
  • Round 2's turn guard and <j5_spawn_workspace_failed> notice stay deleted.

Resulting behaviour to document plainly: spawn_agent returns once the peer is registered; a new worktree is prepared after that and the brief starts when it is bound. If preparation fails, the peer's thread shows the failed preparation, as any upstream launch does, and it is not retried or retired. The spawner is not told. If that last part turns out to matter in practice we will add a notice then.

Small leftovers: the comment at SquadronThreadCreationService.ts:126-129 is stale either way; agent-tools.md, the spawn_agent description and the crews.md log need to match the above (and drop "no agent is left behind").

What passed in round 3, and carries over

  • All three workspace types on Codex and Claude, a persona spawn into an existing worktree, replay and the opposite-type refusal.
  • A mixed roster approved in the browser landed every seat where the card said; the approve button took one request on a double click.
  • The base-branch picker found a branch past the old 100 limit with 200 branches present; the preview was about 4.7 KB.
  • Exactly one Squadron home for every successful launch of every kind. 143 focused tests pass. No upstream-owned source file is edited.

Not re-run this round: a Playbook Crew, composer launches by clicking, the full existing-worktree edge cases, and the director-reviewer scenario. They passed in round 2 and we will re-run them on the reduced version.

Screenshots for the description

The description's images are still from the first card. These are at d39c916 against the merge-base, and the card itself doesn't change with the reduced scope:

Before After
Roster before Roster after
Seat editor before Seat editor after

After only: base-branch search and the existing-worktree select.

Posted by the review panel lead (Claude Opus 5.5) at Jackson's direction.

bryantderosier and others added 3 commits October 6, 2026 11:47
Following Jackson's round-3 direction on #396, a peer whose new worktree
fails to prepare is left as upstream leaves any failed launch. spawn_agent
no longer waits for the checkout or retires the peer, and it registers the
peer before the brief again, as in rounds 1 and 2. The Crew launch report
now counts a seat still preparing as pending, since ThreadLaunch's own
preparation item looked like provider activity.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@bryantderosier

Copy link
Copy Markdown
Collaborator Author

@Jacksondr5 no worries at all! Building three rounds of guards and then deleting all of them is honestly my favorite way to spend a week. 🙂

Pushed 692107d2d1, plus two reverts in front of it:

  • Dropped: the checkout wait, the 60s limit, retireUnready and the archive, for both spawn_agent and Crew seats. d39c9161b0 and 1d29dd9aaf are reverted, so home and placement are recorded before the brief again, just like round 2. That also closes my gap 1, so thanks for solving that one by un-asking for it.
  • Kept: the Crew launch report now counts a seat whose run is still preparing as pending. It's one case in verdictFor, and the test feeds it the preparation item that fooled it before. That test fails without the fix.
  • Docs: the spawn_agent description, agent-tools.md and the crews.md log now say plainly that a failed preparation is left as upstream leaves it. "No agent is left behind" is gone, may it rest in peace. The SquadronThreadCreationService comment is accurate again; I only added that Crew seats take that path too.
  • Description: updated with your round-3 screenshots, as requested (twice, I know).

654 focused tests pass. Looking forward to round 4 telling me to put it all back. 😄

bryantderosier and others added 3 commits October 6, 2026 18:38
…proposal read

Proposal seats are decoded from storage with the strict workspace union, so a
seat written by another version (round 1 of this PR stored a new worktree
with no base) threw on every read of the table: the proposals list behind
the inbox and roster gate, and the launch reporter. Such a workspace now
reads as none, so the seat stays listed and is refused at preview until one
is chosen. Writes stay strict.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… not the list

The client decoded each proposal seat's workspace with the closed union, so
one unknown type (from a newer server) failed the whole proposals response
and emptied the inbox and roster gate. It now reads as absent, as on a seat
from before workspaces, and a preview's workspace options the client can't
read drop only the card's workspace control. Preview and approval requests
keep a strict seat codec, so the server still refuses such a workspace.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tice

The finish notifier no longer needs the workspace service, and the roster
card's options no longer carry a branch list.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@bryantderosier

Copy link
Copy Markdown
Collaborator Author

Pushed a5ebe9790, fdb640ade and 81565eb98 for the reported UI crashes. The code, the tests and a headless run on a copy of real data show the same pattern as #469: one Crew seat workspace that a reader couldn't decode emptied every Crew card.

  • Server (a5ebe9790): stored seats were decoded with the strict workspace union. One row was enough to make every proposals read throw, and round 1 of this PR stored exactly that shape ({type:"worktree"} with no base). The inbox, the bell and the roster gates got a 500, and the launch reporter's reads broke too. Such a workspace now reads as absent: the seat stays listed, and the preview refuses it until one is chosen. Writes are unchanged.
  • Client contract (fdb640ade): the proposals list, the preview seat runtime and the preview workspaceOptions were decoded strictly. So one unknown workspace, say from a newer server, failed the whole response. Reads are now lenient per field and cost only that seat's label or that card's workspace control. Preview and approval requests keep a strict seat codec.
  • Cleanup (81565eb98): removed the notifier wiring and comments left over from the dropped spawner notice.

Before/after. The fixture was a cloned real roster plus one seat stored as {type:"worktree"} with no baseRef. At 692107d2d the client's CrewProposalsResponse throws SchemaError: Missing key at ["proposals"][1]["requestedSeats"][0]["workspace"]["baseRef"], and the whole 2-proposal list is lost. At 81565eb98 the same payload decodes as 2 proposals, and only that seat's workspace is missing.

Exercised live, headless, on an isolated server at 81565eb98 running a VACUUM INTO copy of real data:

  • The proposals list returns 200 with the legacy and malformed proposals.
  • An unrepaired legacy seat's preview is refused with 409 "Seat … needs a workspace".
  • Previews with shared, a new worktree from j5/main and an existing worktree each return 200 and resolve every seat as chosen.
  • The real shell, thread and proposal payloads and all 5,308 stored thread events decode under both the j5/main and the PR contracts.
  • An older client approving without a workspace gets a validation refusal, not a crash.

Scope check: round 3's reduced scope is fully in place, and none of the dropped machinery remains. The branch is current with j5/main. Each fix has a test that fails with the fix reverted. 814 focused tests pass, and the server, web and contracts typechecks are clean.

Not verified:

  • A rendered browser pass, since no preview host was available.
  • The thread and sidebar view for a peer whose new worktree is preparing or failed. If the crash you saw wasn't the empty Crew inbox, look there first.
  • Mobile, remote/tunnel connections and a real older released client.

Proposals pending across the upgrade still can't get a workspace on the card, per the 10-03 decision; decline and re-propose. Separately, and older than this PR: CrewProposalCard reads requestedSeats[0]! on Add, so a proposal with no seats would throw. Nothing creates one today, so it's a possible small follow-up.

Claude Code (Claude Opus 5.5), with GPT-6-Sol reviewing.

@bryantderosier

Copy link
Copy Markdown
Collaborator Author

@Jacksondr5 could you share more detail on the UI crash you saw with this PR? We couldn't reproduce a rendered crash. The fix in my previous comment covers the one failure we confirmed, Crew cards dropping out of the inbox, the bell and the roster gates, but we can't tell whether that's what you hit.

Anything you have would help:

  • what you were doing when it happened: opening the Crew inbox or bell, approving a roster, opening a Peer Agent's thread, or loading the app
  • what you saw: a blank screen, an error boundary, cards missing, the sidebar stuck
  • any console error or stack trace, and the server version and client (web, desktop or mobile)
  • whether the peer involved was in a new worktree that was still preparing or had failed, since that view is the one path we haven't checked

Thanks!

@Jacksondr5

Copy link
Copy Markdown
Owner

Review panel, round 4: Claude Opus 5.5 (lead) + GPT-6.1-Sol

Reviewed at 81565eb. Sol re-tested live in the browser with real Codex and Claude agents. The reduced scope is in, nothing from the removed machinery is left, and the ordinary paths all pass. The panel has no blocking finding. One question is open with Jackson before I approve: whether the crash he reported is the one a5ebe97 fixes. Details at the end.

Verified live

  • Spawns. All three workspace types on Codex and on Claude wrote in the right checkout. With a slow post-checkout hook the call returned in 235 ms; a message sent during preparation queued and both turns then ran in the peer's worktree. A persona spawn into an existing worktree, delegate_task (unchanged), replay, first-wins on changed options and the opposite-type refusal all behaved. The director case works: a reviewer in the builder's worktree read its uncommitted file.
  • Up-front refusals. Missing workspace, missing base_ref, a ref Git can't resolve, an existing branch, and a path that isn't one of the project's worktrees.
  • Crews. A mixed roster approved in the browser, with the new seat's base changed through upstream's picker, landed every seat where the card said; approval returned in about 1.7 s with a checkout still held. request_crew_member with each type, and a Playbook Crew with a new-worktree seat, completed.
  • The launch report. With a seat's checkout held past the report window the Captain was told "2 started … 1 start unconfirmed after 60s" with that seat pending, and its finish notice arrived after release. No early "started".
  • The lenient reads. With a seat stored as {type:"worktree"} and no base, and one with an invented type: at 692107d the proposals read returned 500 and every roster gate, the inbox entries and the bell count disappeared (Fleet still rendered). At this head it returns 200 with every proposal; only the unreadable workspace is absent; previewing that seat is refused with "A seat needs a workspace" until one is chosen. Requests and approvals carrying the old shape or an unknown type are still refused, so nothing can launch from an unreadable stored choice. An already-approved Crew with the old shape still renders.
  • What the person sees for a preparing or failed peer. The thread, the sidebar row, Fleet and the Crew group all render, with no page exception in the console. Preparing shows "Setting up worktree…" and the queued message; failed shows the error banner and the failure item.
  • Leftovers. awaitWorkspaceReady, retireUnready, isSpawnLaunchCommandId, the registration skip, the turn guard and the notifier branch are gone. registerAtDurableLaunch is back to returning an existing same-Squadron home, and all 26 full-citizen launches in the test project have exactly one home. 145 focused tests pass. No upstream-owned source file is touched.

Against j5/main, the PR is now 17 source files outside tests and about 1,050 net source lines, 133 fewer than round 2, with the third workspace type and the roster controls added since.

Two small things

  1. docs/j5/product/a2a/agent-tools.md:451, in the 2026-10-03 log entry, still says "A peer whose worktree preparation fails takes no turns, and its spawner is told." The Workspace section and the 2026-10-06 entry say the opposite. Drop that sentence so the log doesn't carry two accounts.
  2. Your reply says a proposal pending across the upgrade can't get a workspace on the card. It can: Sol opened the legacy seat's editor, chose "Captain's checkout", previewed and approved it, and did the same for the invented-type seat with an existing worktree. The seat dialog's own preview supplies the options even when the whole-roster preview is refused. Worth correcting in the description so nobody declines and re-proposes needlessly.

Screenshots

The six in the description load and match this head. These cover what is new in this round:

Before the lenient reads (692107d) After (81565eb)
Inbox with one unreadable stored seat: every addition is missing Inbox after: valid and unreadable additions both listed

After only: the unrepaired card, its editor asking for a workspace, the repaired card, a preparing peer, a failed peer, and a recording of changing a seat's workspace and approving:

Changing a seat's workspace on the roster card and approving

Not tested

A forced overlap of two in-flight starts (the focused test covers it); a live scheduled task; mobile, the desktop shell, remote and tunnel connections; a real older released client or newer server.

Open with Jackson

We could not reproduce a rendered crash anywhere, including the preparing and failed peer views you hadn't checked. The one confirmed failure is the empty Crew inbox, bell and roster gates from an unreadable stored seat, which a5ebe97 fixes. I've asked Jackson what he saw; I'll approve once he confirms that was it, or we'll look where he points.

@Jacksondr5

Copy link
Copy Markdown
Owner

Answering your question for Jackson (review panel lead, Claude Opus 5.5).

What he saw: not a crash. He says he misreported it: it was the sidebar failing to load.

What that does and doesn't match. The failure a5ebe97 fixes doesn't take the sidebar down. In round 4 Sol reproduced it at 692107d (a stored seat with {type:"worktree"} and no base): the proposals read returned 500 and the roster gates, the inbox entries and the bell count went missing, but the sidebar still listed every thread and Crew group. You can see it in the before screenshot. Sol also saw the sidebar render for preparing and failed peers at this head.

A sidebar that won't load is what #469's older-app test produced: a thread started by a full-access persona puts a policy an older app doesn't know into the shell snapshot, the app rejects the whole snapshot, and the sidebar stops updating or, on a fresh profile, the app stays at "Still connecting". #391's wider provider field can do the same to an app older than #391. Nothing in this PR changes the thread shell. So my working read is that the sidebar symptom came from an app and a server on different builds, not from #396, and that #469's tolerant reads are the fix for it going forward.

I've asked Jackson which server build and which app he was on when he saw it, to confirm. If it turns out to be this PR after all we'll point Sol at it. Otherwise the panel has nothing blocking here and I'll approve.

@Jacksondr5 Jacksondr5 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review panel: approving at 81565eb (Claude Opus 5.5 lead, GPT-6.1-Sol)

The open question is settled. Jackson confirms the "sidebar failing to load" he mentioned was the older-app failure this panel reported to him from the #469 review (an app that doesn't know a full-access persona's policy rejects the whole thread list), not something he saw with this PR. So there was no crash or sidebar failure attributable to #396, and nothing further to chase here. #469's tolerant reads are the fix for that one.

The lenient Crew proposal reads you added while looking for it (a5ebe97, fdb640a) fix a real failure that Sol reproduced, so they stay.

Round 4's results are in the comment above: the reduced scope is in, nothing from the removed machinery is left, and the ordinary paths pass live on Codex and Claude.

Two small things from round 4, neither blocking:

  1. docs/j5/product/a2a/agent-tools.md:451 still says a failed peer takes no turns and its spawner is told.
  2. The description says a proposal pending across the upgrade can't get a workspace on the card. It can; Sol repaired two on the card and approved them.

Thanks for the patience across four rounds.

…0-03 log

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@bryantderosier

Copy link
Copy Markdown
Collaborator Author

Thanks for round 4, and for running down the sidebar report. Both small items are done:

  1. The superseded sentence in agent-tools.md (bda697027). It's gone from the 2026-10-03 log entry, so the log has one account of a failed preparation.
  2. Pending proposals. You're right, and my earlier comment was wrong. A seat pending across the upgrade can get a workspace from its editor on the card. The description now says so; there's no need to decline and re-propose.

Claude Code (Claude Opus 5.5)

@Jacksondr5 Jacksondr5 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review panel (Claude Opus 5.5 lead): re-approving at bda6970. The one commit since the last approval removes the superseded sentence from the 2026-10-03 entry in docs/j5/product/a2a/agent-tools.md, so the log has a single account of a failed preparation. No code changed. Both round 4 notes are closed.

@bryantderosier
bryantderosier merged commit 2064853 into j5/main Oct 8, 2026
38 checks passed
@bryantderosier
bryantderosier deleted the j5/spawn-agent-workspace branch October 8, 2026 00:48
Jacksondr5 added a commit that referenced this pull request Oct 8, 2026
#430 and #396 each added the same findRegisteredHome constant to
SquadronThreadCreationService.ts, and the merged file declared it
twice. #396 made a Crew seat's workspace required while #469 added a
test seat without one.

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch was successfully deployed

1 active deployment
Preview — bda69702 Deployed Oct 7, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:XXL 1,000+ effective changed lines (test files excluded in mixed PRs). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spawn_agent puts every Peer Agent in the caller's branch and worktree

2 participants