fix(agent-status): settle a Codex turn that failed, from its session log - #22753
brennanb2025 wants to merge 2 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughCodex transcript reconciliation now records completed or failed task-completion events by turn ID. Codex lead-state handling uses these statuses to detect turn completion and record failure outcomes. Transcript polling also covers lead turns awaiting transcript completion. Tests cover completion without a Stop hook, turn ordering, late Stop events, unreadable rollouts, active child agents, relay forwarding, and outcome reconciliation. Priority: ➖ Normal Merge Risk: 🔵 Low · up to Codex status can briefly show an active turn as complete, or retain an incorrect failure outcome after a missed start hook. These bounded status risks warrant owner awareness before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Failed turns should now stop appearing active promptly. The remaining design question is whether a completion delivered late could be applied to a newer turn in a remote pane. That could hide ongoing work; the available evidence does not establish whether delivery ordering prevents it. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: ce9e5f82-3e99-46d2-9956-3f623886ecb3
📒 Files selected for processing (7)
src/main/agent-hooks/server-codex-transcript-turn-end.test.tssrc/relay/agent-hook-server-codex-transcript-turn-end.test.tssrc/shared/agent-hook-listener-codex-main-agent.test.tssrc/shared/agent-hook-listener/providers/codex-events.tssrc/shared/agent-hook-listener/providers/codex-state.tssrc/shared/agent-hook-listener/transcript-poll-policy.tssrc/shared/codex-subagent-transcript.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| // Why: the relay alone reads the rollout, so a turn it saw fail arrives as a Stop carrying the verdict. | ||
| const outcome = | ||
| codexOutcomeRestatedByStop(previousLead, leadState).outcome ?? | ||
| (leadState === 'done' ? payload.mainAgent?.outcome : undefined) | ||
| setCodexMainAgentTurnState(state, paneKey, { | ||
| state: leadState, | ||
| ...codexOutcomeRestatedByStop(previousLead, leadState), | ||
| ...(outcome ? { outcome } : {}), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '175,250p' src/shared/agent-hook-listener/providers/codex-state.ts
rg -n 'codexOutcomeRestatedByStop|reconcileRemoteCodexState|turnId|turn_id' src/shared/agent-hook-listener/providers/codex-state.tsRepository: stablyai/orca
Length of output: 3115
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- definitions and surrounding code ---'
sed -n '1,175p' src/shared/agent-hook-listener/providers/codex-state.ts
printf '%s\n' '--- reconciliation continuation ---'
sed -n '240,330p' src/shared/agent-hook-listener/providers/codex-state.ts
printf '%s\n' '--- relevant symbols across repository ---'
rg -n -C 4 'codexOutcomeRestatedByStop|codexLeadStateByPaneKey|setCodexMainAgentTurnState|ParsedAgentStatusPayload|mainAgent.*outcome|outcome.*turn' src test tests 2>/dev/null || true
printf '%s\n' '--- PR diff for target file ---'
git diff --unified=80 9678dfb9aa884546bb8ecefc0ff2c7f48407f161 3e0a76a925fc1739089b7165c60eb0dbeb65ef2a -- src/shared/agent-hook-listener/providers/codex-state.tsRepository: stablyai/orca
Length of output: 45653
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- payload and lead state declarations ---'
rg -n -C 8 'export (type|interface) (ParsedAgentStatusPayload|AgentMainAgentStatus|CodexLeadTurnState)|type ParsedAgentStatusPayload|interface ParsedAgentStatusPayload|type CodexLeadTurnState|interface CodexLeadTurnState' src/shared
printf '%s\n' '--- continuation contract ---'
rg -n -C 12 'function continueMainAgentStatus|export function continueMainAgentStatus|continueMainAgentStatus' src/shared/agent-lead-status-fold.ts src/shared
printf '%s\n' '--- turn identifiers in relevant Codex/listener paths ---'
rg -n -C 3 'turnId|turn_id|turn id|stateStartedAt|parent\.filePath|parent.*turn|transcript' src/shared/agent-hook-listener src/shared/codex-subagent-transcript.ts src/relay/agent-hook-server-codex-transcript-turn-end.test.ts 2>/dev/null
printf '%s\n' '--- reconciliation callers and focused tests ---'
rg -n -C 8 'reconcileRemoteCodexState|codexOutcomeRestatedByStop|codexLeadTurnAwaitsTranscriptEnd|setCodexMainAgentTurnState' src --glob '*test*' --glob '*.ts'Repository: stablyai/orca
Length of output: 42132
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- lead state type ---'
cat -n src/shared/agent-hook-listener/main-agent-turn-state.ts | sed -n '1,75p'
printf '%s\n' '--- parsed payload type ---'
rg -n -C 12 'ParsedAgentStatusPayload|AgentMainAgentStatus' src/shared/agent-status-types.ts
printf '%s\n' '--- Codex event lifecycle ---'
cat -n src/shared/agent-hook-listener/providers/codex-events.ts | sed -n '150,260p'
printf '%s\n' '--- transcript end matching ---'
rg -n -C 10 'turn_id|turnId|transcriptTurnEnd|read.*Turn|task_complete' src/shared/codex-subagent-transcript.ts src/shared/agent-hook-listener/providers/codex-events.ts
printf '%s\n' '--- remote caller ---'
cat -n src/main/agent-hooks/server/server-status-update.ts | sed -n '55,115p'
printf '%s\n' '--- focused reconciliation tests ---'
cat -n src/shared/agent-hook-listener-codex-main-agent.test.ts | sed -n '1,145p'Repository: stablyai/orca
Length of output: 38008
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- continuation implementation ---'
cat -n src/shared/agent-lead-status-fold.ts | sed -n '1,115p'
printf '%s\n' '--- mainAgent contract and normalization ---'
cat -n src/shared/main-agent-status.ts | sed -n '1,100p'
cat -n src/shared/agent-status-types.ts | sed -n '325,430p'
printf '%s\n' '--- remote hook forwarding and event ordering ---'
rg -n -C 8 'hookEventName|UserPromptSubmit|SessionStart|PostToolUse|Stop|connectionId' src/main src/shared/agent-hook-relay* src/relay --glob '*.ts' | head -n 500
printf '%s\n' '--- repeated-turn and remote reconciliation tests ---'
rg -n -C 8 'successive|successive turn|second turn|repeated|late.*Stop|relayed.*Stop|remote.*Codex|reconcileRemoteCodexState|UserPromptSubmit' src --glob '*test.ts' | head -n 500Repository: stablyai/orca
Length of output: 41869
🏁 Script executed:
cat -n src/shared/agent-turn-outcome.ts | sed -n '1,80p'Repository: stablyai/orca
Length of output: 1184
Prefer the incoming mainAgent outcome for a relayed Stop.
If the later turn's start hook is missed during a relay restart, the prior done failure remains cached. The current predicate selects it before the incoming done outcome because it does not compare turn IDs.
Suggested fix
const outcome =
- codexOutcomeRestatedByStop(previousLead, leadState).outcome ??
- (leadState === 'done' ? payload.mainAgent?.outcome : undefined)
+ (leadState === 'done' ? payload.mainAgent?.outcome : undefined) ??
+ codexOutcomeRestatedByStop(previousLead, leadState).outcome📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Why: the relay alone reads the rollout, so a turn it saw fail arrives as a Stop carrying the verdict. | |
| const outcome = | |
| codexOutcomeRestatedByStop(previousLead, leadState).outcome ?? | |
| (leadState === 'done' ? payload.mainAgent?.outcome : undefined) | |
| setCodexMainAgentTurnState(state, paneKey, { | |
| state: leadState, | |
| ...codexOutcomeRestatedByStop(previousLead, leadState), | |
| ...(outcome ? { outcome } : {}), | |
| // Why: the relay alone reads the rollout, so a turn it saw fail arrives as a Stop carrying the verdict. | |
| const outcome = | |
| (leadState === 'done' ? payload.mainAgent?.outcome : undefined) ?? | |
| codexOutcomeRestatedByStop(previousLead, leadState).outcome | |
| setCodexMainAgentTurnState(state, paneKey, { | |
| state: leadState, | |
| ...(outcome ? { outcome } : {}), |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new turn-end settle only runs when the poll's stored body is a root Codex event. If the last Codex hook on the pane was a child lifecycle event (SubagentStart/SubagentStop), the lookup is skipped and the row stays working — the same stuck spinner this PR fixes. Details inline; please confirm that ordering is handled or add a test.
Reviewed changes
- Turn end from the rollout —
turnEndsrecords eachtask_completebyturn_id(failedwhenpayload.erroris set) andcodexTranscriptTurnEndexposes it. - Poll while the root turn is open —
shouldPollHookTranscriptnow also polls when the root turn is unsettled and a rollout path is known, not only while subagents exist. - Poll publishes a
Stop—transcriptPollUpdaterelabels a poll whosemainAgentturneddoneashookEventName: 'Stop', so every host version settles it. - Relay verdict preserved —
reconcileRemoteCodexStatekeeps a relayedStop'spayload.mainAgent.outcomeinstead of dropping it. - Tests — failure with no
Stop, completed-without-Stop, turn-id fence, new-prompt race, late-Stoprestatement, unreadable rollout, child-held row, relay forward/stop-polling, and the desktop verdict.
I verified the load-bearing third-party contract independently: Codex hook stdin carries a required top-level turn_id for every turn-scoped event, and task_complete carries error only on failure (openai/codex generated hook schemas and codex-rs/protocol/src/protocol.rs), so this is not relying on an assumed shape.
ℹ️ A restated verdict can carry a stale failure onto a later successful turn
Because this PR makes Codex failure outcomes persist, the unchanged restatement rule in codexOutcomeRestatedByStop is newly consequential. If a turn's UserPromptSubmit never reaches the host that holds the lead record (relay restart with a spool miss, or a start event fenced at the remote ingress) while the previous turn settled failure, the next genuine Stop restates that failure because previousLead.state === 'done'; the successful turn then publishes mainAgent.outcome: 'failure'. The ?? payload.mainAgent?.outcome operand this PR adds does not help, since the restated operand wins. This is an open question rather than a required change — a turn-id fence on the restated operand would close it, but it is pre-existing behavior and depends on a genuinely lost start hook.
Technical details
# Stale outcome restated by a later Stop
## Affected sites
- `src/shared/agent-hook-listener/providers/codex-state.ts:84-91` — `codexOutcomeRestatedByStop` has no turn-id fence.
- `src/shared/agent-hook-listener/providers/codex-state.ts:224-232` — the new operand only wins when the restated operand is empty.
- `src/shared/agent-hook-listener/providers/codex-events.ts:236-240` — local path has the same precedence.
## Required outcome
- Decide whether a restated verdict should be scoped to the turn that produced it; if yes, fence on the open turn id.
## Open questions for the human
- Is a lost `UserPromptSubmit` (with the spool also failing to replay it) a case worth defending, or out of scope by design?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| if (polled.payload.mainAgent?.state === 'done' && original.payload.mainAgent?.state !== 'done') { | ||
| // Why: a root turn that ended in the rollout is published as its Stop, so every host version settles it. | ||
| return { ...polled, hasExplicitPrompt: undefined, hookEventName: 'Stop' } | ||
| } |
There was a problem hiding this comment.
readTurnEnd tracks only task_complete, not task_started, and the poll fence only compares lastStatusByPaneKey (applied hooks). When a new turn's task_started is already in the rollout but its UserPromptSubmit hook hasn't landed, the poll for the previous turn's body finds the old task_complete and publishes a Stop/done (plus a completion bell) for a pane that is running. It self-corrects once the delayed hook lands, so this is transient unless that hook is lost.
Technical details
# Old turn settles while a newer turn is already open in the rollout
## Affected sites
- `src/shared/agent-hook-listener/transcript-poll-policy.ts:44-47` — condition compares `mainAgent.state` only, never turn identity.
- `src/shared/codex-subagent-transcript.ts:159-171` — `readTurnEnd` ignores `task_started`.
- `src/main/agent-hooks/server/server-status-retries.ts:69-75` — fence is against applied events only.
## Required outcome
- If this window matters, track the latest `task_started` and refuse to settle a turn older than it, or key the settle to the pane's open turn id.
…(STA-7949) A Codex CLI pane stayed "working" after a turn failed. Codex runs its Stop hook only when a turn completes; when a turn ends on an error (a 5xx, a dropped stream, a usage limit) it runs no hook at all. Orca's last hook for the pane was PostToolUse, so the row read working until the 30-minute staleness cutoff. Codex does record every turn end in the session rollout it names in each hook (`task_complete`, carrying `turn_id` and, for a failed turn, `error`). The existing per-pane transcript poll on the execution host (local hook server and SSH relay) now also runs while the root turn is open, the rollout reader records each `task_complete` by `turn_id`, and the normalizer settles the open turn when its own `turn_id` has a recorded end: done, with `mainAgent.outcome: 'failure'` when the turn errored. - Fenced by turn id: an older turn's end never settles a newer turn, and a new prompt that lands first replaces the poll. - Published as that turn's Stop, so desktops of any version settle a relayed row; the desktop keeps the relay's failure verdict on it. - No timeout: a long silent wait stays working until Codex records the end. - Interrupts are unchanged (still settled from the Escape/Ctrl+C keystroke).
…action, and SSH child updates (STA-7949) - Record the root turn's Codex turn_id on the in-memory root record, so a child hook (a hook-reported subagent's tool hook or SubagentStart/Stop) that is the pane's latest event can still settle the root turn from the rollout. Never persisted and never sent on the wire. - Ignore Codex's mid-turn SessionStart(compact): it carries no turn_id and starts no new process. It used to replace the turn's last real hook, so a turn that failed after compaction never settled, and every poll tick reset the pane's rollout state and re-read up to 1 MB of the rollout. - Poll only while the open root turn has a turn_id; hooks without one can never match a rollout end. - Relay: a root hook re-read after its turn ended keeps publishing as Stop on later child-only updates, so the desktop does not revert the root to working when a transcript child finishes after the root failed. - Desktop: a relayed child event whose mainAgent the relay already settled ends the root turn. Co-Authored-By: Claude <noreply@anthropic.com>
0afe576 to
549b300
Compare
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The prior inline finding is addressed; one minor pre-existing concern from the previous review remains open, so this isn't an approval.
Reviewed changes
- Child-bodied polls now settle the root turn —
settleCodexRootTurnFromTranscriptreconciles the parent rollout and settles the open turn from both child hook paths, so a child hook can be the pane's last event when the root turn fails and Codex fires no root hook. - Open turn id tracked in memory — the root record keeps its
turn_idand the transcript poll gate now requires it, so an end recorded for an older turn can never settle a newer one, and a child hook can settle the turn it belongs to. - Mid-turn compaction ignored —
SessionStart(source: 'compact')returns null instead of resetting the poll anchor and dropping the open turn's id. - Relayed verdict and child updates preserved — the desktop keeps a relayed
Stop's failure outcome, adopts a settled main turn from a relayed child update, and a poll whose root ended is relabeledStopunless the polled body is a child's. - Tests — hook-server coverage for a child hook as the latest event, a finished child's
SubagentStop, a post-compaction failure, and the poll gate; relay-to-desktop coverage for a child outliving the failed root and for the root ending under a hook-reported child; desktop coverage for the preserved failure verdict.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Review status: ready for reviewHead Review loops (independent reviewers, run until clean)
Live QA (macOS, isolated background dev instance, real Codex 0.156.1, mock model server), re-run on the final head
The Electron QA ran through a Claude agent, not the Grok agent the process calls for: Grok's usage balance is exhausted (402). Known and disclosed (none block merge)
Not caused by this PR
Not tested: a live SSH host (covered by relay-to-desktop tests), Windows/WSL, and a real provider outage (simulated with a mock). |
This branch has not been deployed
1 participant
ELI5
When a Codex turn ends because of an error (the model service returned a 500, the connection dropped, a usage limit hit), Codex prints the error and goes back to its prompt, but it never tells Orca the turn is over. Orca kept showing that pane as "working" with a spinner for up to 30 minutes. Now Orca notices the turn ended within about a second and marks it done.
What Changed
Before: Codex CLI pane, a turn runs a tool, then the next model request fails. Codex shows "We're currently experiencing high demand…" and returns to
Ask Codex to do anything, but the sidebar row, the tab, and every other status reader keep showing the turn as working until the 30-minute staleness cutoff.After: the same pane settles to done about one second after Codex gives up, and the main agent's status records that the turn failed (
mainAgent.outcome: 'failure', the same field Claude's API-error hook already sets). A normal turn, a long silent turn, and an Esc/Ctrl+C interrupt behave exactly as before.Mechanism. Codex only runs its
Stophook when a turn completes. When a turn ends on an error it runs no hook at all, so Orca's last word for the pane wasPostToolUse→ working. But Codex does write the end of every turn to its session log, the file it names astranscript_pathin every hook: atask_completerecord carrying the turn'sturn_id, plus anerrorwhen the turn failed.turn_id(the id Orca matches against the log).task_completebyturn_id.turn_idhas a recorded end, the pane settles through the same path aStophook takes, and the update is published as that turn'sStop.turn_idin memory (never saved, never sent to other machines). That lets the turn settle even when the pane's latest hook came from a subagent, which keeps firing its own hooks after the main turn fails.SessionStartwithsource: "compact"in the middle of a turn after compacting its context. That hook has noturn_idand does not mean a new Codex process, so Orca now ignores it, the same way it already ignores Claude's compactSessionStart. Before, it hid the turn'sturn_idfrom the poll, and it also reset the pane's saved log position, so every poll re-read up to 1 MB of the log. A side effect: the prompt label is no longer cleared after a compaction.Stopnow keeps the failure verdict the relay read from the log. A relayed subagent event that already carries the settled main turn also ends it, and later updates about subagents only keep publishing as the main turn'sStop, so the desktop row stays settled when a subagent finishes after the main turn failed.Why
The turn end has to come from something Codex itself records; no signal Orca already had can stand in for it:
notifycallback runs only after a successful turn, the same asStop.Interrupthook covers only interrupts, not failures.Orca's structured Codex chat already settles failed turns from this same fact (a turn ended in failure, keyed by turn id), received over its live connection to Codex. A terminal pane has no such connection, so this reads the fact from the session file Codex already hands Orca. No new wire fields: a newer relay talking to an older desktop still settles the row when the main agent's own hook came last, just without the failure label. (When a subagent's hook came last, an older desktop does not adopt the settle; in practice the SSH relay is installed per desktop build, so the two always match.)
Out of scope: interrupts are still settled from the Escape/Ctrl+C keystroke. Using the log's
turn_abortedrecord instead is a natural follow-up, but that code is being changed in #22727 / #22476.Linked Issue
Linear: STA-7949
Visual Proof
Real Codex 0.156.1 in an isolated background Orca dev instance, pointed at a local mock model server that runs one shell command and then returns HTTP 500 on the follow-up request.
Before (
main), 60 s after the failure: Codex is back at its prompt; the tab and sidebar row still spin.After (this PR, final head), 10 s after the failure: same Codex screen; the row shows done and the tab shows the unread "finished" bell.
Testing
Live (macOS, isolated dev instance, real Codex 0.156.1, mock model):
mainStop; no failure label carried over; one settle, not twocancellationNo new main-process or renderer errors versus
main. Re-run on the final head after review.Automated:
server-codex-transcript-turn-end.test.ts(10 tests through the real hook server): failed turn with noStop; completed turn whoseStopnever arrived; an earlier turn's end never settles the open turn; a new prompt racing the old turn's end; a lateStopis a restatement, not a new transition; unreadable log; a failed main turn with a subagent still running keeps the row working until the child ends; the main turn settles when a subagent's tool hook or itsSubagentStopis the latest event; a turn that fails after a mid-turn compaction settles.agent-hook-server-codex-transcript-turn-end.test.ts(3 tests, the relay's output fed into a real desktop hook server as over SSH): a failed turn is forwarded as aStopcarrying the failure, then polling stops; the desktop row stays settled when a subagent finishes after the main turn failed; the desktop row settles when the main turn ends under a subagent's hooks.agent-hook-listener-codex-main-agent.test.ts: the desktop keeps the relay's failure verdict on a relayedStop; the poll runs only when hooks carry aturn_id.Stoprelabel, the relay verdict, the stored turn id, the settle from subagent hooks, ignoring the compactSessionStart, the desktop's adoption of a settled main turn); every new behaviour test fails againstmain.pnpm tc, oxlint, andcheck:code-quality:changedpass.Not tested: a live SSH host (covered by the relay test), Windows/WSL, and a real provider outage (failures were simulated with a mock server). I did not check which older Codex versions write
errorontask_complete; without it a failed turn still settles, just without the failure label.AI Disclosure
Author: @BrennanKB5
Review
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
statper second per open Codex turn whose hooks carry aturn_id, reading only newly appended bytes. If the Codex process dies mid-turn without firing any hook and the shell stays open, the poll keeps running until the pane closes or its next hook arrives, the same as the existing subagent poll. It runs inside the hook server (the local one or the relay), so closing the pane, quitting the app, or stopping the relay ends it. On an SSH pane, an Esc/Ctrl+C interrupt is inferred on the desktop, so the relay keeps that once-a-second check running until the pane's next hook; it never publishes anything wrong. Reading Codex's own interrupt record would end it, which is the follow-up noted above.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)