fix(codex): handle app-server stdio stream errors#101505
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 7, 2026, 5:07 AM ET / 09:07 UTC. Summary PR surface: Source +9, Tests +26. Total +35 across 2 files. Reproducibility: yes. from source and PR proof: current main owns stdin and child process errors but not stdout/readline/stderr error events, and the PR body includes red-test and real app-server stdio output showing the failure boundary. I did not rerun the tests locally in this read-only review. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused owner-boundary fix after normal exact-head checks, keeping stream failures on the centralized CodexAppServerClient closeWithError path. Do we have a high-confidence way to reproduce the issue? Yes, from source and PR proof: current main owns stdin and child process errors but not stdout/readline/stderr error events, and the PR body includes red-test and real app-server stdio output showing the failure boundary. I did not rerun the tests locally in this read-only review. Is this the best way to solve the issue? Yes. Routing read and diagnostic stream errors through CodexAppServerClient.closeWithError is the narrow owner-boundary fix; a no-op listener would avoid throws but would lose pending-request rejection and close-reason preservation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 13c1d3c4083a. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +9, Tests +26. Total +35 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
proof: sufficient
Co-authored-by: Vincent Koc <[email protected]>
ec64376 to
7c95f14
Compare
vincentkoc
changed the title
|
Maintainer repair is pushed at The submitted stderr behavior was corrected. Codex stdout is the JSON-RPC transport, so readline/stdout errors close the client and reject pending requests. Stderr is the upstream tracing channel, so its errors are logged without killing healthy RPC. The stale fork lineage was also replaced with one contributor-authored commit on current
Proof completed:
Direct upstream contract checked:
The exact pushed head is running hosted CI. |
|
Merged via squash.
|
Co-authored-by: Vincent Koc <[email protected]>
Co-authored-by: Vincent Koc <[email protected]>
…cution
Add error handlers for child.stdout and child.stderr streams in
runProcess() to prevent unhandled 'error' events from crashing the
sandbox exec server.
stdout stream errors are treated as fatal (consistent with the existing
child.on('error') handler). stderr stream errors are logged as warnings
(stderr is diagnostic; losing it must not kill an otherwise healthy
process, matching the pattern in openclaw#101505).
Ref: openclaw#101505 (fix(codex): handle app-server stdio stream errors)
2 participants
What Problem This Solves
Prevents asynchronous Codex app-server stdio stream errors from becoming unhandled Node errors that can terminate the gateway.
Why This Change Was Made
CodexAppServerClientalready owns child-process, exit, and stdin pipe failures. It did not own errors from the readline interface, stdout stream, or stderr stream.The submitted patch treated all three streams as terminal. Maintainer review corrected that boundary:
This matches the upstream Codex transport contract:
codex-rs/app-server-transport/src/transport/stdio.rscarries newline-delimited JSON-RPC on stdout, whilecodex-rs/app-server/src/lib.rsconfigures tracing output on stderr.User Impact
A broken stdout RPC pipe now fails pending Codex requests cleanly instead of risking an unhandled process error. Losing only stderr diagnostics no longer aborts healthy Codex turns.
Evidence
Focused local proof:
Testbox-through-Crabbox:
Fresh autoreview completed with no accepted or actionable findings. The test proves that stdout failure rejects pending and later requests, while stderr failure is logged and two consecutive RPC requests still resolve.