Add inbound pairing approval dialog (Mac-parity) - #778
Merged
shanselman merged 5 commits intoJun 21, 2026
Conversation
When another device or node requests pairing with the gateway, the Windows operator now gets a focused approval dialog plus an awareness toast — matching the macOS pairing prompt — instead of only a passive in-page banner. The gateway already pushed device/node.pair.requested to the client; previously it only refreshed an in-page list. - OpenClaw.Connection: PendingApproval, PairingApprovalQueue (pure diff engine with decided-suppression + own-node filter), PairingScopeDescriptions (friendly operator-scope labels) + 32 unit tests - PairingApprovalCoordinator: queue + operator approve/reject RPCs, scope/setting gating, in-flight guards, and a 20s safety-net reconcile poll that recovers pair requests dropped by the gateway's dropIfSlow broadcast - PairingApprovalDialog: code-built WindowEx (avoids the XAML-compiler bug); friendly scope list, kind-aware Approve label, ~1.5s anti-clickthrough delay, queue navigation, localized, accessible - Wiring: GatewayService.PairListsChanged feed, awareness + confirmation toasts, review_pairing toast action, ShowPairingApprovalDialog setting (default on), reset on disconnect, own-node re-reconcile, shutdown teardown - Localization across all 5 locales; CONNECTION_ARCHITECTURE.md updated Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses 7 findings from a Claude Opus 4.8 + GPT-5.3-Codex adversarial review (4 HIGH-consensus, 3 single-model). Confirmed-resolution model (#1/#2 HIGH): a send-ack only means the approve/reject frame left the socket, not that the gateway accepted it. PairingApprovalQueue now treats a decision as optimistic-pending (MarkSubmitted) and reports it confirmed only when the request actually leaves the pending list; the success toast fires on that confirmation, not on send-ack. If the gateway never acts within 10s the submission expires and the request re-surfaces for retry — no permanent hide, no false "approved" toast. Also guards against a disconnect (empty list while not connected) being misread as mass confirmations. Stale/closed dialog continuation (#3 HIGH): PairingApprovalDialog.DecideAsync now captures the decision key, bails if the window closed or the queue advanced while the RPC was in flight, and re-arms the approve guard on failure instead of force-enabling Approve (preserving the anti-clickthrough delay). Render() is IsClosed-guarded. Own-node before identity known (#4 HIGH): NodeId and FullDeviceId can differ, and the filter failed open while FullDeviceId was null. The coordinator now defers ALL node requests while node mode is active but the own device id is unknown; they re-surface (correctly self-filtered) once it is known. Minor (#5/#6/openclaw#7): foreground-steal is limited to once per reconnect burst; legacy device-id-as-requestId fallback is now safe (re-surfaces) and logged; Reset() clears in-flight/poll state. Tests: PairingApprovalQueue tests updated for MarkSubmitted + 4 new cases (confirmed resolution, reject flag, timeout re-surface, node deferral). Connection 301, Shared 2049, Tray 959 all green; full build passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Codex review: needs real behavior proof before merge. Reviewed June 21, 2026, 3:07 AM ET / 07:07 UTC. Summary Reproducibility: yes. for the review blockers: source inspection shows a current-main type-name collision and an approval-target safety regression in the new queue path. This is not an original bug report with a separate runtime reproduction. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land the feature only after the branch is current-main compatible, duplicate legacy fallback IDs remain non-actionable, maintainers accept the default-on security UX, and redacted live gateway proof shows the dialog/toast plus approve/reject behavior. Do we have a high-confidence way to reproduce the issue? Yes for the review blockers: source inspection shows a current-main type-name collision and an approval-target safety regression in the new queue path. This is not an original bug report with a separate runtime reproduction. Is this the best way to solve the issue? No: the Mac-parity direction is plausible, but this patch must preserve existing fallback-target safety, avoid the current-main enum collision, and provide live gateway proof before it is the maintainable solution. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 62a4e774c157. Label changesLabel justifications:
Evidence reviewedSecurity concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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
Bot
added
rating: 🧂 unranked krab
Addresses an adversarial review of the final PR state. The prior "defer all node requests while own id unknown" workaround was a regression, so this replaces it with a robust own-node filter and tightens the confirmed-resolution model. F1 (regression): the deferNodeRequests mechanism dropped the ENTIRE node list while node mode was on but FullDeviceId was null — making legitimate remote node approvals vanish and potentially false-confirming in-flight node decisions. Removed entirely. F4: own-node identification now matches the node's actually-advertised id(s) (NodeId and/or FullDeviceId) rather than assuming NodeId == FullDeviceId. The queue takes a set of candidate own-ids; App supplies both. This removes the need for deferral and closes the identifier-space gap. F6/F2: Reconcile now treats a null device/node list as "no fresh snapshot for that kind" (carried forward), not "now empty" — so a partial snapshot never drops a kind's entries or confirms its submissions by absence. This also kills the transient-empty-while-connected mass-confirmation edge. F3: DecideAsync re-validates the connection after the approve/reject await before recording the optimistic submission; a decision that raced a disconnect is not recorded (it cleanly re-surfaces on reconnect) instead of being stranded and later false-confirmed. F8: read the operator client once per OnPairListsUpdated. Accepted with rationale (documented): F5 (expiry starves only if scope is lost mid-connection — rare, self-heals on reconnect) and F7 (confirmation toast may show a slightly stale display name — cosmetic). F2's multi-operator wrong-verb case is an inherent limit of not having a response-aware ack; single-operator (the common Windows-companion case) is correct. Tests: queue tests updated for the own-id set; +3 new cases (match any advertised id, null-list-no-confirm/carry-forward, null-node-list-doesn't-drop). Connection 303, Tray 959, Shared 2049; full build passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
clawsweeper
Bot
added
P2
No behavior change. Removes two genuinely-unused public members on PairingApprovalCoordinator (CanApprove, Find — prompt gating uses CanApproveWith(client) and lookups use the queue directly) and refreshes doc comments/log text that still referenced the old "decided" model and "null = empty list" semantics. The confirmed-resolution wording now matches the implementation. Connection 303, Tray 959, full build green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkudiess
marked this pull request as ready for review
June 18, 2026 18:59
Avoid recording approvals after the operator client changes during an in-flight decision, and route stale review-pairing toast activations to Connection instead of silently no-oping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
When another device or node requests pairing with the gateway, the Windows operator now gets a focused approval dialog + awareness toast — matching the macOS
DevicePairingApprovalPrompter/NodePairingApprovalPrompter— instead of only a passive "Pending approvals" banner that's easy to miss.The plumbing already existed: the client receives the gateway's real-time
device.pair.requested/node.pair.requestedpush events, but only refreshed an in-memory list. This PR adds the missing "last mile": a proactive, transparent, security-conscious decision surface.How it works
Pure core (
OpenClaw.Connection, fully unit-tested)PendingApproval— unified device/node request model (ID truncation,::ffff:IP normalization, repair flag)PairingApprovalQueue— diff engine producingAdded/ResolvedKeys/ConfirmedDecisions. A submitted approve/reject is optimistic-pending: suppressed from the actionable set so the UI advances, but only reported confirmed once the gateway drops it from the pending list (the authoritative signal — the approve/reject RPC only confirms the frame was sent). If the gateway never acts within 10s the submission expires and the request re-surfaces. A null list for a kind means "no fresh snapshot" (carried forward), never "now empty". Own-node requests are filtered by matching any advertised id.PairingScopeDescriptions— operator scope → friendly label (ported from the Mac prompt)Orchestration + UI (
OpenClaw.Tray.WinUI)PairingApprovalCoordinator— bridges the queue to the live operator client; gated onoperator.pairing/adminscope + theShowPairingApprovalDialogsetting; in-flight + reconnect-race guards; a 20s safety-net reconcile poll that recovers requests dropped by the gateway'sdropIfSlow=truebroadcastPairingApprovalDialog— code-builtWindowEx(sidesteps the documented XAML-compiler bug); shows requester identity + the operator scopes being granted, kind-aware Approve label, ~1.5s anti-clickthrough delay, queue navigation, Approve / Reject / Decide-laterGatewayService.PairListsChangedfeed, awareness + confirmation toasts,review_pairingtoast action, reset on disconnect, foreground-steal limited to once per reconnect burst, shutdown teardownShowPairingApprovalDialogsetting (default on); the in-page banner remains as the passive fallbackLocalized across all 5 locales (en/fr/nl/zh-CN/zh-TW);
CONNECTION_ARCHITECTURE.mdupdated.Security posture
Reviewer guide
PairingApprovalQueue.Reconcile(the optimistic-pending state machine) andPairingApprovalCoordinator.DecideAsync/OnPairListsUpdated.Validation
./build.ps1— all 5 projects ✅OpenClaw.Connection.Tests✅ 303 (incl. confirmed-resolution / timeout-resurface / null-list / own-id-set cases)OpenClaw.Shared.Tests✅ 2049 ·OpenClaw.Tray.Tests✅ 959Runtime popup behavior should be confirmed with a manual smoke test against a live gateway (per AGENTS.md).