Improve multi-gateway credential handling#948
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 8, 2026, 9:09 PM ET / 01:09 UTC. Summary Reproducibility: not applicable. as a PR review; I inspected the changed source, tests, PR proof, and current PR status rather than reproducing a separate current-main bug. 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:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Land after pending CI completes and maintainers accept the auth/compatibility proof level, preserving GatewayRegistry and GatewayConnectionManager as the credential ownership boundary. Do we have a high-confidence way to reproduce the issue? Not applicable as a PR review; I inspected the changed source, tests, PR proof, and current PR status rather than reproducing a separate current-main bug. Is this the best way to solve the issue? Yes for implementation direction: the patch stays within GatewayRegistry/GatewayConnectionManager credential ownership and I found no definite diff-introduced correctness bug; proof sufficiency for the gateway-mediated path remains a maintainer call. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 02f2421a5e75. Label changesLabel changes:
Label 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 (1 earlier review cycle)
|
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. |
Summary
Validation
./build.ps1— passeddotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore— passed: 2705 passed / 31 skippeddotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore— passed: 1586 passeddotnet test .\tests\OpenClaw.Connection.Tests\OpenClaw.Connection.Tests.csproj --no-restore— passed: 420 passedReal 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.8765was already in use by the normal profile MCP server; normal profile MCP proof was used for live discovery/invocation.Review notes
Not verified / blocked
openclaw nodes invoke ...path was not separately proven in this closeout; local MCPwinnodediscovery and invocation were proven.