refactor(connection): extract domain coordinators - #1101
Conversation
|
Codex review: needs changes before merge. Reviewed August 17, 2026, 2:44 PM ET / 18:44 UTC. ClawSweeper reviewWhat this changesThis PR moves Windows-node lifecycle, credential-handoff, and device-pair approval workflows into internal coordinators behind the existing gateway connection facade. Merge readiness⛔ Blocked by patch quality or review findings - 11 items remain Keep open. Two previously reported P1 callback-detachment defects remain on the current head, and four user-facing diagnostic strings still violate repository copy policy. The supplied behavior claims are not tied to the current head. Priority: P1 Review scores
Verification
How this fits togetherThe connection layer takes gateway settings, credentials, and transport events and produces operator and Windows-node connection state for the tray and local MCP service. This PR separates node lifecycle, bootstrap-token handling, and device-pair approval from the public connection manager. flowchart LR
A[Gateway settings and credentials] --> B[Connection manager facade]
B --> C[Node connection coordinator]
B --> D[Bootstrap token lifecycle]
B --> E[Device-pair approval coordinator]
C --> F[Gateway and node connections]
D --> F
E --> F
F --> G[Tray and local MCP status]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve the coordinator split, but detach both remaining connector callbacks before their async methods start, add deterministic lock-boundary regressions, replace the diagnostic punctuation, and publish redacted current-head connection proof. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: invoke either forwarded NodeConnector callback while a transition or durable-clear operation is gated, and the current task construction can run that work before the lock-holding callback returns. Is this the best way to solve the issue? No. The extraction is viable, but the remaining callbacks must be dispatched before their async synchronous prefixes execute, with regressions that prove prompt callback return. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against fc9add75eda7. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (25 earlier review cycles; latest 8 shown)
|
94d660b to
5a8ea3e
Compare
5a8ea3e to
9fbd151
Compare
9fbd151 to
d6a92f8
Compare
d6a92f8 to
2467fbc
Compare
|
Rebased unchanged B1 layer onto latest B0/main at head |
|
Pushed P1 addressed: node token recovery no longer runs on the connector's lifecycle-lock-holding callback stack. Deterministic proof (current head): reverting the one-line owner fix makes Validation (head Review: rubber-duck + Claude Opus 4.8 + GPT-5.6 Sol all clean (fault propagation and background-task draining intact, fencing preserved, no deadlock, discriminators deterministic). Non-blocking follow-up: sibling lock-forwarded callbacks @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
95a88b1 to
63419ce
Compare
63419ce to
e69b5cd
Compare
e69b5cd to
797136b
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
NodeConnector raises ConnectionFailure while holding _clientLifecycleLock and requires subscribers to return promptly. HandleConnectionFailure called TrackBackground(HandleDeviceTokenMismatchAsync(attempt)), which evaluated the recovery's synchronous prefix (attempt-lease acquire, endpoint trust check, DeviceIdentity node-token clearing) on the lock-holding callback stack when awaits completed synchronously, risking blocked lifecycle and deadlock. Wrap the dispatch in Task.Run so the whole recovery runs off the callback stack. The unwrapped proxy task is still tracked in _backgroundTasks and drained on shutdown; generation and credential fencing are unchanged. Adds a deterministic barrier test plus superseded-generation, recovery success, untrusted-endpoint, and after-stop regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 59c9cc10-f388-47ba-a6a0-2afd21dff031
797136b to
eb4add3
Compare
E2E proof plan and resultsCurrent PR head: The proof matrix targets the behavior boundaries moved by this refactor: operator/node readiness, reconnect, credential recovery and preservation, revocation recovery, and fail-closed credential handoff.
Targeted run: 9 passed, 0 failed, 0 skipped in 4m 46s. $env:OPENCLAW_REPO_ROOT = (Get-Location).Path
$env:OPENCLAW_RUN_E2E = '1'
dotnet test .\tests\OpenClaw.E2ETests\OpenClaw.E2ETests.csproj --no-restore --filter '<nine scenarios above>'Real behavior proofThe screenshots were captured from isolated current-head E2E fixtures. The workstation name was replaced with Healthy real-gateway state: device-token pairing, operator active, node active with 9 capabilities, and two connected clients. Recovered after a real WSL gateway restart: the fixture restarted the gateway, waited for operator and node readiness, then captured the recovered state. Capture assertion: 1 passed, 0 failed. Screenshots cannot prove credential withholding or concurrency fencing. Those claims are covered by current-head automated assertions. The fresh adversarial artifacts stamped
For the node case, withholding is established by zero credential-bearing frames plus the ownership recheck, not by the generic node error text alone. Review: Rubber-duck review found no privacy leak in the published screenshots and required the harness distinction and node-proof limitation above. Required PR CI is green; the only failing check shown is the non-required |


