fix: do not block readLoop on a departed error consumer

[SebTardif]

What Problem This Solves

clawgo run keeps a long-lived TCP client to the gateway bridge. connectBridge starts readLoop in a goroutine. That loop already refuses to block forever when publishing frames: it selects on c.done. When the socket ends, it still did a bare send on c.errs.

errs is a buffer-1 channel. After handleFrame returns an error (for example a bridge error frame), runNode calls client.Close() and goto reconnect without receiving the trailing errs value. The same thing happens on ctx.Done(). If that slot is already occupied, readLoop parks on the send until the process exits.

Evidence

Live go run of the old blocking send versus the close-aware select. Buffer-1 slot already full. done already closed (consumer left):

$ go run /tmp/clawgo-f004-send-demo.go
buffer-1 errs already full, done closed (consumer left)
old blocking send still blocked after 2s
select on done returned in 0s

Live clawgo run against a local TCP bridge. The bridge answers hello-ok, then sends {"type":"error","code":"TEST","message":"boom"}. handleFrame returns, the client closes, and runNode reconnects instead of hanging:

$ /tmp/clawgo-f004-bin run -bridge 127.0.0.1:54448 -state /tmp/clawgo-f004-state.json -mdns=false -tts-engine none -chat-subscribe=false -ping-interval 0
connected to bridge 127.0.0.1:54448
hello ok (server=f004)
frame error: bridge error: TEST boom
connected to bridge 127.0.0.1:54448
hello ok (server=f004)
frame error: bridge error: TEST boom

Bridge side accepted the reconnect:

listen 127.0.0.1:54448
accepted connection 1
sent hello-ok + error frame on connection 1
accepted connection 2
sent hello-ok + error frame on connection 2
done connections=2

Real behavior proof

• Behavior or issue addressed: readLoop could block forever on c.errs <- err after clawgo run closed the client and stopped receiving from that channel.
• Real environment tested: macOS 26.6.2, Go 1.27.0, branch fix/readloop-tts-deadline, binary built from ./cmd/clawgo to /tmp/clawgo-f004-bin. Local TCP bridge on 127.0.0.1.
• Exact steps or command run after this patch: Built the binary. Started a local bridge that replies hello-ok then a bridge error frame. Ran clawgo run with a saved token, -mdns=false, -tts-engine none, -chat-subscribe=false. Also ran go run /tmp/clawgo-f004-send-demo.go.
• Evidence after fix: terminal output from the patched binary and helper. After the patch, select on done returned in 0s while the old send stayed blocked for 2s. clawgo run logged frame error: bridge error: TEST boom and connected again on the same port.
• Observed result after fix: readLoop returns when Close() has closed done, even if errs is already full. A live consumer still receives the published error. Reconnect after a handled frame error still runs.
• What was not tested: a remote production gateway, pairing, and TTS speak.

Summary

Call chain: main -> run -> runNode -> connectBridge -> go client.readLoop() -> c.errs <- err / c.errs <- io.EOF.

Consumer leave path: handleFrame on a bridge error frame returns -> client.Close() -> goto reconnect, or ctx.Done() -> client.Close(). Neither path receives the trailing errs value.

Fix: publish through sendErr, the same select on c.done already used for c.frames.

This has been present since f601408 (2026-01-04, 241 days).

Related work:

• The same readLoop already uses select on c.done for frame publish.
• #5 reconnect backoff cancel, #6 waitForPair/waitForHello context, #7 TTS queue leak on reconnect. This PR is only the errs send.
• Go blog: Go Concurrency Patterns: Pipelines and cancellation (done channel so a sender can stop when the consumer leaves)

[clawsweeper]

🦞👀
ClawSweeper picked this up.

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

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 3, 2026, 7:57 AM ET / 11:57 UTC.

ClawSweeper review

What this changes

The PR makes terminal bridge-read errors stop waiting after client shutdown and adds tests for departed and active error consumers.

Merge readiness

⛔ Blocked before merge - 5 items remain

Keep open: the proposed cancellation select is harmless, but the claimed full-error-buffer condition is not reachable in the current bridge client. The prior finding remains unresolved because both the new test and submitted terminal trace bypass or omit that condition.

