fix: drain Brabble subprocess output before waiting

[steipete]

Brabble could lose final transcripts because a concurrent cmd.Wait closed stdout before its reader finished. It could also stall when stderr was not consumed: logging disabled returned without reading, and a long diagnostic exhausted the scanner.

One goroutine now owns process completion and waits for both readers before reaping the child. Stderr is always drained, including after an oversized diagnostic. Cancellation closes the readers and terminates the child; stdout scanner failures also cancel it. Nonzero process exits are logged.

Regression tests launch real subprocesses and cover 500 transcripts with a slow consumer, a 9 MB diagnostic with/without logging, and cancellation. Before the fix, the output test delivered only 56/500 and both stderr tests timed out. Built-CLI proof against a synthetic recognizer and delayed loopback handshake delivered 80/500 before and 500/500 after. The standalone adapter proof improved from 39/500 to 500/500.

go test -race ./..., go vet ./..., build, the pinned deadcode analyzer, and independent P0–P2 autoreview passed. README and Unreleased changelog document the correction. No production gateway or recognizer was used.

[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 real behavior proof before merge. Reviewed September 15, 2026, 11:24 PM ET / September 16, 2026, 03:24 UTC.

ClawSweeper review

What this changes

The PR drains Brabble speech-recognition output before waiting for process completion, handles oversized stderr diagnostics, and adds subprocess regression tests and documentation.

Merge readiness

⛔ Blocked before merge - 2 items remain

This remains a useful, focused fix for defects present on main. No blocking code defect was found, but the reported runtime results need an inspectable transcript before merge.

Likely related people: Mariano Belinky and SebTardif are routing candidates for the adapter and transcript forwarding, respectively.

Priority: P2
Reviewed head: fb85a875dac232f713270a86261ed27013e2b9bd

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The patch is focused and source-consistent, with useful subprocess tests; runtime proof remains summarized rather than auditable.
Proof confidence	🦐 gold shrimp (3/6)	Needs stronger real behavior proof before merge: The body reports the real built CLI exercising the changed Brabble adapter through OS pipes and a loopback bridge, recovering all 500 transcripts, but provides no inspectable execution transcript or trace. Attach the existing run output; a production recognizer or gateway is unnecessary for this internal lifecycle fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The body reports the real built CLI exercising the changed Brabble adapter through OS pipes and a loopback bridge, recovering all 500 transcripts, but provides no inspectable execution transcript or trace. Attach the existing run output; a production recognizer or gateway is unnecessary for this internal lifecycle fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	6 items	Current main still contains the defects: Inspected the complete main-branch adapter: cmd.Wait runs concurrently with stdout reading; disabled logging leaves stderr unread; the stderr scanner stops without draining after an oversized diagnostic. No equivalent configuration or alternate lifecycle implementation was found. Introduced lifecycle repair: The pinned introduced delta makes one goroutine finish stdout reading, await stderr completion, and then call Wait. Cancellation closes both pipes; stderr is discarded when logging is absent or scanning fails. Public engine signatures, parsing, dependencies, and persisted settings remain unchanged. Focused subprocess coverage: Read the complete added tests covering 500 ordered transcripts with a slow consumer, a 9 MiB stderr line with logging enabled and disabled, and cancellation closing the transcript stream. Tests were inspected, not executed during this read-only review.
Findings	None	None.
Security	None	None.

How this fits together

Clawgo launches Brabble as a speech-recognition subprocess and converts its stdout into transcripts. Those transcripts feed local routing and gateway requests, while stderr carries diagnostics.

flowchart LR
 A[Recognizer subprocess] --> B[Read stdout transcripts]
 A --> C[Drain stderr diagnostics]
 B --> D[Transcript stream]
 D --> E[Local routing and gateway]
 B --> F[Wait for both readers]
 C --> F
 F --> G[Reap child and close stream]

Loading

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The body reports the real built CLI exercising the changed Brabble adapter through OS pipes and a loopback bridge, recovering all 500 transcripts, but provides no inspectable execution transcript or trace. Attach the existing run output; a production recognizer or gateway is unnecessary for this internal lifecycle fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Complete next step (P2) - Add inspectable output from the reported runtime runs before merge. Terminal screenshots or recordings are welcome; copied output, logs, or linked artifacts also count. Redact private addresses, endpoints, credentials, and other private information. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test growth	Production +33/-7 lines; tests +113 lines	Production growth is justified by pipe draining and cancellation coordination, with focused subprocess coverage.

Technical review

Best possible solution:

Retain the existing engine contract while ensuring both output pipes finish draining before child reaping and cancellation releases blocked readers.

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

Yes, from source: a slow stdout consumer races concurrent Wait on main, and unconsumed or oversized stderr can block the child. The added subprocess cases target these mechanisms; this review did not execute them.

Is this the best way to solve the issue?

Yes. Coordinating existing pipe readers before Wait directly repairs the lifecycle defect without adding configuration, changing transcript parsing, or duplicating another supported path.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 6889cdb0060e.

Labels

Label changes:

• add P2: This repairs transcript truncation and recognition stalls within the optional Brabble subprocess integration.
• add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body reports the real built CLI exercising the changed Brabble adapter through OS pipes and a loopback bridge, recovering all 500 transcripts, but provides no inspectable execution transcript or trace. Attach the existing run output; a production recognizer or gateway is unnecessary for this internal lifecycle fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

• P2: This repairs transcript truncation and recognition stalls within the optional Brabble subprocess integration.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body reports the real built CLI exercising the changed Brabble adapter through OS pipes and a loopback bridge, recovering all 500 transcripts, but provides no inspectable execution transcript or trace. Attach the existing run output; a production recognizer or gateway is unnecessary for this internal lifecycle fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Current main still contains the defects: Inspected the complete main-branch adapter: cmd.Wait runs concurrently with stdout reading; disabled logging leaves stderr unread; the stderr scanner stops without draining after an oversized diagnostic. No equivalent configuration or alternate lifecycle implementation was found. (modules/stt/brabble.go:50, 6889cdb0060e)
• Introduced lifecycle repair: The pinned introduced delta makes one goroutine finish stdout reading, await stderr completion, and then call Wait. Cancellation closes both pipes; stderr is discarded when logging is absent or scanning fails. Public engine signatures, parsing, dependencies, and persisted settings remain unchanged. (modules/stt/brabble.go:54, fb85a875dac2)
• Focused subprocess coverage: Read the complete added tests covering 500 ordered transcripts with a slow consumer, a 9 MiB stderr line with logging enabled and disabled, and cancellation closing the transcript stream. Tests were inspected, not executed during this read-only review. (modules/stt/brabble_process_test.go:28, fb85a875dac2)
• Reported production-path proof: The complete captured PR body reports built-CLI delivery improving from 80/500 to 500/500 with a synthetic recognizer and delayed loopback handshake, plus standalone adapter delivery improving from 39/500 to 500/500. The live body matched the captured text. Neither body nor discussion supplied execution output, a trace, or an artifact containing these runs; the supplied scratch directory was empty. These are useful scenario-specific results, but currently only summarized claims. (fb85a875dac2)
• Related merged work is distinct: fix: skip non-final brabble transcripts in forwardTranscripts #12 is merged and filters partial transcripts at the forwarding boundary; it does not repair subprocess pipe draining. Inspection of the repository's PR and issue listings found no replacement owning this lifecycle fix. The releases endpoint and local tag listing returned no releases or tags establishing a shipped fix. (cmd/clawgo/main.go:800, eeeb7923f0fe)
• Feature-history routing: Main-branch blame points the adapter to this commit; raw commit metadata records Mariano Belinky and a parent. GitHub confirms the commit added the adapter and its tests. Some deeper local history reads failed because required objects could not be retrieved, so GitHub commit inspection supplemented the available history without claiming complete local traversal. (modules/stt/brabble.go:27, 1c1cebc7043f)

Likely related people:

• Mariano Belinky: Raw commit 1c1cebc adds modules/stt/brabble.go:27 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 1c1cebc7043f; files: modules/stt/brabble.go)
• unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Attach redacted terminal output or a linked transcript from the reported built-CLI and adapter runs, including transcript delivery, stderr recovery, and cancellation observations.

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.