Skip to content

fix: honor context in waitForPair and waitForHello - #6

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f002-wait-pair-hello-ctx
Open

fix: honor context in waitForPair and waitForHello#6
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f002-wait-pair-hello-ctx

Conversation

@SebTardif

Copy link
Copy Markdown

What Problem This Solves

clawgo run (and clawgo pair) already install signal.NotifyContext for SIGINT and SIGTERM. After a first-run connect, waitForPair (6 minutes) and waitForHello (30 seconds) selected only on the deadline, c.errs, and incoming frames.

Ctrl+C during pairing or hello therefore did nothing until that deadline fired. The inner reconnect loop already returns on ctx.Done(). These two waits did not.

This PR threads the process context into both waits and returns on ctx.Done(), matching the reconnect-loop pattern from #5. On cancel, run / pair close the client and exit 0, same as the rest of the run loop.

The wait functions were introduced in f601408 (2026-01-04) without a cancel case.

Evidence

Before (binary from upstream/main). Silent TCP acceptor, empty token, SIGINT during waitForPair. Process still running 2.027s later:

$ /tmp/clawgo-f002-old run -bridge 127.0.0.1:65434 -state /tmp/state.json -mdns=false -tts-engine none -chat-subscribe=false
connected to bridge 127.0.0.1:65434
no token found; requesting pairing
SIGINT pid=45295
still_running=yes elapsed_after_SIGINT=2.027s

After (this patch). Same silent acceptor and SIGINT. Process exits 0 in 0.017s:

$ /tmp/clawgo-f002 run -bridge 127.0.0.1:62813 -state /tmp/state.json -mdns=false -tts-engine none -chat-subscribe=false
connected to bridge 127.0.0.1:62813
no token found; requesting pairing
SIGINT pid=38544
clawgo exit rc=0 elapsed_after_SIGINT=0.017s

Canceled wait returns context.Canceled before the 6m/30s deadline. Without the ctx.Done() case the same command failed; with it, it passed:

$ go test ./cmd/clawgo -run 'TestWaitForPairCancelUnblocks|TestWaitForHelloCancelUnblocks' -count=1 -timeout 15s -v
=== RUN   TestWaitForPairCancelUnblocks
    wait_test.go:32: waitForPair did not return after cancel
--- FAIL: TestWaitForPairCancelUnblocks (2.00s)
=== RUN   TestWaitForHelloCancelUnblocks
    wait_test.go:52: waitForHello did not return after cancel
--- FAIL: TestWaitForHelloCancelUnblocks (2.00s)
FAIL

$ go test ./cmd/clawgo -run 'TestWaitForPairCancelUnblocks|TestWaitForHelloCancelUnblocks' -count=1 -timeout 15s -v
=== RUN   TestWaitForPairCancelUnblocks
--- PASS: TestWaitForPairCancelUnblocks (0.00s)
=== RUN   TestWaitForHelloCancelUnblocks
--- PASS: TestWaitForHelloCancelUnblocks (0.00s)
PASS
ok  	github.com/clawdbot/clawgo/cmd/clawgo	0.218s

Real behavior proof

  • Behavior or issue addressed: SIGINT during first-run pairing or hello no longer waits out the 6 minute / 30 second deadline.

  • Real environment tested: macOS 26.6.2 (Darwin 25.6.0 arm64), go1.27.0, clawgo built from this branch at /tmp/oc-pr-clawgo-F002. Compared against a binary built from upstream/main.

  • Exact steps or command run after this patch: Started a silent TCP acceptor. Wrote a state file with no token. Ran clawgo run -bridge 127.0.0.1:$PORT -mdns=false -tts-engine none -chat-subscribe=false. After requesting pairing, sent SIGINT. Also ran go test ./cmd/clawgo -run TestWaitForPairCancelUnblocks|TestWaitForHelloCancelUnblocks -count=1 -timeout 15s -v before and after the ctx.Done() case.

  • Evidence after fix: terminal output from the patched binary:

    connected to bridge 127.0.0.1:62813
    no token found; requesting pairing
    SIGINT pid=38544
    clawgo exit rc=0 elapsed_after_SIGINT=0.017s

    The unfixed binary stayed up:

    connected to bridge 127.0.0.1:65434
    no token found; requesting pairing
    SIGINT pid=45295
    still_running=yes elapsed_after_SIGINT=2.027s
  • Observed result after fix: After SIGINT during pairing, clawgo run returned in 0.017s with exit 0. The unfixed binary was still in waitForPair 2.027s later.

  • What was not tested: Live pairing against a real OpenClaw gateway, and a 30s hello wait against a gateway that accepts the pair but never sends hello-ok. Hello cancel uses the same select as pair and is covered by TestWaitForHelloCancelUnblocks.

Command: go test ./cmd/clawgo -run 'TestWaitForPairCancelUnblocks|TestWaitForHelloCancelUnblocks' -count=1 -timeout 15s -v and live clawgo run plus SIGINT against a silent TCP acceptor.

Observed: red step failed at 2.00s (did not return after cancel). Green step passed at 0.00s. Live patched clawgo run exited 0 in 0.017s after SIGINT; unfixed binary still running at 2.027s.

Expected: cancel unblocks both waits with context.Canceled before the 6m/30s deadline; SIGINT during pairing exits the process promptly.

Time: 11:11:30 PDT

Date: 2026-08-29

