fix(codex): handle stdout/stderr stream errors in sandbox exec processes by lsr911 · Pull Request #102386 · openclaw/openclaw

lsr911 · 2026-07-09T03:25:19Z

What Problem This Solves

The sandbox exec process spawner (processes.ts) registers child.stdout.on("data") and child.stderr.on("data") handlers but does not register error handlers on the stdio streams. If a pipe errors (e.g. broken pipe, EPIPE), the error is unhandled — only process-level errors (child.once("error")) are caught.

Why This Change Was Made

Adds child.stdout.on("error", onStreamError) and child.stderr.on("error", onStreamError) handlers that set managed.failure and emit process closed, matching the existing child.once("error") behavior.

Same pattern as #101505 (mushuiyu886: codex app-server stdio stream errors) and 5+ other merged PRs.

Files Changed

File	Change
`extensions/codex/src/app-server/sandbox-exec-server/processes.ts`	+6 lines: stream error handlers

🤖 Generated with Claude Code

Co-Authored-By: Claude [email protected]

The sandbox exec process spawns child processes with stdout/stderr data handlers but does not register error handlers on the streams. If a stdio pipe errors (e.g. broken pipe), the managed process state is not updated and the error is unhandled. Add stream-level error handlers that set managed.failure and emit process closed, matching the existing child.once('error') behavior. Co-Authored-By: Claude <[email protected]>

clawsweeper · 2026-07-09T03:29:13Z

Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 2:01 AM ET / 06:01 UTC.

Summary
The PR adds idempotent stdout and stderr error listeners to the Codex sandbox exec-server process spawner so stream errors mark the managed process failed and closed.

PR surface: Source +7. Total +7 across 1 file.

Reproducibility: yes. source-reproducible: current main lacks stdout/stderr stream error listeners in the sandbox exec process spawner, and upstream Codex consumes process/read.failure as the remote process failure channel. I did not run a live stream-error reproduction in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

[P1] Add redacted terminal output, live logs, or a validation transcript from a real Codex sandbox exec stdout/stderr stream-error run.
[P1] Add focused regression coverage for stdout and stderr stream errors if feasible, then update the PR body so ClawSweeper can re-review automatically or ask a maintainer for @clawsweeper re-review.

Proof guidance:

[P1] Needs real behavior proof before merge: The PR still needs redacted after-fix proof such as a live run, terminal output, logs, or validation transcript showing the stdout/stderr stream-error path; private details like IPs, keys, phone numbers, and non-public endpoints should be redacted. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

[P1] No after-fix real behavior proof is present, so the exact sandbox exec stdout/stderr stream-error path has not been demonstrated outside source review.
[P1] The branch is behind current main, so final merge review should use refreshed exact-head CI and proof if the branch is updated.

Maintainer options:

Decide the mitigation before merge
Merge the focused handler after the contributor adds redacted real behavior proof, with focused stdout/stderr stream-error regression coverage preferred if feasible.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

[P1] The remaining blocker is contributor-supplied real behavior proof, not a narrow code defect for ClawSweeper to repair.

Security
Cleared: The diff only adds child stdout/stderr stream listeners in an existing Codex sandbox exec process path and does not change dependencies, secrets, auth, config, CI, package execution, or release policy.

Review details

Best possible solution:

Merge the focused handler after the contributor adds redacted real behavior proof, with focused stdout/stderr stream-error regression coverage preferred if feasible.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible: current main lacks stdout/stderr stream error listeners in the sandbox exec process spawner, and upstream Codex consumes process/read.failure as the remote process failure channel. I did not run a live stream-error reproduction in this read-only review.

Is this the best way to solve the issue?

Yes, the patch is the narrow owner-boundary fix because OpenClaw owns the child process and both stdout and stderr are sandbox exec process output streams. The remaining gap is proof, not a better implementation location.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against a3f00d32c914.

Label changes

Label justifications:

P2: This is a focused Codex sandbox exec hardening fix with limited blast radius and no evidence of a current release-blocking incident.
rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR still needs redacted after-fix proof such as a live run, terminal output, logs, or validation transcript showing the stdout/stderr stream-error path; private details like IPs, keys, phone numbers, and non-public endpoints should be redacted. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence reviewed

PR surface:

Source +7. Total +7 across 1 file.

View PR surface stats

Area	Files	Added	Net
Source	1	7	+7
Tests	0	0	0
Docs	0	0	0
Config	0	0	0
Generated	0	0	0
Other	0	0	0
Total	1	7	+7

What I checked:

