You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
add stdout/stderr error handlers to Matrix dependency bootstrap commands
align the stream-error path with the established src/node-host/invoke.ts lifecycle pattern: record the first stream error, send one SIGTERM, keep the process-exit cleanup registered, escalate to SIGKILL if the child does not exit, and settle only from the child close event
remove the production spawn override that an earlier revision of this branch added for testing; the runtime command path is fixed and takes no injection seam
cover the stream-error lifecycle with a real spawned node child over real stdio pipes in a focused regression test
collision check: no open PR found for the Matrix dependency command stream-error issue or extensions/matrix/src/matrix/deps.ts; the only fuzzy search hit was an unrelated Telegram PR
./node_modules/.bin/oxfmt --check --threads=1 extensions/matrix/src/matrix/deps.ts extensions/matrix/src/matrix/deps.test.ts — all files correctly formatted
./node_modules/.bin/oxlint --tsconfig config/tsconfig/oxlint.extensions.json extensions/matrix/src/matrix/deps.ts extensions/matrix/src/matrix/deps.test.ts — no findings
mutation check: changing the stream-error guard to allow duplicate stream errors made the focused test fail with expected "kill" to be called 1 times, but got 2 times; restoring the guard made the same test pass
Real behavior proof
Behavior addressed: Matrix dependency command stdout/stderr stream errors no longer settle the command result before child close, no longer remove the process-exit cleanup early, and duplicate stream errors cannot trigger teardown twice.
Real environment tested: local Linux Node 22.22 source checkout; the regression test drives the real exported runFixedCommandWithTimeout helper with a real spawned node child process and real pipe-backed stdout/stderr.
Exact steps or command run after this patch: the test destroys the parent-side stdout/stderr Readable with an error while the real child is alive, then observes SIGTERM, SIGKILL escalation, and settlement from the child close event; command: node scripts/run-vitest.mjs extensions/matrix/src/matrix/deps.test.ts.
Evidence after fix: the real-child regression asserts no settlement before close, exactly one SIGTERM for duplicate stream errors, retained process-exit listener during shutdown, SIGKILL escalation, child close { code: null, signal: "SIGKILL" }, and exit-listener cleanup after settlement.
Observed result after fix: Test Files 1 passed (1); Tests 10 passed (10).
What was not tested: a live Matrix bootstrap download. The test injects the Readable error on the parent side because a real download-lib.js child cannot deterministically produce a parent read-side error: normal child exit, fd close, and SIGKILL surface as EOF/close on the parent read side, and EPIPE is a write-side failure.
Summary
The latest durable ClawSweeper review was for head 6ae4180eea824a45da52cfb7bf6c5c363666d6c3, but the PR head is now f9903bfa58bb0ee1c4e8a0baa8b36bfd58ee825b. Its old verdict and PR readiness labels are no longer current.
Next step
Run or wait for a fresh ClawSweeper review on the current PR head.
Review history (5 earlier review cycles)
reviewed 2026-07-07T11:49:19.959Z sha 2471231 :: needs real behavior proof before merge. :: [P2] Give the mock process a typed kill implementation
reviewed 2026-07-07T16:53:22.438Z sha 0155a1e :: needs real behavior proof before merge. :: [P1] Keep spawn injection out of bootstrap runtime
reviewed 2026-07-08T00:25:22.864Z sha b4f31b4 :: needs real behavior proof before merge. :: none
reviewed 2026-07-08T00:33:48.183Z sha b4f31b4 :: needs changes before merge. :: [P2] Use a concrete spy type | [P2] Pass defined spawn options to actual.spawn
Land-ready maintainer pass complete on final head f9903bfa58bb0ee1c4e8a0baa8b36bfd58ee825b.
Source review: the Matrix dependency command helper owns the child and stdio lifecycle. This patch records only the first stream error, sends one SIGTERM, escalates to SIGKILL after a bounded grace period, retains process-exit cleanup, and settles only from child close. It matches the established node-host lifecycle pattern and keeps test injection out of production.
Sanitized AWS Crabbox lease cbx_a369782859a3, Linux Node 24.15.0:
exact-final-head run run_a6768dcfbd19: 10/10 Matrix dependency tests passed, followed by clean targeted oxfmt and extension oxlint.
patch-identical stress run run_f785e18fa042: the same 10-test suite passed three consecutive times.
Fresh final-head gpt-5.5 high-thinking AutoReview: no accepted or actionable findings.
Hosted CI run 28917845974 on the patch-identical immediately preceding rebase: 63 passing checks, 35 expected skips, zero failures. Native scripts/pr prepare-run 101597 accepted the recent-rebase evidence and verified that the remote and reviewed trees match.
Native review artifact validation and prepare gates passed.
Known proof boundary: the regression uses a real spawned child and real pipes but injects the parent-side readable error, because a live Matrix dependency download cannot deterministically produce that failure.
Thanks @vincentkoc, really appreciate the detailed maintainer pass here.
The lifecycle notes around first stream error handling, bounded termination, and settling only from child close are very helpful. I’ll use that node-host pattern for similar stdio/child-process fixes going forward.
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
channel: matrixChannel integration: matrixP2Normal backlog priority with limited blast radius.size: S
2 participants
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.
Summary
errorhandlers to Matrix dependency bootstrap commandssrc/node-host/invoke.tslifecycle pattern: record the first stream error, send one SIGTERM, keep the process-exit cleanup registered, escalate to SIGKILL if the child does not exit, and settle only from the childcloseeventspawnoverride that an earlier revision of this branch added for testing; the runtime command path is fixed and takes no injection seamnodechild over real stdio pipes in a focused regression testextensions/matrix/src/matrix/deps.ts; the only fuzzy search hit was an unrelated Telegram PRVerification
node scripts/run-vitest.mjs extensions/matrix/src/matrix/deps.test.ts— passed; 1 file, 10 tests passed./node_modules/.bin/oxfmt --check --threads=1 extensions/matrix/src/matrix/deps.ts extensions/matrix/src/matrix/deps.test.ts— all files correctly formatted./node_modules/.bin/oxlint --tsconfig config/tsconfig/oxlint.extensions.json extensions/matrix/src/matrix/deps.ts extensions/matrix/src/matrix/deps.test.ts— no findingsexpected "kill" to be called 1 times, but got 2 times; restoring the guard made the same test passReal behavior proof
Behavior addressed: Matrix dependency command stdout/stderr stream errors no longer settle the command result before child close, no longer remove the process-exit cleanup early, and duplicate stream errors cannot trigger teardown twice.
Real environment tested: local Linux Node 22.22 source checkout; the regression test drives the real exported
runFixedCommandWithTimeouthelper with a real spawnednodechild process and real pipe-backed stdout/stderr.Exact steps or command run after this patch: the test destroys the parent-side stdout/stderr Readable with an error while the real child is alive, then observes SIGTERM, SIGKILL escalation, and settlement from the child
closeevent; command:node scripts/run-vitest.mjs extensions/matrix/src/matrix/deps.test.ts.Evidence after fix: the real-child regression asserts no settlement before close, exactly one SIGTERM for duplicate stream errors, retained process-exit listener during shutdown, SIGKILL escalation, child close
{ code: null, signal: "SIGKILL" }, and exit-listener cleanup after settlement.Observed result after fix:
Test Files 1 passed (1); Tests 10 passed (10).What was not tested: a live Matrix bootstrap download. The test injects the Readable error on the parent side because a real
download-lib.jschild cannot deterministically produce a parent read-sideerror: normal child exit, fd close, and SIGKILL surface as EOF/closeon the parent read side, and EPIPE is a write-side failure.