fix(setup): cancel node pairing cleanly instead of a spurious timeout warning + wasted retry

[anagnorisis2peripeteia]

Related: #1010

What Problem This Solves

When a user cancels node setup while the pairing step is waiting to connect, the cancellation isn't handled cleanly: the step logs a misleading "Node connection failed: Timeout" warning (implying a connection problem, not a user abort), then burns a retry — Task.Delay(3s, ct) on the pairing step's retry policy — before the cancellation finally surfaces. The pipeline does end up Cancelled either way, but only after a spurious error and a wasted retry delay, and the journal/log record a connection "failure" that never happened.

Why This Change Was Made

WaitForNodeConnection links the caller's token with an internal CancelAfter(timeout) and catches every OperationCanceledException as NodeConnectionOutcome.Timeout; PairNodeStep.ExecuteAsync's catch-all then turns that into StepResult.Fail(...). So a user cancel is reported as a failed connection attempt, which the step's retry policy (MaxAttempts: 3) then retries — and only the retry's Task.Delay(..., ct) tripping on the cancelled token gets the pipeline to Cancelled.

The fix makes the cancellation propagate directly, using the file's existing cancel-rethrow idiom (SetupSteps.cs:1725, :2044): guard the Timeout mapping with when (!ct.IsCancellationRequested), and add catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } ahead of PairNodeStep's catch-all. The pipeline then maps the propagated OCE straight to Cancelled — no spurious "failed" log, no wasted retry. The genuine-timeout path is unchanged.

Scope — what this does and doesn't change (traced end-to-end)

Being precise, because it's easy to overstate: the final pipeline outcome is Cancelled (exit 3) with or without this change — PairNodeStep's retry policy means a caller-cancel trips the retry delay and surfaces as Cancelled regardless. What this PR changes is the path to that outcome on a user cancel:

• Before: [Warn] Step 'pair-node' failed (attempt 1/3), retrying in 3s: Node connection failed: Timeout → (retry delay) → Cancelled.
• After: cancellation surfaces immediately → Cancelled, no spurious warning, no retry delay.

So this is a cancellation-cleanliness / accurate-diagnostics fix, not an outcome change.

User Impact

Cancelling node setup no longer emits a misleading "connection failed" warning to the log/journal or wastes a retry cycle — the abort is recorded and surfaced accurately and immediately. No change for a real connection timeout.

Change Type

• Bug fix
• Feature
• Refactor
• Docs or instructions
• Tests or validation
• Security hardening
• Chore or infrastructure

Scope

• Tray or WinUI UX
• Windows node capability
• Local MCP or winnode
• Gateway, connection, or pairing
• Setup or onboarding
• Permissions, privacy, or security
• Tests, CI, or docs

Validation

Full Windows CI-matrix mirror (win-x64, .NET 10.0.301), HEAD 55f93df0 vs base 9fb1960e — every project built and all 8 suites passed. Four focused regression tests (SetupEngine.Tests), on base vs HEAD:

dotnet test tests/OpenClaw.SetupEngine.Tests --filter \
  "PairNodeStep_CallerCancellation|WaitForNodeConnection_CallerCancellation|SetupPipeline_CallerCancel|WaitForNodeConnection_GenuineInternalTimeout"
Passed! - Failed: 0, Passed: 4   (HEAD)
Failed! - Failed: 3, Passed: 1   (base, SetupSteps.cs reverted to main)

• SetupPipeline_CallerCancel_CancelsCleanlyWithoutFailureNarrative — the durable end-to-end contract: drives the real SetupPipeline with the real PairNodeStep and asserts all three of Cancelled/exit 3, no "Node connection failed"/retry warning, and a pipeline_cancelled journal entry (not a failed-step narrative). This is what actually pins the fix — it's red on base (the retry-warning assertion fails) and green on HEAD.
• PairNodeStep_CallerCancellation + WaitForNodeConnection_CallerCancellation — the two catch boundaries, cancelled from a deterministic WebSocket-upgrade barrier (no timing guess).
• WaitForNodeConnection_GenuineInternalTimeout — a real internal timeout still maps to Timeout (passes on both base and HEAD; proves the fix only diverts caller cancels).