Current main stream-error gap: Current main registers stdout/stderr data listeners and child process error/close handlers in the sandbox exec process spawner, but no stdout/stderr stream error listeners. (extensions/codex/src/app-server/sandbox-exec-server/processes.ts:118, a3f00d32c914)
PR head patch shape: At the PR head, one shared stream error handler preserves an existing failure, stores the first stream error message, emits process closed, and is attached to both stdout and stderr. (extensions/codex/src/app-server/sandbox-exec-server/processes.ts:122, 4d792f1e95ef)
Failure return path: readProcess returns managed.failure in the process/read response, so the new stream handler's failure message is visible to the Codex remote exec consumer. (extensions/codex/src/app-server/sandbox-exec-server/processes.ts:264, a3f00d32c914)
Codex protocol contract: Upstream Codex ReadResponse includes chunks, exited, closed, and optional failure fields for process/read responses. (../codex/codex-rs/exec-server-protocol/src/protocol.rs:184, dc23c7bcc8fd)
Codex failure consumption: Upstream Codex turns a process/read failure into a failed unified exec process state and cancels output consumption. (../codex/codex-rs/core/src/unified_exec/process.rs:501, dc23c7bcc8fd)
Prior review finding resolved: The second commit adds the guard that prevents a later stdout/stderr stream error from overwriting the first recorded stream failure. (extensions/codex/src/app-server/sandbox-exec-server/processes.ts:123, 4d792f1e95ef)

Likely related people:

RomneyDa: PR metadata, git blame, and git log show the sandbox exec-server process lifecycle and affected file came from the gateway client-contract consolidation work. (role: introduced current behavior; confidence: high; commits: 8fa83d27420b; files: extensions/codex/src/app-server/sandbox-exec-server/processes.ts, extensions/codex/src/app-server/sandbox-exec-server.ts, extensions/codex/src/app-server/sandbox-exec-server.test.ts)
vincentkoc: They merged the closely related Codex app-server stdio stream-error fix that established the adjacent stdout/stderr handling precedent. (role: recent adjacent owner; confidence: medium; commits: 846195c6b001; files: extensions/codex/src/app-server/client.ts, extensions/codex/src/app-server/client.test.ts)
mushuiyu886: They authored the merged adjacent Codex app-server stdio stream-error fix with tests and proof, which is useful comparison evidence for this PR family. (role: adjacent implementation contributor; confidence: medium; commits: 846195c6b001; files: extensions/codex/src/app-server/client.ts, extensions/codex/src/app-server/client.test.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Review history (2 earlier review cycles)

reviewed 2026-07-09T03:34:55.594Z sha 694ca1b :: needs real behavior proof before merge. :: [P2] Preserve the first stream failure
reviewed 2026-07-09T05:55:49.917Z sha 4d792f1 :: needs real behavior proof before merge. :: none

…esses If both stdout and stderr emit errors, the second error would overwrite the first failure message. Add an early return guard so only the first stream error is preserved, matching the expected behavior where Codex consumes process/read.failure. Co-Authored-By: Claude <[email protected]>

lsr911 · 2026-07-09T05:50:58Z

@clawsweeper re-review

clawsweeper · 2026-07-09T05:51:00Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

lsr911 · 2026-07-09T08:07:24Z

Closing: re-review pending without resolution. The sandbox exec stream error approach needs integration-test proof that is blocked by test infrastructure complexity.

openclaw-barnacle Bot added extensions: codex size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 9, 2026

lsr911 mentioned this pull request Jul 9, 2026

fix(codex-supervisor): handle stdout/stderr stream errors in JSON-RPC client #102387

Closed

lsr911 closed this Jul 9, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(codex): handle stdout/stderr stream errors in sandbox exec processes#102386

fix(codex): handle stdout/stderr stream errors in sandbox exec processes#102386
lsr911 wants to merge 2 commits into
openclaw:mainfrom
lsr911:fix/codex-sandbox-stream-errors

lsr911 commented Jul 9, 2026

Uh oh!

clawsweeper Bot commented Jul 9, 2026 •

edited

Loading

Uh oh!

lsr911 commented Jul 9, 2026

Uh oh!

clawsweeper Bot commented Jul 9, 2026 •

edited

Loading

Uh oh!

lsr911 commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

lsr911 commented Jul 9, 2026

What Problem This Solves

Why This Change Was Made

Files Changed

Uh oh!

clawsweeper Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsr911 commented Jul 9, 2026

Uh oh!

clawsweeper Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsr911 commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented Jul 9, 2026 •

edited

Loading

clawsweeper Bot commented Jul 9, 2026 •

edited

Loading