Summary
NodeConnectionCoordinator.BootstrapTokenLifecycle.DevicePairApprovalCoordinator.GatewayConnectionManageras the stable public façade and sole writer of the overall state machine, operator lifecycle, active gateway/identity/tunnel, operator reconnect, v2 persistence, DI, and disposal.GatewayConnectionManagershrinks from 3,892 to 2,657 lines without changingIGatewayConnectionManagerbehavior or constructor shape.Invariants
CredentialResolver; paired identities are never downgraded.Validation
Exact latest-main stacked head
2467fbc4on B02a5fafb8and maincdcb74f4:Real behavior proof
winnode --list-tools: 51 tools, including 3 camera toolsapp.connection.status: Idle, LocalMcpOnly, MCP runningtools/listandtools/callsucceededReview
Architecture
Adds authoritative and closed ledger rows for node connection, bootstrap lifecycle, and device-pair approval. Source guards prevent reintroducing node generations/CTS/start/telemetry, bootstrap timing flags, pairing reconnect fields, or duplicate credential failure formatting into the manager.
Stack
Position 2 of native Connection stack #1102, directly based on B0 protocol-core PR #1095. B2 ConnectionPage ownership will append after A0 settings reconciliation.
ClawSweeper P1 follow-up (head
95a88b12)Addresses the ClawSweeper P1 on this PR: node token recovery ran on the connector's lifecycle-lock-holding callback stack.
Root cause:
NodeConnectorraisesConnectionFailurewhile holding_clientLifecycleLockand requires subscribers to return promptly.HandleConnectionFailurecalledTrackBackground(HandleDeviceTokenMismatchAsync(attempt)), which evaluated the recovery's synchronous prefix (attempt-lease acquire, endpoint trust check,DeviceIdentitynode-token clearing) on the lock-holding stack whenever awaits completed synchronously, risking blocked lifecycle and deadlock. Base B02a5fafb8dispatched this viaTask.Run; the extraction dropped that boundary.Owner fix (one line):
TrackBackground(Task.Run(() => HandleDeviceTokenMismatchAsync(attempt)))inNodeConnectionCoordinator.HandleConnectionFailure. Recovery runs off the callback stack; the unwrapped proxy task is still tracked in_backgroundTasksand drained on shutdown; generation and credential fencing are byte-for-byte unchanged.Deterministic proof (revert fails): with the fix reverted,
ConnectionFailureRecovery_DetachesFromConnectorCallbackThreadfails (callback stays blocked while recovery is suspended at the gated trust check) andConnectionFailureRecovery_SupersededNodeGeneration_PreservesNodeTokenfails (token cleared inline before the supersession lands). With the fix both pass; the two ordering tests were re-run 3x green.Validation (head
95a88b12):Review: rubber-duck, Claude Opus 4.8, and GPT-5.6 Sol all confirmed the fix correct and complete (fault propagation and background-task draining intact, credential/generation fencing preserved, no deadlock, discriminator tests deterministic). The production comment and the barrier assertion message were tightened per review.
Non-blocking follow-up: two sibling callbacks forwarded under the same connector lock share the inline-async shape but do not clear credentials:
NodeConnectionCoordinator.HandleStatusChangedandHandleDeviceTokenReceived. Out of scope for this P1; noted for a future pass.