fix(connection): keep device tokens when a gateway edit does not change the shared token - #1485
Conversation
…ge the shared token Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 24, 2026, 12:19 AM ET / 04:19 UTC (Revision 7). ClawSweeper reviewWhat this changesSaving an existing gateway with an unchanged shared token now retains its paired device credentials and bootstrap token, while an endpoint or token change follows the replacement path. Regression provenancePossible regression — probable (reviewed change; failure trace). No predecessor PR is attributed. Merge readiness✅ Ready for maintainer review This remains a useful fix: current main still clears paired credentials when the Connection form resubmits an unchanged shared token. The earlier endpoint-change concern is addressed, and the updated exact-head native UI and Gateway evidence supports the changed behavior. Priority: P2 Review scores
Verification
How this fits togetherThe Connection page turns saved gateway form values into a direct-connect request. The direct-connect service updates the gateway registry and identity state, then asks the connection manager to authenticate the operator and node. flowchart LR
A[Saved gateway form] --> B[Compare token and endpoint]
B --> C[Direct-connect service]
C --> D[Gateway registry and identity]
D --> E[Connection manager]
E --> F[Operator and node connection]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep the same-realm preservation check in the form and transaction service, with endpoint changes continuing to establish a fresh identity. Do we have a high-confidence way to reproduce the issue? No current-main run was performed in this read-only review. The current-main form and service provide a high-confidence source path for the reported credential clearing. Is this the best way to solve the issue? Yes. The patch changes the form decision while retaining the service as the credential transaction owner, and it guards endpoint changes before preserving credentials. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against cf19144ce18e. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
…hanges Preserve an unchanged shared token only inside the same credential realm. An endpoint edit must send the token the user submitted. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
shanselman
added
the
status: 🚢 actively landing
|
Global triage: TAKE_AFTER_CHECKS. Take confidence 82%; recommendation confidence 92%; effort small; risk medium. Reviewed exact head |
clawsweeper
Bot
added
proof: sufficient
shanselman
removed
the
status: 🚢 actively landing
clawsweeper
Bot
added
rating: 🐚 platinum hermit
3 participants
What Problem
Editing a saved gateway prefills the shared token. Save and connect sent that token and did not set PreserveExistingSharedTokenWhenMissing. A non-empty shared token cleared the operator and node device tokens and nulled the bootstrap token.
Why
The form showed the existing token as the current value. Saving without a change dropped a paired gateway back to the shared token.
User Impact
An unchanged shared token now connects with a null token and PreserveExistingSharedTokenWhenMissing true. The stored bootstrap token and device tokens stay.
This preservation applies only within the same credential realm. A changed endpoint or SSH credential endpoint still submits the token and resets authentication appropriately. Bootstrap preservation is at the save boundary; the existing BootstrapTokenLifecycle may subsequently retire a bootstrap token once both role tokens are durable.
Evidence
Red: BuildCandidate_UnchangedSharedToken_KeepsStoredBootstrapToken expected bootstrap-token and got null. Green: GatewayDirectConnectServiceTests and AppRefactorContractTests 107 passed. .\build.ps1 passed.
The preceding counts are the author's original evidence. Maintainer behavior proof below targets exact PR head
6000006cccde99ccc848b047b9688acbb6532303. A separate local-only prospective-main floor is recorded under Validation. Existing current-head dual review was reused. Production source was not changed for validation. ConnectionPage supplies the form decision; GatewayDirectConnectService remains the commit/rollback owner; GatewayConnectionManager owns connection lifecycle and device-token precedence.Required proof pools
windows-winui-interactive: Verified by a real native Windows UI test on 2026-09-24. One fresh isolated app exercised the actual Save button for a friendly-name-only edit and a subsequent unchanged Save, with visible UIA state, native Gateway authentication and private credential comparisons after both operations.Explicit maintainer selection calibration: this change preserves credentials when saving an existing gateway record. It does not change WSL provisioning, service management, shell commands, wire protocol or MXC. The full WSL SetupAndConnect class exercises setup, service restart, QR flows and MCP shared-token connection, not this Connection-page form/token comparison. For this protocol-agnostic boundary, the selected proof is the actual native Gateway behavior below plus
windows-winui-interactive.windows-wsl-gateway-e2eprovisioning was NOT RUN and is not selected for this scope. No WSL pool pass is claimed, and synthetic/MCP-only evidence is not being substituted for real Gateway authentication.Validation
Native Windows ARM64, private .NET SDK 10.0.400, isolated worktree and tray data.
OPENCLAW_REPO_ROOTpointed to the worktree. Unit-test processes clearedOPENCLAW_STATE_DIR,OPENCLAW_TRAY_LOCALAPPDATA_DIR,OPENCLAW_TRAY_LOCAL_DATA_DIR, fixture-mode and E2E flags. Test projects were built first to prevent fresh-worktree no-op runs. Test commands used counted TRX output.Original PR head and behavior-proof tooling
.\build.ps1at6000006cdotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Connection.Tests\OpenClaw.Connection.Tests.csproj --no-restore--filter "FullyQualifiedName~GatewayDirectConnectServiceTests"--filter "FullyQualifiedName~GatewayFixtureProfileTests|FullyQualifiedName~GatewayFixtureRunTests"--filter "FullyQualifiedName~GatewayFixtureUiTests.PairedGatewayNameOnlySavePreservesCredentials"Connection skips are unrelated Local AI symlink/distinct-volume cases. Skips are not passes. The original head's CI Gate is successful.
The final two rows used an approved validation-only four-test-file patch, retained as an artifact rather than pushed to the contributor branch. Profile/run isolation guards passed before the UI run. The focused UI runner was built without rebuilding product dependencies, then invoked as:
The existing fixture UI opt-in selected the explicitly hash-checked current-head app, private artifacts and owned native paired seed. Screenshots were disabled. The UI worker had a 120-second deadline, its test process had a 180-second outer watchdog, and the owned Gateway had a 600-second bound. The single test passed on its first execution. The validation-only patch was subsequently reversed before prospective integration validation; no tooling changes were pushed to the author branch.
Prospective current-main integration: passed once
The reviewed PR head was locally merged with main
cf19144ce18eef173b2cbbad5e4aade11ab4cc80, without an author-branch push or GitHub merge:007cca3e04a9cb82ae70ca0c031faa3b82c95804.6000006cccde99ccc848b047b9688acbb6532303andcf19144ce18eef173b2cbbad5e4aade11ab4cc80.b8fc7adc17ef1677287a9179b8929bcdfbc2b68c.992ece1b84f7694ee920a308d2d9107c6b2360a1; GatewayDirectConnectService0f42416578bb17971ba8860aeee0edbada80616b..\build.ps1dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --filter "FullyQualifiedName~GatewayDirectConnectServiceTests"This was one full prospective validation pass with no recovery rerun needed. The UI/native behavior proof remains attributed to the original
6000006cbinary, not falsely presented as an integration-binary UI rerun. At closeout, live GitHub head was still6000006c, main was stillcf19144c, the PR was OPEN/CLEAN and all non-skipped checks were successful. Final GitHub merge remains maintainer-coordinated.Real behavior proof
Native product Gateway: 27 assertions passed
A private external harness ran against the verified official OpenClaw 2026.9.5 package with separate disposable profiles/state/home/workspace, random test-only credentials, and dedicated loopback ports. Gateway child environments were cleared and rebuilt from an OS whitelist; no user/provider keys, service installation, WSL, or normal gateway state were used. Approvals matched the owned identity and exact pending request only.
This harness loaded exact-head production GatewayDirectConnectService from Tray.Tests, which links that source, and used the real CredentialResolver, GatewayConnectionManager, GatewayClientFactory, NodeConnector, and production WebSocket clients. No FakeConnectionManager or protocol fixture supplied authentication results. This service-level proof is separate from the actual UI test below.
healthRPC succeeded; no re-pair request remained.localhostand127.0.0.1remained the same credential realm.Actual WinUI Save and reconnect: passed
At 2026-09-24 04:11:40Z, one new
GatewayFixtureRuninstance connected the actual native app to the owned official Gateway 2026.9.5. It did not reuse the earlier aborted manual app. The approved test-only fixture entry point retained the existing app-launch/disposal boundary and enabled a paired seed copied only from this task's disposable native fixture. Its fresh GUID profile used explicitGatewayFixtureIsolation, an OS-whitelisted child environment, separate app/local roots and a freshMXC_DACL_STATE_DIR. Optional node capabilities and unrelated host effects were disabled.After actual operator/node Ready and zero pending approvals, the test captured a private credential baseline. It opened the real saved-row Edit, changed only
AddDirectName, and invoked the realAddSavebutton through UIA. It then reopened Edit and invoked the same button with no field edits. There was no MCP/programmatic settings-write substitute, production reflection, synthetic Gateway response or fabricated client banner.After each Save, the test required the persisted name, form completion, operator/node Ready using
identity.DeviceTokenandidentity.NodeDeviceToken, zero pending approvals, and exact private equality of shared/device tokens, keypair, URL and gateway identity. It also read these actual visible UIA nodes:Connected,ConnectionPageMarker, visible 986 x 40Node active · no capabilities enabled,NodeStatusText, visible 1151 x 28Connected,ConnectionPageMarker, visible 986 x 40Node active · no capabilities enabled,NodeStatusText, visible 1151 x 28The node text accurately reflects intentionally disabled optional capabilities. No node capability invocation or MXC containment is claimed. The test captured allowlisted status text, IDs, visibility and dimensions only, not raw accessibility trees, token fields, screenshots or private values. The fixture disposed its own app/profile; the owned backend was stopped and its listener closed.
Original behavior-proof binary mapping: executable SHA-256
89C8453584CC8CCDE48E23009E31363D383DC3AE2C1706A8E65832519726E540; app DLL SHA-2564B1A8B052C3D464FFE6A5AD3B6590ECCC8FA50C042B853C8BB8709C4DD2163C9. Both match the reviewed-head full build and were unchanged by test-runner compilation. The later prospective integration build is recorded separately above.Retained safe evidence: counted original and prospective TRX files;
pr1485-fixture-guards.trx;pr1485-ui-save.trx; allowlistednative-ui-save-proof.jsonSHA-2560060AD5E9228592242B338C85F91C0A11EAD913BF6E7CE384753D3D2A5D826BB;native-run.json; validation-only tooling patch SHA-256B81DDD176BF593223A4558E6D5DE7591CBF5EE547BA98E741D320FF775623D98; original 27-assertion receipt SHA-2568BF0C15885FCDCC6368349266021FF8B16C1F8E099ACBC0C5D8FDFCE695CC17D. Raw settings, identities, keys, tokens, machine names and profile paths are not published.The earlier user-launched attempt established Ready and staged a name edit but never saved; it was safely aborted. It is not counted as Save proof. The subsequent independent automated UI test above closes that gap.
Not verified: live SSH transport negative control and WSL provisioning. The corresponding decision-level SSH coverage and explicit WSL scope calibration are stated above. No replacement PR, force-push, author-branch push or production-source edit was performed by this validation lane.