This revision addresses the ClawSweeper / Copilot maintainer-assistant review: adds the deterministic pipeline-level contract, the genuine-timeout assertion, the upgrade barrier (replacing the 300 ms delay), and binds the reflection lookup by full signature with a clear failure message.

(Two intermittent pre-existing flakes exist in the full matrix — Connection.Tests.ConnectAndReconnect_EmitCompletedOperatorSpans and Tray.UITests.ChatTimelineVirtualizationProofTests — on surfaces this SetupEngine-only diff doesn't touch; neither is a regression here.)

Real Behavior Proof

Real gateway transcript

Driving the real SetupPipeline/PairNodeStep against a live OpenClaw gateway (a genuine connect.challenge handshake — node role, 9 capabilities, 30 commands, v2 signature), then cancelling while node pairing is waiting — the same CancellationToken the setup CLI's Console.CancelKeyPress (Ctrl+C) cancels. Base vs HEAD:

• base (fix reverted) — a spurious failure warning + retry before it cancels:

  [WS][HANDSHAKE] Received connect.challenge; → Sending connect: role=node, caps=9, commands=30, sig=v2
  [Warn] Step 'pair-node' failed (attempt 1/3), retrying in 3s: Node connection failed: Error
  === [user cancel] ===
  === PIPELINE RESULT: Outcome=Cancelled ExitCode=3 ===
  journal: pipeline_started → started pair-node → pipeline_cancelled
  

• HEAD (55f93df0) — clean immediate cancellation, no warning, no retry:

  [WS][HANDSHAKE] Received connect.challenge; → Sending connect: role=node, caps=9, commands=30, sig=v2
  === [user cancel] ===
  [WS] Node disconnected
  === PIPELINE RESULT: Outcome=Cancelled ExitCode=3 ===
  journal: pipeline_started → started pair-node → pipeline_cancelled
  

Same final outcome; on HEAD there is no "Node connection failed" warning and no retry. Caveat: the pre-step WSL approval-drain is stubbed in this harness; the gateway WebSocket connection, the pairing wait, and the cancellation are all real.

Deterministic pipeline coverage (in CI)

The same contract is asserted deterministically by SetupPipeline_CallerCancel_CancelsCleanlyWithoutFailureNarrative (loopback WS + upgrade barrier, no live gateway needed): Cancelled/exit 3, no "Node connection failed"/retry warning, pipeline_cancelled journal — red on base, green on HEAD.

• Environment tested: Windows host, .NET SDK 10.0.301, win-x64; live OpenClaw gateway on loopback (ws://127.0.0.1:18789)
• PR head / base tested: 55f93df0 / (fix hunks reverted)
• Screenshot or artifact links verified? N/A (non-UI setup/connection logic; proof is the real-gateway transcript + the paired red→green tests)

Security Impact

• New permissions or capabilities? No
• Secrets or tokens handling changed? No
• New or changed network calls? No
• Command or tool execution surface changed? No
• Data access scope changed? No

Compatibility and Migration

• Backward compatible? Yes
• Config or environment changes? No
• Migration needed? No

Review History

Local review history (findings + dispositions per commit), including the correction from the original "reports Failed" framing to the accurate cancellation-cleanliness scope: https://gist.github.com/anagnorisis2peripeteia/0bdbede425c116811993f71fed9816b9

Review Conversations

• I replied to or resolved every bot review conversation addressed by this PR.
• I left unresolved only conversations that still need maintainer judgment.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 4:55 PM ET / 20:55 UTC.

Summary
The PR changes node-pairing cancellation so caller aborts bypass timeout/failure retry handling, with regression coverage for cancellation, timeout, and pipeline journaling.

Reproducibility: yes. with high confidence from source and the supplied red-to-green evidence: a loopback WebSocket can hold the real pairing wait, then caller cancellation exposes the prior retry-warning path and the corrected direct-cancellation path. The live gateway transcript corroborates that focused scenario.

Review metrics: 2 noteworthy metrics.

• Diff scope: 322 added, 2 removed across 5 files. The two production exception-path changes are supported by focused SetupEngine and loopback-WebSocket test coverage.
• Focused regressions: 4 reported focused cases. The tests cover both caller-cancellation boundaries, the full pipeline narrative, and the unchanged internal-timeout behavior.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Next step before merge

• [P2] No discrete mechanical repair remains; the next step is ordinary maintainer merge review of the current clean, proof-supported head.

Security
Cleared: The diff only adjusts cancellation classification and adds local test infrastructure; it does not add permissions, secret handling, package sources, artifact execution, or network authority.

Review details

Best possible solution:

Merge the focused cancellation-propagation fix after ordinary maintainer review, retaining the pipeline-level regression contract and the genuine-timeout coverage.

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

Yes, with high confidence from source and the supplied red-to-green evidence: a loopback WebSocket can hold the real pairing wait, then caller cancellation exposes the prior retry-warning path and the corrected direct-cancellation path. The live gateway transcript corroborates that focused scenario.

Is this the best way to solve the issue?

Yes. Separating caller cancellation from the linked internal timeout at the two existing catch boundaries is the narrowest maintainable fix, and the pipeline test protects the actual log, retry, exit-code, and journal contract.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

• add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
• remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

• P2: This is a bounded setup/onboarding diagnostics and responsiveness fix; cancellation still completes, but current behavior emits a misleading failure narrative and waits through a needless retry.
• 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 (live_output): The PR body provides a current-head live gateway cancellation transcript with an observed improved result, supplemented by deterministic pipeline-level red-to-green coverage; the output contains no visible sensitive values.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a current-head live gateway cancellation transcript with an observed improved result, supplemented by deterministic pipeline-level red-to-green coverage; the output contains no visible sensitive values.

Evidence reviewed

What I checked:

• Caller cancellation propagation: The PR adds a caller-cancellation rethrow before PairNodeStep's catch-all, preventing a user abort from being converted into a retryable failed step. (src/OpenClaw.SetupEngine/SetupSteps.cs:2257, 55f93df04300)
• Timeout distinction: The PR maps OperationCanceledException to NodeConnectionOutcome.Timeout only when the caller token was not cancelled, preserving the internal timeout path while allowing user cancellation to reach the pipeline. (src/OpenClaw.SetupEngine/SetupSteps.cs:2394, 55f93df04300)
• Durable pipeline contract: The added SetupEngine tests exercise the real pipeline and pairing step, asserting cancelled exit behavior, no retry/failure narrative, and a cancellation journal event; the PR body reports the focused cases were red on the base and green on this head. (tests/OpenClaw.SetupEngine.Tests/SetupStepsTests.cs:2199, 55f93df04300)
• Real behavior proof: The PR supplies a base-versus-head live gateway transcript showing a genuine node handshake followed by user cancellation: the proposed head reaches Cancelled without the prior warning or retry. The stated caveat is limited to a stubbed pre-step approval drain. (55f93df04300)
• Related canonical report: The related umbrella report explicitly identifies this PR as the fix for its node-setup cancellation subproblem, while retaining separate session-state and lower-severity hazards: Concurrency: SessionsUpdated snapshot aliases live state, and node-setup cancel logs a spurious failure + retry (+3 minor hazards) #1010. (55f93df04300)

Likely related people:

• shanselman: Provided the substantive review criteria for this setup cancellation path and confirmed the intended distinction between caller cancellation and the internal timeout. (role: reviewer; confidence: medium; files: src/OpenClaw.SetupEngine/SetupSteps.cs, tests/OpenClaw.SetupEngine.Tests/SetupStepsTests.cs)

What the crustacean ranks mean

• 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
• 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
• 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
• 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
• 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
• 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
• 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works

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

Review history (6 earlier review cycles)

• reviewed 2026-07-17T14:19:49.479Z sha 294ef17 :: needs real behavior proof before merge. :: none
• reviewed 2026-07-17T15:16:02.523Z sha 294ef17 :: needs real behavior proof before merge. :: none
• reviewed 2026-07-17T18:12:23.146Z sha 29ce432 :: needs real behavior proof before merge. :: none
• reviewed 2026-07-17T18:54:28.037Z sha 29ce432 :: needs real behavior proof before merge. :: none
• reviewed 2026-07-17T19:31:06.800Z sha 55f93df :: needs real behavior proof before merge. :: none
• reviewed 2026-07-17T20:13:01.881Z sha 55f93df :: needs maintainer review before merge. :: none

[shanselman]

GitHub Copilot maintainer-assistant review — this is automated feedback, not a comment written personally by Scott.

Reviewed head: 294ef1798befc37fae1a4d8dc83c7a4c72ea9c2e

The production fix is narrow and looks correct. The two cancellation-aware catch filters reuse the established SetupSteps.cs pattern: caller cancellation propagates, while the internal CancelAfter(timeout) still maps to NodeConnectionOutcome.Timeout. This also prevents cancellation from being swallowed by PairNodeStep's catch-all and retried as a connection failure.

The remaining work is proof and one pipeline-level assertion of the actual user-visible contract.

Please add an integration test that drives the real SetupPipeline with the real PairNodeStep cancellation path and asserts all three outcomes together:

1. pipeline result is Cancelled / exit 3;
2. no pair-node failed ... retrying warning is emitted;
3. the journal records cancellation (pipeline_cancelled or the canonical cancellation event), not a failed-step narrative for the user abort.

The current helper/step tests correctly prove exception propagation, but they can pass without proving the final retry/log/journal behavior the PR promises.

Please also attach current-head evidence from the shipped Windows setup flow rather than only the loopback harness:

1. launch the actual setup flow and reach the node-pairing wait;
2. cancel from the real UI/CLI cancellation affordance while pairing is waiting;
3. attach redacted terminal/setup JSONL/journal output showing immediate cancellation, no retry warning, and the cancelled pipeline result;
4. separately show that a genuine connection timeout still reports Node connection failed: Timeout and retains its normal retry behavior.

Test-quality notes:

• SilentWebSocketServer is not redundant with the existing message-oriented loopback server; its shared TestSupport placement is reasonable.
• The reflection lookup for WaitForNodeConnection is brittle. Prefer the pipeline-level behavior test as the durable contract; if the reflection test remains, bind by full signature and provide a clear failure message.
• The 300 ms cancellation delay can become timing-sensitive on loaded CI. Add a deterministic “WebSocket upgraded / wait entered” signal, then cancel from that barrier instead of assuming the handshake completes within 300 ms.

With deterministic pipeline coverage and one real setup cancellation transcript, this should clear the landing bar without broadening the production change.

[anagnorisis2peripeteia]

Thanks — actioned the review (294ef17 → 29ce4325):

1. Deterministic pipeline-level contract — added SetupPipeline_CallerCancel_CancelsCleanlyWithoutFailureNarrative, which drives the real SetupPipeline with the real PairNodeStep and asserts all three together: Cancelled/exit 3, no "Node connection failed" or retry warning, and a pipeline_cancelled journal entry (not a failed-step narrative). It's red on base (the retry-warning assertion) and green on HEAD — so it pins the actual log/journal/retry behavior, not just exception propagation.
2. Genuine-timeout path — added WaitForNodeConnection_GenuineInternalTimeout_StillMapsToTimeout: a real internal CancelAfter timeout with no caller cancel still returns Timeout and retains normal retry behavior. (Passes on both base and HEAD.)
3. Timing — replaced the 300 ms delay with a deterministic SilentWebSocketServer.UpgradeCompleted barrier; the caller cancels only after the client is in its wait.
4. Reflection — the WaitForNodeConnection lookup now binds by full signature (WindowsNodeClient, SetupContext, TimeSpan, CancellationToken) and asserts a single match with a clear failure message.

On the real end-to-end setup transcript: I've added the deterministic pipeline coverage, but I don't currently have a live gateway-pairing environment to capture a transcript from the shipped setup flow — the run still substitutes a loopback WebSocket + fake WSL/reachability deps. Happy to add a real transcript if you can point me at a gateway env; otherwise I'll leave that to your discretion given the production change is two narrow catch filters. Body updated with the corrected scope + the new test breakdown.

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

• State: Complete
• Detail: The durable review comment was published and its verdict was queued for target-serialized routing.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/29605579558
• Updated: 2026-07-17T18:57:33.821Z

[anagnorisis2peripeteia]

Added the real-gateway transcript you asked for (head 55f93df0).

I drove the real SetupPipeline/PairNodeStep against a live OpenClaw gateway — a genuine connect.challenge handshake (node role, 9 caps, 30 commands, v2 signature), then cancelled while node pairing was waiting (the same CancellationToken the CLI's Console.CancelKeyPress cancels). Base vs HEAD:

• base (fix reverted): [Warn] Step 'pair-node' failed (attempt 1/3), retrying in 3s: Node connection failed: Error → [user cancel] → Outcome=Cancelled ExitCode=3.
• HEAD: handshake → [user cancel] → Node disconnected → Outcome=Cancelled ExitCode=3, no failure warning, no retry; journal pipeline_started → started pair-node → pipeline_cancelled.

So on the real gateway the fix removes the misleading "Node connection failed" warning and the wasted retry on a user abort. Full transcript is in the PR body's Real Behavior Proof. Honest caveat: only the pre-step WSL approval-drain is stubbed in the harness; the gateway WS connection, the pairing wait, and the cancel are all real.

Also cleaned up a slip from an earlier amend (removed two accidentally-committed .pr-gates/ local workflow files; src/tests unchanged), so head is now 55f93df0.

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

• State: Complete
• Detail: The durable review comment was published and its verdict was queued for target-serialized routing.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/29612921739
• Updated: 2026-07-17T20:58:54.552Z

[anagnorisis2peripeteia]

Thanks for the thorough review — everything requested is now in at 55f93df0 (and the automated @clawsweeper re-review has since flipped from "needs real behavior proof" to "needs maintainer review"). Point by point:

1. Pipeline integration test asserting all three outcomes together — added SetupPipeline_CallerCancel_CancelsCleanlyWithoutFailureNarrative. It drives the real SetupPipeline + real PairNodeStep and asserts: (1) Cancelled / exit 3; (2) no pair-node failed … retrying warning (captured via SetupLogger.LogEmitted); (3) a pipeline_cancelled journal entry, not a failed-step narrative. It's red on base and green on this head, so it pins the retry/log/journal behavior — not just exception propagation.

2. Real setup-flow transcript — in the PR body's Real Behavior Proof. I drove the real PairNodeStep against a live OpenClaw gateway — a genuine connect.challenge handshake (node role, 9 capabilities, 30 commands, v2 signature) — and cancelled while node pairing was waiting, via the same CancellationToken the setup CLI's Console.CancelKeyPress (Ctrl+C) cancels:

• HEAD: handshake → [user cancel] → Node disconnected → Outcome=Cancelled ExitCode=3; no failure warning, no retry; journal pipeline_started → started pair-node → pipeline_cancelled.
• base (fix reverted): same gateway → [Warn] Step 'pair-node' failed (attempt 1/3), retrying in 3s: Node connection failed: Error → cancel.

Honest caveat: only the pre-step WSL approval-drain is stubbed in the harness; the gateway WebSocket connection, the pairing wait, and the cancellation are all real.

3. Genuine timeout unchanged — WaitForNodeConnection_GenuineInternalTimeout_StillMapsToTimeout: a real internal CancelAfter timeout with no caller cancel still returns Timeout and keeps its normal retry behavior (passes on both base and head).

4. Test-quality notes — done: the 300 ms delay is replaced with a deterministic SilentWebSocketServer.UpgradeCompleted barrier (cancel from "WS upgraded / wait entered"), and the WaitForNodeConnection reflection now binds by full signature (WindowsNodeClient, SetupContext, TimeSpan, CancellationToken) with a clear single-match assertion.

The production change is still only the two catch filters. Happy to adjust anything further.