Skip to content

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

Closed
Alix-007 wants to merge 5 commits into
openclaw:mainfrom
Alix-007:alix/codex-supervisor-stdio-stream-errors
Closed

fix(codex-supervisor): handle stdout/stderr stream errors in stdio transport#101599
Alix-007 wants to merge 5 commits into
openclaw:mainfrom
Alix-007:alix/codex-supervisor-stdio-stream-errors

Conversation

@Alix-007

@Alix-007 Alix-007 commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • add a stdout error handler for the Codex Supervisor stdio JSON-RPC transport and route stdout stream failures through the existing connection failure and child shutdown path
  • add a stderr error handler as diagnostic-only stderr-tail capture, preventing an unhandled readable-stream error without tearing down an otherwise healthy JSON-RPC session
  • keep stdio close() idempotent with a closing guard so stream failures and cleanup cannot send duplicate shutdown signals to the child
  • cover both mocked initialization paths and real spawned node child stdio paths in focused regression tests
  • collision check: extensions/codex-supervisor/src/json-rpc-client.ts has open PR fix(codex-supervisor): disable app-server websocket compression #88550, but that diff is isolated to WebSocket compression; no open PR was found for the stdio stdout/stderr readable-stream error issue

Verification

  • TMPDIR=/media/vdc/0668001470/tmp/openclaw-task-tmp node scripts/run-vitest.mjs extensions/codex-supervisor/src/json-rpc-client.test.ts - passed; Test Files 1 passed (1), Tests 4 passed (4).
  • TMPDIR=/media/vdc/0668001470/tmp/openclaw-task-tmp node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.extensions.json extensions/codex-supervisor/src/json-rpc-client.ts extensions/codex-supervisor/src/json-rpc-client.test.ts - passed; exit 0.
  • TMPDIR=/media/vdc/0668001470/tmp/openclaw-task-tmp node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.extensions.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/codex-supervisor-stream-errors-repair.tsbuildinfo - passed; exit 0.
  • git diff --check - passed; exit 0.

Real behavior proof

Behavior addressed: Codex Supervisor stdio stdout readable-stream failures now fail the JSON-RPC connection and shut down the stdio child, while stderr readable-stream failures are handled as diagnostics instead of becoming unhandled errors.

Real environment tested: local Linux Node source checkout; the regression tests drive the real exported connectCodexAppServerEndpoint stdio transport with mocked streams and with a real spawned node child using pipe-backed stdout/stderr.

Exact steps or command run after this patch: TMPDIR=/media/vdc/0668001470/tmp/openclaw-task-tmp node scripts/run-vitest.mjs extensions/codex-supervisor/src/json-rpc-client.test.ts

Evidence after fix: one real-child regression destroys the parent-side stdout Readable and asserts the connection rejects with the stream error, sends exactly one SIGTERM, and observes child close { code: 143, signal: null }. A separate real-child regression destroys the parent-side stderr Readable and asserts the connection still initializes and no kill is sent until explicit close.

Observed result after fix: Test Files 1 passed (1) and Tests 4 passed (4).

What was not tested: a live Codex app-server binary; the proof uses a deterministic real Node stdio child through the same OpenClaw stdio transport. No OS-generated parent Readable error is claimed, because stdio pipe close/kill normally surfaces as EOF/close rather than a parent read-side error.

@openclaw-barnacle openclaw-barnacle Bot added extensions: codex-supervisor Extension: codex-supervisor size: S labels Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 9, 2026, 2:45 AM ET / 06:45 UTC.

Summary
The branch adds stdout/stderr stream-error handling and an idempotent close guard to the Codex Supervisor stdio JSON-RPC transport, plus focused mocked and real-child regression tests.

PR surface: Source +14, Tests +240. Total +254 across 2 files.

Reproducibility: yes. Current main clearly attaches stdout/stderr data listeners without owning stdout/stderr readable-stream errors in the Codex Supervisor stdio transport, and the PR adds focused tests that exercise those failure paths.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: extensions/codex-supervisor/src/json-rpc-client.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: canonical
Canonical: #101599
Summary: This PR is the current canonical Codex Supervisor stdio stream-error fix candidate; related items are older unmerged attempts or adjacent stream-error hardening on other surfaces.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • none.

Next step before merge

  • No ClawSweeper repair job is needed; the remaining action is normal maintainer review plus exact-head merge checks.

Security
Cleared: The diff changes Node stdio error handling and tests only; it adds no dependencies, workflows, package-resolution changes, permissions, secrets handling, or new command sources.

Review details

Best possible solution:

Land the focused stdio transport hardening after ordinary maintainer review and exact-head merge checks, preserving stdout as the JSON-RPC failure channel and stderr as diagnostics.

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

Yes. Current main clearly attaches stdout/stderr data listeners without owning stdout/stderr readable-stream errors in the Codex Supervisor stdio transport, and the PR adds focused tests that exercise those failure paths.

Is this the best way to solve the issue?

Yes. The fix is at the narrow stdio transport boundary and matches both upstream Codex stdout/stderr semantics and the merged sibling OpenClaw Codex app-server client behavior.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a focused bundled-plugin availability hardening fix with limited blast radius and no evidence of a widespread release-blocking incident.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix copied live output and describes real spawned Node child stdout/stderr stream failures through the exported stdio transport; it does not claim a live Codex binary run, but the proof is sufficient for this transport-level fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix copied live output and describes real spawned Node child stdout/stderr stream failures through the exported stdio transport; it does not claim a live Codex binary run, but the proof is sufficient for this transport-level fix.
Evidence reviewed

PR surface:

Source +14, Tests +240. Total +254 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 14 0 +14
Tests 1 240 0 +240
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 254 0 +254

What I checked:

  • Current-main stdio gap: Current main spawns the Codex app-server stdio proxy, attaches stdout/stderr data listeners, and handles stdin/process/close errors, but it has no stdout/stderr readable-stream error listeners on this transport. (extensions/codex-supervisor/src/json-rpc-client.ts:190, 20f355f2368c)
  • PR runtime behavior: The PR head adds a stdout error path that fails the connection and closes the child, keeps stderr errors diagnostic-only, and guards close() so repeated failure/cleanup paths do not send duplicate SIGTERM calls. (extensions/codex-supervisor/src/json-rpc-client.ts:198, df9e433d1546)
  • Regression coverage: The added tests cover mocked stdout-fatal and stderr-diagnostic initialization paths, plus real spawned Node child stdout shutdown and stderr diagnostic-only behavior through connectCodexAppServerEndpoint. (extensions/codex-supervisor/src/json-rpc-client.test.ts:101, df9e433d1546)
  • Upstream Codex stdio contract: Sibling Codex source reads newline-delimited JSON-RPC from stdin and serializes outbound JSON-RPC messages to stdout for stdio app-server transport; separate app-server setup configures tracing output on stderr. (../codex/codex-rs/app-server-transport/src/transport/stdio.rs:24, dc23c7bcc8fd)
  • Sibling OpenClaw precedent: The merged Codex app-server client uses the same semantic split: stdout/readline errors close the JSON-RPC client, while stderr stream errors are logged as diagnostics and do not tear down a healthy stdout connection. (extensions/codex/src/app-server/client.ts:145, 846195c6b001)
  • Live PR state and proof: Live PR metadata shows the current head df9e433 has proof:sufficient and ready-for-maintainer-look labels, successful relevant checks, and a PR body with copied live output for the focused regression run. (df9e433d1546)

Likely related people:

  • steipete: Authored and merged the Codex Supervisor plugin introduction and later touched the same JSON-RPC client area for session-list stabilization and documentation. (role: introduced current implementation and recent area contributor; confidence: high; commits: 9dd3bce549b4, 69c3b56bdecb, 8b477d2887ed; files: extensions/codex-supervisor/src/json-rpc-client.ts, extensions/codex-supervisor/src/supervisor.ts, extensions/codex-supervisor/openclaw.plugin.json)
  • RomneyDa: Recently touched the Codex Supervisor JSON-RPC client during the plugin record-guard consolidation that changed imports and guard usage in the same module. (role: recent adjacent refactor author; confidence: medium; commits: de34dfdbe8c4; files: extensions/codex-supervisor/src/json-rpc-client.ts, extensions/codex-supervisor/src/supervisor.ts)
  • mushuiyu886: Authored the merged Codex app-server stdio stream-error fix that established the closest current-main stdout-fatal and stderr-diagnostic behavior precedent. (role: adjacent implementation contributor; confidence: medium; commits: 846195c6b001; files: extensions/codex/src/app-server/client.ts, extensions/codex/src/app-server/client.test.ts)
  • vincentkoc: Merged the adjacent Codex app-server stream-error fix and is assigned on this PR in live GitHub metadata, making them a likely routing owner for the final maintainer look. (role: adjacent merger and current reviewer signal; confidence: medium; commits: 846195c6b001; files: extensions/codex/src/app-server/client.ts, 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 (6 earlier review cycles)
  • reviewed 2026-07-07T11:48:08.404Z sha 89c6cec :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-08T00:28:23.126Z sha 1333d55 :: needs changes before merge. :: [P2] Fix the real-child test helper types
  • reviewed 2026-07-08T00:41:02.948Z sha 1333d55 :: needs changes before merge. :: [P2] Fix the real-child test helper types
  • reviewed 2026-07-08T04:21:48.286Z sha 7351faa :: needs changes before merge. :: [P1] Keep stderr errors diagnostic-only
  • reviewed 2026-07-08T05:32:56.799Z sha bd9acd0 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-08T05:40:29.149Z sha bd9acd0 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 7, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 8, 2026
@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jul 8, 2026
Closed
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. label Jul 8, 2026
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 8, 2026
@vincentkoc vincentkoc self-assigned this Jul 8, 2026
This was referenced Jul 8, 2026
Merged
Merged
@wangmiao0668000666

wangmiao0668000666 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

@Alix-007 Hi! Following the same stream-error campaign pattern, I just opened two parallel PRs:

The fix shape mirrors your matrix deps #101597 (kill child on stream error + idempotent settle) and your realtime.ts pattern in #101596. No collision: I verified there's no open PR for either of those two files. Let me know if you'd like to coordinate or if you're already planning to cover those surfaces.

@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jul 8, 2026
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jul 9, 2026
Closed
@Alix-007

Alix-007 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Closing this as superseded by current main rather than resolving the conflict by restoring the deleted codex-supervisor plugin.

Conflict proof:

  • Fresh local merge of upstream/main into the fork head b77d2f3ca8 fails with a single modify/delete conflict: extensions/codex-supervisor/src/json-rpc-client.ts was deleted in upstream/main and modified by this PR.
  • Current main removed the old extensions/codex-supervisor plugin in f94a7dc183 / feat(codex): supervise native Codex sessions #104045 and moved native Codex supervision into the bundled codex plugin (extensions/codex/src/supervision-tools.ts).
  • The replacement app-server client already has the behavior this PR was adding: stdout stream errors close/reject the JSON-RPC transport, while stderr stream errors are treated as diagnostics and do not tear down an otherwise healthy stdout JSON-RPC connection (extensions/codex/src/app-server/client.ts).
  • The replacement behavior is covered by focused tests in extensions/codex/src/app-server/client.test.ts: handles stdout stream errors without crashing the process and keeps RPC requests usable after stderr stream errors.
  • I also checked upstream Codex stdio source: start_stdio_connection sends protocol messages over stdout and treats stdout write failure as connection failure, while stderr remains diagnostic/logging side-channel behavior.

Restoring extensions/codex-supervisor/src/json-rpc-client.ts would reintroduce a retired owner path and duplicate the new extensions/codex supervision architecture, so the safe resolution is to close this stale PR.

@Alix-007 Alix-007 closed this Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

extensions: codex-supervisor Extension: codex-supervisor P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Alix-007 @wangmiao0668000666 @vincentkoc