fix(codex-supervisor): suppress unhandled stdout/stderr stream errors in JsonRpcClient by lsr911 · Pull Request #100785 · openclaw/openclaw

lsr911 · 2026-07-06T09:17:24Z

What Problem This Solves

JsonRpcClient.start() attaches data listeners to this.proc.stdout and this.proc.stderr, but does not attach error listeners. If either stream emits an error event, the unhandled error can crash the OpenClaw process.

Why This Change Was Made

Add no-op error handlers to both stdout and stderr streams:

this.proc.stdout.on("error", () => {});
this.proc.stderr.on("error", () => {});

Stream read errors are treated as non-fatal because the process error/close handlers already report the real outcome.

Evidence

Vitest regression test

The branch includes json-rpc-client.stream-errors.test.ts which creates real EventEmitter-backed stdout/stderr streams, verifies error listeners are registered, and exercises connectCodexAppServerEndpoint through the actual production code path.

CI

Latest commit fixes the check-test-types failure flagged by ClawSweeper (ChildProcess mock cast through unknown). All other CI checks are passing.

Issue

Fixes #98584

🤖 Generated with Claude Code

clawsweeper · 2026-07-06T09:59:13Z

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

Summary
Adds no-op stdout/stderr error listeners to codex-supervisor's stdio JSON-RPC client plus a mocked Vitest regression test.

PR surface: Source +2, Tests +59. Total +61 across 2 files.

Reproducibility: yes. by source inspection: current main attaches stdout/stderr data listeners in the stdio JSON-RPC transport without error listeners, so an emitted stream error can be unhandled on that path. I did not run a live app-server crash reproduction in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup 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 real behavior proof for the codex-supervisor stdio path, such as terminal output or logs from an after-fix run.
Correct or remove the unrelated Fixes reference so the PR body points to the actual bug context.

Proof guidance:

