Repository navigation
test(native-chat): main's Codex child tests expect a turn to end on its own turn/completed - #23783
Conversation
#22553 ended a child thread's turn on an `error` Codex will not retry, reading the verdict module #23682 deleted when it made turn/completed the only end of a Codex turn. The two landed minutes apart with no textual conflict, so main no longer typechecks. Codex runs every thread's events, spawned children included, through the same per-thread handler: a turn-ending error is recorded as the turn's last error and the turn then completes as failed. So a child's failed turn/completed is its end, as on the primary thread, and a closed thread stays the one child ending with no completion.
 |
|
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 (2)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe frame reader now emits a Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable issue is established that would prevent merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change affects how child-work outcomes are reported, but the reviewed path does not show a new privileged operation or broader access. Some event-ordering and producer-trust details remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Dangling import removed —
codex-background-task-frames.tsno longer imports thecodex-structured-journal-provider-verdictsmodule that #23682 deleted, sopnpm tc:nodecompiles again. - Child turns now end only on
turn/completedorthread/closed— the child-only "fatalerrorends the turn" branch and theCodexChildTurnEndingtype are gone;readCodexChildTurnEndingbecomesreadCodexChildThreadClosed, and a failed child completion flows through the existingturnframe branch (codexChildTurnState). - Tests retargeted to the surviving rule —
codex-child-work-evidence.test.tsnow asserts a fatal error leaves the childliveand the following failedturn/completedsettles itfailed, plus thethread/closed→unknownpath; the producer test adds the same live-then-failed sequence.
Verified locally at 77bd1cc: the three codex test files pass (59 tests), oxlint is clean on the changed files, and pnpm tc:node passes.
ℹ️ Child-thread parity is inferred from Codex source, not captured
The comment added at src/main/codex/codex-background-task-frames.ts:41 claims the error-then-completion pairing holds "on child threads as on the primary", but every capture in src/main/codex/__fixtures__/codex-app-server-turn-endings.jsonl is a single-thread (primary) session, and codex-structured-journal-turn-endings-replay.test.ts only exercises those. The change is well-supported — child threads run through the same per-thread app-server handler, and Orca can only mark a child working after seeing that child's own turn/started, so the subscription that delivers the start also delivers the completion — but per this repo's evidence convention the child case is the one assertion this PR adds without a transcript behind it. No action required to merge; worth a follow-up child capture (or a note in the fixture) so the thread/closed fallback is also covered by observation.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
With a child's turn now ending only on its own turn/completed, the turn-ended frame carried a fixed turnId (null) and state (unverifiable), and endTurn took parameters nothing passed. Name the one remaining case: a thread-closed frame, and closeThread ending the running turn as unverifiable. Drop a test step that no longer exercised anything.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Split
turn-endedinto a dedicatedthread-closedframe — a closed Codex child thread now carries only itsthreadId; theturnId/statefields and theCodexChildTurnEndingtype are gone, andreadCodexChildTurnEndingbecomesreadCodexChildThreadClosed. CodexSubagentExecutions.endTurnbecomescloseThread— it always settles a still-working child's running turn asunverifiable, dropping parameters that could only ever benull/'unverifiable'; a child running no turn is a no-op, as before.- Tests retargeted to the two surviving paths — the evidence suite asserts a fatal error keeps the child
liveand the following failedturn/completedsettles it, plus thethread/closed→unknownpath; the producer suite adds the live-then-failed sequence.
Verified at cbcd782: both changed test files pass (33 tests) and pnpm tc:node passes. The change is behavior-preserving — the old endTurn caller also passed turnId: null and 'unverifiable', so closeThread computes the same settlement.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Review status: ready for Brennan's decision (not merged; auto-merge not enabled)Verdict: the fix is correct and in scope. It turns main's Commits on this PR
Reviews: two review loops (loop 1: that one cleanup, fixed above; loop 2: clean), plus start and end readiness checks and an independent read-only review. Codex's app-server source was read first-hand: spawned helper threads use the same per-thread event handler as the main thread, and every error that ends a turn is followed by a failed Checks run locally at CI at Live QA (real Codex 0.157.0 in a background test build of Orca; a Claude agent ran it because the Grok seat was out of credit)
Not verified live: a helper that Orca has registered, failing and reading "failed" in the chat. Helpers are registered only for models on Codex's newer multi-agent tools (GPT-6 and GPT-5.6 Sol/Terra), and those take only a text message, so none of the three attempts could make one fail. The code path from the failed Found during QA, not caused by this PR: with GPT-5.5 (Codex's older multi-agent tools), helpers are never registered on this build. So a helper that fails shows as raw red error cards at the parent's top level, with no subagent row, while the parent tab shows a green check. This PR doesn't touch the code that registers or draws those rows; open PR #22619 is the change that registers these helpers.
Scope: 4 files in the main-process code that reads Codex events. No UI, wire-format, mobile, SSH or platform change; a helper's record keeps the same shape and states. |
brennanb2025
changed the title
Resolves the conflict with #23808 in favour of this PR's model: a Codex child's turn ends on its own turn/completed, and the one ending without it is a closed thread. #23808's child error arm and readCodexErrorWillRetry (no other consumer) are dropped; its pr-workflow-parallelism.test.mjs fix is kept.
There was a problem hiding this comment.
Important
Merging this branch silently reverts #23808 ("keep a child turn settling on an error Codex will not retry"), which landed on main at 06:42 UTC after this PR's last review. The merge of #23810 resolved the conflict in the branch's favour, so the head has no child error path while origin/main (b8a7dafa) does. The PR description's "that matches main's code" is now stale — please reconcile explicitly.
Reviewed changes
The only incremental change since the prior pullfrog review is main being merged into the branch (1dbb4c8 #23801, 215ce16 #23802, 15e7a42 #23810). The branch's own tracked files are byte-identical to the previously reviewed 215ce16, so the substance of this delta is the last merge's conflict resolution.
mainmerged, dropping #23808's child error path — the head lacksreadCodexErrorWillRetryand the "a non-retryableerrorends a child's turn" branch that currentmainhas, so the merge reverted a fix merged hours earlier rather than reconciling with it.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| * The one way Codex ends a child's turn without `turn/completed`: a closed thread ran its last | ||
| * turn and Codex never said how it went. A turn-ending `error` is not one — Codex follows it with | ||
| * a failed `turn/completed` for the same turn, which is that turn's end and carries the duration | ||
| * and receipt time the error does not. |
There was a problem hiding this comment.
error branch it describes — now contradicts main, not just the pre-#23808 code. #23808 restored exactly this path, and the merge of #23810 (15e7a42) resolved the conflict by keeping the branch's removal, so head has no child error path while origin/main does. Merging this PR would revert #23808, whose stated risk is a child that latches on working for the session when no turn/completed follows the error; please reconcile deliberately (keep the path, or record why it is being withdrawn and on what evidence).
Technical details
# Merge reverts #23808's child error-settling path
## Affected sites
- `src/main/codex/codex-background-task-frames.ts:40-43` — JSDoc asserts a turn-ending `error` is not an end; `main`'s `readCodexChildTurnEnding` returns `{ kind: 'turn-ended', threadId, turnId, state: 'failed' }` for a non-retryable `error`.
- `src/main/codex/codex-structured-thread-facts.ts` — `readCodexErrorWillRetry` exists on `origin/main` (`b8a7dafa`) and was added back by #23808; this branch deletes it.
- `src/main/codex/codex-child-work-evidence.test.ts:173-183` — the error step asserts `live`; `main`'s version asserts the error settles `failed`.
## Evidence
- `git show 215ce16:src/main/codex/codex-background-task-frames.ts` and `git show 15e7a42:...` are identical; `git diff 215ce16 15e7a42 -- src/main/codex/` is empty.
- `git show f8f656ca19:...` and `git show b8a7dafa:...` (current `main`) both contain the error branch.
- `git diff f8f656ca19 15e7a42 -- src/main/codex/` removes it (6 files, +49/-75).
## Required outcome
- `main`'s #23808 must not be dropped without an explicit decision: either retain the error path, or state in the PR why it is being reverted, with evidence that resolves the disagreement with #23808's no-guarantee argument.
…3745, #23783, #22971, #22565, #22846, #23724, #23721, #23693, #23810) into #23059 #23671 (group chat rows by the turn that produced them) is ported onto #23059's turn-scope model, so there is one grouping mechanism, not two: - A host that states each row's turn scope groups by that scope, as before. #23671's journal-order derivation (rows between one turn record and the next belong to it) moves from the timing selector into src/shared/native-chat-turn-grouping.ts and runs only for a host that states no scope, replacing the positional fallback there. - #23671's anchor rules live in structuredAgentTurnAnchors, which membership, settled timing and the live clock all read: a turn whose opener key nothing resolves yet takes the send still in flight ahead of its record (Codex before its echo), else its own record. - The live turn is one key again. #23671 made row liveness follow the owning turn, so the bar key and the liveness key are always the same; nativeChatTurnMembership now returns only liveTurnKey (barTurnKey, activeTurnOpenedBy and turnKeysByItemId are gone). - A turn with no user bubble draws its bar above its first row (nativeChatTurnBarRows, desktop and mobile), and a turn's diff rollup goes under its last row. - The Codex full-history restore writes each record ahead of its items; that also fixes the derived turn scope on #23059's journal. #23026 (Stop from the moment a message is sent) is ported onto #23059's command turns: the cancel paths ask the translator for a command's provider turn instead of the removed compaction tracker, and a Stop that names no turn still ends a running /compact through its child. performCancel moved to structured-agent-session-turns-cancel.ts to keep the turns module under the line limit. #22090's awaitingInput replaces showTurnStatus/showLiveTurnActivity; the waiting-behind-/compact rows are kept.
ELI5
#23808 put back the rule "a Codex helper agent's turn ends when Codex reports a fatal error". This PR takes it out again, because Codex always sends its own "turn finished (failed)" message for that turn a few milliseconds after the error, for helpers exactly as for the main agent. A helper's turn now ends on that message. The one case with no such message, a helper thread that closed, keeps its own clearly named ending.
What Changed
errorwithwillRetry: falsesettles it as failed, and the failedturn/completedCodex sends 0-32 ms later is a second ending for the same turn.turn/completedthat follows.readCodexErrorWillRetryand the child error arm incodex-background-task-frames.tsare removed; nothing else reads them.{ kind: 'turn-ended', turnId: null, state: 'unverifiable' }(two fields that can only hold one value, under a name that really means "the thread closed") becomes{ kind: 'thread-closed', threadId }.endTurn(threadId, turnId, state)becomescloseThread(threadId). A test step that could no longer fail is dropped. No behaviour change.Conflicts with #23808
This merge reverses #23808's child error arm. #23808 restored "a child's turn settles as failed on an
errorwithwillRetry: false" (an arm incodex-background-task-frames.ts, plusreadCodexErrorWillRetryincodex-structured-thread-facts.ts, which nothing else reads) on the claim that a child'sturn/completedmay never arrive. Its only evidence was the older tests.Codex's source says the completion always follows. Every thread, spawned helpers included, runs through the same per-thread handler (
app-server/src/lib.rs:1282-1298attaches it onnotify_thread_createdfromcore/src/agent/control/spawn.rs:875;app-server/src/request_processors/thread_lifecycle.rs:344). A turn-ending error breaks the turn loop and returns normally (core/src/session/turn.rs:818-834). The task wrapper then always finishes the turn (core/src/tasks/mod.rs:395-398) withTurnComplete(core/src/tasks/mod.rs:844-857), orTurnAbortedif it was cancelled; both reach the client asturn/completed. The app server records the error as the turn's last error and completes the turn asfailed(app-server/src/bespoke_event_handling.rs:185-200,1503-1531,1607-1631).Live on Codex 0.157.0, a helper's
willRetry: falseerror was followed 3 ms later by that helper's failedturn/completedfor the same turn.The merge keeps #23808's unrelated fix to
pr-workflow-parallelism.test.mjs, which still read the deletedtrack-community-prs.yaml.A child still settles if Codex dies after the error: the exit handler sends the session's
endedevent andsettleSession()sets every working child tounverifiable, and a helper thread that closes with no completion settles throughcloseThread. Both paths are covered by the session-end and closed-thread tests incodex-child-work-evidence.test.tsandcodex-subagent-roster.test.ts.Why
turn/completedthe only live end of a Codex turn, backed by captures from the real app server, and fix(native-chat): drop the Codex error path #23682 removed #23801 applied that to helper threads in code. Tests written for the older rule now fail onmain.willRetry: falsewas followed byturn/completedwith statusfailedfor the same turn, 3 ms later.Linked Issue
Follow-up to #23682 and #23801: main's unit tests still expected the error ending those PRs removed.
Visual Proof
N/A: no UI change. The live QA screenshots are in the status comment.
Testing
pnpm tc:nodepasses.oxlintis clean on the changed files.The Codex child-work, producer, tracker, background-task, projection and close suites pass: 7 files, 79 tests. On main (with fix(native-chat): keep a child turn settling on an error Codex will not retry #23808), the child-work tests assert the error ending instead.
Live QA on a real Codex 0.157.0 (background rig, isolated home): a succeeding helper settles on its completion. The error-then-failed-completion sequence was captured on a real helper thread.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
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
This is main-process Codex event reading and its tests only. It has no platform, SSH, mobile or wire effect.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)