Skip to content

Improve multi-gateway credential handling#948

Open
bkudiess wants to merge 1 commit into
openclaw:mainfrom
bkudiess:bkudiess-multi-gateway-credentials
Open

Improve multi-gateway credential handling#948
bkudiess wants to merge 1 commit into
openclaw:mainfrom
bkudiess:bkudiess-multi-gateway-credentials

Conversation

@bkudiess

@bkudiess bkudiess commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Make active gateway switching validated, persisted, observable, and rollback-safe.
  • Add detailed credential resolution states for missing/unreadable/corrupt/fallback/bootstrap outcomes and project them into connection diagnostics/UI.
  • Harden stale old-gateway event handling by using generation + gateway-id checks with captured identity context.
  • Protect shared-token and setup-code mutation paths against credential loss and persistence failures.
  • Gate browser-proxy shared-token guidance on actual active gateway shared-token availability.

Validation

  • ./build.ps1 — passed
  • dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore — passed: 2705 passed / 31 skipped
  • dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore — passed: 1586 passed
  • dotnet test .\tests\OpenClaw.Connection.Tests\OpenClaw.Connection.Tests.csproj --no-restore — passed: 420 passed

Real behavior proof

  • Computer-use verified isolated setup launch: OpenClaw Setup window, Welcome to OpenClaw, security notice, and Continue button visible.
  • Computer-use verified normal profile companion: Connected, ws://localhost:18789/ • paired via device token • v2026.6.10, saved gateway row Local (OpenClawGateway) with paired via device token, Shared token, and Node active · 8 capabilities.
  • Local MCP proof via normal profile:
    • winnode --list-tools --verbose succeeded against http://127.0.0.1:8765/ with bearer auth from token file.
    • winnode --command system.which --params '{"bins":["git","node"]}' --verbose returned paths for both git and node.
  • Isolated MCP proof was blocked because port 8765 was already in use by the normal profile MCP server; normal profile MCP proof was used for live discovery/invocation.

Review notes

  • Completed five rubber-duck rounds, then two dual-model review passes.
  • Fixed follow-up findings around stale event races, shared-token TOCTOU, registry save rollback/temp cleanup, non-object identity JSON handling, and Command Center browser-proxy warning coverage.

Not verified / blocked

  • Gateway-mediated openclaw nodes invoke ... path was not separately proven in this closeout; local MCP winnode discovery and invocation were proven.

@clawsweeper

clawsweeper Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 8, 2026, 9:09 PM ET / 01:09 UTC.

Summary
The branch changes gateway registry persistence, credential resolution diagnostics, connection-manager gateway switch and token rollback paths, Command Center browser-proxy warning gating, documentation, and focused tests.

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.

  • Diff surface: 30 files changed, +1457/-222. The PR spans connection, shared identity, WinUI diagnostics, docs, and tests, so auth and upgrade behavior need focused review.
  • Test surface: 10 test/project files changed. The branch adds or updates focused coverage for credential resolution, gateway registry, connection manager, device identity, and tray projections.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Let the pending CI test and E2E jobs finish before merge.
  • [P2] Add a gateway-mediated invoke or existing-profile upgrade smoke if maintainers want stronger auth-path proof.

Risk before merge

  • [P1] The diff changes persisted gateways.json ActiveId handling, token clearing, registry rollback, and per-gateway identity use, so existing installations need upgrade-path confidence beyond fresh setup tests.
  • [P2] The diff changes operator and node credential resolution, fallback diagnostics, and shared/bootstrap/device-token handling, which is auth-provider sensitive even though the code preserves device-token precedence.
  • [P1] The PR body reports local MCP winnode discovery/invocation but says gateway-mediated openclaw nodes invoke was not separately proven, leaving a maintainer proof-sufficiency choice for the gateway path.

Maintainer options:

  1. Add one gateway or upgrade proof before merge (recommended)
    Collect a real existing-profile upgrade or gateway-mediated openclaw nodes invoke smoke so the persisted credential and gateway path is covered beyond local MCP.
  2. Accept the auth/compatibility risk after review
    Maintainers can decide the focused tests and current UI/local-MCP proof are enough and merge after pending CI completes.
  3. Pause and split the bundle
    If the proof burden is too high for this broad PR, split the registry persistence, credential diagnostics, and Command Center warning changes into narrower review units.

Next step before merge

  • [P2] No narrow automation repair remains; the next action is maintainer review of pending CI, auth/compatibility risk, and whether additional gateway-path proof is required.

Maintainer decision needed

  • Question: Is the PR body's UI plus local MCP proof sufficient for this auth/compatibility-sensitive gateway credential change, or should merge wait for a gateway-mediated invoke or existing-profile upgrade smoke?
  • Rationale: This read-only review cannot run the Windows gateway upgrade path, and the PR explicitly notes that gateway-mediated openclaw nodes invoke was not separately proven.
  • Likely owner: shanselman — The baseline connection/registry implementation and recent identity-path work point to shanselman as the best independent owner for the upgrade-proof choice.
  • Options:
    • Require one gateway/upgrade smoke (recommended): Ask for a short existing-profile upgrade or gateway-mediated invoke proof before merge while keeping the current patch intact.
    • Accept current proof after CI: Treat the current UI/local-MCP proof plus focused tests as sufficient and merge once pending checks pass.
    • Split if proof remains unavailable: If the gateway-path proof cannot be collected, split credential resolution, active-gateway persistence, and browser-proxy diagnostics into smaller PRs.

