fix(connection): startup connects with a weaker token when the legacy device key is still present - #1512
Open
SebTardif wants to merge 1 commit into
Open
fix(connection): startup connects with a weaker token when the legacy device key is still present#1512SebTardif wants to merge 1 commit into
SebTardif wants to merge 1 commit into
Conversation
…oken When the per-gateway identity file is missing, startup used the shared or bootstrap token and never copied the legacy device key for that URL. Copy the legacy identity and resolve it again before accepting the weaker token. - Add LegacyStartupDeviceToken.Prefer for operator and node startup - If the copy fails, still use a device token read from the legacy directory - Do not copy or replace a credential that is already a device 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
P1
|
Codex review: needs real behavior proof before merge. Reviewed September 24, 2026, 2:17 PM ET / 18:17 UTC. ClawSweeper reviewWhat this changesThe branch adds a startup credential helper that copies a legacy device identity into the active gateway directory and prefers its device token over a shared or bootstrap token. Merge readiness⛔ Blocked before merge - 10 items remain The startup downgrade is still present on current main, so this PR addresses a real gap. The patch needs repair before merge: its copy-failure fallback does not reach the connection manager, and copying a corrupt legacy identity can block a previously usable shared-token connection. Priority: P1 Review scores
Verification
How this fits togetherThe tray reads the active gateway record and stored identity before starting a connection. The connection manager resolves credentials again from the per-gateway directory before contacting the Gateway. flowchart LR
A[Active gateway record] --> B[Startup credential check]
C[Legacy identity file] --> B
B --> D[Per-gateway identity file]
D --> E[Connection manager resolves credential]
E --> F[Gateway connection]
Before merge
Findings
Agent review detailsSecurityNeeds attention: The changed token choice is security sensitive because the claimed device-token fallback can be lost before the final Gateway authentication. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep credential selection and final connection resolution consistent, validate a legacy identity before persisting it, and show the upgrade and failed-copy outcomes through the running tray-to-Gateway path. Do we have a high-confidence way to reproduce the issue? Yes, from source: with an active matching gateway, a missing per-gateway identity, a legacy device token, and a failed copy, startup selects the legacy token but the manager resolves from the empty per-gateway directory. A corrupt legacy file plus a usable shared token exposes the separate upgrade failure. Is this the best way to solve the issue? No. The helper corrects the successful-copy case, but its selected fallback is discarded before final connection and it copies identity bytes before validating them. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 273b0182745a. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Collaborator
|
Global triage: HOLD_FOR_AUTHOR. Take confidence 5%; recommendation confidence 99%; effort medium; risk high. Two credential-precedence failures block this exact head:
Please make the final connection owner choose credential plus matching identity directory atomically, validate legacy identity before persistence, and cover operator, node-only, copy-failure, and corrupt-file paths end to end. Required pools are |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Fixes: the next launch connects with a shared or bootstrap token when the per-gateway identity file is missing and the legacy device key for that same gateway URL is still on disk.
User Impact
User impact: a paired gateway keeps its device token across that startup instead of dropping back to the weaker token.
Why This Change Was Made
Startup now copies the legacy identity into the per-gateway directory and resolves it again before it accepts a shared or bootstrap token. If that copy fails, a device token read from the legacy directory is still used. A credential that is already a device token is left alone.
Evidence
Terminal output from the patched tree at
1327a863, callingLegacyStartupDeviceToken.Preferthrough the connection test host. The legacy file is a realdevice-key-ed25519.jsonwritten byDeviceIdentity.StoreDeviceTokenForRole.Change Type
Scope
winnodeRequired proof pools
none: no installer, WSL gateway, or WinUI window is required. The choice is a file copy plus credential resolution.Validation
dotnet test ./tests/OpenClaw.Connection.Tests/OpenClaw.Connection.Tests.csproj --filter LegacyStartupDeviceTokenTestson1327a863: Passed 5, Failed 0.dotnet build src/OpenClaw.Tray.WinUI/OpenClaw.Tray.WinUI.csproj -p:Platform=x64succeeded. One existing Win2D warning../build.ps1was not run.Real Behavior Proof
1327a863.dotnet test ./tests/OpenClaw.Connection.Tests/OpenClaw.Connection.Tests.csproj --filter LegacyStartupDeviceTokenTestspaired-tokasidentity.DeviceTokenandnode-tokasidentity.NodeDeviceToken. The blocked-directory path still returnedidentity.DeviceTokenand did not create the per-gateway file.Security Impact
NoYesNoNoNoYes, explain the risk and mitigation: The device token is preferred and copied into the existing per-gateway identity directory. The legacy file is not deleted. Tokens are not logged.Compatibility and Migration
YesNoNoReview Conversations