Repository navigation
fix(native-chat): a Codex stream retry is one warning row that updates in place - #23684
Conversation
…try row A retry row whose fact carries a detail the provider wrote for a person now quotes it, the same way a rejected message or failed compaction does, so the row says how the retry is going. A log detail still stays out of the sentence.
…s in place An error Codex says it will retry used to fall through to the generic frame row: red, and a new row for every attempt. It now writes one providerRetrying row per retry run, warning-toned, revised by each attempt with Codex's own progress sentence. A run is the retry frames of one turn with nothing else the thread journals between them; every attempt still publishes, so the idle sweep keeps seeing activity. Errors Codex will not retry are unchanged.
… frame behind Details The quoted retry sentence leads with "is retrying", which holds for any provider's progress text. The Codex retry row also keeps the whole bounded frame behind the row's Details, as the generic row did, so Codex's additionalDetails stays available to diagnose a retry.
…its one row Pins the run being opened before the write: a first attempt whose publish is refused and is handed back must revise the row it already wrote, not open a second run. Also stops the fixture claiming Codex sends an idle thread status beside each retry, which the app server does not do.
…fied Ending a retry run classified every non-retry frame, and classifying walks the whole payload: every streaming delta, and every large item/completed, paid a walk about as costly as parsing the frame. Only a thread with a run open needs the answer, so the classification now runs only there.
…th what failed on its second line A stream error Codex says it will retry is written as its own warning row with a providerRetrying fact, under the same per-frame identity every Codex frame row gets. The host no longer tracks retry runs or rewrites one row in place, so there is no run state to open, end or clear, and no frame has to be classified to end a run. Every attempt publishes, which keeps renewing the idle clock while Codex retries. Codex's additionalDetails, which its own UI shows under the progress message, is kept on the fact as the retry's cause and printed on the row's second line. Errors Codex will not retry are unchanged.
…r retry run The shared structured message projection, which both the desktop and the mobile transcript read, collapses a run of retry rows into its latest row. A run is retry rows from the same agent with no other drawn row between them; a row that draws nothing, or a queued send drawn after the conversation, does not split it. The earlier attempts stay in the journal.
… later one never revises it Frame rows were named provider-frame:codex:<n> from a counter that starts over with every connection, so the first frame row after a reconnect in the same session revised an earlier connection's row in place, at its old spot. Each connection's frame rows now carry the acquisition generation, minted once before the translator is built: provider-frame:codex:<generation>:<n>. Rows already written keep their identities.
…m the row's own agent
…tion that received them
…never splits it Each agent's rows are drawn apart: the session's own rows are the conversation, and a subagent's rows open in that subagent's section. Splitting a run on any other row in the flat list left two adjacent retry rows on screen whenever another agent wrote between two attempts: a subagent finishing a command while the session reconnected, or the session working while a subagent reconnected. A run is now per agent: an agent's retry rows with none of its own other rows between them, drawn as its latest.
…on, and the run rule's words say same agent
Review status: clean at 04467ce, CI green (still a draft)Where it stands. Review loops are finished and the last loop came back clean. The branch includes current main (7afa4ee). What the review changed
Architecture check. More than three loops ran, so a separate review questioned the design itself. Its verdict was to keep it.
Readiness checklist. It passed at the start (100e92c, with one P2) and at the end (7af0a14). The P2 was the subagent-section issue fixed in 1a86450. Live QA. Real Codex 0.157.0 in live Electron. A local stand-in server answered 502 to every model request, set by a per-process Codex flag in an isolated test profile. No Codex config file was written, and the real Codex hook and config files were hashed before and after every run with no change. Before (main): every attempt is its own red row. After (7af0a14): one yellow row per reconnect, counting up, with the failure on its second line. After reloading the window, the transcript is identical and the journal still holds all nine attempts. The renderer console showed no errors. Known and out of scope
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Repository guideline files applied to this review (1)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 (26)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughCodex retry notifications now produce warning journal rows with provider details and acquisition-specific identities. Shared failure facts preserve sanitized retry causes, and retry copy can include provider details. Structured-session projection collapses consecutive retry rows for each agent. Added tests cover journal handling, idle sweeps, retry-row projection, and desktop and mobile rendering. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No merge-blocking issue is established. Retry attempts remain in the journal while the transcript displays the latest warning per agent; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change improves retry-history isolation and keeps diagnostic text bounded. No introduced security vulnerability was established, but diagnostic confidentiality and some failure-recovery behavior remain incompletely verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed and covers the user impact, implementation, rationale, visual proof, testing, scope, and checklist. However, the required Linked Issue section states "None," while the repository template requires an actual issue reference. Full details: Docstring CoverageExplanation Docstring coverage is 37.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 19 files. (7 skipped: 7 unsupported.)
✨ 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 |
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — full review of this PR's 17 files: the host-side warning row, the client-side run collapse, and the per-connection frame-row identity, plus seven new test suites.
- Host writes one warning row per attempt. Each Codex
errorframe withwillRetry: trueis journaled as its owntone: 'warning'status row via a newproviderRetryingfact;detailcarries Codex's own message andretrycarries{error, status, cause}, withcausea new optional field. The frame is written throughappendFrameRow, so it publishes and keeps the idle sweep treating a reconnecting Codex as busy. - Shared client collapse draws only the latest row.
collapseProviderRetryRunsdrops every retry row but the latest of each run, keyed byagentJournalItemSubagentId— the same key the transcript's subagent-section split uses — and wired intoprojectStructuredAgentSessionMessages, so desktop, mobile and the conversation outline agree. - Per-connection frame-row names. Frame rows are now
provider-frame:codex:<acquisitionId>:<n>, ending the reconnect overwrite; the acquire path mints the generation before creating the translator and passes it in. - The cause prints on the second line. The
providerRetryingsentence now quotes a person-audience detail and appendsretry.cause; Codex messages are person text, so Claude's log-audience retry rows keep their old wording.
Verified against callers and adjacent behavior: the retry branch sits after item routing and before the generic notification:error fallback; retry frames no longer reach commands.errorShown, which already no-ops for willRetry frames (src/main/codex/codex-journal-command-turn.ts:151); orchestration worker transcripts intentionally still call projectStructuredItemsToNativeChat directly; and NativeChatMessageRow prefers the toned notice over providerFrame, so the sentence renders yellow with the raw frame behind Details.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…try-one-row # Conflicts: # src/main/codex/codex-structured-journal-generic-frames.ts # src/shared/agent-session-failure-words.ts
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the merge with origin/main since the prior review, where the branch reconciled its Codex retry-row feature with main's failure-copy refactor (#23674).
- Retry sentence ported onto the shared copy table.
providerRetryingnow composesproviderRetrying/providerRateLimited/providerRetryingQuotedthrough thesayfunction, andquotingPersonDetailtakes the quoted sentence's copy id instead of concatenating strings. - New quoted copy and translations.
providerRetryingQuoted({{agent}} is retrying: {{detail}}.) joinsAGENT_SESSION_FAILURE_COPY,en-runtime-required.json, all six locale catalogs, and the desktopPIECESmap. withRetryCauseextracted. The provider's own account of what failed still prints on the row's second line, and a read now re-bounds thecausethroughreadProviderRetry/readAgentSessionFailureFact.- Main's frame-classification signature adopted in
codex-structured-journal-generic-frames/translation; per-connection frame-row naming is unchanged.
Verified the reconciliation is behavior-preserving: the person-detail path yields the same sentence as before, the log-audience Claude path keeps its rate-limited/temporary wording with no cause, and the focused host/shared/render suites pass (93 tests).
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…ges crossed (#24231) * test: two tests catch up with the journal-database store and the close cause #24006 moved the session store into the chat journal database, and #23467 made close take a cause; #23684's and an older tab-table test still used the old calls, so main's typecheck fails. * test: the startup-reconcile close test passes the close cause too




ELI5
When Codex loses its connection mid-answer, it quietly tries again and tells Orca about each try. Orca printed every try as a new red error, so one dropped connection filled the chat with red rows that looked like the turn failing over and over. Now the chat shows one yellow line per reconnect that counts up ("Codex is retrying: Reconnecting... 3/5.") with what went wrong on the line under it. A real failure is still red.
What Changed
The problem (before)
When Codex's connection to the model drops mid-answer, Codex quietly retries ("Reconnecting... 1/5", "2/5", ...) and sends Orca an
errornotification withwillRetry: truefor every attempt. Orca drew each one as its own red error row. A single dropped stream filled the chat with five to ten red rows, as if the turn had failed over and over, when Codex was only reconnecting and often recovered on its own. What actually went wrong (Codex'sadditionalDetails, e.g. "unexpected status 502 Bad Gateway") was hidden behind each row's raw-frame Details.A second, smaller bug sat under it: every Codex frame row was named
provider-frame:codex:<n>from a counter that restarted on each new Codex connection. After a reconnect, the new connection's frame rows reused the old names and silently overwrote the earlier rows in the chat history.After, for the user
willRetry: false) is unchanged: a red row, and the turn fails.Not affected: Claude sessions (its retry rows keep their current behaviour), terminal (TUI) sessions, and orchestration worker transcripts, which still list one row per attempt.
How it works
Host. Each Codex
errorframe withwillRetry: trueis written to the session journal as its own warning row, through the same frame-row writer every other Codex frame uses (same caps, admission and backpressure, and it publishes activity, so the idle sweep keeps treating a reconnecting Codex as busy). The row carries aproviderRetryingfailure fact: Codex's message as the person-facing detail, andretry: { error, status, cause }read fromcodexErrorInfoandadditionalDetails.causeis a new optional field; the shared failure-words table prints it on the line under the sentence. The host keeps no retry state.Client. The shared structured-message projection, which desktop, mobile and the conversation outline all read, draws only the latest row of each run. A run belongs to one agent: that agent's retry rows with none of its own other rows between them. Another agent's rows never end it, because each agent's rows are drawn apart (the conversation, or the subagent's own section). Rows that draw nothing (turn records, pending approvals) do not end a run; queued sends are drawn after the conversation, so they do not either. Every attempt stays in the journal; only the drawing skips the earlier ones.
Frame row names. Codex frame rows are now named
provider-frame:codex:<connection id>:<n>, where the connection id is the random id Orca already mints for each Codex acquisition. The acquire path mints it before creating the translator and passes it in.Why
A retry is not a failure yet, so a red row per attempt reads as repeated failure and buries the conversation. An earlier revision of this PR rewrote one host row in place per reconnect. That needed the host to decide, while frames streamed past, where a reconnect ends: in-memory run state per thread, run numbers in the row name, classifying every other frame just to end a run, and clearing all of it on session end and restore. Getting that boundary wrong either hides a later reconnect above newer content or splits one reconnect into several rows.
Writing each attempt as its own row and collapsing at draw time removes all of that. The journal stays append-only and keeps an honest record of every attempt. "Which rows belong together" is decided from what the screen actually draws next to each other, which is the question the person is asking. The rule lives in one shared module, so desktop and mobile cannot disagree. Runs are per agent because that is exactly how the transcript splits rows into the conversation and subagent sections; collapsing on the flat list showed two stacked rows whenever another agent wrote between two attempts.
The cause is printed on the row because it is usually the only line that says what actually went wrong, and Codex's own interface shows it under its progress message. It is stored in the row text by the host, like every other row's words, so older clients show it too.
Alternatives considered:
Differences from the common pattern
Known limits
Linked Issue
None.
Visual Proof
Live Electron, real Codex 0.157.0 on macOS, with the model endpoint forced to answer 502 through a per-process Codex flag in an isolated test profile (no Codex config file written). Both builds also carry #23552, which the isolated test setup needs. Before: main at 8b01590. After: this PR at 7af0a14. The red "codex · notification:thread/status/changed" row in both is the separate bug #23704 fixes.
Before (main): one red row per attempt, nine for one dropped stream.
After (this PR): one yellow row per reconnect with what failed on its second line; the WebSockets-to-HTTPS fallback warning sits between the two reconnects; the red final error ends the turn. Reloading the window draws the same thing from the journal, which still holds all nine attempts.
Testing
codex-provider-retry-row.test.ts: every attempt writes its own warning row under its own name, with theproviderRetryingfact and Codex's cause on the second line; a missing or repeated cause leaves one line; every attempt publishes; a new connection in the same session keeps earlier rows; the real acquire path names rows for the acquisition that received them (fails if the acquire path stops passing the id); a captured real Codex run gives warning rows, then the red row, then a failed turn.codex-provider-retry-idle-sweep.test.ts: retry frames ten minutes apart keep the conversation open past the thirty-minute idle window; once they stop, the sweep closes it.native-chat-provider-retry-runs.test.ts: a run draws only its latest row; the same agent's other rows split it; agents retrying at once each keep one row; another agent's row does not split a run, checked through the transcript's section split (one row in the conversation, one in the subagent's section); rows that draw nothing and queued sends do not split it; an older host's single row draws as is; nothing changes when nothing retried.NativeChatMessageList.provider-retry-runs.test.tsx(desktop) anduse-mobile-structured-agent-session-retry-rows.test.tsx(mobile): a three-attempt run renders as one row whose text carries the cause on its second line.agent-session-failure-words.test.tsandagent-session-failure.test.ts: the cause prints on the second line and survives a read, bounded like any provider detail.Each piece was ablated and turned at least one test red. Related suites pass: Codex journal translation, settlement, subagents, turn-ending replays, session adapter and lifecycle, generic frames, idle sweep, transcript projection, conversation outline, the desktop message list (including subagent sections) and mobile session tests.
tc:node,tc:weband mobile typecheck, oxlint, andcheck:code-quality:changedpass.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
SSH: rows are written on the execution host like every other journal row, and the collapse runs wherever the transcript is drawn. No path, shell or platform-specific behaviour. Folder workspaces and git worktrees behave the same.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)