Security
Cleared: No concrete security or supply-chain defect was found; the credential-sensitive changes preserve device-token precedence and do not add dependencies, workflows, or new secret exposure.

Review details

Best 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 changes

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-change UI state proof plus live winnode list/invocation output; the missing gateway-mediated invoke proof is tracked as residual maintainer risk rather than absent proof.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This is a normal-priority credential/session hardening PR with meaningful user impact but no evidence of an active outage, data loss, or release-blocking emergency.
  • merge-risk: 🚨 compatibility: Merging changes persisted ActiveId handling, token clearing, rollback behavior, and per-gateway identity paths that existing installations can hit during upgrade.
  • merge-risk: 🚨 auth-provider: Merging changes operator/node credential resolution and shared/bootstrap/device-token fallback behavior for gateway authentication.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-change UI state proof plus live winnode list/invocation output; the missing gateway-mediated invoke proof is tracked as residual maintainer risk rather than absent proof.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-change UI state proof plus live winnode list/invocation output; the missing gateway-mediated invoke proof is tracked as residual maintainer risk rather than absent proof.
Evidence reviewed

What I checked:

  • Repository policy applied: AGENTS.md was read fully; its credential precedence, GatewayRegistry ownership, UI/MCP proof, and validation guidance applies because this PR touches gateway connection, pairing credentials, diagnostics, Command Center, and MCP-adjacent proof surfaces. (AGENTS.md:1, 02f2421a5e75)
  • PR status and proof context: GitHub reports head 37f1252 as MERGEABLE/UNSTABLE; repo-hygiene/security checks passed while test and E2E checks were still pending at review time, and the PR body reports build/shared/tray/connection tests plus UI and local MCP proof. (37f1252dce3c)
  • Previous finding resolved: The earlier ClawSweeper finding was a timeout merge conflict; the current PR is now mergeable and the only remaining check-state issue observed is pending CI, not a concrete diff defect. (tests/OpenClaw.Tray.Tests/AssistantBridgeServiceTests.cs:202, 37f1252dce3c)
  • Credential resolution implementation: The PR adds detailed operator/node credential resolution while preserving device-token precedence and shared/bootstrap fallback behavior through ResolveOperatorDetailed and ResolveNodeDetailed. (src/OpenClaw.Connection/CredentialResolver.cs:34, 37f1252dce3c)
  • Gateway switch persistence and rollback: SwitchGatewayAsync now validates the target gateway, persists ActiveId before disconnecting, and rolls back the in-memory active gateway if registry save fails. (src/OpenClaw.Connection/GatewayConnectionManager.cs:523, 37f1252dce3c)
  • Focused regression coverage: The PR adds tests for persisted ActiveId reload, save-failure rollback, stale old-gateway event guards, credential fallback visibility, and shared-token replacement under the transition semaphore. (tests/OpenClaw.Connection.Tests/GatewayConnectionManagerTests.cs:172, 37f1252dce3c)

Likely related people:

  • bkudiess: Merged PR history shows recent work on GatewayConnectionManager, ConnectionStateMachine, ConnectionPagePlan, AppStateSnapshot, and Command Center state in the same connection snapshot area. (role: recent area contributor; confidence: high; commits: 74604aebafef; files: src/OpenClaw.Connection/GatewayConnectionManager.cs, src/OpenClaw.Connection/ConnectionStateMachine.cs, src/OpenClaw.Tray.WinUI/Pages/ConnectionPagePlan.cs)
  • shanselman: Git blame/log show the baseline connection registry/manager/resolver files date to 4166e0f, with a later identity-path stabilization touch in the same manager file. (role: introduced behavior and recent adjacent owner; confidence: high; commits: 4166e0fd63f8, b3a2b4ee75e2; files: src/OpenClaw.Connection/GatewayConnectionManager.cs, src/OpenClaw.Connection/GatewayRegistry.cs, src/OpenClaw.Connection/CredentialResolver.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-09T01:01:52.827Z sha 6811204 :: found issues before merge. :: [P2] Resolve the timeout merge conflict

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels Jul 9, 2026
@bkudiess bkudiess force-pushed the bkudiess-multi-gateway-credentials branch from 6811204 to 37f1252 Compare July 9, 2026 01:03
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 9, 2026
@shanselman

Copy link
Copy Markdown
Contributor

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:

  1. Please add or paste one gateway-mediated proof path, not only local winnode MCP. Ideally something like openclaw nodes invoke ... against the current profile, or an equivalent real gateway path that proves the active gateway + node credential path still works after this change.
  2. Please explicitly address the unreadable/corrupt device-token fallback behavior. The current resolver appears to fall back to shared/bootstrap when the stored device token is unreadable/corrupt. That may be acceptable as a pragmatic recovery path, but it sits right next to our guardrail: durable device tokens must not be silently downgraded to shared/bootstrap. If the fallback is intentional, please call out why it is safe and make sure tests cover the exact unreadable/corrupt case; if not intentional, prefer fail-closed for unreadable/corrupt stored device tokens unless the user explicitly chooses repair/re-pair.
  3. If possible, add a quick existing-profile smoke: two gateway records, switch active gateway, restart/reload, confirm ActiveId persists and the expected per-gateway identity directory/token is used.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants