Repository navigation
fix(native-chat): drop the Codex error path #23682 removed - #23801
Conversation
#23682 established that only `turn/completed` ends a Codex turn and deleted `codex-structured-journal-provider-verdicts.ts`, but the child-turn reader kept importing `readCodexProviderVerdict` and branching on its `turn-failed` verdict. The module is gone, so the import resolves to nothing: typecheck and static analysis fail on every pull request, which skips the whole unit matrix behind them, and the esbuild pass behind Bun profile scope detection throws, so every run falls back to all six platforms. A closed thread is now the only child-turn end without `turn/completed`, which is what #23682 intended: Codex follows a turn-ending `error` with a failed `turn/completed` for the same turn 0-32 ms later, and that completion carries the duration and receipt time the error does not.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return event.method === 'thread/closed' | ||
| ? { kind: 'turn-ended', threadId: event.threadId, turnId: null, state: 'unverifiable' } | ||
| : null |
There was a problem hiding this comment.
Two existing tests send a non-retryable error without a turn/completed event and expect the child to settle as failed. With this branch removed, those errors produce no frame, so the child stays working and both assertions fail when the full unit suite runs. Update the tests to use the intended completion-based sequence.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The production change is correct, but dropping the error path left three existing child-settlement assertions stale, so the unit suite now fails on this branch.
Reviewed changes
- Drops the dangling import —
codex-background-task-frames.tsno longer importsreadCodexProviderVerdictfrom the module #23682 deleted, which is what broketscand the Bun profile change-scope bundle onmain. - Collapses
readCodexChildTurnEndingto one case — a non-retriederrorno longer ends a child turn; onlythread/closeddoes, soturn-endedframes always carryturnId: null/state: 'unverifiable'and theCodexChildTurnEndingalias is gone.
⚠️ Three child-settlement assertions still encode the removed error path — the unit suite is red
The PR body says "no test asserted the removed error path", but three assertions do, and they fail now that the error branch is gone. On this branch pnpm tc:node passes, yet pnpm exec vitest run --config config/vitest.config.ts src/main/codex/ reports three AssertionErrors in codex-child-work-evidence.test.ts and codex-structured-child-work-producer.test.ts — so the unit matrix this PR is meant to unblock will itself fail. The fix has to update those files alongside the frames change.
Technical details
# Update the child-turn tests to the post-#23682 rule
## Affected sites
- `src/main/codex/codex-child-work-evidence.test.ts:181-199` — the two error cases (`childError('c1', false)` and `childError(undefined, false)`) expect `{ membership: 'settled', outcome: 'failed' }`; after this change the `error` frame is ignored, so the child stays `live`/`working`. The `thread/closed` case in the same `it.each` is still correct.
- `src/main/codex/codex-structured-child-work-producer.test.ts:336-344` — mid-step asserts `settled`/`failed` on the non-retried error; it should stay `working`.
- `src/main/codex/codex-structured-child-work-producer.test.ts:402-406` — final expected list has `lint` as `settled`/`failed`; under the new rule it is only settled at session close, as `unknown`.
- `src/main/codex/codex-background-task-tracker.test.ts` — passes; no change needed.
## Required outcome
- A non-retried `error` no longer settles a child: assert it is ignored (stays `working`) and that the child settles only on `turn/completed` or, absent one, `unknown` at session close.
- `pnpm exec vitest run --config config/vitest.config.ts src/main/codex/` is green apart from the sandbox-only integration tests (`codex-app-server-*.integration.test.ts` fail in this environment regardless of the change).
## Suggested approach
Reuse the shape of the existing "keeps a child working through a retried error" case (`codex-child-work-evidence.test.ts:201`) for the non-retried variant rather than inventing a new fixture.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
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 (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe turn-ended frame type now requires a null Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable issue is established with the narrowed child-turn ending behavior. The change is ready for normal merge checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change narrows an internal completion path without adding a new privileged action or broader entry point. One event-ordering assumption remains unverified, so the risk is low rather than minimal. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
…ot retry #23801 removed the child-path reading of Codex's turn-ending `error` along with the import of the module #23682 deleted. That was the wrong half to remove: the primary journal path can rely on Codex's failed `turn/completed` arriving within ~32 ms, which is what #23682 established, but a child turn has no such guarantee, and without the error as its end the child's lifecycle row latches on `working` for the life of the session. Three tests assert exactly that and could not run, because the unresolved import had been skipping the unit matrix since #23682 merged. The reading is restored inline against `readCodexErrorWillRetry`, itself restored to `codex-structured-thread-facts.ts`, rather than by reviving the deleted module: its `thread-stopped-running` arm lost its only consumer when #23682 rewrote the primary path, so restoring the file would re-add dead code. Also drops `pr-workflow-parallelism.test.mjs`'s read of `.github/workflows/track-community-prs.yaml`, which #23796 deleted while leaving the assertion behind. Same failure class, and it fails the same shard.
…ot retry (#23808) #23801 removed the child-path reading of Codex's turn-ending `error` along with the import of the module #23682 deleted. That was the wrong half to remove: the primary journal path can rely on Codex's failed `turn/completed` arriving within ~32 ms, which is what #23682 established, but a child turn has no such guarantee, and without the error as its end the child's lifecycle row latches on `working` for the life of the session. Three tests assert exactly that and could not run, because the unresolved import had been skipping the unit matrix since #23682 merged. The reading is restored inline against `readCodexErrorWillRetry`, itself restored to `codex-structured-thread-facts.ts`, rather than by reviving the deleted module: its `thread-stopped-running` arm lost its only consumer when #23682 rewrote the primary path, so restoring the file would re-add dead code. Also drops `pr-workflow-parallelism.test.mjs`'s read of `.github/workflows/track-community-prs.yaml`, which #23796 deleted while leaving the assertion behind. Same failure class, and it fails the same shard.
…3700, #22943, #23720, #23793, #23776, #23796) into #23059 #23573 (keep a turn's bar on the prompt that opened it) is ported onto the turn-record membership instead of kept as a second owner lookup: - The running turn's bar comes from nativeChatTurnMembership, which now returns barTurnKey beside liveTurnKey. Where the host states each row's turn they are the same key: the running root turn's anchor, and a message handed over mid-turn is already scoped to that turn. Where the host states no scope, rows still group by position, so barTurnKey is the user entry the running record names (through the same anchors) while liveTurnKey keeps the positional live group, as #23573 kept row liveness on the newest prompt. - activeTurnOpenedBy, selectStructuredAgentTurnBars' openedBy and selectNativeChatActiveTurnKey are removed; the desktop list, the slot builder and both turn-status hooks read barTurnKey. - The queued-send clock (queuedUntil: a turn counts from the previous turn's end when its send waited behind it) is kept, read in the same one-pass walk over root turn records that keys timing by anchor. - Its tests are adapted to pass a journal: each runs on a host that states no scope (the original expectations), and the desktop and mobile bar tests also run on a scoped host. #23621 (a message Claude folds into its running turn no longer splits the turn) is host-only and merged as is: the translator contract carries both openTurnInLiveProviderCycle and #23059's command-turn members. Both sides grew the Claude translator past its line budget, so its result branch moves unchanged into claude-result-journaling.ts, beside claude-message-journaling.ts (the pre-commit lint refuses the merge without it). #23801's Codex child path (no error-driven end) and #23059's Codex /compact guard (a command turn ends only on turn/completed) both stand.
ELI5
A recent change decided that Codex only finishes a task when Codex says "turn completed". It deleted the old helper that used to finish a task early when Codex reported an error — but one file still tried to use that deleted helper. Nothing could compile, so every open pull request went red. This deletes the leftover line.
What Changed
Before:
mainwas red. Every PR failedtypecheckandstatic analysis, and because the unit-test matrix only runs when typecheck succeeds, no unit tests ran on any PR at all. Bun profile persistence also silently ran all six platforms on every run instead of the platforms it selected.After: all three go green again and the unit matrix runs.
The mechanism: #23682 deleted
src/main/codex/codex-structured-journal-provider-verdicts.tsbut leftcodex-background-task-frames.tsimportingreadCodexProviderVerdictfrom it and branching on itsturn-failedverdict. An import of a module that no longer exists is a hardtscfailure, and it also throws inside the esbuild bundle that Bun profile scope detection uses to work out which platforms a change can affect — that failure path fails closed to all six platforms.readCodexChildTurnEndingnow has a single case: a closed thread is the only way a child's turn ends withoutturn/completed. Theturn-endedframe narrows toturnId: nullandstate: 'unverifiable'accordingly, since nothing producesfailedthrough this path any more, and the now-unusedCodexChildTurnEndingalias is gone.Why
This is the deletion #23682 intended, not a workaround for it. That PR captured real app-server behaviour on 0.141.0 and 0.158.0 across eight failure scenarios: Codex follows every turn-ending
error(willRetry: false) with a failedturn/completedfor the same turn 0-32 ms later, and only the completion carries the turn's duration and receipt time. Ending the turn on the error threw that away and ended the turn twice.The alternative — restoring the deleted module — would reinstate exactly the double-ending #23682 removed, and would have to be justified against that PR's captured evidence. No test asserted the error path, which is consistent with it being intentionally removed.
Linked Issue
No pre-existing issue; this is a merge-time regression from #23682 found while auditing CI runner demand.
Visual Proof
N/A— no UI change. The user-visible effect is a child agent's failed turn now settling on Codex'sturn/completedrather than on itserror, which #23682 already established as correct.Testing
Local, macOS:
pnpm tc— exit 0 (was failing onTS2307for the missing module)pnpm exec vitest run --config config/vitest.config.ts src/main/codex/codex-background-task-tracker.test.ts src/main/codex/codex-structured-session-background-tasks.test.ts— 32 passedpnpm exec vitest run --config config/vitest.config.ts config/scripts/bun-profile-change-scope.test.mjs— 37 passed, including the real-dependency-graph test that was failing onmainoxlintandpnpm run check:code-quality:changed— cleanCross-platform: pure TypeScript control flow, no platform-dependent behaviour. SSH/mobile unaffected.
Existing coverage in
codex-background-task-tracker.test.tsandcodex-structured-session-background-tasks.test.tsexercises the child-turn reader; no test asserted the removed error path, so there is no assertion to update. Added none: this restores the state #23682 intended, and the regression's real gate istsc, which now passes.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
Security: none. Cross-platform: none. SSH/remote: none. Mobile: none. Backwards compatibility: none — internal translator only. Performance: removes a per-event call.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)