Repository navigation
fix: Companion cannot onboard with an isolated Gateway - #1553
Conversation
|
🦞👀 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. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Codex review: needs real behavior proof before merge. Reviewed September 30, 2026, 12:42 AM ET / 04:42 UTC (Revision 7). ClawSweeper reviewWhat this changesCompanion now prepares, pairs with, and reconnects to an isolated Gateway package, with listener ownership checks, setup recovery, wizard console guidance, and supporting tests and documentation. Merge readiness⛔ Blocked before merge - 7 items remain This PR remains useful because current main does not connect Companion onboarding to the Gateway package’s isolated session. The previous source findings appear addressed, but the package dependency and native proof gates remain unresolved. Priority: P0 Review scores
Verification
How this fits togetherCompanion’s setup wizard prepares a local Gateway and saves its connection record. The connection manager later uses that record to verify the Gateway listener before sending credentials and connecting the tray app. flowchart LR
A[Installed Gateway package] --> B[Companion setup wizard]
B --> C[Agent-owned configuration]
C --> D[Listener ownership check]
D --> E{Ownership verified?}
E -->|Yes| F[Pair and save connection]
E -->|No| G[Block credential handoff]
F --> H[Tray reconnect]
Decision needed
Why: The PR deliberately fails closed for published legacy records, but that can interrupt an existing setup and current evidence does not establish the signed upgrade experience. Before merge
Findings
Agent review detailsSecurityNeeds attention: The new credential boundary depends on an unresolved package check that may write agent configuration, and final-effect listener proof is incomplete. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land a read-only package contract, then prove a signed existing-profile upgrade, current-head native recovery, and allowed and forbidden listener outcomes at credential I/O before accepting the manual reconfiguration policy. Do we have a high-confidence way to reproduce the issue? No high-confidence current-main live reproduction was established in this review. Current source shows Companion’s same-user profile path, and the PR documents a prior-head isolated-package onboarding run. Is this the best way to solve the issue? Unclear pending native proof and the upgrade decision. Package-owned configuration and listener attribution fit the ownership boundary, but the dependent check and existing-record transition must be settled. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6bcc68cde97c. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
4050932 to
9c65f59
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
Hanselman review: PR #1553PR: fix: Companion cannot onboard with an isolated Gateway Base: Final evaluated head: The checkout advanced from Models: Both models flagged: HIGH consensus
Only one model flagged: LOW consensus
Reconciliation and exclusions
Validation and proof limitsFocused tests rerun while HEAD remained
Selectors: Disposable coordinator harness output: Harness uses synthetic data and fake package/process commands, not the user's Gateway. Source is preserved under This was a read-only product review: no repository edits, PR comments, or fixes. Full build/shared suites were not rerun by this review. Prior PR body reported them passing at Current-head UI including recovery/WinGet, negative-path credential I/O suppression, signed MSIX upgrade, ARM64, and strict MXC proof remain unverified here. The recorded local strict MXC attempt failed in fixture setup. At final check, Network recovery E2E and Setup and connect E2E were still in progress. The package dependency openclaw/openclaw-windows-packaging#134 (fix: Companion cannot configure an isolated Gateway) remained open, with head |
Follow Store migration recovery's metadata-only discard policy. Retain configuration, workspace and identity after connection removal; verify old-format drafts and update recovery copy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fd22b9-0532-4fcc-96d0-79ce77725e98
Keep package start ownership separate from listener proofs, scope rollback to the current start, bound passive inspection, and restart pre-existing services only on explicit request. Retry transient wizard log failures and keep terminal recovery visible across steps. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fd22b9-0532-4fcc-96d0-79ce77725e98
|
Implemented the six verified Hanselman findings in the requested two commits:
Validation on Still not a merge-ready verdict: Current-head screenshots/live native fault-recovery, real isolated-service restart/detach, signed upgrade, and native ARM64 attribution proof remain uncollected. The WSL/MXC proof does not substitute for native-MSIX proof. The packaging dependency's separately recorded read-only-check blocker is not fixed by these commits. The PR body now distinguishes current-head validation from prior-head native/UI evidence. |
Hanselman repeat review:
|
| Issue | Opus 5.5 | GPT-6 Astra | Fix confidence |
|---|---|---|---|
Initial logs.tail anchor bypasses retries. One transient initial failure stops inline feedback for the remaining wizard, although the terminal fallback is now visible. |
not flagged | MEDIUM | 95% |
WinRT package resolution can throw COMException, which passive inspection does not normalize. The fire-and-forget auth handler then exits before recording the intended Network outcome; its semaphore is released. |
not flagged | MEDIUM | 95% |
Valid package ok:false / gateway.state:"unknown" responses lose the unavailable-inspection reason. With a live listener, auth recovery reports LocalPortConflict instead of Network. Credentials remain withheld. |
not flagged | MEDIUM | 90% |
Coordinator verified all three source paths and the third finding against packaging contract head c90844ad. These are source/contract findings, not reproduced native failures. Locations: WizardConsoleTail.cs:163-177, IsolatedGatewayRuntime.cs:143-148,251-252 plus NativeGatewayPackageResolver.cs:19-34, and IsolatedGatewayRuntime.cs:124-126 plus NativeGatewayPackageClient.cs:119-125.
Reconciled concerns: Opus retracted the purported unhealthy-listener restart defect after checking that package Running means verified owned listener, not HTTP/auth health. The five-second inspection budget also applies to active callers, but no cold/warm native latency measurement establishes a regression; retain this as a proof gate, not a medium defect. Failed-restart rollback is an intentional fail-closed trade-off. Astra also noted that separate stop/status/start differs from the package's atomic restart and can lose AutostartDisabled metadata; user-visible impact is unestablished, so this is a contract question rather than a verified autostart bug.
Still required: current-head native isolated-service restart/detach and failure-path proof, cold/warm status latency, changed-state UI evidence, signed upgrade, native ARM64 attribution, and resolution of the separate packaging dependency. Previously recorded full build/test passes and strict WSL/MXC 17/17 remain valid for this unchanged head but do not replace native proof. No source changes were made during this review.
Retry the initial log cursor anchor before starting onboarding. Classify WinRT deployment failures and explicit unknown package status as unavailable inspection without releasing credentials. Cover authentication recovery using the real isolated runtime and package parser. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9fd22b9-0532-4fcc-96d0-79ce77725e98
|
Follow-up fixes are pushed in Final validation on this exact code: full build passed; Shared 4,168 passed / 34 skipped; Tray 3,344 passed; Connection 1,419 passed / 1 skipped; SetupEngine 1,452 passed / 1 skipped. Focused Connection and Tray selectors each passed 42/42. Fresh strict WSL/MXC proof passed 17/17, zero skips, including all three required real Gateway/MXC checks. Disposable fixture uninstall exited 0 and teardown completed. The PR remains ready for review, as requested. Its body now records current-head evidence and the remaining native/UI, signed-upgrade, status-latency, restart-contract, and independent packaging-dependency gates. These fixes do not imply merge approval or completion of those proof gates. |
Manual verification: native setup completed with the Gateway PR 134 artifactThe developer confirms that manual testing of Companion head
This is developer-reported manual setup completion on the current Companion code, not a signed Store-install/upgrade test. No screenshot or recording is attached. It does not independently verify chat, restart/detach, injected failure recovery, or every remaining native proof gate. |
Related: openclaw/openclaw-windows-packaging#134 at head
c90844ad2047f109e2ab2d02c4f62b187ebcd147. Merge that Gateway package contract first; this Companion PR depends on it. Itsclawctl companion preparepreserves the agent's effective literal port/token, andgateway-service status --jsonprovides listener ownership evidence; packagedopenclaw devicesremains the sole device-command owner. The dependency's last recorded blocker is that--checkuses an upstream config loader that may modify suspicious config during supposedly read-only verification. This Companion follow-up does not resolve that packaging issue.What Problem This Solves
Fixes: Native Companion onboarding cannot configure or connect to the Gateway MSIX when it writes a separate human-user profile that the isolated Gateway cannot read.
User Impact
Companion sets up, pairs, and reconnects through the installed Gateway package's isolated agent session instead of creating a competing same-user Gateway. During onboarding, the wizard displays agent-side console guidance such as OAuth instructions. An unfinished same-user native draft from before a package upgrade presents an explicit, localized discard-and-reconfigure choice. An already-published incompatible profile remains fail-closed with directions to remove that profile and create a new isolated one. Legacy proof-package and WSL paths remain distinct.
Recovery deletes only
native-setup-draft.json, preserving the old Gateway's workspace, configuration, identity, and credentials, including after a formerly published profile is removed from Connections. These files are not imported into the isolated account. This follows the metadata-only discard boundary in Natalie's #1519 (feat(migration): ship the Inno-to-Store migration), without coupling native setup to Inno-specific bindings or receipts.Why This Change Was Made
Package-qualified
clawctlprepares and checks the agent-owned port/token. Packaged upstreamopenclaw deviceslists and approves only the exact Companion device request after fresh listener attribution. Companion saves its record only after wizard completion and verified authenticated health. The wizard uses bounded authenticatedlogs.tailRPC rather than a host-user or WSL log. Unknown or changed listener identity and unsupported Windows process-sequence inspection fail closed before credential handoff.The initial Hanselman fixes remain in the two requested separate commits:
4f1ab424: preserve existing Gateway data when replacing an incompatible setup draft; recursive byte-for-byte preservation regressions and localized copy.93829418: fix the other five findings. Keep per-package start ownership separate from per-record listener evidence; roll back only a start issued by the failing call; explicitly restart verified pre-existing services without taking passive-detach ownership; bound status inspection and classify unavailable probes as network failures; retry transient console reads and keep terminal recovery visible across wizard questions.After the requested repeat review, the user authorized three narrower follow-up fixes in
dcc5556b: retry the initial log cursor anchor beforewizard.start, handle WinRTCOMExceptionduring inspection, and classify explicit packageunknownstate as unavailable inspection rather than a port conflict. All still deny credential handoff when ownership cannot be established. Regression tests exercise the real isolated runtime/parser and authentication-recovery path with injected package failures.Evidence
Original dual-model verdict: #1553 (comment).
Repeat dual-model verdict for
77a7eb64..93829418: #1553 (comment). Claude Opus 5.5 and GPT-6 Astra found no verified CRITICAL/HIGH source defect in that delta. The three verified MEDIUM follow-ups in that comment are fixed bydcc5556b; a bounded Astra rubber-duck review of that new six-file diff found no actionable new issue. This is not a claim of a fresh full dual-model review ofdcc5556b. The PR was marked ready for review at the user's request, not declared merge-ready.On disposable Windows x64 Developer Mode package and isolated Companion data, prior head
4050932131fa3ecac4228a3e49bf31cb1511e0c9paired exactly one operator device matching the Companion identity, left no pending request, completed the shared provider/auth/model wizard with the user's input, published one activeisolated-session-v1record, and recordedLastConnectedafter restart. The user manually approved the separate node role and confirmed the first chat message completed. This is prior-head user-observed UI evidence, not a signed Store install or a current-head screenshot. The tray and Gateway session were stopped; the user elected to handle package unregistration and disposable-profile deletion.The package dependency's last recorded follow-up head was
c90844ad2047f109e2ab2d02c4f62b187ebcd147. A disposable x64 run on its prior head8961ca1bproved repeat prepare/check preserved an included literal token and valid config bytes. Its mode guard rejects partial port/token configuration withoutgateway.mode; recorded quality checks had zero warnings/errors, 1,237 Release tests passed, and NativeAOT CLI passed. Its read-only-check blocker must be resolved independently before merge.Change Type
Scope
winnodeRequired proof pools
windows-winui-interactive: Native onboarding, localized data-preserving recovery, and persistent console-terminal warning need visible changed-state proof. Earlier user-observed x64 UI proof is prior-head only; no current-head screenshot is retained.windows-11-arm64: Current-head local ARM64 build/tests and strict WSL/MXC proof passed. Native isolated-package aliases and cross-account attribution remain unverified live on ARM64.windows-wsl-mxc: Current-head strict setup/connect lane passed all 17 tests, including all three required real Gateway/MXC proofs, without-AllowSkip.Validation
On
dcc5556bd93572cadcd7f4c9ac6062137cfd7081, withOPENCLAW_REPO_ROOTset to this worktree and separateOPENCLAW_TRAY_DATA_DIR:.\build.ps1: passed (Shared, WinNodeCli, CLI, WinUI, SetupEngine).dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore --verbosity quiet: 4,168 passed, 34 skipped.dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --verbosity quiet: 3,344 passed.dotnet test .\tests\OpenClaw.Connection.Tests\OpenClaw.Connection.Tests.csproj --no-restore --verbosity quiet: 1,419 passed, 1 skipped.dotnet test .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --no-restore --verbosity quiet: 1,452 passed, 1 skipped.FullyQualifiedName~NativeGateway_|FullyQualifiedName~IsolatedGatewayRuntimeTests): 42 passed. Focused Tray (FullyQualifiedName~WizardConsoleTailTests|FullyQualifiedName~NativeGatewaySetupUxContractTests): 42 passed..\scripts\validate-mxc-e2e.ps1 -NoBuild -ResultsDirectory <session-artifacts>\round2-mxc: 17 passed, zero skipped, including all required real Gateway/MXC proofs. Fixture uninstall exited 0 and teardown completed. This does not prove native isolated-MSIX recovery UI.git -c core.whitespace=cr-at-eol diff --check: passed before commit.Real behavior proof
Current-head runtime proof, Windows ARM64, disposable WSL Gateway and isolated tray data, from the strict command above:
The fixture uninstalled its temporary distro/data and reported successful teardown. TRX and console output are retained in the local session artifacts; no secret-bearing Gateway profile is attached.
Prior-head native evidence, not re-run for these review fixes:
Run-CompanionLiveProof.ps1 -Port <ephemeral> -RejectUnrelatedListeneraccepted a managed listener, rejected an unrelated same-port listener, and recovered after it stopped.Run-CredentialBoundaryProof.ps1exercised productionGatewayConnectionManagerwith real Windows listener snapshots and a synthetic credential sink: the unrelated listener produced Error with zero sink calls/TCP connections; the positive control reached the sink once. No real token or WebSocket was sent. Task-owned fixture sessions were removed.Not verified / blocked: Current-head visible native UI proof for the localized metadata-only replacement dialog and persistent console-failure terminal control has not been collected. No isolated live fault-injection fixture was configured for these states; source-contract tests are not presented as screenshots. Fresh WinGet installation, signed-MSIX upgrade, native isolated-package restart/detach and failure behavior on real services, cold/warm package status latency, and native ARM64 cross-account attribution remain live-proof gaps. Separate stop/status/start also needs comparison with the package's atomic restart and retained autostart metadata; no user-visible autostart defect was established by this review. The package dependency's read-only-check blocker remains independent. Prior x64 wizard/chat proof does not satisfy these gates.
Screenshot or artifact links verified: N/A; no screenshot or credential-bearing profile attached. Review comments are linked above.
Security Impact
logs.tailreads bounded console guidance, not auth-profile databases. Package failures/timeouts remain fail-closed. Explicit recovery removes only setup metadata; it neither deletes old credentials/workspaces/configuration nor imports them into the agent account. Registered profiles cannot be discarded by this draft operation.Compatibility and Migration
0.0.0.0/0.0.0.1proof packages and existing WSL profiles retain their separate paths.openclaw.json.clawctl gateway-servicehealth still works.Review Conversations