[P1] Needs real behavior proof before merge: Proof is mock-only: add redacted terminal/live output, logs, or a reproducible transcript from a real codex-supervisor stdio setup after the fix, redacting private details; updating the PR body should trigger re-review or a maintainer can comment @clawsweeper re-review. 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] Real behavior proof remains mock-only: the PR body has mocked Vitest coverage and CI, but no redacted live run/log/terminal transcript from an actual codex-supervisor stdio setup.
[P1] The PR body says it fixes fix(transcripts): close stream on parse error in readUtterancesFromDir (fixes #98467) #98584, which is a closed unrelated transcript stream-cleanup PR and should be corrected before merge.

Maintainer options:

Decide the mitigation before merge
Land the narrow stdio transport guard after the contributor adds redacted real behavior proof and fixes the unrelated PR reference in the body.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

[P1] Contributor action is needed for real behavior proof and PR-body cleanup; there is no remaining automated code repair to queue.

Security
Cleared: The diff only adds runtime stream listeners and a test; it does not touch dependencies, workflows, lockfiles, permissions, secrets, or package execution surfaces.

Review details

Best possible solution:

Land the narrow stdio transport guard after the contributor adds redacted real behavior proof and fixes the unrelated PR reference in the body.

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

Yes, by source inspection: current main attaches stdout/stderr data listeners in the stdio JSON-RPC transport without error listeners, so an emitted stream error can be unhandled on that path. I did not run a live app-server crash reproduction in this read-only review.

Is this the best way to solve the issue?

Yes for the code shape: adding listeners at the stdio transport boundary is the narrowest maintainable fix and matches sibling OpenClaw process helpers. Merge readiness is still limited by missing real behavior proof and the unrelated body reference.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

P2: This is a focused availability bug fix for one bundled plugin transport path with limited blast radius and no evidence of a widespread outage.
rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish 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: Proof is mock-only: add redacted terminal/live output, logs, or a reproducible transcript from a real codex-supervisor stdio setup after the fix, redacting private details; updating the PR body should trigger re-review or a maintainer can comment @clawsweeper re-review. 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 +2, Tests +59. Total +61 across 2 files.

View PR surface stats

Area	Files	Added	Net
Source	1	2	+2
Tests	1	59	+59
Docs	0	0	0
Config	0	0	0
Generated	0	0	0
Other	0	0	0
Total	2	61	+61

What I checked:

Current main behavior: Current main's stdio Codex JSON-RPC transport sets stdout/stderr encodings and data listeners but has no stdout/stderr error listeners before this PR. (extensions/codex-supervisor/src/json-rpc-client.ts:190, 09977a9328bc)
PR runtime change: The PR adds no-op error listeners immediately after the stdout/stderr data listeners in the stdio transport. (extensions/codex-supervisor/src/json-rpc-client.ts:193, ad191ed40fe0)
PR test path: The added test mocks the spawned child process, calls connectCodexAppServerEndpoint, and asserts stdout/stderr error listeners were registered; this is regression coverage, not real behavior proof. (extensions/codex-supervisor/src/json-rpc-client.stream-errors.test.ts:20, ad191ed40fe0)
Sibling OpenClaw invariant: The process supervisor child adapter already installs no-op stdout/stderr error listeners so output pipe read errors do not throw out of band while close/exit handling remains authoritative. (src/process/supervisor/adapters/child.ts:127, 09977a9328bc)
Codex stdio contract: The sibling Codex source shows app-server supports --listen stdio:// and its stdio transport reads stdin lines and writes JSON messages to stdout. (../codex/codex-rs/app-server-transport/src/transport/stdio.rs:24, cca16a108782)
Codex CLI contract: The sibling Codex app-server CLI defines --listen with stdio:// as a supported/default transport URL. (../codex/codex-rs/app-server/src/main.rs:25, cca16a108782)

Likely related people:

vincentkoc: The codex-supervisor JSON-RPC client appears in the latest release commit authored by Vincent Koc, and the contributor explicitly referenced vincentkoc's maintainer repair pattern in the PR discussion. (role: feature owner and shipped-area contributor; confidence: medium; commits: e085fa1a3ffd; files: extensions/codex-supervisor/src/json-rpc-client.ts, extensions/codex-supervisor/src/supervisor.ts, extensions/codex-supervisor/src/types.ts)
obviyus: Current-main blame/log for the codex-supervisor JSON-RPC client points to Ayaan Zaidi's recent commit that reintroduced the file on main, making this a useful current routing signal even though the commit subject is adjacent. (role: recent area contributor; confidence: medium; commits: dc5d4138f9ae; files: extensions/codex-supervisor/src/json-rpc-client.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 (3 earlier review cycles)

reviewed 2026-07-06T10:03:01.455Z sha d05ab2e :: needs real behavior proof before merge. :: [P2] Run the proof through the production client | [P2] Fix the proof script catch variable lint
reviewed 2026-07-06T11:51:55.812Z sha 46eac4f :: needs real behavior proof before merge. :: [P2] Fix the mock child process cast
reviewed 2026-07-07T03:23:20.358Z sha ad191ed :: needs real behavior proof before merge. :: none

…rrors in JsonRpcClient Add no-op error listeners on child stdout/stderr in StdioCodexJsonRpcConnection to prevent unhandled EventEmitter errors from crashing the process. This matches the pattern used in sibling process helpers (child.ts, child-process.ts). Replace standalone proof script with Vitest regression test that mocks spawn and verifies error listeners are registered on the production code path. Co-Authored-By: Claude <[email protected]>

lsr911 · 2026-07-06T11:04:50Z

Refactored following vincentkoc's maintainer repair pattern:

Removed standalone proof scripts
Kept the same production fix
Added Vitest regression tests that exercise the actual production code path
Fixed all lint issues

@clawsweeper re-review

clawsweeper · 2026-07-06T11:04:52Z

🦞🧹
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.

…atibility ClawSweeper review flagged the direct EventEmitter → ChildProcess cast at json-rpc-client.stream-errors.test.ts:37. Cast through unknown so check-test-types passes without weakening the test assertions. Co-Authored-By: Claude <[email protected]>

lsr911 · 2026-07-07T05:56:16Z

@clawsweeper re-review

clawsweeper · 2026-07-07T05:56:19Z

🦞🧹
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.

vincentkoc · 2026-07-07T10:43:11Z

Closing this as unsafe in its current shape.

stdout is the Codex app-server JSON-RPC transport. Silently swallowing a stdout stream error can leave pending and future RPC requests hanging instead of closing them with the transport failure. The added test only verifies listener registration; it never emits a stream error or proves request behavior. The body also points Fixes at unrelated PR #98584.

The transport semantics proven and landed in #101505 are the right model: stdout failure closes RPC state; stderr is diagnostic-only.

openclaw-barnacle Bot added scripts Repository scripts extensions: codex-supervisor Extension: codex-supervisor size: S labels Jul 6, 2026

lsr911 force-pushed the fix/codex-supervisor-stream-errors branch from d05ab2e to 46eac4f Compare July 6, 2026 11:04

openclaw-barnacle Bot removed the scripts Repository scripts label Jul 6, 2026

wings1029 mentioned this pull request Jul 7, 2026

fix(agent-core): handle stdout/stderr stream errors in harness exec #101370

Merged

7 tasks

mushuiyu886 mentioned this pull request Jul 7, 2026

fix(codex): handle app-server stdio stream errors #101505

Merged

vincentkoc self-assigned this Jul 7, 2026

vincentkoc closed this Jul 7, 2026

This was referenced Jul 8, 2026

fix(codex-supervisor): handle stdout/stderr stream errors in stdio transport #101599

Closed

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(codex-supervisor): suppress unhandled stdout/stderr stream errors in JsonRpcClient#100785

fix(codex-supervisor): suppress unhandled stdout/stderr stream errors in JsonRpcClient#100785
lsr911 wants to merge 2 commits into
openclaw:mainfrom
lsr911:fix/codex-supervisor-stream-errors

lsr911 commented Jul 6, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jul 6, 2026 •

edited

Loading

Uh oh!

lsr911 commented Jul 6, 2026

Uh oh!

clawsweeper Bot commented Jul 6, 2026 •

edited

Loading

Uh oh!

lsr911 commented Jul 7, 2026

Uh oh!

clawsweeper Bot commented Jul 7, 2026 •

edited

Loading

Uh oh!

vincentkoc commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

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

Uh oh!

What Problem This Solves

Why This Change Was Made

Evidence

Vitest regression test

CI

Issue

Uh oh!

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

Uh oh!

Uh oh!

lsr911 commented Jul 6, 2026

Uh oh!

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

Uh oh!

Uh oh!

lsr911 commented Jul 7, 2026

Uh oh!

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

Uh oh!

Uh oh!

vincentkoc commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lsr911 commented Jul 6, 2026 •

edited

Loading

clawsweeper Bot commented Jul 6, 2026 •

edited

Loading

clawsweeper Bot commented Jul 6, 2026 •

edited

Loading

clawsweeper Bot commented Jul 7, 2026 •

edited

Loading