Repository navigation
Journal what a Codex session streams, and prove it end to end - #13394
brennanb2025 wants to merge 8 commits into
Conversation
A provider starts streaming the moment its process exists, and that moment is inside `adapter.acquire` — before the journal is open and before the host has registered the session. The adapter now receives a deferred sink whose writes queue in arrival order and drain once the journal is bound. One sink lives for the session rather than for one acquisition, so an adapter can hold it across a re-acquire without noticing the new journal object. It is bound last in `onAttached` so nothing the provider streamed can interleave with the attach result the client is about to read, and a refused attach that never reached a journal closes it instead of leaving writes nothing will drain. The host was one line under the 300-line cap before this, so the mutation admission pipeline moved to its own module. That is a straight extraction: recompute fingerprint, admit through the ledger, check the lease, run the plan — the same route, now somewhere a new method cannot sit next to.
Providers emit one notification per token. Journaling each one writes a row and wakes every subscriber per token, so one long answer costs thousands of appends and thousands of stream frames on a phone. Deltas accumulate and flush on a 60ms window, and the row carries a SNAPSHOT of the text so far rather than the increment — which is also what makes dropping intermediate frames safe for a reconnecting client. All streams share one timer, so the deadline bounds latency the same way at one wakeup per window instead of one per stream. Lifecycle bypasses the window by flushing first, so nothing can be journaled ahead of the text that preceded it.
Codex item ids are not stable across resume, so a journal keyed by them cannot reconcile a recovered thread against a live one: the same message would arrive under a new id and append a second copy of itself. The durable key is instead the position the item holds — thread, turn, and ordinal within that turn, counting message items only. Non-message items get no ordinal at all rather than a number from a second counter, because a counter a resumed history cannot reproduce is worse than no key: it looks reconcilable and reconciles wrongly. Those rows fall back to a live-session namespace that a recovered journal simply will not contain, which is correct, since Codex does not persist them either. Approvals and questions become journal rows the same way, naming the command the item already announced so the user is not asked to approve an opaque id, and one row per question rather than one row per request.
The adapter now writes what Codex streams straight into the session's journal, so nothing between the child and the durable record has to remember state: streamed text folds through the coalescing window, lifecycle events flush it first, and every write is a highest-revision-wins upsert on the durable key, which is what lets a completed body land on the row its own deltas opened. Journal first, observers second — a tap must never see a row the journal has not been told about. A session with no sink still runs, unjournaled, which is how the existing adapter tests keep working unchanged. Starting or resuming the thread moved into its own module on the way past the adapter's line cap; the check it carries is unchanged, and still refuses a resume that lands on a different thread, since recording that would make the durable handle chain lie about what the session proved.
Command, cwd, pinned account home, and the thread to resume all come back out of the record the lease already wrote, never from the client-declared identity: a caller that could name the thread could point a session at someone else's conversation. It refuses rather than guesses — a record pinned to another execution host or a WSL distro is not this host's to spawn for, a record for another provider or another account-home variable is not this adapter's to speak for, and a workspace that no longer resolves surfaces instead of falling back to a default cwd.
The record store and the journals live under the profile's user-data path, which is not final until Electron is ready, and a runtime that never serves a structured session should not pay for a store it will never read. So the host is built by the first `agentSession.create` or `.ensure` rather than at startup, and the memoized slot clears on failure so the next call retries. The capability gate runs first: building the store and spawning a child is an observable side effect, and a client that cannot read structured sessions must not be able to cause it. Owner probing is scoped to this host. Checking another host's pid against this machine's process table is exactly how a live owner gets declared dead, so a foreign owner reports `indeterminate` and routes to manual recovery instead. So does a reservation that named no process — freeing it needs positive proof that nothing spawned under its token, and this host has no spawn-token process scan yet.
One test walks create, send, stream, approval, cancel, reconnect, and paged history against the real dispatcher, the real schemas, the real capability gate, the real record store, journal, lease, adapter, and translation. Only the Codex child is scripted, and it answers the same JSON-RPC calls the real app-server does. It is the first thing in the stack that proves the parts fit: that the provider's echo of a user message reconciles into the bubble the client already has instead of appending a second copy, that a client reconnecting from its cursor replays only what it missed while a takeover advances the fence and reaps the old child, and that the whole conversation reconstructs from the durable journal alone with no page overlapping another.
| const ordinals = new CodexTurnOrdinals() | ||
| /** Identity assigned when an item was announced, reused by its deltas and by | ||
| * its completion so all three upsert one row. */ | ||
| const identities = new Map<string, AgentJournalItemIdentity>() |
There was a problem hiding this comment.
[P1] Scope translator state by thread. The adapter deliberately forwards root and subagent threads over one connection, but identities, coalescer keys, and command details use only positional item.id, while currentTurnId is one session-wide scalar. Interleaved threads that both emit item-0 can therefore merge text or write the child body under the root durable key, and a child turn/completed clears the root turn. Use composite thread/item keys, current turns per thread, and thread+turn ordinal state; also use readCodexTurnId here so the supported envelope-level turnId shape is not discarded. Add an interleaved two-thread/same-item-ID regression test.
| let chain: Promise<void> = Promise.resolve() | ||
|
|
||
| const enqueue = (operation: SinkOperation): void => { | ||
| const bound = target |
There was a problem hiding this comment.
[P1] Unbind the sink during replacement acquisition. On a second-fence attach, eventSinkFor returns the sink still targeting the old journal; the new adapter drains its acquisition-window events before performAttach opens and binds the replacement journal, so enqueue captures this old target. Those rows can be rejected at the stale fence, race the new journal open, or remain absent from the new in-memory snapshot/subscriber because their publish callback also targets the old journal. Add an explicit rebinding/unbound phase (or a fresh acquisition sink) and test a prompt/notification emitted during thread/resume.
| if (!identity) { | ||
| return | ||
| } | ||
| deps.sink.appendItem(identity, codexStreamingMessageBody(text)) |
There was a problem hiding this comment.
[P2] Avoid appending the full accumulated answer every 60 ms. The coalescer emits a snapshot, and this append-only journal stores every prior snapshot verbatim, making disk growth quadratic in response length; a long streamed answer can exhaust the 256 MiB per-session budget, after which later snapshots and even the final body are refused and dropped by the sink. Persist bounded/incremental deltas or compact/geometrically checkpoint snapshots while preserving reconnect semantics, and add a long-stream byte-growth test.
|
|
||
| /** Drops the host and reaps every Codex child under it. Runtime teardown and | ||
| * test isolation take the same path, so neither can leave a live app-server. */ | ||
| export async function stopStructuredAgentSessionRuntime(): Promise<void> { |
There was a problem hiding this comment.
[P1] Wire this stop path into production teardown. stopStructuredAgentSessionRuntime is currently referenced only by tests, and src/main/index.ts's will-quit barrier never calls it. After any structured session has been used, quitting Orca can therefore orphan the long-lived codex app-server child, which may continue running a turn or command after the UI/runtime exits. Import it in the main quit path and join it to settleTeardownWithinDeadline (after fixing the part-A in-flight acquisition gap), with a teardown registration test.
brennanb2025
left a comment
brennanb2025
left a comment
There was a problem hiding this comment.
Follow-up adversarial pass on exact head 76cce58 found four additional provider-shape gaps; each is pinned below. No branch changes or push made.
| return items | ||
| } | ||
|
|
||
| function questionOptions( |
There was a problem hiding this comment.
[P1] Preserve a free-text answer path for Codex questions. The app-server permits questions with options: null and options marked isOther, but this maps missing options to an empty offered set; performPrompt then rejects every optionId because it must exactly match prompt.options. Such a request is journaled but can never be answered, leaving the turn blocked forever. Represent and validate an Other/free-text response through the journal and mutation contract, with null-options and isOther tests.
| : input.method === CODEX_COMMAND_APPROVAL_METHOD | ||
| ? 'Run a command?' | ||
| : 'Approve this action?', | ||
| detail: readString(params, 'reason') ?? input.detail, |
There was a problem hiding this comment.
[P1] Prefer the approval request's own command/detail. Bridge command approvals can re-ask for distinct subcommands under one parent item, and those requests carry the exact command being approved; using only reason or the earlier item detail makes multiple approval rows show the same parent command, so the user may authorize a different command than the UI displays. Read the request command first (and include file-change grantRoot/changes when no reason exists), with a repeated-subcommand test.
| if (!Array.isArray(content)) { | ||
| return [] | ||
| } | ||
| const blocks: NativeChatBlock[] = [] |
There was a problem hiding this comment.
[P2] Translate image content parts in provider user-message echoes. turnInputFor sends image and localImage blocks, but this echo path retains only text; because an accepted submission aliases to the provider key, the echo then revises the optimistic row and replaces its complete mixed text/image body with text only. Map image/localImage back to image-ref blocks and test reconciliation of mixed content.
| state: commandState(item) | ||
| } | ||
| } | ||
| if (item.type === 'reasoning') { |
There was a problem hiding this comment.
[P2] Handle array-shaped reasoning content. Current app-server reasoning items expose summary/content as arrays, so readString returns null and the item is silently skipped. Join and bound the supported array elements into the status body, and add a real-wire-shape fixture so reasoning does not disappear from the journal.
brennanb2025
left a comment
brennanb2025
left a comment
There was a problem hiding this comment.
Final cleanup pass on exact head 76cce58 found one additional close-boundary loss; pinned below. No branch changes or push made.
| @@ -352,6 +349,7 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap | |||
| } | |||
| this.sessions.delete(sessionId) | |||
| session.prompts.clear() | |||
There was a problem hiding this comment.
[P1] Flush coalesced text before gracefully closing a published session. closeSession calls translator.dispose directly, which cancels the 60 ms timer and clears dirty streams without emitting; only unexpected exit takes the ended path that flushes first. A normal closeAll or fence supersession during streaming therefore loses the last token burst from the durable journal. Add an explicit flush-before-dispose path for published sessions (while retaining discard semantics for failed acquisitions) and a close-before-window-expiry regression test.
…nto brennanb2025/codex-structured-journal
Summary
Part B of the structured agent-session stack: the layer that turns what a provider streams into the durable conversation a client reads back. Nothing here is reachable from the UI yet — the payoff is the last commit, one test that drives a structured Codex session end to end over
agentSession.*with no renderer involved.Six commits, each its own reviewable step:
structured-agent-session-event-sink.ts. A provider starts streaming the moment its process exists, which is insideadapter.acquire, before the journal is open. The adapter now gets a deferred sink whose writes queue in arrival order and drain once the journal is bound. The sink lives for the session rather than for one acquisition, so an adapter can hold it across a re-acquire; it is bound last inonAttachedso nothing streamed interleaves with the attach result the client is about to read, and a refused attach that never reached a journal closes it instead of leaving writes nothing will drain. This is the additive adapter extension —AgentSessionAdapter.acquiregains an optionaleventsfield; an adapter that ignores it behaves exactly as before, which is why the pre-existing adapter tests are unchanged. It has its own tests (structured-agent-session-event-sink.test.ts).agent-session-delta-coalescer.ts. One journal row and one stream frame per token costs thousands of appends and thousands of wakeups on a phone for a single long answer. Deltas accumulate and flush on a 60ms window, all streams share one timer, and the row carries a snapshot of the text so far rather than the increment — which is what makes dropping intermediate frames safe for a reconnecting client. Lifecycle events bypass the window by flushing first, so nothing is ever journaled ahead of the text that preceded it.codex-structured-item-translation.ts,codex-structured-prompt-items.ts. Codex item ids are not stable across resume, so a journal keyed by them cannot reconcile a recovered thread against a live one. The durable key is the position the item holds: thread, turn, and ordinal within that turn, counting message items only. Non-message items get no ordinal rather than a number from a second counter — a counter a resumed history cannot reproduce is worse than no key, because it looks reconcilable and reconciles wrongly — and fall back to a live-session namespace a recovered journal will not contain, which is correct, since Codex does not persist them either.codex-structured-journal-translation.ts. The one place Codex events become journal rows: streamed text folds through the window, lifecycle flushes it first, and every write is a highest-revision-wins upsert on the durable key, which is what lets a completed body land on the row its own deltas opened. Journal first, observers second — a tap must never see a row the journal has not been told about. A session with no sink still runs, unjournaled.codex-structured-launch-resolution.ts. Command, cwd, pinned account home, and the thread to resume all come out of the record the lease already wrote, never from the client-declared identity: a caller that could name the thread could point a session at someone else's conversation. It refuses rather than guesses — another execution host, another WSL distro, another provider, another account-home variable, or a workspace that no longer resolves all surface instead of falling back to a default.structured-agent-session-runtime.ts, wired fromorca-runtime.ts.setStructuredAgentSessionHostis constructed by the firstagentSession.create/.ensurewithjournalRoot,claimKeyId, andprobeOwner, because the user-data path is not final until Electron is ready and a runtime that never serves a structured session should not pay for a store it will never read. The memoized slot clears on failure so the next call retries. The capability gate runs first: building a store and spawning a child is an observable side effect a client without the capability must not be able to cause.Integration proof
structured-agent-session-integration.test.tswalks create → send → stream → approval → cancel → reconnect → page history against the real dispatcher, real schemas, real capability gate, real record store, journal, lease, adapter, and translation. Only the Codex child is scripted, and it answers the same JSON-RPC calls the real app-server does. It proves the three things the parts only claim individually: the provider's echo of a user message reconciles into the bubble the client already has instead of appending a second copy; a client reconnecting from its cursor replays only what it missed while a takeover advances the fence and reaps the old child; and the whole conversation reconstructs from the durable journal alone with no page overlapping another.Review findings from #13381
The adversarial review of the parent PR posted six findings at head
b6c98f8. All six are fixed; because they live in files this PR builds on, most landed on the parent branch and are already in #13381's diff, not this one:itemIdcodex-structured-prompt-replies.tson #13381 (registry keyed byapprovalId ?? itemId) and in this PR:codex-structured-prompt-items.ts/codex-structured-journal-translation.tsnow key the journal row by that prompt identity too, so two approvals from one tool item are two answerable rowscodex-structured-session-adapter.tson #13381 — events carry the thread they came from, and only a root-threadturn/startedanswers the root turn waitercodex-structured-session-adapter.ts+codex-structured-acquisition-window.tson #13381 — publication is one atomic step, and the failure path reaps only this attempt's childcodex-app-server-connection.tson #13381 — stdin errors route through the terminal path, reap the child, and mark the connection closedcodex-app-server-connection.tson #13381 — once-only terminal guard that keeps the first, specific causecodex-structured-acquisition-window.tson #13381 — traffic that arrives before the session is published is buffered in arrival order and drained at publicationEach fix has a test that fails against the pre-fix source (verified by reverting the source under the new test).
Screenshots
No visual change. There is nothing to screenshot: no renderer code is touched, and the structured path is unreachable from the UI until the mobile client lands (part C).
Testing
pnpm lintpnpm typecheckpnpm testpnpm buildNew:
structured-agent-session-event-sink.test.ts,agent-session-delta-coalescer.test.ts,codex-structured-item-translation.test.ts,codex-structured-prompt-items.test.ts,codex-structured-journal-translation.test.ts,codex-structured-launch-resolution.test.ts,structured-agent-session-runtime.test.ts,structured-agent-session-integration.test.ts.Gate results
pnpm lint— pass.pnpm typecheck— pass, all three tsc projects.pnpm build— pass.pnpm test— 48992 passed / 6 failed / 97 skipped across 4576 files. All six failures are environmental and none is in a file this branch touches: three in the root-directory guard test, whose shell script usesdeclare -Aand so exits 2 under macOS's system bash 3.2 (reproduced by hand on a fresh temp repo; Linux CI has bash 4+); two in the relay agent-exec handler, where this machine's git environment contributes duplicateGIT_CONFIG_KEY_*entries so the expected count of 2 arrives as 4; and one 30s renderer timeout that passes when run alone.AI Review Report
Reviewed for: ordering hazards between the coalescing window and lifecycle events (a row journaled ahead of the text it follows), rows a superseded child could still write after a takeover, journal keys that survive resume, prompt identity collisions, capability-gate bypass, and cross-platform behavior. What changed as a result: the translator is per-session-per-acquisition and disposed on exit so a reaped child cannot keep writing; lifecycle flushes the window before it emits; the completed body forgets any pending snapshot so a stale flush cannot land after it; and non-reproducible ordinals were removed rather than guessed. Cross-platform: this diff adds no shell invocation, no path literal, no keyboard handling, and no Electron platform branch — paths come from Electron's user-data path and
path.join, the launch resolver refuses a record pinned to a different execution host or WSL distro instead of assuming local spawn, and the SSH/remote case is served by the same host-side modules since nothing here is renderer-resident. Remote wire compatibility: no new opcodes and no new required fields — the adapter extension is an optional field on an internal main-process interface, not on the wire.Security Audit
Reviewed input handling, command execution, path handling, and IPC. The launch resolver is the notable surface and is deliberately restrictive: command, cwd, account home, and resume target come from the durable record the lease wrote, never from client-supplied parameters, so a client that can name a thread id cannot point a session at another conversation, another workspace, or another account's home. Prompt option ids are encoded/decoded with an explicit separator scheme so a question id containing the separator round-trips instead of forging another question's answer. The capability gate runs before any store construction or child spawn, so an unauthorized client cannot cause the side effect. Journal writes are append-only upserts keyed by identity, so a late frame cannot rewrite history under a different key. No secrets are logged; the stderr tail carried into an exit error is truncated. No new dependencies.
Notes
brennanb2025/codex-structured(Run a Codex session over the structured agent-session wire #13381) so the diff is only this part; it retargets tobrennanb2025/native-chat-structured-baseonce Run a Codex session over the structured agent-session wire #13381 merges.turn.itemsinto journal rows, becauseattachJournalrecovers the conversation from the durable journal and the rollout file is the provider's own record — wiring both would give two writers for the same rows. Also deferred: the spawn-token process scan that would let a reservation which named no process be freed automatically; until it exists, that case reportsindeterminateand routes to manual recovery, which is the safe direction.indeterminaterather than being reaped.