Environment: macOS 26.6.2, Darwin 25.6.0 arm64, go1.27.0 darwin/arm64

waitForPair (6m) and waitForHello (30s) selected only on deadline,
bridge errors, and frames. SIGINT during first-run pairing or hello
stayed blocked until timeout.

Honor the process context in both selects. Map cancel at the pair/run
call sites to a clean exit, matching the reconnect loop.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 2, 2026, 9:17 PM ET / September 3, 2026, 01:17 UTC.

ClawSweeper review

What this changes

The PR passes the process context into bridge pairing and hello waits so SIGINT or SIGTERM exits clawgo run and clawgo pair promptly instead of waiting for their timeouts.

Merge readiness

Ready for maintainer review

Keep open: current main still leaves pairing and hello waits outside the process cancellation context, while this focused PR adds the missing cancellation paths and provides real before/after CLI evidence.

Priority: P2
Reviewed head: 77d0db30d05f8b88367f781131f9acab372507e9

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) A focused fix with direct regression tests and convincing real CLI before/after evidence.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The changed production owner is the CLI startup path through pairing and hello waits. The submitted terminal evidence exercises clawgo run against a silent TCP bridge, sends SIGINT during pairing, and records an after-fix clean exit in 0.017 seconds; the paired hello select has matching source handling and direct regression coverage.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The changed production owner is the CLI startup path through pairing and hello waits. The submitted terminal evidence exercises clawgo run against a silent TCP bridge, sends SIGINT during pairing, and records an after-fix clean exit in 0.017 seconds; the paired hello select has matching source handling and direct regression coverage.
Evidence reviewed 5 items Current main remains affected: Current default-branch source calls waitForPair(client) and waitForHello(client) without a context; the wait implementations select only on timeout, bridge errors, and frames.
Introduced cancellation handling: The PR's introduced hunk adds ctx.Done() cases to both waits and maps cancellation at the run and pair call sites to a clean exit.
Focused regression coverage: Four new tests cover cancellation of both waits and preserve successful pair and hello frame handling.
Findings None None.
Security None None.

How this fits together

Clawgo is a headless node client that connects to the gateway bridge, pairs when needed, then completes hello before starting its node services. These waits sit between the bridge connection and the running node, so process cancellation must reach them to allow a prompt shutdown.

flowchart TD
  A[Operator starts run or pair] --> B[Process signal context]
  A --> C[Gateway bridge connection]
  C --> D[Pairing or hello wait]
  B --> D
  D --> E[Paired and connected node]
  B --> F[Clean client close and exit]
  D --> F
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +21/-5; tests +81 The behavioral change is confined to the two startup waits and is accompanied by direct regression coverage.

Technical review

Best possible solution:

Merge the narrow context propagation and retain the regression tests so startup pairing and hello waits honor the same shutdown contract as the rest of the node loop.

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

Yes. The PR supplies a concrete before/after SIGINT scenario against a silent bridge, and current main retains the same non-cancelable wait implementation used by the baseline.

Is this the best way to solve the issue?

Yes. Threading the existing process context into the two bounded wait loops is the narrowest solution and matches the cancellation contract already used by the run loop.

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

  • P2: Ignoring Ctrl+C during a 30-second or six-minute startup wait is a normal-priority CLI shutdown defect with limited blast radius.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owner is the CLI startup path through pairing and hello waits. The submitted terminal evidence exercises clawgo run against a silent TCP bridge, sends SIGINT during pairing, and records an after-fix clean exit in 0.017 seconds; the paired hello select has matching source handling and direct regression coverage.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the CLI startup path through pairing and hello waits. The submitted terminal evidence exercises clawgo run against a silent TCP bridge, sends SIGINT during pairing, and records an after-fix clean exit in 0.017 seconds; the paired hello select has matching source handling and direct regression coverage.

Evidence

What I checked:

  • Current main remains affected: Current default-branch source calls waitForPair(client) and waitForHello(client) without a context; the wait implementations select only on timeout, bridge errors, and frames. (cmd/clawgo/main.go:268, c6e46796a1c8)
  • Introduced cancellation handling: The PR's introduced hunk adds ctx.Done() cases to both waits and maps cancellation at the run and pair call sites to a clean exit. (cmd/clawgo/main.go:638, 77d0db30d05f)
  • Focused regression coverage: Four new tests cover cancellation of both waits and preserve successful pair and hello frame handling. (cmd/clawgo/wait_test.go:18, 77d0db30d05f)
  • Real behavior proof: The submitted PR body records a before/after run against a silent local TCP acceptor: the patched binary exited with status 0 in 0.017 seconds after SIGINT, while the baseline remained running after 2.027 seconds. (77d0db30d05f)
  • Feature-history routing: Available history shows Mariano Belinky authored the preexisting series of cmd/clawgo/main.go changes, including the oldest available relevant commit; the local partial clone could not retrieve older blobs to prove exact-line introduction. (cmd/clawgo/main.go, f60140892c55)

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 (11 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-30T12:01:27.839Z sha 77d0db3 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-30T16:02:13.644Z sha 77d0db3 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-30T20:14:18.751Z sha 77d0db3 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-30T23:59:02.984Z sha 77d0db3 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-31T11:05:48.511Z sha 77d0db3 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-01T04:56:38.330Z sha 77d0db3 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-01T10:10:31.415Z sha 77d0db3 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-02T11:54:35.767Z sha 77d0db3 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant