Skip to content

fix(pairing): reconnect after device approval#598

Merged
vincentkoc merged 1 commit into
masterfrom
fix-pairing-approval-auto-reconnect
May 30, 2026
Merged

fix(pairing): reconnect after device approval#598
vincentkoc merged 1 commit into
masterfrom
fix-pairing-approval-auto-reconnect

Conversation

@vincentkoc
Copy link
Copy Markdown
Member

Summary

  • keep gateway auto-reconnect enabled while device pairing is awaiting approval
  • preserve the PairingRequired state so the UI still shows approval guidance
  • add a regression test for pairing-required reconnect eligibility

Validation

  • git diff --check origin/master...HEAD
  • Windows crabbox: dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --logger "console;verbosity=minimal" (2039 passed, 29 skipped)
  • Windows crabbox: dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --logger "console;verbosity=minimal" (870 passed)
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/master --parallel-tests "git diff --check origin/master...HEAD" (clean)

Notes

  • Full ./build.ps1 was attempted on the clean Windows crabbox image, but that image lacks Node.js and the Windows 10 SDK. CI should cover the full WinUI build on the proper runner image.

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 30, 2026

Codex review: needs changes before merge. Reviewed May 30, 2026, 1:33 AM ET / 05:33 UTC.

Summary
The PR enables operator gateway auto-reconnect while pairing approval is pending and adds a shared-client regression test for that reconnect eligibility.

Reproducibility: yes. Source inspection gives a high-confidence path: simulate a pairing-required connect, let the socket close, then an approved reconnect can produce hello-ok while the manager is still in PairingRequired, where HandshakeSucceeded is rejected.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🌊 off-meta tidepool
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

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

Rank-up moves:

  • Update GatewayConnectionManager or ConnectionStateMachine so hello-ok after PairingRequired reaches Connected.
  • [P2] Add a regression test that covers the manager snapshot leaving PairingRequired after approval reconnect.
  • [P2] Rerun the repository-required build and test validation on a runner with the Windows prerequisites.

Risk before merge

  • [P1] As written, enabling reconnect from PairingRequired without a manager/state-machine transition can leave the operator snapshot stuck in PairingRequired after an approved reconnect succeeds.
  • [P1] The PR body reports shared and tray tests, but the full ./build.ps1 validation was not completed on the crabbox image because Node.js and the Windows 10 SDK were missing.

Maintainer options:

  1. Fix the approved-reconnect transition (recommended)
    Update GatewayConnectionManager or ConnectionStateMachine so a hello-ok after PairingRequired moves the operator snapshot to Connected, then add a regression test for that full manager path.
  2. Pause until the draft covers the full flow
    Keep the draft open without merging until the state-machine gap and full build validation are addressed.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Update the pairing approval reconnect path so a successful hello-ok after PairingRequired transitions the operator snapshot to Connected, preserves PairingRequired only while approval is pending, and adds focused GatewayConnectionManager or ConnectionStateMachine regression coverage.

Next step before merge

  • [P2] A focused automated repair can add the missing manager/state-machine transition and regression coverage before this draft PR is mergeable.

Security
Cleared: The diff only changes reconnect eligibility and a regression test; no concrete security or supply-chain concern was found.

Review findings

  • [P1] Advance the manager state after approved reconnect — src/OpenClaw.Shared/OpenClawGatewayClient.cs:162
Review details

Best possible solution:

Land a focused fix that lets an approved reconnect transition the operator from PairingRequired to Connected while preserving the PairingRequired UI only during the pending-approval window.

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

Yes. Source inspection gives a high-confidence path: simulate a pairing-required connect, let the socket close, then an approved reconnect can produce hello-ok while the manager is still in PairingRequired, where HandshakeSucceeded is rejected.

Is this the best way to solve the issue?

No. Changing only the low-level reconnect predicate is not enough; the manager/state-machine path also needs to recognize the successful approved reconnect and leave PairingRequired.

Full review comments:

  • [P1] Advance the manager state after approved reconnect — src/OpenClaw.Shared/OpenClawGatewayClient.cs:162
    With this return value, a pairing-required socket can reconnect after the gateway closes it, but the connection manager remains in RoleConnectionState.PairingRequired. A later hello-ok only raises HandshakeSucceeded, and ConnectionStateMachine rejects that trigger from PairingRequired, so the UI can stay stuck and node startup will not run after approval. Please update the manager/state-machine flow and cover the approved reconnect path, not only the client helper predicate.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against b163d741ed4d.

Label changes

Label changes:

  • add P2: This is a normal-priority pairing bug fix with limited blast radius, but the current patch still needs a bounded state-transition repair before merge.
  • add merge-risk: 🚨 availability: Merging the current diff could leave the operator connection stuck in PairingRequired or retrying after approval instead of reaching Connected.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The contributor proof gate does not apply to this MEMBER-authored draft PR, though full behavior validation is still useful after the state-machine repair.

Label justifications:

  • P2: This is a normal-priority pairing bug fix with limited blast radius, but the current patch still needs a bounded state-transition repair before merge.
  • merge-risk: 🚨 availability: Merging the current diff could leave the operator connection stuck in PairingRequired or retrying after approval instead of reaching Connected.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The contributor proof gate does not apply to this MEMBER-authored draft PR, though full behavior validation is still useful after the state-machine repair.
Evidence reviewed

Acceptance criteria:

  • [P1] ./build.ps1.
  • [P1] dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore.
  • [P1] dotnet test ./tests/OpenClaw.Connection.Tests/OpenClaw.Connection.Tests.csproj --no-restore.
  • [P1] dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore.

What I checked:

  • Repository policy read: The full 48-line AGENTS.md was read; its connection/pairing guidance led the review through the connection architecture and manager state machine rather than only the touched client method. (AGENTS.md:1, b163d741ed4d)
  • Current main suppresses pairing auto-reconnect: On current main, OpenClawGatewayClient.ShouldAutoReconnect returns false while _pairingRequiredAwaitingApproval is true, so the reported stuck-after-approval behavior is still present before this PR. (src/OpenClaw.Shared/OpenClawGatewayClient.cs:160, b163d741ed4d)
  • Transport loop depends on ShouldAutoReconnect: WebSocketClientBase checks ShouldAutoReconnect after disconnects and inside the reconnect loop, so the PR's one-line gate change directly changes whether a pairing-required client keeps retrying. (src/OpenClaw.Shared/WebSocketClientBase.cs:229, b163d741ed4d)
  • Manager state remains PairingRequired on disconnect/error: GatewayConnectionManager intentionally ignores Disconnected/Error events while the data client reports pairing pending, preserving the UI PairingRequired state during the reconnect window. (src/OpenClaw.Connection/GatewayConnectionManager.cs:445, b163d741ed4d)
  • Handshake cannot currently complete from PairingRequired: ConnectionStateMachine only accepts HandshakeSucceeded from Connecting or Error, so a hello-ok received after the PR-enabled reconnect would not transition the operator snapshot from PairingRequired to Connected. (src/OpenClaw.Connection/ConnectionStateMachine.cs:51, b163d741ed4d)
  • PR diff scope: The provided PR context shows head b5088b3 changes only OpenClawGatewayClient.ShouldAutoReconnect plus a private-method unit test; it does not adjust GatewayConnectionManager or ConnectionStateMachine. (src/OpenClaw.Shared/OpenClawGatewayClient.cs:160, b5088b3a98b3)

Likely related people:

  • vincentkoc: Recently authored the pairing bootstrap reconnect hardening commit that touched OpenClawGatewayClient and its tests, and this PR continues that same operator pairing path. (role: recent area contributor; confidence: high; commits: 64b650cb3313, b5088b3a98b3; files: src/OpenClaw.Shared/OpenClawGatewayClient.cs, tests/OpenClaw.Shared.Tests/OpenClawGatewayClientTests.cs)
  • Scott Hanselman: Recently changed GatewayConnectionManager lifecycle behavior and tests, which is the missing state-transition surface needed for this PR to work end to end. (role: adjacent connection-manager contributor; confidence: medium; commits: 0d4fcbd50ad5; files: src/OpenClaw.Connection/GatewayConnectionManager.cs, tests/OpenClaw.Connection.Tests/GatewayConnectionManagerTests.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.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels May 30, 2026
@vincentkoc vincentkoc marked this pull request as ready for review May 30, 2026 05:41
@vincentkoc vincentkoc merged commit 7f19e81 into master May 30, 2026
24 checks passed
@vincentkoc vincentkoc deleted the fix-pairing-approval-auto-reconnect branch May 30, 2026 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant