feat(orchestration): deliver worker results to a structured chat coordinator - #22631
brennanb2025 wants to merge 26 commits into
Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — three test-strength suggestions inline.
Reviewed changes
Read all 20 changed files across the 7 commits at 3d12695 against base brennanb2025/d3-cli-injection (51f9685).
- Handle-less coordinator delivery —
handleLessCoordinatorSessionIdresolves a Run's coordinator fromcoordinator_actoronly whencoordinator_handleis null, so a chat coordinator'srun:mailbox now reaches the session instead of falling between the two lanes. session:<id>recipients —readSessionRecipient/refuseUndeliverableSessionRecipientmake any reachable session addressable, reusing the existingsession_caller_*codes and storing atsession:<id>before pointing it as a turn.- Wake on eviction — the pointer lane takes a transient
host.holdaround each attempt and releases it infinally, resuming a chat the release clock evicted and handing it back afterwards. - Idle-edge redrive —
onStructuredSessionStatusForMailretries parked mail and re-derives the Runs plus direct mailbox a session owns on every non-working status edge, host-wide rather than per-dispatch. - Terminal-view ownership — a terminal view speaks for its session: the mailbox owner takes the session's Run/Dispatch, and the delivery target resolves the PTY the write gate binds to that session.
- Shared session rules —
structured-session-mail-address.tscentralizes reach, delivery view andsessionOrchestrationIdentity, consumed by both the caller resolver and mail delivery. - Worker preamble settlement —
sendStructuredWorkerPreamblewaits out apendingfirst answer viawaitForSendSettlementinstead of discarding a worker whose turn had begun.
I traced the wake/hold pair against the real StructuredAgentSessionHolds and release clock (resume works, the holder is always removed, release lands after send, and an active turn re-arms the clock rather than evicting), and audited the visibleSessionIds writers (a released lease does not remove the id, so a backgrounded or non-active-workspace chat stays reachable). The remaining notes are about test strength, not production behavior.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| { dispatchState: 'accepted', reason: null } | ||
| ) | ||
| ) | ||
| ).resolves.toBeUndefined() |
There was a problem hiding this comment.
This asserts the resolved outcome but never that waitForSendSettlement was awaited, so a pending → return short-circuit (claiming delivered before the provider echo) would also resolve undefined and pass. A toHaveBeenCalledWith(sessionId, 'c1') assertion on the host's settlement method is what the test name promises.
| await host.flushStreamedEvents(COORDINATOR) | ||
|
|
||
| await finishWorker(taskId) | ||
| await new Promise((resolve) => setTimeout(resolve, 20)) |
There was a problem hiding this comment.
The fixed 20 ms sleep is the only thing separating "retained mid-turn" from "folded into the running turn" — if the pointer send lands just after this check, the test still ends green with two turns, and the assertion at line 417 cannot tell the difference. Synchronize on a delivery marker (or assert the pointer is still parked) instead of wall-clock.
3d12695 to
a7f58f3
Compare
There was a problem hiding this comment.
Important
The new pointer constant is a POSIX-only shell expansion applied to every structured session, including Windows where it is shell-invalid. It is anchorable, so the detail lives inline.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (3d12695). The branch was rebased onto #22568 (c0767e101a), rewriting SHAs, so the net change is one commit, a7f58f3, plus #22568 dropping out of the PR diff.
- CLI named by its env var in the pointer turn — added
STRUCTURED_SESSION_CLI_COMMAND = '"$ORCA_CLI_COMMAND"'and pass it from the structured pointer lane, so a login shell that rebuildsPATH(Codex'szsh -lc) cannot reroute a bareorcato a global install. formatMessagePointerwidened — itscliCommandparameter now acceptsOrchestrationCliCommand | typeof STRUCTURED_SESSION_CLI_COMMAND; the PTY lane's resolved command and the default are unchanged.- Integration test updated — the real-host test's
POINTERregex and the peer-chat assertion now expect the env-var form.
The three open threads from the prior review (the eviction-retention test's missing wake assertion, the preamble test's missing waitForSendSettlement assertion, and the fixed 20 ms mid-turn discriminator) are unchanged by this commit and left open.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
a7f58f3 to
1c08143
Compare
There was a problem hiding this comment.
Important
The pointer now renders the Windows form for Codex, but the win32 + Claude arm still assumes Git Bash unconditionally — an assumption Orca's own Claude hook code does not make.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (a7f58f3): three commits (a520e9b8f2..1c08143) across 12 files, after the branch was rebased onto #22568.
- Per-shell pointer invocation — the fixed
STRUCTURED_SESSION_CLI_COMMANDconstant becamestructuredSessionCliInvocation({ platform, provider }); the structured pointer lane reads the provider off the session record and now renders& $env:ORCA_CLI_COMMANDfor Windows Codex alongside the POSIX form. - Session recipients addressed where their
checkreads — asession:<id>or bare-id recipient for a structured worker now resolves to the worker's own handle/Dispatch mailbox (sessionOrchestrationIdentity().address) instead of thesession:<id>mailbox itschecknever reads. - Released-lease caller wording — a released lease now refuses as "is not running right now … retry then" rather than "has ended", matching the fact that mail can revive it.
- Idle-edge redrive wiring pinned — a new test pins the runtime's own
onSessionStatusChanged→onStructuredSessionStatusForMailline, which the integration test's own callback had previously masked.
I traced the worker-address change through resolveBareOrchestrationRecipient (it reaches the Dispatch branch via the worker's paneKey, and the flagless check reads both the handle and Dispatch mailboxes), and confirmed the 42 tests in the four touched suites pass.
ℹ️ Nitpicks
SessionRecipient.actor(session-recipient.ts:28) is written but never read —recipient-routing.tsandrefuseUndeliverableSessionRecipientconsume onlysessionId.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| }): StructuredSessionCliInvocation { | ||
| return session.platform === 'win32' && session.provider === 'codex' | ||
| ? '& $env:ORCA_CLI_COMMAND' | ||
| : '"$ORCA_CLI_COMMAND"' |
There was a problem hiding this comment.
The Codex arm is now correct, but the win32 + claude fall-through still hardcodes the POSIX form on the premise that Claude on Windows always runs commands in Git Bash. Upstream Claude Code docs say Git Bash is optional: the PowerShell tool is primary when enabled, and PowerShell is the only shell when Git for Windows is absent — there "$ORCA_CLI_COMMAND" is a PowerShell parse error and the pointer turn cannot run check.
Orca already makes this exact distinction for Claude hooks: getWindowsManagedLifecycleHook (src/main/claude/hook-settings.ts:180-192) uses the direct POSIX command only when isGitBashAvailable(), otherwise it emits a PowerShell command. The pointer lane should branch the same way, or the JSDoc at cli-command.ts:18 should state the Git Bash precondition so the assumption is auditable.
Technical details
# Windows Claude structured sessions may receive an unrunnable pointer
## Affected sites
- `src/main/runtime/orchestration/cli-command.ts:24-26` — `win32` + `claude` returns `"$ORCA_CLI_COMMAND"`, treating Git Bash as guaranteed.
- `src/main/runtime/orchestration/cli-command.ts:18` — the JSDoc states that premise as fact.
- `src/main/runtime/orchestration/structured-mailbox-pointer-host.ts:85-91` — `cliInvocation` has the session record (so `process.platform` and `provider`) but no Git Bash availability.
- Precedent: `src/main/claude/hook-settings.ts:180-192` — `getWindowsManagedLifecycleHook` branches on `isGitBashAvailable()`.
- Availability source: `src/main/git-bash.ts:108` (`isGitBashAvailable`), already surfaced over RPC by `host-capabilities.ts`.
## Required outcome
- On a Windows host, a Claude structured session without Git Bash must receive a pointer its shell can run (`& $env:ORCA_CLI_COMMAND …`), or the POSIX mapping must be provably safe under Orca's structured-Claude launch.
## Suggested approach
- Thread Git Bash availability into `structuredSessionCliInvocation` (e.g. an optional `gitBashAvailable` on the session arg) and pass `isGitBashAvailable()` from `cliInvocation`; keep the POSIX form only when Git Bash is present.
- If Orca guarantees Git Bash for structured Claude on Windows, record that guarantee in the JSDoc.
## Open questions for the human
- Does Orca require Git for Windows for structured Claude on Windows, or can it run with the PowerShell tool as the primary shell?
There was a problem hiding this comment.
ℹ️ No critical issues — one design question in the body and one stale comment inline.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (1c08143): six commits across 18 files. This run covers the /clear adoption path, the pointer-eligibility rework, and the wake diagnostic.
/clearadoption —adoptClearedPredecessorMailrebinds a cleared predecessor's Runs to the successor through the ordinarybindRun(generation bump, fenced deliveries, coordinator-mail reroute) and re-addresses its unread direct mail withdelivered_at = NULL;runStructuredConversationCommandreports the replacement at the commit, and the runtime redrives the successor's mail at that edge and on every idle edge.- Pointer eligibility reworked — the "an unacknowledged batch exists" latch is gone. The lane now excludes the batch the reader already holds, points newer mail, and names
--ack <deliveryId>so the reader can release the held batch and reach the new mail. pendingcounts as pointed — an admitted-but-not-yet-echoed send now marks the rows delivered (structuredDispatchDeliveredreturns true forpending);unknownstill retains.- Wake diagnostics — a
host.holdthat cannot resume the session now logs the reason instead of failing silently. - Test budgets — coordinator-mail waits use a 10s budget.
I traced the eligibility change through check (it returns read = 0 rows regardless of delivered_at, so marking delivered never hides mail from a reader), confirmed getOrCreateMailboxDelivery only ever creates deliveries for run:/dispatch: handles (so the named --ack always reaches a check path that supports it), and verified runBoundToCoordinator restricts adoption to handle-less bindings.
ℹ️ Run mail pointed at a /clear predecessor is not re-pointed at the successor
The direct-mail half of the adoption deliberately resets the pointer fact (delivered_at = NULL) because "its old pointer, if any, went to a session nobody runs". The Run half does not: bindRun bumps the generation and fences outstanding deliveries, but leaves delivered_at set on the Run mailbox's rows. Pointer eligibility is delivered_at IS NULL, so a worker_done that was pointed at the predecessor but not read before the clear stays unprompted on the successor — the successor's idle-edge deliverPendingMessagesForHandle('run:<id>') selects nothing. The result is still readable with an explicit check --run <id>, but nothing tells the new coordinator it is there. Raising to confirm the window is intentional/accepted rather than an omission.
Technical details
# Run-mail already pointed at the predecessor is not re-pointed to the successor
## Affected sites
- `src/main/runtime/orchestration/structured-session-mail-target.ts:131-145` — `adoptClearedPredecessorMail` rebinds each Run via `bindRun`, then re-addresses *direct* mail; no step resets `delivered_at` on the Run mailbox.
- `src/main/runtime/orchestration/db/runs/run-binding.ts:119-160` — `bindRun` bumps `consumer_generation` and calls `fenceUnacknowledgedMailboxDeliveries`, but only touches the `deliveries` table, not `messages.delivered_at`.
- `src/main/runtime/orchestration/db/messages/direct-mailbox-routing.ts:189-200` — the direct-mail path *does* set `delivered_at = NULL`, which is the inconsistency.
## Why it matters
`selectOrchestrationPointerBatch` selects via `getUndeliveredUnreadMessages` (`delivered_at IS NULL`, `read = 0`). The structured pointer lane calls `markAsDelivered(staged)` as soon as `host.send` returns accepted/pending — before the provider turn reads the mail. If the pointer turn settles without the chat running `check`, then `/clear` replaces the session, the row survives the rebind as `read = 0, delivered_at` set, and the successor is never pointed at it.
## Required outcome
Decide whether a Run result pointed at a cleared predecessor should be re-pointed at the successor, matching the direct-mail path. If so, reset `delivered_at` (e.g. `markAsUndelivered`) on the rebounded Run's unread messages during adoption, before `structuredSessionOwnedMailboxes` re-derives the mailbox.
## Open questions for the human
- Is the pointer turn's read effectively guaranteed before `/clear` is admitted (making the window unreachable), or is this an accepted gap?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| // A pending send is not an acknowledgement, so the mail is retained and retried on every edge | ||
| // until the host confirms it: before the echo, and again at the turn's idle edge. |
There was a problem hiding this comment.
This comment describes the pre-delta behavior. Now that pending is a pointed state (structuredDispatchDelivered), the pointer's send marks the rows delivered immediately, so the retries at lines 356-358 are no-ops rather than the retention this comment claims to exercise. The assertion still holds; only the rationale is stale.
| // A pending send is not an acknowledgement, so the mail is retained and retried on every edge | |
| // until the host confirms it: before the echo, and again at the turn's idle edge. | |
| // The pointer marks the rows delivered as soon as the host admits the turn, so these retries | |
| // are idempotent no-ops; the test pins that they never produce a second pointer. |
ca9b471 to
6abb9d9
Compare
There was a problem hiding this comment.
ℹ️ No critical issues — one design tradeoff in the body worth confirming.
Reviewed changes
Re-reviewed the 4 commits on top of the prior pullfrog review (ca9b4718): the pending-settlement give-back, the /clear observer guard, and their tests.
- An admitted pointer is given back when its turn never settles —
StructuredMailboxPointerHost.sendnow returns asettlementpromise forpending(built fromwaitForSendSettlement), andsettlePendingPointerdeletes this send's operation row and callsmarkAsUndelivered+retainon any verdict other thanaccepted. The row is dropped so the re-point mints a fresh id instead of replaying the recordedunknown, which would land no turn at all. - A committed
/clearno longer fails on a throwing replacement observer —runStructuredConversationCommandwrapsonConversationReplacedin try/catch and warns; the idle edge re-derives adoption, so bookkeeping behind an already-committed clear cannot turn it into a failure. - Tests — provider death before the echo now gives the result back and re-points it once on the idle edge; a chain of clears is adopted fully and the predecessor's
checkreads nothing of the successor's mail; the host's settlement mapping covers echo, provider death, timeout, and a disappeared send.
I traced the give-back against the real DB semantics (getUndeliveredUnreadMessages requires delivered_at IS NULL; markAsUndelivered guards read = 0; resolveStructuredPointerOperation reuses an id only for the same mailbox+session+batch) and the real trigger paths (deliver/inFlight, onJournalActivity, the idle edge). The message-row half of the give-back has no operation_id guard, but delivered_at exclusivity plus inFlight serialization keeps a message out of two staged batches, so it cannot clobber a newer pointer's claim. structured-mailbox-pointer-delivery.test.ts, structured-mailbox-pointer-host.test.ts, and structured-conversation-command.test.ts pass (55 tests).
ℹ️ An ambiguous pending settlement can produce a second pointer turn
settlePendingPointer now gives the batch back on any non-accepted verdict, which is the right call for a dead provider — but unknown also covers the 30s waitForSendSettlement timeout, where the turn may still be queued and later echo accepted. By then the operation row is gone and the rows are undelivered again, so the accepted echo's journal edge mints a new id and sends a second pointer turn for the same mail. It is rare (a normal echo is sub-second, and the PR notes legacy clients already abandon at 15s) and strictly better than the silent strand it replaces, but the coordinator can now receive a duplicate "you have mail" turn. Confirming that is acceptable; if not, giving back only on a definitive verdict (session close, not timeout) would bound it.
Technical details
# Duplicate pointer turn on an ambiguous pending settlement
## Affected sites
- `src/main/runtime/orchestration/structured-mailbox-pointer-delivery.ts:284-325` — the `pending` branch marks rows delivered and defers; `settlePendingPointer` deletes the op row and `markAsUndelivered`s on any non-`accepted` verdict, including the timeout-derived `unknown`.
- `src/main/runtime/orchestration/structured-mailbox-pointer-host.ts:135-154` — `settlement` maps an `undefined` result (the 30s timeout) to `'unknown'`.
- `src/main/native-chat/agent-session-wire/structured-agent-session-send-settlement.ts:84-88` — the timeout resolves the waiter `undefined` even though the submission is still live; a later `publish` would have resolved it `accepted`, but the waiter is already removed.
## Why it matters
`getUndeliveredUnreadMessages` gates on `delivered_at IS NULL`. After the give-back, the accepted echo publishes a journal item → `onJournalActivity` → `deliver` finds the rows selectable again and mints a fresh operation id (the row was deleted), dispatching a second provider turn for the same batch. The mail is not lost or double-read; the cost is a redundant turn in the coordinator chat.
## Required outcome
Decide whether the duplicate turn on an ambiguous `unknown` is acceptable. If not, distinguish a timeout/waiter-cap `unknown` from a definitive dead-session verdict and only give back in the definitive case.
## Open questions for the human
- Is the duplicate turn acceptable given it replaces the prior silent strand, or should the give-back be limited to definitive failures?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
2d9dc67 to
fef6854
Compare
6abb9d9 to
a32a526
Compare
fef6854 to
3af56b1
Compare
e1e205d to
55692e7
Compare
There was a problem hiding this comment.
ℹ️ No critical issues — the
/clearredesign is clean; two informational notes below.
Reviewed changes
Re-reviewed the three commits since the prior pullfrog review (55692e7e6b), which replace /clear continuity handling and add a restart-recovery path.
/clearcontinuity is derived, not rewritten —structuredSessionLineageresolves any session of a clear chain to its lineage root (the conversation's stablesession:<root>address) and its live end;sessionOrchestrationIdentitytakes the actor from the root and the worker identity from the live session. Runs, direct mail and the caller identity all stay under one address with nothing rewritten at the clear, andadoptClearedPredecessorMail,readdressUnreadSessionMailand theonConversationReplacedwiring are removed.- Restart recovery for structured pointer claims —
releaseRestoredStructuredPointerClaimsruns when the orchestration DB opens: for each durable pointer-operation row it finds that row's stamped batch by fingerprint among the mailbox's pointed-unread rows, marks it undelivered and drops the row, so a claim whose settlement waiter died with the previous process is re-pointed by the normal undelivered scan. - Idle-edge redrive opens the DB itself —
structuredSessionIdleEdgeMailboxescallsgetOrchestrationDb()so a restarted chat's idle edge redrives mail stored before the restart; a DB that cannot open is logged rather than skipped silently.
ℹ️ The /clear section of the PR description describes the mechanism this delta removed
The PR body is the design doc here, and §6 still explains /clear continuity as a rebind of Runs plus a re-address of direct mail, driven by an onConversationReplaced observer. All three were removed by 060790e567, so a reviewer reading the body against the code will look for machinery that is gone.
Technical details
# Stale PR description for `/clear` continuity
## Affected sites
- PR body §6 "A `/clear`-replaced coordinator's Runs and mail follow the new session" — describes `bindRun` rebinding and `readdressUnreadSessionMail`, both removed.
- PR body §6 "Two edges drive it" — names the clear's commit (`onConversationReplaced`) and the successor's first status publication; the observer and its runtime wiring are gone.
- PR body "Testing" and "Ablations" — list `/clear`-predecessor, chain-adoption and `structured-conversation-command.test.ts` replacement-observer cases that no longer exist.
## Required outcome
- §6 should describe the lineage-derived conversation address (root actor for the address, live session for the delivery target, nothing rewritten at the clear); the Testing/Ablation lists should drop the removed cases.ℹ️ Restart recovery assumes one outstanding pointer claim per mailbox
The recovery releases only the batch whose fingerprint matches the single operation row per mailbox. Because that row is an upsert keyed by mailbox, a newer batch pointed while an earlier one is still pending replaces the earlier row, and the earlier batch is then not released on restart. Worth confirming this window is accepted (it needs process death inside the pending interval).
Technical details
# An earlier pending batch is not released when a newer batch overwrote its operation row
## Affected sites
- `src/main/runtime/orchestration/structured-pointer-claim-restore.ts:28-34` — iterates `listStructuredPointerOperations()` (one row per `mailbox_handle`) and releases only the batch whose fingerprint matches that row.
- `src/main/runtime/orchestration/db/messages/structured-pointer-operation-store.ts:21-41` — `putStructuredPointerOperation` upserts on `mailbox_handle`, so a newer batch overwrites the earlier row.
- `src/main/runtime/orchestration/structured-mailbox-pointer-delivery.ts:283-297` — a `pending` send marks rows delivered and keeps the op row; a later batch on the same mailbox mints a new id and overwrites it (`resolveStructuredPointerOperation`).
## Why it matters
If batch X is admitted `pending` (rows `markAsDelivered`ed, op row = X) and batch Y is pointed before X settles, Y's `putStructuredPointerOperation` replaces the row. On restart `getPointedUnreadMessages` returns `[X, Y]`, `stampedBatch` matches only Y, and X stays `delivered_at`-stamped with no row: the undelivered scan never re-points it (it stays readable via an explicit `check`). In-process this is fine — `settlePendingPointer`'s `markAsUndelivered(staged)` is not guarded by the op-row check — so only process death inside the pending window loses X.
## Required outcome
- Confirm whether a single row per mailbox is the intended scope. If earlier batches must also be recovered, note that "stamped rows with no matching row" cannot be released wholesale: an accepted-but-unread batch legitimately has no operation row.
## Open questions for the human
- Is the multi-batch window accepted, or should each batch's claim be stored separately?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the one follow-up commit (e79d321f0a) on top of the prior review.
- The idle edge no longer creates the orchestration database — a new
getExistingOrchestrationDb()opens the database only when its file already exists (or it is already open), andstructuredSessionIdleEdgeMailboxestakes() => OrchestrationDb | null, returning no mailboxes when there is none. A profile that never orchestrated stays without a database file and logs nothing; failing to open an existing file is still logged. - Tests updated — the restart test seeds a real database file under a fake
userDataand asserts the idle edge opens it; a new test pins that no file is created and nothing is logged.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…dinator Resolve a Run's handle-less session coordinator and a session:<id> mailbox to the live session, wake an evicted session for the delivery, redrive a session's own mail on its idle edge, route a terminal view's pointer through its PTY, and accept session:<id> (or a bare Orca session id) as a recipient.
…n host, wake, idempotence and session addresses
…and treat a worker without its identity as undeliverable
…carding the started worker
…uards instead of assertions
…AND in its pointer turn
… the session runs in
…, so a structured worker gets its mail
…say a released session is not running, not ended
…body has acked, naming the ack a held batch needs
…il to the session that replaced it
… commit, the edge its replacement's own status misses
…ad of retaining silently
…under a loaded parallel run
…y type guard instead of a cast
A pending send stamped its rows delivered and dropped its operation row, so a provider that died before echoing left the last result pointed at nobody. The lane now awaits the admitted turn's settlement: accepted consumes the claim, anything else returns the rows and drops this send's operation row, so the re-point on the next edge is a new send rather than a replay of unknown.
…ment observer The observer runs after the clear's durable commit; a throwing adoption turned a committed clear into a failed RPC. It is now best-effort and logged, like the status feed's observer, and the successor's idle edges re-derive the adoption.
… a predecessor's check A session cleared twice before any edge hands both predecessors' Runs and mail to the end of the chain. A predecessor still live in the clear's tail reads none of the re-addressed mail: a session's direct mailbox is consume-on-read and holds no replayable batch.
…ut without an assertion
… by deriving its lineage A chat's orchestration address is now the first session of its /clear lineage. Every session of the lineage resolves to that one actor when it acts and when it is reached, and delivery goes to the lineage's live session. Nothing is rewritten at a clear, so the predecessor-adoption path is gone: the commit-edge observer, the idle-edge rebind, and the unread-mail re-address. That path could unbind the successor's own Run and orphan all but one Run of a chain. The idle edge now opens the orchestration database through its lazy getter and logs when it cannot, instead of reading a field that stays null until the first orchestration call after a restart.
…rocess left open A pointer the host admits as pending stamps its batch delivered, and only an in-memory settlement waiter gives it back if no turn ran. A process that died in that window left the batch stamped with nothing to release it, so the last result on that mailbox was never pointed again. When the database opens, every surviving pointer operation row is from an earlier process: its stamped batch is found by the row's fingerprint, released, and the row dropped, before the restored-mailbox scan points it again. A row whose batch was never stamped keeps its id for the retry.
…nly when it already exists A profile with no orchestration database has no mail to redrive, so a structured chat's idle edge no longer creates one, and says nothing. An existing database is still opened lazily there.
… actor's generation A handle-less Run names its coordinator session only by an actor that still counts at the Run's current generation, the same rule every other binding read uses; an actor an older binary's rebind or unbind left behind no longer routes the Run's mail. A test pins that a cleared chat's run-create and run-use write its conversation's root actor at the Run's current generation.
0ef7f31 to
3f5f798
Compare
e79d321 to
92ecd41
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the one commit on top of the prior pullfrog review (e79d321f0a). The branch was rebased, so the new commit is isolated by subject rather than ancestry.
- A handle-less Run's coordinator actor now counts only at the Run's current generation —
handleLessCoordinatorSessionIdreads the coordinator throughcurrentRunCoordinatorActor, so an actor an older binary's rebind or unbind left behind no longer routes the Run's mail. This is the same rulerunCoordinatorKey,runBoundToCoordinator,runsBoundToCoordinatorand the v42 backfill already apply, so delivery and ownership can no longer disagree about which session a Run is bound to. - Tests pin both the read and the write —
structured-session-mail-target.test.tsbumpsconsumer_generationand asserts therun:mailbox stops resolving (this case fails against the pre-commit raw-actor read);structured-chat-coordinator-mail.test.tsasserts a cleared chat'srun-create/run-usestore the conversation's root actor at the Run's current generation.
I traced the write paths (createRun writes coordinator_actor_generation = 1 beside consumer_generation = 1; bindRun always writes the actor at consumer_generation or consumer_generation + 1; the backfill stamps consumer_generation) and confirmed no other production reader consumes the raw coordinator_actor column.
Four earlier Pullfrog threads remain open (they are test-strength/design notes, unchanged by this commit) and are not re-raised here.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
1 participant
ELI5
A native chat can now coordinate workers and actually hear back from them. When a worker finishes, Orca drops a short note into the coordinator chat: "You have 1 orchestration message. Run
orca orchestration check". It arrives as a real message in that chat and starts a turn, with nobody touching anything. The chat runscheckand reads the worker's result. If Orca had put that chat to sleep because nobody was looking at it, the note wakes it back up.The same mechanism lets anyone message any live chat by its id (
session:<id>), not only a coordinator.Merge order
Stacked on #22568. Merge in this order: #22522 → #22555 → #22568 → this PR. The PR base is
brennanb2025/d3-cli-injection(head2d9dc6738e).What works end to end
Proven live in the running app, not only in tests. The rig was an isolated dev profile under
ORCA_BACKGROUND_LAUNCH=1, driven over CDP with a hidden window.Claude: a real structured Claude chat does all of this with no user action:
coordinator_actor = session:<id>, no handle);worker_done;orca orchestration check, which returns theworker_done.Also proven live with Claude:
session:<id>gets a pointer turn within a second and reads the message with a flaglesscheck.checkand did not ack.You have 1 new orchestration message. Run `"$ORCA_CLI_COMMAND" orchestration check --run run_… --ack delivery_…`./clearhands the Run to the new session (proven live at an earlier head, where the clear's commit rebound the Run to the new session). That rebinding is gone at this head: the chat keeps its address across/clearinstead (What Changed §6, Architecture review). The new mechanism is covered by the real-host integration test and was not re-run live.releasedat fence 2. Mail to itssession:address resumed it (live, fence 3), and a pointer turn landed. The chat ran"$ORCA_CLI_COMMAND" orchestration checkand read the mail. The release clock then evicted it again (released, fence 4) about 20s after its turn.gate-list --runandtask-list --runrefuse a conflicting--fromunder a session.Codex: the same loop was proven live with a real structured Codex coordinator, with no absolute path supplied by me:
"$ORCA_CLI_COMMAND" orchestration run-createandworker-start.ORCA_CLI_COMMANDin the Codex child is this app's absolute launcher (feat(orchestration): let a structured chat run orchestration as itself #22568).worker_done.Run `"$ORCA_CLI_COMMAND" orchestration check --run run_…`.worker_done."$ORCA_CLI_COMMAND" orchestration checkreturned it too.What Changed
1. A Run whose coordinator is a chat now delivers to that chat
Before, the structured pointer lane resolved a Run's coordinator only through
coordinator_handle, and only forstructworker_handles. A chat coordinator has no handle; #22522/#22555 store onlycoordinator_actor = session:<id>. The PTY lane read the same null handle. So both lanes declined, and a result to a chat coordinator was stored and never pointed.Now
resolveStructuredCoordinatorMailboxTargetreads a handle-less binding's actor (handleLessCoordinatorSessionId). Acoordinator_actorbeside a handle is still ignored, the same stale-actor rule #22555 applies to Run binding. The session is then resolved from its durable record:/clearchain that names a session with no record, another host, or a structured worker whose identity is gonecheck2. The pointer invokes the session's own CLI, in the syntax of its shell
structuredSessionCliInvocation({ platform, provider })incli-command.tsrenders the invocation for the shell that session's commands actually run in. The pointer host reads the provider from the session's durable record.zsh -lc)"$ORCA_CLI_COMMAND""$ORCA_CLI_COMMAND"& $env:ORCA_CLI_COMMANDWhy not bare
orca: a shell that loads a profile rebuildsPATH, and bareorcathen resolves to a global install. Live, it was/usr/local/bin/orca.ORCA_CLI_COMMANDis this app's absolute launcher (#22568), so no profile can swap it.The Codex shell was verified in Codex's own source:
Claude's Git Bash comes from Claude Code's own Windows requirement; Orca configures neither shell. Not run on Windows: every arm is source-verified and unit-tested, and the POSIX arms were also proven live on macOS.
3. The delivery is a real
agentSession.send, hop by hopIt is not a side channel. The hops:
send→sendPointToPointMessagecommits the row torun:<id>.notifyMessageArrived.OrchestrationMailboxNotificationCoordinator.deliverForHandle.OrchestrationStructuredMailboxPointerDelivery.deliverForHandle→resolveStructuredMailboxTarget.attempt:host.hold, released after the attempt;createStructuredMailboxPointerHost().send→StructuredAgentSessionHost.send, the same function theagentSession.sendRPC calls (sendStructuredAgentSessionForClient).sendStructuredAgentSessionTurn→admitAndRunAgentSessionMutation, i.e. the operation ledger, lease and fence checks.The live journals show exactly that: a
submissionrow, adispatch: acceptedrow, and then aturn: runningrow.4. Resume on demand: mail wakes a chat the host evicted
The renderer holds a chat only while it is visible, and the host evicts an unheld chat 15s after its last turn. Before this PR, mail to such a chat parked on "not attached" and nothing ever re-attached it.
The pointer lane now calls
host.wake(sessionId). That takes a resume-capable hold, which is the same hold a chat surface takes, so it reuses the existing resume path: reconcile, attach, provider child. The send then goes out as usual, and the lane releases the hold. The release clock evicts the session again once its turn settles.A resume the host refuses (a lease another owner holds, a failed acquisition) retains the mail. There is no retry loop.
5. A pointer is owed until it is pointed, not until the reader acks
Pointer eligibility is "mail on this mailbox not yet pointed".
delivered_atis the existing per-row "pushed at most once" fact, and it is set when the host takes the pointer turn:accepted, orpending(admitted, awaiting its echo). It is keyed per message id, and the durable operation id still makes a retried pointer one turn.pendingis only a claim. A pointer the provider admitted but never echoed does not stay pointed: if the send settles rejected or the provider dies before the echo, the rows are returned to "not yet pointed" and the next edge re-points them. The claim is consumed only when the echo confirms the turn. The given-back send's operation id is dropped, because the host's ledger replays a recorded send's verdict and never reaches the provider twice: re-pointing under the old id would replayunknownand land nothing.Before, the lane skipped a mailbox whenever an unacknowledged delivery existed. That is a latch with nothing behind it: a chat that read a result and ended its turn without
--ackwas never pointed at another result.Now:
--ack <id>that releases the held batch, becausecheckreplays an unacknowledged batch until it is acked.The check/ack protocol for reading is unchanged.
6. A chat keeps its orchestration address across
/clear/clearmints a new Orca session (clear-<hash>) that continues the conversation, and the records already chain them (conversationCommand.replacementSessionId). The conversation's address is the first session of that chain, its lineage root, andstructured-session-mail-address.tsderives it from the records on every call:session:<root id>. Itsrun-create,sendanddispatchwrite that actor, and itsrun-currentandcheckread under it. A--fromnaming its own new id is still accepted.session:<id>spelled with any session of the chain resolves to the same address, and mail is stored under the root spelling.Nothing is rewritten at a clear: no Run is rebound, no row is re-addressed, and there is no commit-edge observer. A clear that commits leaves every Run bound exactly as it was, generation included.
7. The idle edge of every structured session redrives its mail
Before, only a dispatched structured worker had a redrive edge: a per-session journal subscription. A chat coordinator had none. That matters because a live send first answers
pending(the pointer lane never counts that as delivered), and because a result often arrives while the coordinator is still in its own turn.Now
onSessionStatusChanged, the host's existing per-change status callback, callsonStructuredSessionStatusForMailfor every session. At an idle edge it opens the orchestration database if it exists and nothing has opened it yet (logging when that fails; a profile with no database file has no mail, so nothing is created), then:session:mail.Deriving the mailboxes rather than remembering them is what lets mail that arrived while the chat was closed, evicted or in its terminal view be found again.
8.
session:<id>is a valid recipient for any live sessionresolveBareOrchestrationRecipientnow readssession:<id>, and a bare Orca session id this host has a record for.run:<id>.checkreads (sessionOrchestrationIdentity().address), and pointed through the same lane.session:<id>recipient that names a structured worker is delivered at the worker's own mailbox (itsstructworker_handle), the same address its dispatch mail uses, so its flaglesscheckreads it. One session is one identity, whichever spelling the sender used.Refusals happen before anything is stored, using the existing
session_caller_*codes:session_caller_unknownsession_caller_provider_idsession_caller_host_boundary/clearchain that names a session with no record, or a structured worker whose identity is gonesession_caller_not_live9. Terminal view: delivery follows the session
For a chat in its terminal view:
OrchestrationMailboxDeliveryTargetresolves the chat'srun:mailbox to the handle of the PTY the write gate binds to that session.OrchestrationMailboxOwnerresolves that leaf's Run and Dispatch as the session. The CLI there already acts as the session (feat(orchestration): let a structured chat run orchestration as itself #22568).10. Shared session rules, one copy
structured-session-mail-address.tsholds the session lookup (Orca id or provider id), the "can mail reach this session" rule, and the session's orchestration identity. Both the #22555 caller resolver and this PR's delivery and recipient code now call it, instead of each carrying its own copy.11. Lower-stack defect fixed here: a structured worker start discarded a started worker
This is on
main, not in #22522–#22568.sendStructuredWorkerPreambletreated the provider's first answer,pending(admitted, not yet echoed), as unacknowledged. It threwoperation_unknown, and worker-start then closed the worker session. Live, the worker journal showed the preambleacceptedabout a second later, a turnrunning, and then anexpected-close.It now waits out
pendingwith the host's existingwaitForSendSettlement, the same wait an older chat client gets, bounded at 30s. It judges the settled state by the same rules as before. The first live worker-start failed this way; after the fix it returnedstate: ready.Why
Delivery. The failure mechanism was two lanes that each believed the other owned a handle-less coordinator's mailbox. Resolving the coordinator by the actor that #22522/#22555 already store removes that. It adds no new mapping: the session record decides which view delivers, so exactly one lane claims a mailbox at a time.
Waking, and the redrive edge. A result reaching a sleeping coordinator should start a turn in it, through the same turn-submit path a user message takes, resuming the provider if needed. A busy coordinator's result should wait for its turn to settle and be drained on the "turn completed, now idle" event, for any session.
This PR builds both out of existing mechanisms:
Deviations, stated plainly. The chat receives a pointer to its mail, not the mail itself; that is the existing pointer-lane design. A busy chat's pointer waits for the idle edge; it is never folded into the running turn.
Alternatives rejected:
Architecture review
An architecture review of this PR found that the
/clearhandling stored a copy of a fact the session records already hold, and that the copy could destroy user state. The fixes, in this order:1. A chat's address is its
/clearlineage root, derived, not copied. Before,/clearrotated the chat's address, and an adoption path rewrote the predecessor's Runs and unread mail onto the new session at the clear's commit and again on every idle edge. It rebound throughbindRun, which is an exclusive takeover. So if that rebind was missed at the commit (it was best-effort, and it silently did nothing while the database was closed after a restart), the successor's ownrun-createwas later unbound by the idle-edge backstop: its worker's result then went to a Run with no coordinator, and it stranded. A chain of clears orphaned all but the last adopted Run. Both were reproduced as failing tests at the previous head (55692e7e6b); they are now real tests.Now the address is derived (What Changed §6). This deletes the adoption subsystem:
adoptClearedPredecessorMail,readdressUnreadSessionMail, the host'sonConversationReplacedcommit edge and its try/catch, the idle-edge rebind, and their tests. With nothing rewritten, there is nothing to miss, and no background write can unbind a Run.session:<id>in the same columns; what changes is which id. An agent's address is its lineage root, the conversation's first session id, stable across/clear. Checked:runs.coordinator_actor,dispatch_contexts.creator_actorand a chat's messagefrom_handleall take it from the one resolver, so they store the root (tests pin the Run's actor andfrom_handle;creator_actorby readingdispatch-creator.ts).dispatch_contexts.assignee_actoris a structured worker's own session and is unchanged.orca statusshows after/clear(with feat(orchestration): tell each agent its own orchestration address #22636 on top, which adds the caller address): in the cleared chat,orca status --jsonreportscaller.address=session:<root id>, the same address as before the clear, andcaller.sessionId= the new session's own id. feat(orchestration): tell each agent its own orchestration address #22636's guidance that/cleargives a chat a new address and Orca moves its Runs no longer holds and needs updating there.run-createfrom that build would store the new session's id, and after upgrading it would not match the chat's root identity. None of them is onmain, so no such rows exist; they should land together.2. The idle edge opens the database itself. It read a field that stays null until the first orchestration call after a restart, and a null there was a silent skip. It now opens the database lazily when its file exists, so the first structured idle edge after a restart opens it, which also runs the restored-mailbox scan. A profile with no database file has no mail: the edge creates nothing and logs nothing. A failure to open an existing one is logged.
3. A pending pointer claim has a way to die across a restart. A pointer the host admits as
pendingstamps its batch delivered, and only an in-memory settlement waiter gave it back. A crash in that window left the last result on that mailbox stamped and never pointed again. When the database opens, every surviving pointer-operation row was minted by an earlier process. Its stamped batch is found by the row's batch fingerprint (one statement stamped it, so it shares onedelivered_at), given back, and the row dropped, before the restored-mailbox scan points it again. This is the structured-lane counterpart of the PTY lane's restore-time pointer release.Deviations, recorded:
/clearstill mints a new session; the address is derived from the clear chain in the session records on every resolution. That holds because session records are never pruned. If record retention is ever added, the root must be persisted at first orchestration use (one column)./clearwiped the context that saw it. At the previous head, direct mail was re-pointed and Run mail was not; now neither is, andcheckstill returns it. Whether a pointed-but-unread batch is re-pointed at all belongs to the pending pointer-versus-payload decision, which this PR does not change.Ablations at this head (each mechanism deleted, the reachable test files run, then only the touched files restored):
55692e7e6b(tests kept)/cleartestsValidation: the PR's 11 test files plus the claim-restore, coordinator, undelivered-mailbox, stale-leaf and three CLI identity suites: 18 files, 310 passed, 4 skipped.
pnpm tc:nodeandpnpm tc:cliclean, fullpnpm exec oxlintexits 0,check:code-quality:changed0 findings,audit:anti-slopclean, no lockfile or docs in the range. Not run: a live Electron/clearat this head, Windows, WSL, mobile.Linked Issue
None — part of the structured chat status/orchestration program. Stacked on #22568.
Visual Proof
Live, in a hidden dev window, captured over CDP:
orca orchestration check --run run_107bf5ee8eab", then the chat's flaglesscheckoutputmsg_fdd77c533621 [worker_done] … "Confirmed run".session:<id>: the pointer turn "You have 1 orchestration message. Runorca orchestration check", then the chat reading the ping.checkreturningmsg_892655ba6b90 [worker_done].Screenshots and journal excerpts are kept in the QA workspace, not attached here.
Testing
New tests
structured-chat-coordinator-mail.test.tsuses the real structured session host, record store, journal, lease and Codex adapter, and the real orchestration DB, dispatcher and pointer lanes. Only the Codex app-server child is faked. It covers:worker_donelands as a user turn in the coordinator journal, and a flaglesscheckreturns it;session:<id>mail lands as a turn in that chat;/clear: the new session'srun-currentreturns the conversation's Run, a worker's result lands as a turn in it, and the Run is not rewritten;run-createand the last session'ssendare stored under the root's address;structured-session-mail-target.test.tsresolves through the real runtime methods:session:mailboxes are resolved;structured-pointer-claim-restore.test.ts: a stamped pending batch left by an earlier process is given back at open; an earlier batch stamped in the same second is left alone; an unstamped claim keeps its id; the open-time scan runs the release first.orchestration-session-recipient.test.ts, through the dispatcher:session:<id>and a bare id to a non-coordinating chat are accepted;terminal_not_found.structured-session-terminal-view-mail.test.ts: the terminal view owns the chat's Run, the pane is the fallback, and the delivery target resolves the view's handle.unknownandrejectedgive it back and re-point under a new operation id; a newer batch's operation row is left alone. The pointer host maps each way the settlement wait can end (echo, provider death, a wait that gave up, a vanished send) to a verdict.Ablations (earlier heads; the
/clearadoption rows were removed with that code, and this head's ablations are in the Architecture review): 10 mechanisms, each deleted or neutralized with the replacement asserted to match exactly once, then restored. Each run covered the 6 reachable test files (64 tests), and every one went red:session:address target"$ORCA_CLI_COMMAND"(reverted to bareorca)onSessionStatusChanged→ redrive wiring line deleted--ackpendingnot counted as pointedSuites (
env -u ORCA_STRUCTURED_SESSION -u ORCA_AGENT_SESSION_ID npx vitest run --config config/vitest.config.ts …): at the previous headca9b47189b(on #22568 @ 2a688ed), 458 files / 4,171 tests passed, 10 skipped, 0 failed. At this head (on #22568 @ 2d9dc67), the PR's 11 test files pass (161 tests), and a wider run of the suites this round can reach (src/main/runtime/orchestration,src/main/native-chat/agent-session-wire,src/main/runtime/rpc/methods/orchestration, the coordinator-mail, redrive-wiring, integration and runtime-exit files) passed 2,601 of 2,602 run tests (6 skipped). The one failure,orchestration-all-start-versions-migration.test.ts, is a 30s timeout (38s alone on a loaded machine); this PR changes no schema or migration code, and CI runs it on every push.src/cli,src/main/native-chat/agent-session-wire,src/main/runtime/orchestration,src/main/runtime/rpc/methods/orchestration, the session caller, coordinator and recipient suites, the structured worker session and redrive suites, the mailbox routing and consistency suites,structured-agent-session-integration,structured-agent-session-runtime,orchestration-cli-subprocess,rpc/errors, feat(orchestration): let a structured chat run orchestration as itself #22568's child-env and login-shell suites, and the wiring test.waitFor. It passed 3/3 in isolation, and the file's waits now use a 10s budget.Checks
pnpm tc:nodeis clean.check:code-quality:changed: 0 findings.pnpm exec oxlintexits 0 (the changed-lines gate cannot seemax-lines), andoxfmtis clean on the changed files.audit:anti-slopis clean.pnpm-lock.yamlis absent from the range, and no docs are added.AI Disclosure
Review
check, never a lost result. The re-point waits for the session's next edge rather than reviving a chat whose provider just died, so a provider that dies on every turn is not respawned in a loop.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Lower-stack findings from the first end-to-end run
pending(What Changed §11). It is onmain.PATH, so bareorcaresolved to a global install. feat(orchestration): let a structured chat run orchestration as itself #22568 now setsORCA_CLI_COMMANDto this app's absolute launcher, and this PR's pointer text uses it (§2). Re-proven live above.Which runtime a bare global
orcareaches from inside a session. The global CLI here is/usr/local/bin/orca→/Applications/Orca.app1.4.210.runtime/metadata.jsreadsORCA_USER_DATA_PATH. The session's login shell keeps that variable set to the launching app's profile; it was/tmp/d4qalive.ORCA_AGENT_SESSION_IDsupport, so it cannot claim the session and refuses on the identity-less marker (no_active_sender_terminal, nothing applied; seen live in the first run)./usr/local/bin/orca.A coordinator cleared by
/clearis handled here (What Changed §6). If a coordinator chat is cleared mid-run, it keeps its address, so its Run and its mail stay with it and delivery continues in the new session. Nothing moves.Caller refusal wording. A released lease now refuses a caller as
Agent session <id> is not running right now. A new message or user turn revives it; retry then. No effects were applied.It used to say "has ended". The two rules stay separate in one module: can act needs a live process, can be reached needs only a record that resume can revive.Observed, not fixed here: sessions that could not be resumed.
reserved→releasedwhile it was visible. Not investigated./clearsuccessor that had not yet run a turn could not be resumed after an app restart. Sending in it failed with "The session has no live owner to accept writes." (Seen at an earlier head; not re-checked.)[orchestration] could not wake a structured session for its mailwith the reason, so the next occurrence names its cause.Claude's shell on Windows is Git Bash because Claude Code requires it on Windows. That was not verified from Claude Code's source; Orca configures no shell for it.
Other environments
session_caller_host_boundary, and a remote record is never delivered to. Structured sessions are local and non-WSL only.session_caller_*codes, already passed through.pendingholds one settlement waiter (the host's existing 30s-bounded wait) until its echo. The wake hold is taken only when a mailbox has undelivered mail.Not verified
session:<id>. A live wake of an evicted coordinator by Run mail was not re-run after §5; the one attempt before it was blocked by the old unacknowledged-batch gate.livefor over a minute" did not reproduce after a restart. Every hidden chat wasreleased, and a woken hidden chat evicted again about 20s after its turn. So no hold leak was found on the wake path. One separate, uninvestigated observation: showing a tab moved its lease fromreservedtoreleasedwhile visible.orcainside a session was not run (see "Which runtime a bare globalorcareaches").