feat: bring up collaboration role-children (collab sessions P1b) - #256
Merged
Conversation
Completes P1 of docs/collaborative-session-design.md §11. P1a landed the contract; this makes it do something. Creating a collaborative session now brings up its role-children on their own backends, and destroying it tears them down — P1's stated exit criterion, end to end. Key structural decision: a role-child is a normal long-lived session, NOT a dispatch-spawned disposable worker. The dispatcher destroys a spawn-task worker the moment its turn ends (#finishWorkerTask), which is exactly wrong for a role that has to survive the implement↔review fix-loop. A `kind:"send"` dispatch, by contrast, delivers and completes without taking ownership of the target's lifetime — so children receive fleet_send and the collaboration owns teardown. That means per-goal lifetime needs NO change to the disposable-worker path, which stays as it is for fleet_spawn. Second decision: no brief is SENT at spawn. A child's role, contract, and goal ride in its compiled pack constitution instead, so bringing up a fleet of N costs zero tokens and no child burns a turn just to learn it should wait. - protocol: `write?: boolean` on CollaborationRole — absent = read-only. The default is the point: §3 gives review/search no repo write and §6 wants a reviewer that *provably* cannot write, so write authority is opt-in and enforced two ways, not requested in a prompt. Plus `collaborationRole` on SessionInfo (parent id, role, ordinal, write) — the mirror of `collaboration`, so a client can group a fleet without parsing names. - collaboration.ts: planChildren (flattens fan-out to ordinals, EXCLUDES the orchestrator since the session itself plays it), compileGoalPack (the ephemeral one-goal pack for the orchestrator — synthetic id, never installed, so pack vocabulary stays hidden per §9), childBrief, childSessionName. - MAX_COLLABORATION_CHILDREN = 12. The schema bounds multiply: 15 worker roles × 8 fan-out is 120 agent subprocesses from one session.create. The real governor is P3's live-worker cap; this is the blast-radius backstop that has to exist before anything spawns. Rejects rather than truncating — a collaboration quietly missing a reviewer is worse than one that refused. - session-manager: children planned BEFORE the parent is created, so an over-ceiling request builds nothing; all-or-nothing spawn with rollback of parent + siblings, since an orchestrator with a partial fleet would delegate to children that don't exist; cascade teardown on destroy, children first so OK is never returned over live orphans. Membership is DERIVED from the live session set rather than a side registry that could drift and orphan a subprocess. collaboration + pack now rejected as mutually exclusive — two topologies competing for one constitution. - Read-only enforcement is doubled up: shape "scout" gives a LEAF identity with no tools:write (so it cannot mint write authority even via a sub-agent), and the child's RoleDef makes roleDeniesTool hard-deny write tools at the canUseTool fence. `network: "read-only"` not false, or the search role loses the web tools §3 gives it. Tests: +13. Asserts children land on the backend their role named, ordinals, orchestrator exclusion, read-only default, cascade teardown, pre-create rejection, and that the envelope genuinely denies Write/Edit/MultiEdit/ NotebookEdit while still permitting Read/Grep/Bash/WebSearch — the security claim, checked rather than labelled. Mutation-checked: dropping the per-child providerId fails the backend test; dropping the cascade fails the teardown test. Suite 1915 pass / 0 fail, typecheck + biome + build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yash Datta <yd2590@columbia.edu>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
rsharath
approved these changes
Jul 26, 2026
saucam
added a commit
that referenced
this pull request
Jul 27, 2026
…#259) origin/main at e48527a fails `bun run typecheck`: src/daemon/session-manager.ts(1583,27): error TS2339: Property 'recordDestruction' does not exist on type 'RateLimiter'. The main CI run for e48527a is red. How it got in, because the mechanism matters more than the one-line fix: #256 (collaboration role-children) was branched before #258 landed. #258 removed RateLimiter.recordDestruction — the concurrency count is now derived from the live session set rather than stored, precisely because a stored counter reset on restart and only drifted upward. #256's new collaboration-rollback path called the method that #258 deleted. Neither PR conflicted: they touched different lines of session-manager.ts, so git merged them cleanly. And CI only ever validated #256 against the main it was branched from, never against the merge result. A textually-clean merge of two independently-green branches is not a compiling merge, and nothing in the current setup checks that. The fix: drop the call. Removing the session from #sessions IS the rollback now, since the count is derived. The hourly creation timestamp stays recorded — the create was attempted, and refunding it would let a failing spawn drive a retry loop for free. Verified against pristine origin/main in a detached worktree, so this is the merge result and not a local artifact: typecheck + biome + build clean, 2007 tests / 0 fail. Worth considering separately: requiring branches to be current with main before merge would have caught this at the source. Signed-off-by: Yash Datta <yd2590@columbia.edu> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
saucam
added a commit
that referenced
this pull request
Jul 27, 2026
…b UI (#262) * feat: render collaboration fleets as one unit in the web session list `collaborationRole` has been on the wire since #256 and the UI ignored it entirely, so an N-role collaboration rendered as N+1 unrelated sessions — the exact opposite of the mental model a collaborative session exists to create. Group them. `lib/fleet.ts` is a pure grouping/filtering module (no Solid, so the interesting logic is testable without a reactive root); the pane consumes it. What the rows now say: - Children nest under their orchestrator behind a continuous left rail, keyed by role (`review#2`) rather than by their daemon-generated `<parent>:<role>-N` name, whose parent prefix is pure repetition once the row is already nested. Full name stays in the tooltip. - The orchestrator shows its goal, and folds its fleet shut from a toggle that is a SIBLING of the row button, not a child of it — nesting a button inside a button is invalid HTML. - Read-only roles are badged `ro`. That badge is §6's independence property made visible: a scout's leaf identity carries no `tools:write` at all. - Children always show their backend chip, INCLUDING "claude". Standalone rows suppress the default-backend chip, but which model sits behind which role is the entire point of a mixed fleet and must not be inferable only from a chip's absence. Two cases that are easy to get wrong, so both are pinned by tests: - An orphan child — `parentSessionId` naming a session not in the list, because the parent was destroyed while children drain or hasn't reached this client yet — is promoted to top level rather than dropped. A session that silently disappears from the sidebar is a far worse failure than one rendered without its group. It keeps its role badges but shows its full name, since no parent row is there to supply context. - Grouping happens BEFORE filtering. A query matching only a child keeps the orchestrator as (dimmed) context; a bare `reasoning` row with no indication of its goal is less useful than no filter at all. Web suite 319 → 341. * feat: owner-facing goal blackboard verbs — blackboard.index + blackboard.read The blackboard is only reachable through the role-scoped MCP tools, which means the person who created the collaboration and is paying for it cannot see what their fleet produced. Add two read verbs for the human. The exemption, stated plainly because it looks like a hole in §6: role scoping keeps the AGENTS independent of each other — a reviewer that can read its peers is an echo, not a panel. The goal's owner is not a participant. They can already read every child's transcript, so withholding the artifacts those transcripts produced protects nothing and only makes the fleet unobservable. That reasoning is load-bearing, so it is encoded in the type system rather than in a comment alone. `Blackboard#forOwner` returns a distinct `OwnerBlackboard` class, not a `RoleBlackboard` with a flag: a flag makes "unscoped" reachable by passing a boolean, whereas an explicit `forOwner()` is greppable and cannot be handed where a role handle is expected. It exposes no `write` — an owner write would land unattributed and unscoped on a board whose entire contract is attributable handoffs — and tenant scoping still applies in full. Ownership is checked before any of that, and `sessionId` may name either the orchestrator or one of its role-children. Clients focus children as often as parents, and a client that walked `parentSessionId` wrongly would get an EMPTY board rather than an error — the least debuggable outcome available. The parent is re-fetched through `#getOwnedSession` rather than trusted from the child's field, so the ownership check binds to the session whose artifacts are about to be read. Scopes, no new one minted: `index` takes `session:list` (metadata about a session the holder can already enumerate, no bodies) and `read` takes `session:watch` (a body is session content, the same class as streamed output). Both already sit in WATCHER_SCOPES and OPERATOR_SCOPES, so every token minted before this keeps working — a new scope would have 403'd them all. Also: an unwritten artifact returns `null`, not an error. A collaboration in flight legitimately has empty lanes, and a client can't render that as "pending" if the daemon calls it a failure. An orphaned child whose orchestrator was destroyed DOES error, because teardown drops the artifacts and "empty" would otherwise be indistinguishable from "gone". Daemon suite 2044 → 2063. The three security-relevant tests were mutation-checked — dropping the tenant check, swapping the two scope tiers, and letting a plain session through each fail exactly one test. * feat: goal-blackboard drawer — read what a collaboration actually produced The session list now shows the fleet; this shows its OUTPUT. §4 has the orchestrator hold an index and never the bodies — and so, until now, did the UI: every handoff between role-children happened entirely off-screen. Two panes. The index (kind · slot · version · author · size) on the left, one artifact body on the right, fetched only when picked. That split is the design's, not a layout preference: a `diff` can be 256 KB, and the index exists precisely so you can see what's on the board without paying to load it. The test asserts no `<pre>` renders until you click. Details that carry meaning rather than decoration: - Rows sort in SDLC flow order (spec → research → adr → task-list → diff → findings), not alphabetically, so the board reads as the pipeline it is. Alphabetical buries `spec` under `diff`. - Every writer slot is shown. Two `findings` rows reading `review` and `review#2` is the visible form of MULTI_WRITER_KINDS; one row would mean a panel had silently collapsed into a single voice. - A `null` artifact renders as "not written yet", never as an error. The daemon returns null for exactly this reason — an empty lane is a normal state of a collaboration in flight. - Polls every 4s while open and tears the timer down on close. There's no push channel for the board, and a stale panel makes a working fleet look stalled; a timer surviving the close would hit the daemon for the life of the tab, so a test pins that too. The trigger is a `board` chip in SessionControls, shown for an orchestrator OR any role-child (the daemon resolves the hop, so both land on the same board) and gated on the daemon advertising the `blackboard` capability — against an older daemon the affordance doesn't appear rather than appearing and erroring on click. The state slice takes `goalSessionId` from the RESULT, not from the id it asked about, so focusing a child and focusing its orchestrator converge. Both async paths drop replies for a board or artifact the user has since navigated away from, and a refresh that no longer lists the selected artifact clears the body pane instead of stranding it. Web suite 341 → 370. * fix: a literal NUL in refKey made blackboard.ts invisible to code review The artifact-ref key separator was written as a raw control byte instead of an escape. Nothing that normally catches a mistake said a word: it typechecked, it linted, all 370 web tests passed. The only symptom was `git diff --stat` reporting `Bin 0 -> 6081 bytes` — git classifies a file containing a NUL as binary, so the file stopped producing diffs and would have reached the PR unreviewable. The separator itself was the right idea and stays, now as an explicit `\u0000` escape in a named constant. `kind` is an open namespace (`extra/<key>`) and `slot` is daemon-generated, so any printable delimiter is one future naming choice away from collapsing two distinct artifacts onto one key — which would render the wrong body under the right row. Adds `src/tests/source-hygiene.test.ts` to make the class of mistake loud: it walks every source file in the repo and fails on a raw NUL. The walker asserts its own file count first, so a broken walk can't pass by finding nothing, and the guard is mutation-checked: planting a NUL in lib/fleet.ts fails it, removing it passes. Daemon suite 2063 to 2064.
saucam
added a commit
that referenced
this pull request
Jul 27, 2026
…e back missing (#263) `collaborationRole` has been persisted since P1b (#256) and `resumeSessions` read `meta.collaboration` while silently ignoring it. The visible symptom was cosmetic: role-children came back detached from their orchestrator, so the fleet grouping shipped in #262 held only until the daemon bounced. The invisible symptoms were the problem. A resumed child came back with: - **no `workerShape`**, so its next turn registered a full session agent instead of a scope-capped `scout` leaf (`#ensureAgentIdentity`); - **no capability role**, so `roleDeniesTool` had nothing to deny with and a read-only reviewer's `Write`/`Edit` degraded from denied to merely asked; - **no blackboard mount**, so it could not publish a handoff; and - **no autonomous budget** — it resumed `guarded`, and since nobody ever attaches to a child, its first non-safe tool call parks at `waiting_approval` with zero clients. The fleet rendered as a live collaboration and was dead. Two of those are a privilege regression across a restart, not just a lost feature. §6's "a reviewer that provably cannot write" held on the create path and quietly stopped holding on the resume path. ## Derive, don't re-invent No new persisted field was needed: a child's identity (`collaborationRole`) plus its goal's config reproduces the plan it spawned under. `plannedChildFor` recovers the `PlannedChild`, and it is implemented BY calling `planChildren` rather than re-deriving shape/write/reads/writes a second time — a resumed child that computed its own shape would be one edit from disagreeing with the one it spawned under, and the direction that drift fails is a read-only reviewer coming back able to write. Sharing the derivation makes that unrepresentable. `roleChildPosture` is now the single definition of a child's restrictions (worker shape + capability role + brief + collaborationRole), called by both `#spawnCollaborationChildren` and resume. `#blackboardMountFor` takes a `GoalScope` instead of a live parent `Session`, because resume is capped and time-boxed and a parent can legitimately miss the window its own children made — while the board itself is keyed on (tenant, goal id) in SQLite and needs no resident orchestrator. `#attachOrchestratorBlackboard` is likewise shared, since an orchestrator's mount is scoped to its own id either way. Attribution already anticipated this: `authorSub` is keyed to the ROLE within the goal, never the session id, so a resumed child writes under the same subject its pre-restart versions carry. ## The torn case Child transcript present, orchestrator's gone (teardown normally removes both). It restores the FENCE from what the child itself carries — `write` is on `collaborationRole` — and nothing else: no mount for artifacts that died with the goal, and deliberately no autonomous budget, because an agent that cannot coordinate should not burn turns unattended. It gets an honest brief saying so, and resume logs it at warn level, since a silent degrade is indistinguishable from a healthy fleet in the session list. ## Verification Daemon suite 2064 to 2081. Adds `Session.hasBlackboardMount` and `SessionManager._sessionForTest`. All seven restart tests were mutation-checked against the pre-fix state (dropping the posture spread) and all seven fail. Three further mutations each fail exactly the tests that should catch them: minting a mount but never attaching it, not tracking resumed tokens for revocation, and minting a mount on the orphan path. Mutation testing also caught two flaws in the tests themselves, both fixed: `activeTokens` counts tokens ever MINTED, so it could not tell a mount a session holds from one dropped on the floor (now asserted per-session via `hasBlackboardMount`); and the per-child assertions sat inside a loop over a list the pre-fix state empties, so that test passed vacuously until a length check was added. Verified live across a real process restart, not just a second SessionManager in-process: boot, create a 3-role collaboration, publish an artifact, SIGTERM, boot again on the same config dir. 15/15 — children reattach with roles and ordinals, write authority stays per-role, the capability role is active, they return autonomous with a fresh 50-turn budget, the board and its attribution survive, a resumed child still resolves to its goal and reads a body back, and cascade teardown still collapses the whole resumed fleet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes P1 of
docs/collaborative-session-design.md§11. #248 landed the contract; this makes it do something.P1's exit criterion, met end to end: create a collaborative session from the CLI → children come up on the right backends with the right leaf scopes → they're torn down at goal end.
Two structural decisions worth reviewing
1. A role-child is a normal long-lived session, not a dispatch-spawned disposable worker.
The dispatcher destroys a spawn-task worker the moment its turn ends (
dispatch.ts#finishWorkerTask→destroyWorker), which is exactly wrong for a role that must survive the implement↔review fix-loop. But akind: "send"dispatch delivers and completes without taking ownership of the target's lifetime (dispatch.ts:317-325).So children receive
fleet_send, and the collaboration owns teardown. Per-goal lifetime therefore needs no change to the disposable-worker path — it stays as-is forfleet_spawn. This also resolves the "per-goal child lifetime maps onto spawnWorker/continueWorker" assumption from the original design handoff: it doesn't, and it doesn't need to.2. No brief is sent at spawn. A child's role, contract, and goal ride in its compiled pack constitution instead. Bringing up a fleet of N costs zero tokens, and no child burns a turn just to learn it should wait for instructions.
Read-only is enforced twice, not requested
writeon a role defaults to absent = false, and that default carries the §6 guarantee. Two independent mechanisms:"scout", whoseWORKER_SCOPE_PROFILESentry has notools:writeat all, so it cannot mint write authority even through a sub-agent.RoleDefmakesroleDeniesToolhard-denyWrite/Edit/MultiEdit/NotebookEdit.networkis"read-only", deliberately notfalse—falsewould stripWebSearch/WebFetch, which §3 gives the search role.The child ceiling exists because the schema bounds multiply
MAX_COLLABORATION_CHILDREN = 12. 15 worker roles × 8 fan-out is 120 agent subprocesses from onesession.create. P3's live-worker cap is the real concurrency governor; this is the blast-radius backstop that has to exist before anything spawns. It rejects rather than truncates — a collaboration quietly missing a reviewer is worse than one that refused to start.Failure handling
collaboration+packare now rejected as mutually exclusive: two topologies competing for one constitution, where either would silently override the other.Verification
bun buildclean. Rust side 354 tests green.providerIdfails the backend test; dropping the cascade fails the teardown test.Not in this PR
collaborationpersists) but not its children. Mid-goal resume is a blackboard-phase guarantee, so it lands with the durable goal state rather than being half-built here.sendtasks — the orchestrator learns a child's result through the blackboard, per §4, not a dispatch digest.🤖 Generated with Claude Code