Improve multi-gateway credential handling#948
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 9, 2026, 7:29 PM ET / 23:29 UTC. Summary Reproducibility: not applicable. as a PR review: there is no open line-level bug finding to reproduce, but the PR body and tests exercise the changed active-gateway and credential-resolution paths. Review metrics: 3 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:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Land the constrained, tested credential-handling changes only after a maintainer accepts the same-gateway fallback and upgrade-risk posture; keep GatewayConnectionManager/GatewayRegistry as the ownership boundary. Do we have a high-confidence way to reproduce the issue? Not applicable as a PR review: there is no open line-level bug finding to reproduce, but the PR body and tests exercise the changed active-gateway and credential-resolution paths. Is this the best way to solve the issue? Yes for the implementation shape: it stays within the existing GatewayConnectionManager/GatewayRegistry/CredentialResolver ownership boundary and adds focused regression coverage, while the fallback policy still needs maintainer acceptance. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c8cd9869d828. Label changesLabel justifications:
Evidence reviewedWhat 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
Review history (4 earlier review cycles)
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
6811204 to
37f1252
Compare
|
Thanks for the very thorough work here. The direction looks valuable: keeping GatewayConnectionManager/GatewayRegistry as the ownership boundary, making active-gateway changes rollback-safe, surfacing credential-resolution state, and guarding stale old-gateway events are all the right instincts for this area. Before I’d be comfortable merging this, I’d like one focused proof/clarification pass because this PR touches auth/credential precedence and existing-profile upgrade behavior:
This is not a broad rejection; it’s a confidence request for a high-blast-radius connection/auth PR. The tests and local proof already look strong. With the gateway-mediated proof plus an explicit decision on the fallback behavior, this should be much easier to take confidently. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Thanks — I addressed the confidence items you called out.
I also reran validation after the follow-ups:
|
shanselman
left a comment
There was a problem hiding this comment.
Reviewed with >90% confidence after follow-ups. Maintainer accepts the explicit same-gateway unreadable/corrupt identity fallback policy: readable device tokens always win, fallback never crosses gateway records, and FallbackUsed/PrimaryStatus remains visible in diagnostics. Gateway-mediated invoke and existing-profile proof are present. I also merged this PR locally with current main and validated build plus Shared (2724/31 skipped), Tray (1662), and Connection (422) tests.
Summary
Validation
./build.ps1— passeddotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore— passed: 2724 passed / 31 skippeddotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore— passed: 1616 passeddotnet test .\tests\OpenClaw.Connection.Tests\OpenClaw.Connection.Tests.csproj --no-restore— passed: 422 passedNotes: prior full validation attempts hit transient queued-chat / exec-approval timing failures; each failing test passed in isolation and the full suites passed on rerun.
Real behavior proof
OpenClaw Setupwindow,Welcome to OpenClaw, security notice, andContinuebutton visible.Connected,ws://localhost:18789/ • paired via device token • v2026.6.10, saved gateway rowLocal (OpenClawGateway)withpaired via device token,Shared token, andNode active · 8 capabilities.winnode --list-tools --verbosesucceeded againsthttp://127.0.0.1:8765/with bearer auth from token file.winnode --command system.which --params '{"bins":["git","node"]}' --verbosereturned paths for bothgitandnode.openclaw nodes list --jsonfrom theOpenClawGatewayWSL distro showed pairedWindows Node (PERSEID)with commands includingsystem.which.openclaw nodes invoke --node 'Windows Node (PERSEID)' --command system.which --params '{"bins":["git","node"]}' --json --invoke-timeout 30000succeeded through the gateway and returned resolvedgitandnodeexecutable paths.gateways.jsonhadActiveId=adceebb2401a4428, one saved gateway, active URLws://localhost:18789, friendly nameLocal (OpenClawGateway), shared token present, bootstrap token absent.device-key-ed25519.jsonexisted under%APPDATA%\OpenClawTray\gateways\adceebb2401a4428\.Corrupt/unreadable device-token fallback decision
GatewayRecord; credential reads never fall back to another gateway identity directory.GatewayCredentialResolutionStatus.FallbackUsedwithPrimaryStatus=UnreadableorCorrupt, so UI/diagnostics can make repair or re-pairing visible.[]/null), and unreadable-token fallback cases.Review notes
Not verified / blocked
8765was already in use by the normal profile MCP server; normal profile local MCP and gateway-mediated invoke proof were collected instead.