test: drain CLI subprocess output before waiting for exit

[steipete]

CLI subprocess tests could hang when the child filled stdout or stderr: the helper waited for process exit before reading either pipe. On a Mac whose available pipe capacity had fallen to 512 bytes, even the missing-database diagnostic reproduced the deadlock, including with tests run serially.

Drain both streams concurrently through the existing test pipe reader, then collect them after the child exits. Keep the three-second timeout and all CLI assertions unchanged. FileHandle owns each read descriptor so concurrent capture and cleanup cannot double-close it.

Validation: the new real subprocess regression writes 128 KiB to each stream; it timed out before the repair and passed in 0.059 seconds afterward. The previously failing built-CLI diagnostic test and existing large-stdout capture regression also pass. Full-suite, lint, and review results are recorded in the proof comment.

This is independent of #277: that PR isolates transport choice, while this change fixes output capture. Changelog notes are consolidated in the final release-notes PR.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed September 7, 2026, 4:17 AM ET / 08:17 UTC.

ClawSweeper review

What this changes

The PR drains CLI subprocess stdout and stderr concurrently, shares the existing test pipe reader with FileHandle ownership, and adds a large-output regression test.

Merge readiness

✅ Ready for maintainer review

This remains a useful, focused fix: current main and v0.15.1 retain the pipe-blocking sequence. No introduced correctness defect was found, and the collaborator-authored PR is protected from automatic closure.

Priority: P2
Reviewed head: aa6a1d36842f4f03cbd6e459fefc076bb4360399

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused repair with relevant regression coverage and no identified correctness or security blocker.
Proof confidence	🌊 off-meta tidepool	Not applicable: The collaborator-authored test-only PR is exempt from the external-contributor proof gate; its body reports successful after-fix shell subprocess and built-CLI diagnostic runs matching the changed capture helper.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: The collaborator-authored test-only PR is exempt from the external-contributor proof gate; its body reports successful after-fix shell subprocess and built-CLI diagnostic runs matching the changed capture helper.
Evidence reviewed	6 items	Current main still contains the defect: The subprocess helper waits for exit before reading either pipe; a child blocked writing a full pipe therefore reaches the timeout instead of completing normally. Introduced patch verified: The complete pinned delta changes only two test files. Both readers start before the existing timeout wait, and the regression checks 131,072 bytes on each stream. Release comparison: The v0.15.1 tag points to the inspected main revision, which still has the old capture sequence.
Findings	None	None.
Security	None	None.

How this fits together

imsg's CLI tests launch subprocesses and inspect their output and exit status. This test helper collects both output streams so pipe capacity cannot prevent the child from finishing.

flowchart TD
  A[CLI test arguments] --> B[Launch subprocess]
  B --> C[Standard output reader]
  B --> D[Standard error reader]
  B --> E[Bounded exit wait]
  C --> F[Captured output and exit status]
  D --> F
  E --> F
  F --> G[Test assertions]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Regression payload	128 KiB stdout + 128 KiB stderr	The test exercises pipe backpressure on both captured streams.

Technical review

Best possible solution:

Keep concurrent draining in the shared test helper, preserving exit-status assertions, timeout behavior, and single-owner descriptor cleanup.

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

Yes, source inspection establishes the failure path: output exceeding pipe capacity blocks the child while the parent waits for exit. The supplied body reports a before/after reproduction; this review did not execute it.

Is this the best way to solve the issue?

Yes, reusing the existing reader for both streams is a narrow repair, and FileHandle ownership preserves safe cleanup without changing production behavior.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 646ea7af9616.

Labels

Label changes:

• add P2: This repairs false subprocess-test timeouts with a limited, test-only blast radius.
• add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored test-only PR is exempt from the external-contributor proof gate; its body reports successful after-fix shell subprocess and built-CLI diagnostic runs matching the changed capture helper.

Label justifications:

• P2: This repairs false subprocess-test timeouts with a limited, test-only blast radius.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored test-only PR is exempt from the external-contributor proof gate; its body reports successful after-fix shell subprocess and built-CLI diagnostic runs matching the changed capture helper.

Evidence

What I checked:

• Current main still contains the defect: The subprocess helper waits for exit before reading either pipe; a child blocked writing a full pipe therefore reaches the timeout instead of completing normally. (Tests/imsgTests/CommandRouterTests.swift:103, 646ea7af9616)
• Introduced patch verified: The complete pinned delta changes only two test files. Both readers start before the existing timeout wait, and the regression checks 131,072 bytes on each stream. (Tests/imsgTests/CommandRouterTests.swift:109, aa6a1d36842f)
• Release comparison: The v0.15.1 tag points to the inspected main revision, which still has the old capture sequence. (646ea7af9616)
• Reported validation and separate related work: The supplied PR body reports that the real shell subprocess regression timed out before the fix and passed afterward in 0.059 seconds, alongside the built-CLI diagnostic and existing large-stdout test. fix: make RPC send tests independent of host bridge availability #277 instead pins RPC transport selection and does not replace this output-capture repair. (aa6a1d36842f)
• Historical routing: Available main-branch path history identifies Peter Steinberger in prior work on both helpers. Older blob retrieval and blame failed because GitHub DNS was unavailable, so exact line introduction is not asserted. (Tests/imsgTests/StdoutCapture.swift, 93a644b5e893)
• Review scope and validation limits: Root AGENTS.md was read fully; no nested applicable policy or matching maintainer notes were found. The checkout remained clean. Builds and tests were not executed under the read-only contract; a GitHub comments request failed, leaving the supplied snapshot as discussion evidence. (AGENTS.md, aa6a1d36842f)

Likely related people:

• Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

• 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.

[steipete]

Verification for aa6a1d36842f4f03cbd6e459fefc076bb4360399:

• Measured this host's pipe capacity at 512 bytes. The existing built-CLI missing-database test deadlocked both in the parallel suite and in a serial rerun, while the same binary completed in 0.024 seconds when its pipes were drained.
• New real subprocess regression writes 128 KiB to stdout and 128 KiB to stderr. Before: the unchanged three-second deadline killed it and output was incomplete. After: both streams were complete, exit 0, 0.059 seconds.
• make test: 745 Swift tests passed (442 CLI and 303 core), plus documentation and native helper tests. System Bash was used locally because Homebrew Bash's here-document implementation independently blocked on this host's constrained pipes.
• make lint: passed, 15 existing warnings and zero serious violations. git diff --check: passed.
• Both local and committed-branch autoreviews against origin/main are clean at P0–P2.
• Ran the built, matching-Developer-ID-signed CLI: version output, missing-database error with empty stdout, and a read-only real Messages chat query all behaved correctly. Private chat fields withheld.

Exact-head macOS/Linux CI: https://github.com/openclaw/imsg/actions/runs/34099630319. This test-only repair can land before the independent transport-isolation PR #277.