fix: route native chat orchestration by agent session identity - #19648
brennanb2025 wants to merge 26 commits into
Conversation
This reverts commit ebd6360.
📝 WalkthroughWalkthroughThe change adds durable agent session identity and runtime fence support to orchestration CLI handlers and RPC schemas. Native sessions can create, use, and query runs without terminal handles. Run and dispatch database records store session identifiers through schema version 41 migrations. Run-scope resolution validates session authority and runtime fences before selecting the current run. Structured worker environments propagate session credentials. Legacy terminal-handle behavior remains available when no agent session is supplied. Priority: ➖ Normal Merge Risk: 🟠 High · up to Native orchestration can fail to start workers, route an explicit terminal request to the wrong run, create unbound runs, or reject valid session workflows. These paths should be corrected before merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the main changes and lists validation results, but it omits several required template sections. It has no linked issue, explicit visual-proof statement, testing checklist, AI disclosure, review section, boundary confirmation, notes, or completion checklist. Resolution Add all required template sections. Include a valid issue reference under Linked Issue, state N/A with a reason or attach visual proof, document manual testing and platforms, complete the AI Disclosure and Agent skill upstream boundary sections, add security and compatibility notes, and complete the Review and Checklist sections.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: a28b1f07-ba76-4a2e-a09c-f9c8ee65da72
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
src/cli/handlers/orchestration/gate-handlers.tssrc/cli/handlers/orchestration/message-send-handler.tssrc/cli/handlers/orchestration/run-handlers.tssrc/cli/handlers/orchestration/task-handlers.tssrc/cli/handlers/orchestration/terminal-identity.tssrc/main/runtime/orchestration/db/contract-constants.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-context-store.tssrc/main/runtime/orchestration/db/dispatch-row-writer.tssrc/main/runtime/orchestration/db/row-column-lists.tssrc/main/runtime/orchestration/db/runs/run-binding.tssrc/main/runtime/orchestration/db/runs/run-create.tssrc/main/runtime/orchestration/db/runs/run-lookup.tssrc/main/runtime/orchestration/db/schema/create-core-tables-sql.tssrc/main/runtime/orchestration/db/schema/create-graph-tables-sql.tssrc/main/runtime/orchestration/db/schema/migrate-v2-v12.tssrc/main/runtime/orchestration/db/schema/migrate-v41.tssrc/main/runtime/orchestration/db/schema/migrate.tssrc/main/runtime/orchestration/types.tssrc/main/runtime/rpc/methods/orchestration/gates/gates.tssrc/main/runtime/rpc/methods/orchestration/messaging/message-methods.tssrc/main/runtime/rpc/methods/orchestration/runs/dispatch-methods.tssrc/main/runtime/rpc/methods/orchestration/runs/run-scope.tssrc/main/runtime/rpc/methods/orchestration/runs/runs.tssrc/main/runtime/rpc/methods/orchestration/schemas.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| objective: getRequiredStringFlag(flags, 'objective'), | ||
| from | ||
| ...(from ? { from } : {}), | ||
| ...(sessionId ? { agentSessionId: sessionId, runtimeFence: Number(fence) } : {}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require a complete session payload before disabling terminal routing.
If ORCA_AGENT_SESSION_ID is set and ORCA_AGENT_SESSION_RUNTIME_FENCE is absent, these branches omit from and send runtimeFence: Number(undefined), which is NaN. This breaks the terminal compatibility path for an incomplete native environment. Build orchestrationSessionPayload() once, and use its agentSessionId to choose session routing.
src/cli/handlers/orchestration/run-handlers.ts#L26-L26: derive the native payload fromorchestrationSessionPayload()instead of converting an optional fence.src/cli/handlers/orchestration/run-handlers.ts#L40-L40: apply the same complete-payload check fororchestration.runUse.src/cli/handlers/orchestration/run-handlers.ts#L54-L54: apply the same complete-payload check fororchestration.runCurrent.
📍 Affects 1 file
src/cli/handlers/orchestration/run-handlers.ts#L26-L26(this comment)src/cli/handlers/orchestration/run-handlers.ts#L40-L40src/cli/handlers/orchestration/run-handlers.ts#L54-L54
| params.coordinatorHandle ?? null, | ||
| params.coordinatorPaneKey ?? null, | ||
| params.coordinatorAgentSessionId ?? null |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reject run creation without a coordinator identity.
When a caller omits all identity fields, Lines 32-34 store a run with no session ID, handle, or pane key. Neither current-run lookup can resolve that run. Require coordinatorAgentSessionId or a complete terminal handle and pane pair before the insert.
| agentSessionId: OptionalString, | ||
| runtimeFence: z.number().int().positive().optional(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bind native callers in orchestration.run.
When a caller supplies agentSessionId, this handler ignores both native fields. It creates the run with the fallback handle "coordinator" and does not persist a coordinator session ID. The caller then cannot resolve that run through the session-based scope path.
Validate the lease fence and create the coordinator run with the native session ID, or reject native identity for this method.
| agentSessionId: OptionalString, | ||
| runtimeFence: OptionalFiniteNumber, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forward native identity for all task operations.
TaskCreateParams, TaskListParams, and TaskUpdateParams now accept these fields, but their handlers pass only callerTerminalHandle to resolveRunScope. A native caller without a terminal handle therefore gets run_required instead of resolving its session-bound run.
Pass callerAgentSessionId: params.agentSessionId and callerRuntimeFence: params.runtimeFence in all three task-handler scope calls.
Also applies to: 207-208, 229-230
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
The native coordinator path authorizes full run/task/gate/mailbox scope on a guessable sessionId + runtimeFence credential, dropping the random handle + pane key + launch-token attestation the terminal path (and the documented design) rely on — see the inline comment on run-scope.ts. Independently, the path is functionally incomplete: the env vars that would drive it are never set, and task scope is never actually routed by session identity.
Reviewed changes
Reviewed the full PR: persistent agent-session identity columns (runs, dispatch_contexts) with a v40→v41 migration, the resolveNativeCoordinatorSession authorization/scope path plus getCurrentRunForAgentSession / getRunMailboxOwnerIdsForAgentSession lookups, CLI propagation of agentSessionId / runtimeFence, and RPC schema/param threading through run/gate/message/dispatch.
- Session-identity schema + migration — adds nullable
coordinator_agent_session_id(runs) andassignee_agent_session_id(dispatch_contexts) columns, bumpsSCHEMA_VERSIONto 41, with amigrate-v41step and av2-v12backfill. - Native session authorization + scope —
resolveNativeCoordinatorSessionvalidates a session id against the durable lease and itsruntimeFence;resolveRunScopeand the run methods route by session id whenagentSessionIdis present. - CLI identity propagation — CLI orchestration handlers read
ORCA_AGENT_SESSION_ID/ORCA_AGENT_SESSION_RUNTIME_FENCEand substitute them for the terminal handle. - RPC surface —
agentSessionId/runtimeFenceadded to run/gate/message/dispatch/task/check/reply params.
⚠️ The session-identity path is unreachable: the env vars are never set
The CLI now reads ORCA_AGENT_SESSION_ID and ORCA_AGENT_SESSION_RUNTIME_FENCE (terminal-identity.ts:53,58), but nothing in the tree ever sets them. structuredWorkerChildIdentityEnv (src/main/runtime/structured-worker-child-identity-env.ts:43-58) — the single place that stamps a structured worker's child environment with orchestration identity, and which both the Codex and Claude launchers call — still exports only ORCA_TERMINAL_HANDLE + ORCA_CLI_COMMAND. A grep for both variable names across the whole repo returns only those two read sites and zero writers. As merged, resolveOrchestrationAgentSessionId() always returns undefined, so the CLI falls through to the terminal-handle path and the new session-identity routing never occurs end-to-end.
Technical details
# Session-identity env propagation is missing
## Affected sites
- src/main/runtime/structured-worker-child-identity-env.ts:43-58 — the child identity env builder; sets ORCA_TERMINAL_HANDLE + ORCA_CLI_COMMAND only.
- src/main/codex/codex-structured-child-environment.ts:13 — calls structuredWorkerChildIdentityEnv.
- src/main/claude/claude-structured-launch-resolution.ts:243 — calls structuredWorkerChildIdentityEnv.
- src/cli/handlers/orchestration/terminal-identity.ts:52-69 — the new readers (ORCA_AGENT_SESSION_ID / ORCA_AGENT_SESSION_RUNTIME_FENCE).
## Required outcome
- The structured worker child environment (or the CLI launch path) must actually export ORCA_AGENT_SESSION_ID and ORCA_AGENT_SESSION_RUNTIME_FENCE for a dispatched native session, so the CLI can take the session path. Otherwise this PR is server-side plumbing with no production caller.
## Open questions for the human
- Is the env propagation intentionally split into a follow-up PR, or was it simply omitted?ℹ️ Nitpicks
RunRow.coordinator_agent_session_id?: string | nullis optional while the siblingcoordinator_handle: string | nullis required; the column now exists on every fresh and migrated DB, so it should be non-optional (string | null).coordinator_runs.coordinator_agent_session_idis added increate-graph-tables-sql.ts:209but (a) is never migrated for existing DBs and (b) is never read or written anywhere — a dead column with an inconsistent migration story.run-handlers.ts:26,40,54useNumber(fence)guarded only bysessionId ?; ifORCA_AGENT_SESSION_IDis set withoutORCA_AGENT_SESSION_RUNTIME_FENCE,Number(undefined)yieldsNaN(JSONnull) and the zod.positive()rejects withinvalid_argument. The other handlers sidestep this viaorchestrationSessionPayload(), which returns{}unless both vars are present.orchestrationSessionPayload()is evaluated twice per conditional (gate-handlers.ts:16,task-handlers.ts:116) to test-and-spread the same env read.runtimeFenceis typedz.number().int().positive().optional()inruns.ts/gates.tsbutOptionalFiniteNumberinschemas.ts— inconsistent (both positive, so not a bug).pnpm-lock.yamladds an@pnpm/exe@12.0.0packageManagerDependenciesentry that looks unrelated to this feature; confirm it's intentional.run-currentstill prints "No Run is bound to this terminal." for a native session.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| const authority = identity | ||
| ? resolveStructuredWorkerAuthority(identity.handle, runtime.getOrchestrationDb()) | ||
| : null | ||
| if (!authority || authority.record.lease.runtimeFence !== runtimeFence) { |
There was a problem hiding this comment.
Authorizing the native coordinator path on sessionId + runtimeFence replaces the random structworker_ handle + random pane key + launch token that the terminal path (and the documented design) require, with two low-entropy, non-secret values: sessionId is plaintext in tab ids and runtimeFence is a small monotonic counter starting at 1. Unlike the terminal branch (run-scope.ts:135), this path never calls assertCallerHandleMatchesEvidence, so any same-user sibling process that reaches the orchestration socket can act as a session's coordinator — run takeover/redirect, mailbox consume, and gate/dispatch actions — by supplying the session id and guessing the fence.
| run: getOptionalStringFlag(flags, 'run'), | ||
| callerTerminalHandle | ||
| callerTerminalHandle, | ||
| ...session |
There was a problem hiding this comment.
task-create (and task-list at :70, task-update at :116-118) propagate agentSessionId/runtimeFence, but the RPC handlers orchestration.taskCreate/taskList/taskUpdate (message-methods.ts:152,191,225) still pass only callerTerminalHandle into resolveRunScope and ignore agentSessionId. For a native session callerTerminalHandle is undefined, so these calls fall through to run_required — task scope never resolves through the session lease, contradicting the PR summary's claim that task scope routes by session identity.
There was a problem hiding this comment.
Important
Delta 429011f..2ab2405 closes the env-propagation gap and hardens the lease check, but it does not resolve the authorization concern: resolveNativeCoordinatorSession now reads the durable record directly and no longer requires the random structworker_ handle, so the coordinator credential is still just sessionId (public, in tab ids) + runtimeFence (a counter starting at 1). See inline.
Reviewed changes (delta since 429011f)
- Env propagation added —
structuredWorkerChildIdentityEnvnow stampsORCA_AGENT_SESSION_ID/ORCA_AGENT_SESSION_RUNTIME_FENCEand strips inherited credentials; Codex/Claude launchers thread the fence through; WSL and TUI-launch env passthrough included (wsl-orca-env.ts,orca-runtime-get-agent-session-execution-namespace.ts). This resolves the prior "env vars never set" finding. - Session authority rewritten —
resolveNativeCoordinatorSessionreads the durable record directly and addsclaimStatus === 'live',!unreconciled,handoffStage === nullguards, dropping the registry/random-handle lookup. - Check + worker-start routed by session —
orchestration.check/checkRunMailboxandworker-startnow resolve Run scope viaagentSessionId, withresolveWorkerStartCallerHandle. - Worker-start schema —
frombecomes optional (.superRefinerequiresfrom || agentSessionId).
⚠️ Task scope remains unrouted by session identity
The new commits route check/worker-start/dispatch/send/gate/run by session, but orchestration.taskCreate/taskList/taskUpdate (message-methods.ts) are still unchanged: they pass only callerTerminalHandle into resolveRunScope and ignore agentSessionId. For a native session the CLI sends callerTerminalHandle = undefined + agentSessionId, so these calls still fall through to run_required. This is unchanged from the prior review and now stands out as the one gap in an otherwise-consistent session routing surface.
ℹ️ Nitpicks
resolveNativeCoordinatorSessionreturns{ worktreeId: record.location.workspaceId }— a folderworkspaceId, not necessarily a git worktree id — and no caller uses the return value.resolveWorkerStartCallerHandlereturns''for an ordinary native coordinator with no registry identity;resolveDispatchCreator('')degrades to{ kind: 'system' }(acceptable), buttaskCreatedByTerminalHandle/coordinatorHandleare then written as the empty string.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| sessionId: string, | ||
| runtimeFence: number | ||
| ): { sessionId: string; worktreeId: string } { | ||
| const record = readStructuredAgentSessionRecord(sessionId) |
There was a problem hiding this comment.
Rewriting this to read the durable record directly removes the last possession requirement. Previously reaching authority required a registered structworker_ handle (a random secret); now a caller needs only sessionId (plaintext, embedded in tab ids via structuredAgentSessionTabId) plus runtimeFence (a monotonic counter minted from 1). The added claimStatus === 'live' / !unreconciled / handoffStage === null guards strengthen liveness, not authorization: no caller attestation, launch token, or pane key is checked, so any same-user sibling process that reaches the orchestration socket can act as this session's coordinator by supplying the id and brute-forcing the fence.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/runtime/rpc/methods/orchestration/gates/gates.ts (1)
17-18: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBind native session authority during
orchestration.runcreation. WhenagentSessionIdis present, validate itsruntimeFencewithresolveNativeCoordinatorSessionand persist the session binding throughcreateCoordinatorRun. The current handler passes onlyspec,coordinatorHandle, andpollIntervalMs, so a native request can create an unbound run without lease-fence validation.src/cli/handlers/orchestration/run-handlers.ts (1)
10-56: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
orchestrationSessionPayload()for all three run handlers.When only
ORCA_AGENT_SESSION_IDis set,run-create,run-use, andrun-currentomit terminal fallback and sendruntimeFence: NaN. The RPC schemas require a positive integer, so all three calls are rejected. Branch onsession.agentSessionIdand spread the all-or-nothingsessionpayload instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 92925fc6-a34a-45f0-9040-bedfa63ac765
📒 Files selected for processing (21)
src/cli/handlers/orchestration/dispatch-handlers.tssrc/cli/handlers/orchestration/message-check-handler.tssrc/cli/handlers/orchestration/question-handler.tssrc/cli/handlers/orchestration/worker-launch-handler.tssrc/main/claude/claude-structured-launch-resolution.tssrc/main/codex/codex-structured-child-environment.test.tssrc/main/codex/codex-structured-child-environment.tssrc/main/codex/codex-structured-session-acquire.tssrc/main/pty/wsl-orca-env.tssrc/main/runtime/orca-runtime-get-agent-session-execution-namespace.tssrc/main/runtime/orca-runtime-structured-agent-session-launch-tui.tssrc/main/runtime/rpc/methods/orchestration/messaging/check-methods.tssrc/main/runtime/rpc/methods/orchestration/messaging/check-run.tssrc/main/runtime/rpc/methods/orchestration/runs/dispatch-creator.tssrc/main/runtime/rpc/methods/orchestration/runs/native-session-authority.test.tssrc/main/runtime/rpc/methods/orchestration/runs/run-scope.tssrc/main/runtime/rpc/methods/orchestration/worker/local-worker-start.tssrc/main/runtime/rpc/methods/orchestration/worker/worker-start-schema.tssrc/main/runtime/rpc/methods/orchestration/worker/workers.tssrc/main/runtime/structured-worker-child-identity-env.test.tssrc/main/runtime/structured-worker-child-identity-env.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| ...(Object.keys(sessionPayload).length > 0 | ||
| ? sessionPayload | ||
| : { from: await resolveCoordinatorTerminalHandle(flags, cwd, client) }), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Honor an explicit --from terminal handle.
When orchestrationSessionPayload() returns a non-empty payload, this branch ignores --from and sends only agentSessionId and runtimeFence. The worker then starts through the native session instead of the terminal selected by the caller. This breaks the legacy compatibility override and can select the wrong run or worker owner.
Create the session payload only when flags.has('from') is false, as the dispatch and question handlers already do.
Proposed fix
- const sessionPayload = orchestrationSessionPayload()
+ const sessionPayload = flags.has('from') ? {} : orchestrationSessionPayload()| const identity = params.agentSessionId | ||
| ? structuredWorkerIdentities.getBySessionId(params.agentSessionId) | ||
| : undefined | ||
| return identity?.handle ?? params.from ?? '' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Resolve the native caller worktree without a terminal handle.
A valid native folder session can have no worker identity or PTY. Line 37 then returns '' when from is absent. Line 60 passes that value to resolveDispatchCallerWorktreeId, which falls through to runtime.showTerminal(''). Worker startup fails before it can create the dispatch.
src/main/runtime/rpc/methods/orchestration/runs/dispatch-creator.ts#L34-L37: Do not represent an unresolved native session as an empty terminal handle. Return native workspace authority separately.src/main/runtime/rpc/methods/orchestration/worker/local-worker-start.ts#L52-L60: Use the validated native session workspace ID for native callers. Only useresolveDispatchCallerWorktreeIdfor terminal-backed callers.
📍 Affects 2 files
src/main/runtime/rpc/methods/orchestration/runs/dispatch-creator.ts#L34-L37(this comment)src/main/runtime/rpc/methods/orchestration/worker/local-worker-start.ts#L52-L60
|
Superseded by #22568, stacked on #22555 and #22522 as part of a fresh orchestration stack for structured-session coordinators. This PR held the only CLI conversion, so its intent carried forward. The design changed underneath it:
#22568 keeps this PR's check pane-key suppression, its send lifecycle guard, and the flagless |
1 participant
Summary
Native chat orchestration now uses durable agent-session identity alongside the existing terminal compatibility path. Native coordinators can create, bind, and inspect Runs without fabricating a PTY projection, and task, gate, dispatch, and message scope can resolve through the authenticated session lease.
Changes
Validation
pnpm tc:nodeORCA_BACKGROUND_LAUNCH=1 env -u ORCA_STRUCTURED_SESSION pnpm test src/cli/handlers/orchestration src/main/runtime/rpc/methods/orchestration --run— 913 passed.