Priority: P2
Reviewed head: 55aa8262ac45047c633eaf5fb7b17ab892e9ec8a

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The patch is small, but its asserted failure mode and regression coverage are not supported by the current bridge-client data flow.
Proof confidence	🦪 silver shellfish (2/6)	Needs stronger real behavior proof before merge: The terminal evidence shows a patched clawgo run reconnecting after a gateway error frame, but that exercises frame handling rather than a full error queue before readLoop's sole terminal publication. Provide a redacted real trace of a legitimate full-queue production path, if one exists, then update the PR body for re-review; a maintainer can request @clawsweeper re-review if needed.
Patch quality	🦪 silver shellfish (2/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The terminal evidence shows a patched clawgo run reconnecting after a gateway error frame, but that exercises frame handling rather than a full error queue before readLoop's sole terminal publication. Provide a redacted real trace of a legitimate full-queue production path, if one exists, then update the PR body for re-review; a maintainer can request @clawsweeper re-review if needed.
Evidence reviewed	4 items	Only terminal error publication: On current main, readLoop is the only production writer to the error queue and exits immediately after publishing either one scanner error or EOF. Gateway error frames instead enter the frame queue and are returned by the frame handler. Introduced test preloads an impossible state: The new regression test directly inserts an unread error before starting readLoop. No current production path performs that insertion, so the test does not model the stated handleFrame-to-Close sequence. Submitted live trace misses the asserted state: The PR body shows reconnect after a gateway error frame, but that frame is delivered via frames; it does not establish that the terminal error queue was already full before readLoop attempted its sole terminal send.
Findings	1 actionable finding	[P2] Cover a reachable full error buffer
Security	None	None.

How this fits together

The clawgo CLI keeps a long-lived TCP connection to the gateway bridge. Its read loop sends received frames to the run loop and reports a terminal socket result through a one-slot error queue that drives reconnect or shutdown.

flowchart TD
A[Gateway TCP socket] --> B[Bridge read loop]
B --> C[Frame queue]
C --> D[Run loop and frame handler]
B --> E[Terminal error queue]
D --> F[Reconnect or shutdown]
E --> F

Loading

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The terminal evidence shows a patched clawgo run reconnecting after a gateway error frame, but that exercises frame handling rather than a full error queue before readLoop's sole terminal publication. Provide a redacted real trace of a legitimate full-queue production path, if one exists, then update the PR body for re-review; a maintainer can request @clawsweeper re-review if needed.
• Cover a reachable full error buffer (P2) - The new test preloads errs directly, but current production has only one writer—readLoop—and it performs one terminal send before returning. Gateway error frames travel through frames, so the submitted run cannot create the asserted unread error; establish a real producer sequence or remove this unsupported regression and change.
• Resolve merge risk (P1) - Merging would leave a regression test that claims to cover a production deadlock even though the current implementation has no producer that can fill the error queue before its one terminal publication.
• Complete next step (P2) - Establish a real production path that fills the error queue before readLoop's terminal send, or remove the unsupported change and test.
• Improve patch quality - Either provide a redacted production trace that legitimately fills errs before the terminal send, or remove the unsupported change and test.

Findings

• [P2] Cover a reachable full error buffer — cmd/clawgo/readloop_test.go:30-33

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production versus test delta	production +9/-2; tests +77	Most of the change is a regression test whose direct queue preload does not occur in the production bridge lifecycle.

Merge-risk options

Maintainer options:

1. Establish or remove the claimed case (recommended)
   Show a production path that legitimately fills the error queue before the terminal send, or remove the unsupported behavior change and regression test.
2. Pause the narrow fix
   Close this PR if no current bridge lifecycle can create the asserted full-queue state.

Technical review

Best possible solution:

Retain direct terminal error delivery unless a real lifecycle can produce multiple queued errors; if one exists, model that lifecycle end-to-end in the regression test before adding cancellation behavior.

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

No. Current-source inspection shows readLoop is the sole producer of terminal errors and sends only once, while gateway error frames use the separate frame queue; the supplied run therefore cannot fill errs as claimed.

Is this the best way to solve the issue?

No. A close-aware send is only justified after identifying a real producer sequence that fills the queue; the current test injects that sequence directly rather than exercising the bridge lifecycle.

Full review comments:

• [P2] Cover a reachable full error buffer — cmd/clawgo/readloop_test.go:30-33
  The new test preloads errs directly, but current production has only one writer—readLoop—and it performs one terminal send before returning. Gateway error frames travel through frames, so the submitted run cannot create the asserted unread error; establish a real producer sequence or remove this unsupported regression and change.
  Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

• P2: The PR concerns a bounded bridge-client reliability claim that needs correction before merge.
• merge-risk: 🚨 other: The patch would add misleading regression coverage for an unproven production state, which green checks would not detect.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The terminal evidence shows a patched clawgo run reconnecting after a gateway error frame, but that exercises frame handling rather than a full error queue before readLoop's sole terminal publication. Provide a redacted real trace of a legitimate full-queue production path, if one exists, then update the PR body for re-review; a maintainer can request @clawsweeper re-review if needed.

Evidence

What I checked:

• Only terminal error publication: On current main, readLoop is the only production writer to the error queue and exits immediately after publishing either one scanner error or EOF. Gateway error frames instead enter the frame queue and are returned by the frame handler. (cmd/clawgo/main.go:549, c6e46796a1c8)
• Introduced test preloads an impossible state: The new regression test directly inserts an unread error before starting readLoop. No current production path performs that insertion, so the test does not model the stated handleFrame-to-Close sequence. (cmd/clawgo/readloop_test.go:33, 55aa8262ac45)
• Submitted live trace misses the asserted state: The PR body shows reconnect after a gateway error frame, but that frame is delivered via frames; it does not establish that the terminal error queue was already full before readLoop attempted its sole terminal send. (55aa8262ac45)
• Prior finding remains applicable: The previous completed ClawSweeper review identified the same missing reachable full-buffer case, and the reviewed head is unchanged. (cmd/clawgo/readloop_test.go:27, 55aa8262ac45)

Likely related people:

• Mariano Belinky: 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.

History

Review history (1 earlier review cycle)

• reviewed 2026-09-02T16:41:14.681Z sha 55aa826 :: needs real behavior proof before merge. :: [P2] Cover a reachable full error buffer