Skip to content

feat: collaboration config on session.create (collab sessions P1a) - #248

Merged
saucam merged 2 commits into
mainfrom
feat/collab-p1-create-path
Jul 26, 2026
Merged

feat: collaboration config on session.create (collab sessions P1a)#248
saucam merged 2 commits into
mainfrom
feat/collab-p1-create-path

Conversation

@saucam

@saucam saucam commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

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:

  • P1a — this PR. The contract: a collaboration's goal + role→backend bindings can be requested, validated, persisted, and read back. The daemon stays fully working; the config round-trips but drives nothing yet.
  • P1b — next. The consumer: compile to an ephemeral one-goal pack, spawn role-children on their bound backends with leaf scopes, tear down at goal end.

Splitting also isolates the cross-repo protocol change into one small, auditable diff.

Lockstep: paired with highflame-ai/codeoid-ui#35, per codeoid-ui CONTRIBUTING ("if you change codeoid-protocol … update the daemon's src/protocol/ in lockstep").

What changed

Area Change
packages/protocol CollaborationConfig / CollaborationRole / ORCHESTRATOR_ROLE; on SessionCreateMsg, echoed on SessionInfo; 3 published LIMITS
schemas.ts Zod — shape only, deliberately
daemon/collaboration.ts new — the semantic rules + the CLI role grammar
store.ts / transcript.ts collaboration JSON column + TranscriptMeta field
pipeline/pack.ts roleSchema gains optional (provider, model) — §8.1
cli.ts / terminal/client.ts --collaborate <goal> + repeatable --role
codeoid new api-limits ~/repo \
  --collaborate "Add rate limiting to the public API" \
  --role orchestrator:claude \
  --role reasoning:openai:gpt-5-codex \
  --role review:gemini*3

Design 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 to providerId being 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.

  • 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 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 left collaboration: undefined on disk. The durability guarantee this PR advertises held only for sessions that had never taken a turn.
  • Medium — a session's providerId could 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. providerId is now derived from the orchestrator role, and an explicit conflicting value is rejected.
  • Medium — 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.
  • Low ×3validateCollaboration re-checks the published LIMITS (embedded frontends bypass Zod); role names stored lowercased; parseRoleSpec bounds count.

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, count defaulted, model resolved against its own role's backend) so nothing downstream re-resolves — and nothing re-resolves against the wrong provider. This reuses P0's resolveModelIdForProvider, which is what catches "review on gemini with model opus" 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-reviewer must stay a config change.

Persisted in both places. TranscriptMeta is what the resume path actually reads; the sessions.collaboration column keeps a collaboration visible to anything querying the table directly. The column is additive; NULL = a normal session.

Verification

  • Suite 1871 pass / 0 fail (+29), typecheck + biome + bun build clean.
  • New src/tests/collaboration.test.ts covers 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 into every session and would hide whether the config survives create → SessionInfo → persistence).
  • Mutation-checked: dropping the collaboration passthrough fails 2 tests; disabling the fail-closed provider check fails 2 others.
  • Two pre-existing bun test "errors" come from packages/core/src/messages.test.ts deliberately throwing from a listener to prove later listeners still run. Unrelated; that test passes.

Scope notes

🤖 Generated with Claude Code

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>
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

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>
@saucam
saucam merged commit 1075f42 into main Jul 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants