Skip to content

fix(agents): handle stderr stream errors in tool_search_code child#101022

Closed
cxbAsDev wants to merge 1 commit into
openclaw:mainfrom
cxbAsDev:fix/tool-search-code-mode-stream-errors
Closed

fix(agents): handle stderr stream errors in tool_search_code child#101022
cxbAsDev wants to merge 1 commit into
openclaw:mainfrom
cxbAsDev:fix/tool-search-code-mode-stream-errors

Conversation

@cxbAsDev

@cxbAsDev cxbAsDev commented Jul 6, 2026 •
edited by steipete
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

tool_search_code owns a Node child whose stderr is piped for bounded diagnostics. A read-side stderr error had no listener, so Node could surface it as an uncaught exception and crash the gateway. The child also created a stdout pipe even though this worker sends logs, bridge calls, and results exclusively over IPC.

Why This Change Was Made

The spawn owner now rejects the run through its existing once-only settlement path when stderr fails. Unused stdout is set to ignore, removing both a needless pipe and its unhandled-error/backpressure surface. The focused regression asserts stderr rejection, child termination, and the exact stdio contract. A bulky one-off proof script and a redundant testing export were removed.

User Impact

An OS-level read failure on the tool_search_code diagnostic pipe now fails only that tool call instead of risking the Gateway process. Normal code-mode logs and results remain on IPC; stderr diagnostics remain bounded and included in exit errors.

Evidence

  • Sanitized direct AWS Crabbox (provider=aws, cbx_7d04459d7c6f, public network, no Tailscale, no instance profile, no hydration), exact reviewed head f2aab46b8dda8f19cb86b57e8bdc50311f996e40: pnpm test src/agents/tool-search.stream-errors.test.ts passed 1/1. Run
  • Fresh structured Codex autoreview on the final code bundle: clean, no accepted/actionable findings.
  • Exact-head GitHub CI is the broad remote gate. The observed check-lint failure is outside this PR in src/agents/model-fallback.test.ts; the same unused import exists on current origin/main.

@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts agents Agent runtime and tooling labels Jul 6, 2026
@cxbAsDev

cxbAsDev commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@clawsweeper clawsweeper Bot mentioned this pull request Jul 6, 2026
Merged
2 tasks
@clawsweeper

clawsweeper Bot commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 6, 2026, 10:46 PM ET / 02:46 UTC.

Summary
The PR changes tool_search_code child stdio from piping stdout to ignoring it, adds a stderr stream error handler, and adds a focused regression test for rejection, stdio shape, and child cleanup.

PR surface: Source +6, Tests +90. Total +96 across 2 files.

Reproducibility: yes. source inspection gives a high-confidence reproduction path: emit an error on the current-main stderr stream after runCodeModeChild attaches only a data listener. I did not run a live current-main gateway crash repro in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🦞 diamond lobster
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:

  • [P2] Rerun or wait for required CI after current main's unrelated app-sidebar conflict markers are cleared.

Risk before merge

  • [P1] Exact-head broad CI is currently red because current main contains unrelated merge conflict markers in ui/src/components/app-sidebar.ts; merge should wait for green required checks or an explicit maintainer override.

Maintainer options:

  1. Decide the mitigation before merge
    Land the narrow child-process hardening after required CI is green, keeping stream ownership and regression coverage in src/agents/tool-search.ts.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No ClawSweeper repair lane is needed; the patch has no supported code findings, and the remaining action is maintainer and CI validation.

Security
Cleared: No concrete security or supply-chain regression was found; the diff hardens an existing Node child-process path and adds a local regression test without new dependencies or workflow changes.

Review details

Best possible solution:

Land the narrow child-process hardening after required CI is green, keeping stream ownership and regression coverage in src/agents/tool-search.ts.

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

Yes, source inspection gives a high-confidence reproduction path: emit an error on the current-main stderr stream after runCodeModeChild attaches only a data listener. I did not run a live current-main gateway crash repro in this read-only review.

Is this the best way to solve the issue?

Yes, this is the best narrow fix: the spawn owner handles the stream failure through the existing one-shot settlement path, and ignoring stdout matches the child source's IPC-only log/result contract.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. For this non-visual process failure, the PR body/comments provide terminal proof context and the exact-head proof check passed; focused regression coverage shows the after-fix stderr rejection path.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): For this non-visual process failure, the PR body/comments provide terminal proof context and the exact-head proof check passed; focused regression coverage shows the after-fix stderr rejection path.

Label justifications:

  • P2: This is a normal-priority agent runtime availability hardening fix with limited blast radius and a focused regression path.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): For this non-visual process failure, the PR body/comments provide terminal proof context and the exact-head proof check passed; focused regression coverage shows the after-fix stderr rejection path.
  • proof: sufficient: Contributor real behavior proof is sufficient. For this non-visual process failure, the PR body/comments provide terminal proof context and the exact-head proof check passed; focused regression coverage shows the after-fix stderr rejection path.
