Repository navigation
fix(codex): log a late reply to a timed-out request instead of showing it in the chat - #23893
Conversation
Status: ready for reviewHead What it fixes: a Codex reply that arrived after Orca had already timed out its request used to show in the chat as a raw Review: one focused loop, CLEAN (no P0, P1 or P2 findings). The loop confirmed three things:
Both of the loop's small notes (P3) are fixed in Validation:
Not done:
Not merged. It's ready for a maintainer's decision. |
|
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 (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe connection now delegates request timeouts to the dispatcher. The dispatcher records the method for each timed-out request, up to 64 entries, and logs responses that have no pending request. It reports whether a response matches a remembered timeout or has no waiting request. Tests cover late replies, unmatched replies, unclassified frames, and mock cleanup. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The timeout and late-reply changes appear ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Late replies no longer appear as raw chat frames, but an error message supplied by Codex can now enter application logs. The privacy implications depend on how those logs are handled. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const error = isAppServerRecord(message.error) ? message.error.message : undefined | ||
| console.warn( | ||
| timedOutMethod | ||
| ? `[codex-app-server] late reply to ${timedOutMethod} after timeout (id ${message.id})` | ||
| : `[codex-app-server] reply with no waiting request (id ${message.id})`, | ||
| ...(typeof error === 'string' ? [error] : []) | ||
| ) | ||
| return |
There was a problem hiding this comment.
When a turn/start request times out and Codex later replies with an error, this branch writes that error only to the log. The timeout gives the user no provider explanation, while the previous path showed the late error in the chat. The user therefore loses the reason Codex gave for the failure. Please keep error-bearing late replies visible while suppressing routine late results.
Knowledge Base Used: Restore Native Chat Session Admission
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const error = isAppServerRecord(message.error) ? message.error.message : undefined | ||
| console.warn( | ||
| timedOutMethod | ||
| ? `[codex-app-server] late reply to ${timedOutMethod} after timeout (id ${message.id})` | ||
| : `[codex-app-server] reply with no waiting request (id ${message.id})`, | ||
| ...(typeof error === 'string' ? [error] : []) | ||
| ) |
There was a problem hiding this comment.
Every unmatched reply now produces a warning, including its full error message. The reader accepts lines of unlimited length, and the 64-entry limit only caps remembered request methods—not warnings. Repeated replies or a very large error can therefore produce excessive host log output. Consider bounding or coalescing these diagnostics.
![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
A Codex app-server reply whose id has no pending request is no longer surfaced to the chat; it is logged at the point that pairs replies with requests.
- Timed-out replies are logged, not rendered.
codex-app-server-connection.tsnow callsdispatcher.timeOutPending(id)on the per-request deadline; the dispatcher remembers the request's method (bounded at 64 per connection) and, when the reply finally arrives with no waiter,console.warnslate reply to <method> after timeout (id N)(orreply with no waiting request (id N), with the reply's error message) instead of callingonUnhandledFrame('response:unmatched', …). Nothing reaches the journal or chat. - Coverage moved, not lost. The new connection test drives a real timeout followed by a late result and a late error reply, asserting no frame is forwarded and both log lines land; the existing unclassified-frame test now uses
id: nullto keep pinning theframe:unclassifiedpath. The test fails onmain, where both replies surfaced asresponse:unmatched. - Scope is contained. Notifications, server requests, non-numeric-id frames, invalid/oversized frames, and error frames with no id all keep their previous behavior. The remembered-timeout map is per connection and capped, so it dies with the connection and cannot grow.
I verified the new test passes (32/32), check:code-quality:changed is clean, and no producer or consumer of the response:unmatched string remains anywhere in the repo. The console.warn prefix follows the established [codex-…] convention in this module.
One deliberate tradeoff is worth noting but not blocking: an unmatched reply carrying error used to be classified error-surface and shown as a visible error row. It is now only logged. Since no pending request remains to attribute it to (it timed out or was never sent), and the PR description documents this, the choice is reasonable.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
1 participant
ELI5
Sometimes Orca asks Codex for something, gives up waiting, and tells you the request failed. If Codex's answer turned up after that, Orca had nowhere to put it, so it printed it into the chat as a raw line: "codex response:unmatched" plus a block of JSON. You no longer see that raw line for a late Codex reply: it goes to the app log, and the chat only shows what matters to you.
What Changed
The problem. During Stop QA on a Codex chat, a Stop request (
turn/interrupt) timed out, and Orca showed its usual timeout line. Codex's reply came later ({"id":7,"result":{}}), and the chat then showed a second, meaningless line:codex response:unmatchedwith that JSON. This happened because a reply whose request was no longer being waited on was handed to the generic "frame Orca doesn't understand" path. That path writes a visible status line into the chat.Scope. Codex chats only, local, WSL and SSH hosts alike. This affects only replies to a request Orca already stopped waiting for, or to an id it never sent. Codex notifications, Codex's requests to Orca, unreadable lines, and error frames with no id still take the same path as before.
Before
codex response:unmatchedplus raw JSON in the chat, just after the timeout line that had already told you the request failed.After
codex response:unmatchedline for a late Codex reply. The chat shows only the timeout line. The late reply is logged as[codex-app-server] late reply to turn/interrupt after timeout (id 7). Any other reply with no request waiting for it (for example one arriving after the connection already failed its requests) is logged asreply with no waiting request (id N), with the error message if the reply carries one.Mechanism
codex-app-server-record-dispatch.ts: a reply with no waiting request is logged withconsole.warninstead of being passed toonUnhandledFrame. The dispatcher, which pairs replies with requests, is now the only place that decides about them. Nothing reaches the chat history.timeOutPending. It drops the waiter as before and remembers the request's method, capped at the 64 most recent per connection, so the log can name what the late reply was for.Why
A reply only means something to the request that asked for it. Once that request has given up and reported its own outcome, nobody is left to act on the reply, so it is a transport diagnostic rather than part of the conversation. Logging it at the point that pairs replies with requests keeps one owner for the decision.
Alternatives considered:
Linked Issue
No issue. Found during Codex Stop QA for #23026.
Visual Proof
N/A: the change removes a status line that appears only when Codex answers after Orca's own timeout. That can't be forced from the app without a rig that delays Codex's reply. It is covered by the connection test below, which drives a real timeout followed by a late reply.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
codex-app-server-connection.test.ts:turn/interrupttimes out, then a late{id, result: {}}and an error reply with no waiting request arrive. The test asserts nothing is forwarded to the chat and checks both log lines. It fails onmain, where the reply is forwarded asresponse:unmatched.id: null, such as a parse error. That frame still reaches the chat as before.Removing each change fails its test: putting the forward back fails the "nothing forwarded" check; dropping the timeout record fails the "late reply to turn/interrupt" log check.
pnpm tc:node, oxlint,check:code-quality:changed,check:react-doctor:changedand the anti-slop audit pass.AI Disclosure
Review
One focused review loop: CLEAN. It checked that nothing else reads the removed frame kind, that the remembered timeouts can't collide (ids only grow per connection) and go away with the connection, and that frames the user needs still reach the chat. Two small fixes from its notes: the log no longer calls a reply whose request Orca did send "unknown", and the test's log spy is always restored.
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
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Author: @BrennanKB5