fix(acp): handle terminal stdout and stderr stream errors - #507
Open
SebTardif wants to merge 2 commits into
Open
fix(acp): handle terminal stdout and stderr stream errors#507SebTardif wants to merge 2 commits into
SebTardif wants to merge 2 commits into
Conversation
ACP terminal/create attached data listeners on the child pipes with no error listeners. A broken pipe after the child exited could kill the ACP process. Ignore pipe-death codes on those streams so wait/release can still finish. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
CI type-aware oxlint rejected the emit() argument assertions. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
clawsweeper
Bot
added
P2
Contributor
|
Codex review: needs maintainer review before merge. Reviewed August 23, 2026, 1:59 PM ET / 17:59 UTC. ClawSweeper reviewWhat this changesThe PR adds error listeners to ACP-created child-process stdout and stderr streams and tests that pipe-death errors do not prevent terminal cleanup. Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readinessThis remains a useful, focused ACP reliability fix: current main and v0.13.1 still lack child stdout/stderr error listeners. Manual source review found no actionable patch defect; the locally required structured autoreview must be rerun from a full-history checkout because this review tree has no merge base. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: PASS (completed) Assertions:
How this fits togetherACP terminal requests spawn and track child commands. Their output streams feed terminal state, which then enables wait-for-exit and release operations to finish a session. flowchart LR
A[ACP terminal create] --> B[Spawned child command]
B --> C[stdout and stderr pipes]
C --> D[Stream error listener]
D --> E[Terminal exit state]
E --> F[Wait and release operations]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve the stream listeners and lifecycle regression test so broken child pipes cannot abort the ACP host before wait and release complete. Do we have a high-confidence way to reproduce the issue? Yes. Current-main source has output data listeners but no error listeners, and the contributor supplied a compiled before/after runtime transcript that emits pipe errors then completes kill, wait, and release. Is this the best way to solve the issue? Yes. Installing harmless listeners at the child-pipe ownership boundary is the narrowest solution and matches the existing queue-owner pipe-error pattern. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d4c16ab32154. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Fixes an issue where users running an ACP session that calls
terminal/createwould lose the whole client process when the child command died and left a broken stdout or stderr pipe. Node treats anerrorevent on those streams as fatal when no listener is attached, soEPIPEorEIOafter the child exits could kill the ACP host instead of lettingterminal/wait_for_exitandterminal/releasefinish.Why This Change Was Made
terminal/createalready attachesdatalisteners on the child pipes and settles the terminal from the processexitevent. It did not attacherrorlisteners. This change ignores pipe-death codes on stdout and stderr (EPIPE,EIO,ECONNRESET,ERR_STREAM_DESTROYED) and never throws or exits the host from those streams. The existingexithandler still records the status, so wait and release can finish.This is not the CLI
process.stdoutpath from closed #441. That helper throws non-EPIPE and can exit. Child pipes must not. Same-file #501 timed out hung process-list helpers; it did not cover streamerrorevents. The child stdio pattern already used on queue-owner stderr ischild.stderr.on("error", () => {}).The missing listeners date to
1c94396(2026-02-19, "feat: implement full stable ACP spec coverage"), about 180 days onmain.User Impact
An agent that creates a terminal, then sees the child die with a broken pipe, keeps the ACP session alive. Operators can still wait for exit and release the terminal. The host no longer exits because a leftover child pipe raised
EPIPE.Evidence
terminal output from live
nodeagainst compiledcreateTerminal(dist-test/src/acp/terminal-manager.js).Before, on unpatched
src/acp/terminal-manager.tscompiled todist-test, the same script emits{ code: "EPIPE" }on the child stdout and the compiled path throws. Exit code 1:After, on the patched compiled
createTerminalpath, the same emit stays in-process.kill/waitForTerminalExit/releasefinish and the host exits 0:The Node contract with no listener is the same class. A raw
EventEmitterplusemit("error", { code: "EPIPE" })ends the process with exit 1:Patched bundle contains the listeners:
Real behavior proof
Behavior or issue addressed: ACP
terminal/createattacheddatalisteners on the child stdout and stderr pipes with noerrorlisteners. After the child died, a broken pipe (EPIPE/EIO) was an unhandled EventEmitter error and could kill the ACP process. Wait and release never got a chance to finish.Real environment tested: macOS Darwin 25.6.0 arm64, Node v26.7.0, full clone of
openclaw/acpxat/tmp/pr-acpx-term-err, compileddist-test/src/acp/terminal-manager.jsplus the production bundle underdist/.Exact steps or command run after this patch:
That script imports compiled
TerminalManager, callscreateTerminalwithnode -e "setInterval(() => {}, 1000)", emits{ code: "EPIPE" }on the child stdout and{ code: "EIO" }on stderr, thenkillTerminal,waitForTerminalExit, andreleaseTerminal.Evidence after fix: terminal output from the patched compiled
createTerminalpath:The same script against unpatched compiled
createTerminalprinted"code": "EPIPE"and exited 1. A raw EventEmitter with no listener also exited 1 withError: broken pipe.Observed result after fix: The ACP host process stayed alive (
hostAlive: true). Child pipeEPIPE/EIOno longer threw.waitForTerminalExitreturnedsignal: "SIGTERM"andreleaseTerminalcompleted.What was not tested: A live network ACP agent driving
terminal/createover stdio to a third-party coding agent. Windows-only pipe teardown. Writing to child stdin (stdiofor stdin isignoretoday).