Repository navigation
fix(a2a): isolate Peer Agent workspaces by default - #396
Conversation
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>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Comment |
Review panel: Claude Opus 5.5 (lead) + GPT-6.1-SolReviewed 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 1. A peer whose worktree fails later runs in the project root (blocking)
Reproduced live:
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:
Related: the PR's invariants say an explicit worktree request Git cannot resolve is refused, but a nonexistent 2. Docs and tool text say setup finishes before the brief starts; it doesn't
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 3. The FIFO start permit can be a refusalThe 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). 4. Merge conflictAgainst current Confirmed, no action
Not testedRemote 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 |
# 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>
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.
|
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_refis required, with no fallback to the spawner's branch. No assumptions.existing_worktreeis 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 hasexisting_worktreewith a path). It should be cheap: it is thesharedpath 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, thedoorargument andinWorktree. - The Git inspection for
shared(it reads Git today for no reason) and the current-branch lookup forworktree, sincebase_refis 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>
|
@Jacksondr5 thanks, this was a really useful pass. All four findings are fixed and pushed:
On your design direction: I'm going with it in this PR. |
…re its checkout Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks, this was a really useful pass. You were right that it's one problem. I pushed The rule. A worktree peer from Your repro. I used your FIFO setup: hold the What it deletes. The turn guard and its One thing the plan missed. Two gaps I'd like your read on.
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. |
…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>
|
@Jacksondr5 update on gap 2 from my last comment: I closed it here instead of leaving it for a follow-up. Pushed 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
There's a unit test for the failure case that fails without the fix, and Gap 1 (with no setup script, the first turn can start before the home write commits) is unchanged. Still keen on your read there. |
Review panel, round 3, and a change of direction: stop handling the failed-checkout caseReviewed 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 foundAll four are in the failed-or-slow-checkout path. None touches an ordinary spawn.
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)
The reduced scopeKeep (all of this affects ordinary use, and it tested well in rounds 2 and 3):
Drop:
Resulting behaviour to document plainly: Small leftovers: the comment at What passed in round 3, and carries over
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 descriptionThe 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:
After only: base-branch search and the existing-worktree select. Posted by the review panel lead (Claude Opus 5.5) at Jackson's direction. |
…ready" This reverts commit d39c916.
…ome before its checkout" This reverts commit 1d29dd9.
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>
|
@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
654 focused tests pass. Looking forward to round 4 telling me to put it all back. 😄 |
…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>
|
Pushed
Before/after. The fixture was a cloned real roster plus one seat stored as Exercised live, headless, on an isolated server at
Scope check: round 3's reduced scope is fully in place, and none of the dropped machinery remains. The branch is current with Not verified:
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: Claude Code (Claude Opus 5.5), with GPT-6-Sol reviewing. |
|
@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:
Thanks! |
Review panel, round 4: Claude Opus 5.5 (lead) + GPT-6.1-SolReviewed 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
Against Two small things
ScreenshotsThe six in the description load and match this head. These cover what is new in this round:
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: Not testedA 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 JacksonWe 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. |
|
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 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
left a comment
There was a problem hiding this comment.
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:
docs/j5/product/a2a/agent-tools.md:451still says a failed peer takes no turns and its spawner is told.- 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>
|
Thanks for round 4, and for running down the sidebar report. Both small items are done:
Claude Code (Claude Opus 5.5) |
Jacksondr5
left a comment
There was a problem hiding this comment.
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.








Summary
Closes #274.
spawn_agentused 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.How a spawn starts, by type:
What else changed, in short:
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.preparingcounts as pending, not started. ThreadLaunch's own "Preparing worktree" item used to read as provider activity.<j5_spawn_workspace_failed>notice are gone.door,inWorktree, the git read forshared, 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.agent-tools.md,crews.mdanddocs/user/personas.md, and thecrews.mdandagent-tools.mdlogs credit both decisions to Jackson.No upstream files touched. Everything lives under
apps/server/src/j5,apps/web/src/j5andpackages/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):
After only: base-branch search and the existing-worktree select.
spawn_agentwrote into the parent's checkout. The review panel reproduced it live.After: every input requires
workspace. A worktree spawn runs in its own worktree, anexisting_worktreespawn binds that worktree's live branch, andsharedstays in the caller's checkout. The panel verified all three on Codex and Claude in round 3.apps/server/src/j5/a2a,apps/server/src/j5/agentsandapps/web/src/j5/crew, 654 passed, 1 env-gated skip. Server typecheck and lint are clean.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
workspaceis now required onspawn_agent,propose_crewandrequest_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.existing_worktreecan put a peer into a worktree another spawn is still setting up. That's inherent to "no preparation step".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