feat: collaboration config on session.create (collab sessions P1a) - #248
Merged
Conversation
P1 of docs/collaborative-session-design.md §11 splits in two along the repo's "contracts before consumers" rule. This is P1a: the contract. A collaborative session's goal + role→backend bindings can be requested, validated, persisted, and read back. P1b spawns the role-children. The daemon stays fully working: the config round-trips but drives nothing yet. - protocol: CollaborationConfig / CollaborationRole + ORCHESTRATOR_ROLE, on SessionCreateMsg and echoed on SessionInfo. Role `name` is a free-form string, not an enum (§3, "a role is data") — adding "security-reviewer" must stay a config change. Three published LIMITS so clients can pre-validate. - schemas: shape only, deliberately. The semantic rules stay in the daemon for the same reason providerId is a bounded string rather than an enum — the frame must PARSE so the daemon can answer with a specific error instead of the schema opaquely rejecting the whole create. - collaboration.ts: the semantic rules, fail-closed. Provider registered; exactly one orchestrator; orchestrator on claude in v1 (#245); no orchestrator fan-out; case-insensitive unique role names; model valid for its OWN role's backend, reusing P0's resolveModelIdForProvider so "review on gemini with model opus" is caught at create rather than first turn. Returns a normalized config (goal/name trimmed, count defaulted, model resolved) so nothing downstream re-resolves against the wrong backend. - persistence: `collaboration` JSON column on sessions + on TranscriptMeta. Both, because meta is what the resume path actually reads while the column keeps a collaboration visible to anything querying the table directly. - pack.ts: roleSchema gains optional (provider, model) — §8.1, the shared seam that lets one engine cover a sequential pack phase and a collaborative fan-out. Absent = the session's own backend, so every existing pack is unaffected. - CLI: `codeoid new … --collaborate <goal> --role name:provider[:model][*count]` (repeatable). Parsed locally for a fast error, then re-validated by the daemon so both paths fail identically. The client echoes the RESOLVED bindings, not the requested ones. Why fail-closed matters more here than on a plain session: the whole point of a collaboration is that roles sit on different vendors, so a silent fallback to the default backend would produce a "multi-model" session that is secretly single-model. Tests: new src/tests/collaboration.test.ts — 29 cases across the validator, the CLI grammar, and the real SessionManager.handle() create path driven by a genuine two-backend ProviderRegistry (not _testProviderFactory, which injects one mock everywhere and would hide whether the config survives create → SessionInfo → persistence). Mutation-checked: dropping the passthrough fails 2, disabling the provider check fails 2. Suite 1871 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
Six findings from a pre-merge audit of P0 + P1a. One was a real durability bug that would have shipped silently. HIGH — collaboration was erased from transcript meta on the first status write. #writeMetaAtomic serializes the whole object and renames over the file, so a field present only in the create-time saveMeta is gone after the next status transition — and that file is exactly what the resume path reads. Reproduced: create + one turn left `collaboration: undefined` on disk, so the "survives a restart" guarantee held only for sessions that had never run a turn. P1b reads this field to rebuild children, so it would have compounded. Every sibling field (role, providerId, forkedFrom, worktree) was already in both writes; this was specifically the new one. MEDIUM — a session's providerId could contradict its own orchestrator role. The claude-only rule was enforced on the role entry, but the thing that has to mount the fleet MCP server is the session, and `providerId: "gemini"` with `orchestrator: claude` was accepted. A collaborative session IS its orchestrator (§9), so providerId is now derived from the orchestrator role and an explicit conflicting value is rejected. MEDIUM — resolveBackend's model check could never reject anything. The chain `resolveAgainstList(...) ?? resolveModelIdForProvider(...)` read as strict validation, but the fallback's last branch returns its input unchanged, so a typo always survived — while the comment claimed typos were caught pre-spawn. Kept the permissive policy (models.ts: "the live backend is the real validator", because a cached catalog goes stale on any vendor point release), dropped the dead branch, and made the comment describe what the code does: canonicalize against the catalog, reject only Claude-shaped ids on non-Claude backends. LOW — validateCollaboration now re-checks the published LIMITS instead of leaning on Zod alone; embedded frontends hold the SessionManager directly and never cross parseClientMessage. LOW — role names are stored lowercased, so `ORCHESTRATOR:claude` can't validate case-insensitively and then miss an exact-match lookup. LOW — parseRoleSpec bounds count, so `*99999` gets the CLI's message instead of a raw schema error from the daemon. Tests: +7. The meta regression drives a real turn before asserting, which is what the original tests missed — they only checked the create-time write. Mutation-checked: removing the meta line fails it. Suite 1878 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>
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.
P1 of
docs/collaborative-session-design.md§11, following P0 (#247).P1 is split in two along the repo's own contracts before consumers rule:
Splitting also isolates the cross-repo protocol change into one small, auditable diff.
What changed
packages/protocolCollaborationConfig/CollaborationRole/ORCHESTRATOR_ROLE; onSessionCreateMsg, echoed onSessionInfo; 3 publishedLIMITSschemas.tsdaemon/collaboration.tsstore.ts/transcript.tscollaborationJSON column +TranscriptMetafieldpipeline/pack.tsroleSchemagains optional(provider, model)— §8.1cli.ts/terminal/client.ts--collaborate <goal>+ repeatable--roleDesign notes worth reviewing
Shape in the schema, semantics in the daemon. Zod checks only structure. The rules — provider registered, exactly one orchestrator, orchestrator on claude in v1, model valid for its role's backend — live in
collaboration.ts. Same reasoning the codebase already applies toproviderIdbeing a bounded string rather than an enum: the frame must parse so the daemon can answer with a specific, actionable error instead of the schema opaquely rejecting the whole create.Fail-closed matters more here than on a plain session. The entire point of a collaboration is that roles sit on different vendors. A silent fallback to the default backend would produce a "multi-model" session that is secretly single-model — so an unregistered provider rejects the create outright.
Audit pass (commit
8806fb7)A pre-merge audit found six issues in this PR; all are fixed here.
collaborationwas erased from transcript meta on the first status write.#writeMetaAtomicserializes the whole object and renames over the file, so a field written only at create time is gone after the next status transition — and that file is what the resume path reads. Reproduced: create + one turn leftcollaboration: undefinedon disk. The durability guarantee this PR advertises held only for sessions that had never taken a turn.providerIdcould contradict its own orchestrator role. The claude-only rule guarded the role entry, but the thing that must mount the fleet MCP is the session.providerIdis now derived from the orchestrator role, and an explicit conflicting value is rejected.resolveBackend's model check could never reject anything (carried in from feat: per-child provider/model on dispatched workers (collab sessions P0) #247).resolveAgainstList(...) ?? resolveModelIdForProvider(...)read as strict validation, but the fallback returns its input unchanged. Kept the permissive house policy, deleted the dead branch, corrected the comment. This also corrects an overstatement in feat: per-child provider/model on dispatched workers (collab sessions P0) #247's description.validateCollaborationre-checks the publishedLIMITS(embedded frontends bypass Zod); role names stored lowercased;parseRoleSpecboundscount.The meta regression test drives a real turn before asserting — the original tests only checked the create-time write, which is exactly why the High slipped through. Suite now 1878 pass / 0 fail.
Validation returns a normalized config (goal/name trimmed,
countdefaulted,modelresolved against its own role's backend) so nothing downstream re-resolves — and nothing re-resolves against the wrong provider. This reuses P0'sresolveModelIdForProvider, which is what catches "review on gemini with modelopus" at create time rather than on the first turn.Role names are free-form strings, not an enum (§3, "a role is data"). Adding
security-reviewermust stay a config change.Persisted in both places.
TranscriptMetais what the resume path actually reads; thesessions.collaborationcolumn keeps a collaboration visible to anything querying the table directly. The column is additive;NULL= a normal session.Verification
bun buildclean.src/tests/collaboration.test.tscovers the validator, the CLI grammar, and the realSessionManager.handle()create path — driven by a genuine two-backendProviderRegistry, not_testProviderFactory(which injects one mock into every session and would hide whether the config survives create →SessionInfo→ persistence).collaborationpassthrough fails 2 tests; disabling the fail-closed provider check fails 2 others.bun test"errors" come frompackages/core/src/messages.test.tsdeliberately throwing from a listener to prove later listeners still run. Unrelated; that test passes.Scope notes
roleSchema.providerabsent = the session's own backend, so every existing pack is unaffected; it is consumed only on the fleet/child path, never to mutate a bound session mid-run.MODEL_CATALOGstaleness (opus→claude-opus-4-8) still outstanding from feat: per-child provider/model on dispatched workers (collab sessions P0) #247 — unchanged here.🤖 Generated with Claude Code