Evidence reviewed

PR surface:

Source +6, Tests +90. Total +96 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 7 1 +6
Tests 1 90 0 +90
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 97 1 +96

What I checked:

Likely related people:

  • steipete: Introduced the generic code-mode runtime in src/agents/tool-search.ts, has multiple recent code-mode/tool-search commits, is assigned to this PR, and authored the latest hardening commit on the PR branch. (role: introduced behavior and recent area contributor; confidence: high; commits: 0844e771a8df, 4150c6ff8258, b41bd6d6d346; files: src/agents/tool-search.ts, src/agents/code-mode.ts)
  • hugenshen: Authored the merged adjacent fix that bounded tool_search_code stderr accumulation in the same file shortly before this PR. (role: recent adjacent area contributor; confidence: medium; commits: 9ef7fff57098; files: src/agents/tool-search.ts, src/agents/tool-search.test.ts)
  • fuller-stack-dev: Recent Tool Search directory-mode work changed the same module and nearby Tool Search behavior, making them a useful routing candidate for this surface. (role: adjacent feature contributor; confidence: medium; commits: 965fa05df38b; files: src/agents/tool-search.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-06T17:49:21.865Z sha 09fd255 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-06T19:16:33.373Z sha a087f40 :: needs maintainer review before merge. :: none

@cxbAsDev
cxbAsDev force-pushed the fix/tool-search-code-mode-stream-errors branch from e5bd55e to 09fd255 Compare July 6, 2026 17:38
@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 6, 2026
@cxbAsDev
cxbAsDev force-pushed the fix/tool-search-code-mode-stream-errors branch from 09fd255 to 2c33f06 Compare July 6, 2026 17:59
@cxbAsDev

cxbAsDev commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

补充了 proof 脚本的实际运行输出到 PR body。本地验证：

  • node scripts/run-vitest.mjs src/agents/tool-search.stream-errors.test.ts passed
  • OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs ... passed
  • node --import tsx scripts/proof/tool-search-code-mode-stream-errors.mts → PASS

@clawsweeper

clawsweeper Bot commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@cxbAsDev

cxbAsDev commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review\n\nBranch rebased onto latest upstream/main and resolved the testing-export conflict. Test and proof both pass locally; proof output is in the PR body.

@cxbAsDev
cxbAsDev force-pushed the fix/tool-search-code-mode-stream-errors branch from 2c33f06 to a087f40 Compare July 6, 2026 19:03
@clawsweeper clawsweeper Bot added 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 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 6, 2026
@steipete steipete self-assigned this Jul 7, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the scripts Repository scripts label Jul 7, 2026
@steipete
steipete force-pushed the fix/tool-search-code-mode-stream-errors branch from f2aab46 to b41bd6d Compare July 7, 2026 02:34
@clawsweeper clawsweeper Bot added 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. 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. labels Jul 7, 2026
@steipete
steipete force-pushed the fix/tool-search-code-mode-stream-errors branch 4 times, most recently from c2310b1 to b19f539 Compare July 7, 2026 03:02
@cxbAsDev
cxbAsDev force-pushed the fix/tool-search-code-mode-stream-errors branch from b19f539 to 6461f49 Compare July 7, 2026 03:04
@openclaw-barnacle openclaw-barnacle Bot added the scripts Repository scripts label Jul 7, 2026
@steipete steipete mentioned this pull request Jul 7, 2026
Merged
@steipete

steipete commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Landed the completed fix through stable maintainer PR #101295 as commit 118e5bd76223605bef1b7427b3f471c4a2bc34e8.

The maintainer version keeps the code-mode worker IPC-only, removes the unused stdout pipe, routes stderr stream errors through once-only settlement, terminates the unusable child, and adds the focused regression. It preserves the original contributor credit via Co-authored-by.

Proof:

  • Exact-head sanitized AWS run run_4f37de478300: focused test 1/1 passed.
  • Exact-head hosted CI run 28838695336: 71 relevant checks succeeded, none pending/failing.
  • Fresh final-head Codex autoreview: clean.

This PR is being closed as superseded because its fork branch reset during maintainer fixup and exact-head validation, dropping the completed hardening commit. A stable maintainer branch avoided an ongoing force-push loop. Thanks @cxbAsDev for identifying the stream-error bug. For future PRs, please keep Allow edits by maintainers enabled and avoid force-pushing the head while a maintainer fixup/landing pass is underway.

@steipete steipete closed this Jul 7, 2026
@wings1029 wings1029 mentioned this pull request Jul 7, 2026
Merged
7 tasks
@cxbAsDev
cxbAsDev deleted the fix/tool-search-code-mode-stream-errors branch July 15, 2026 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

agents Agent runtime and tooling 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. scripts Repository scripts size: S 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.

2 participants

@cxbAsDev @steipete