Skip to content

fix: Companion cannot onboard with an isolated Gateway - #1553

Merged
karkarl merged 5 commits into
openclaw:mainfrom
paulcam206:fix/companion-isolated-gateway-setup
Sep 30, 2026
Merged

karkarl merged 5 commits into
openclaw:mainfrom
paulcam206:fix/companion-isolated-gateway-setup

Conversation

@paulcam206

@paulcam206 paulcam206 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Related: openclaw/openclaw-windows-packaging#134 at head c90844ad2047f109e2ab2d02c4f62b187ebcd147. Merge that Gateway package contract first; this Companion PR depends on it. Its clawctl companion prepare preserves the agent's effective literal port/token, and gateway-service status --json provides listener ownership evidence; packaged openclaw devices remains the sole device-command owner. The dependency's last recorded blocker is that --check uses 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 clawctl prepares and checks the agent-owned port/token. Packaged upstream openclaw devices lists 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 authenticated logs.tail RPC 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 before wizard.start, handle WinRT COMException during inspection, and classify explicit package unknown state 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 by dcc5556b; 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 of dcc5556b. 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 4050932131fa3ecac4228a3e49bf31cb1511e0c9 paired 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 active isolated-session-v1 record, and recorded LastConnected after 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 head 8961ca1b proved repeat prepare/check preserved an included literal token and valid config bytes. Its mode guard rejects partial port/token configuration without gateway.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

  • Bug fix
  • Feature
  • Refactor
  • Docs or instructions
  • Tests or validation
  • Security hardening
  • Chore or infrastructure

Scope

  • Tray or WinUI UX
  • Windows node capability
  • Local MCP or winnode
  • Gateway, connection, or pairing
  • Setup or onboarding
  • Permissions, privacy, or security
  • Tests, CI, or docs

Required 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, with OPENCLAW_REPO_ROOT set to this worktree and separate OPENCLAW_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.
  • Focused Connection (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.
  • Bounded rubber-duck implementation review completed with no actionable new issues. No claim of a fresh hosted ClawSweeper verdict.

Real behavior proof

Current-head runtime proof, Windows ARM64, disposable WSL Gateway and isolated tray data, from the strict command above:

Test Run Successful.
Total tests: 17
     Passed: 17
Gateway MXC proof passed: MirroredWslSafeGatewayPort_IsListeningAndRecorded
Gateway MXC proof passed: RealGateway_SystemRun_ExecutesThroughWindowsNodeMxcSandbox
Gateway MXC proof passed: RealGateway_SystemRun_BlocksWritesToTrayDataDirectoryInMxcSandbox
MXC validation completed successfully.

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> -RejectUnrelatedListener accepted a managed listener, rejected an unrelated same-port listener, and recovered after it stopped. Run-CredentialBoundaryProof.ps1 exercised production GatewayConnectionManager with 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

  • New permissions or capabilities? No.
  • Secrets or tokens handling changed? Yes.
  • New or changed network calls? Yes.
  • Command or tool execution surface changed? Yes.
  • Data access scope changed? Yes.
  • Mitigation: Companion receives the agent's effective token only through a package-qualified command and never puts it on upstream CLI argv or in logs. Missing, stale, or unrelated listener evidence denies credential handoff. Device approval is limited to its exact request and matching identity. Authenticated logs.tail reads 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

  • Backward compatible? No silent migration from a same-user profile/draft to the isolated runtime. Known legacy 0.0.0.0/0.0.0.1 proof packages and existing WSL profiles retain their separate paths.
  • Config or environment changes? Yes; isolated native setup configures the Gateway through its package, not another human-user openclaw.json.
  • Migration needed? Yes for existing records/drafts whose package switches runtime contracts. There is no automatic import of old host-user config or credentials.
  • Upgrade steps: For unfinished setup, choose Discard and set up again to remove only the incompatible draft descriptor and start isolated setup with a new Companion identity. For a published same-user profile, open Connections, select it, choose Remove, then Add Gateway > Install a local native Gateway. Existing workspace, configuration, and credential files remain on disk even when that removed profile's descriptor is subsequently discarded. Windows versions without process-sequence support must be updated before credential handoff; baseline clawctl gateway-service health still works.

Review Conversations

  • I replied to or resolved every bot review conversation addressed by this PR.
  • I left unresolved only conversations that still need maintainer judgment.

@clawsweeper

clawsweeper Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@clawsweeper clawsweeper Bot added P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 30, 2026
@clawsweeper

clawsweeper Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 30, 2026, 12:42 AM ET / 04:42 UTC (Revision 7).

ClawSweeper review

What this changes

Companion 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
Reviewed head: dcc5556bd93572cadcd7f4c9ac6062137cfd7081
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The source and focused tests are substantial, but native final-effect proof, upgrade compatibility, and the package dependency still limit merge confidence.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: Authority-chain proof required: the current-head terminal trace exercises a real WSL/MXC Gateway, while the changed production owner is the installed isolated Gateway package. Earlier-head native observations and a synthetic credential sink do not establish current-head allowed and unrelated or reassigned listener outcomes before real credential I/O. Native UI recovery and signed existing-record upgrade also lack current-head evidence; stored record and draft contracts change, so existing-state compatibility remains insufficient. Redact addresses, tokens, endpoints, and other private details from proof; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: Authority-chain proof required: the current-head terminal trace exercises a real WSL/MXC Gateway, while the changed production owner is the installed isolated Gateway package. Earlier-head native observations and a synthetic credential sink do not establish current-head allowed and unrelated or reassigned listener outcomes before real credential I/O. Native UI recovery and signed existing-record upgrade also lack current-head evidence; stored record and draft contracts change, so existing-state compatibility remains insufficient. Redact addresses, tokens, endpoints, and other private details from proof; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 9 items Introduced package contract: The new client invokes package-qualified companion preparation and check commands, consumes their port and token, and reads package listener status.
Credential boundary: The isolated runtime compares package attribution with live listener and process-sequence snapshots; the endpoint authorization path permits credentials only after expected ownership.
Stored-record upgrade boundary: The new serialized runtime contract distinguishes isolated records from existing same-user records. Existing records without it are refused when their package reports the isolated contract.
Findings None None.
Security Needs attention Resolve the package check’s read-only contract: Companion calls the package’s companion prepare --check during pairing and completion. The dependency PR reports that this supposedly read-only call may alter suspicious agent configuration, so its contract must be repaired or explicitly redesigned before this consumer lands.

How this fits together

Companion’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]
Loading

Decision needed

Question Recommendation
Is removing and recreating an existing same-user native profile an acceptable upgrade path once the isolated package ships? Require upgrade proof before acceptance: Keep the fail-closed policy under review until a signed existing-record upgrade demonstrates clear recovery and preserved data.

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

  • Add real behavior proof - Needs stronger real behavior proof before merge: Authority-chain proof required: the current-head terminal trace exercises a real WSL/MXC Gateway, while the changed production owner is the installed isolated Gateway package. Earlier-head native observations and a synthetic credential sink do not establish current-head allowed and unrelated or reassigned listener outcomes before real credential I/O. Native UI recovery and signed existing-record upgrade also lack current-head evidence; stored record and draft contracts change, so existing-state compatibility remains insufficient. Redact addresses, tokens, endpoints, and other private details from proof; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve security concern: Resolve the package check’s read-only contract - Companion calls the package’s companion prepare --check during pairing and completion. The dependency PR reports that this supposedly read-only call may alter suspicious agent configuration, so its contract must be repaired or explicitly redesigned before this consumer lands.
  • Resolve merge risk (P1) - The dependent Gateway package PR still has a read-only check defect: Companion invokes a check that may modify agent configuration during verification.
  • Resolve merge risk (P1) - Existing same-user native records stop connecting after the package changes contracts and require removal and reconfiguration; signed existing-state upgrade behavior and the intended user-facing policy have not been accepted or proven.
  • Resolve merge risk (P1) - Current-head evidence does not show the installed isolated package rejecting an unrelated or reassigned listener before real credential-bearing I/O, or show the changed native recovery UI and service restart behavior.
  • Complete next step (P2) - Resolve the dependent package check defect, obtain current-head installed-package authorization and recovery proof, and decide the existing-profile upgrade policy before merge.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

  • [medium] Resolve the package check’s read-only contract — src/OpenClaw.Connection/NativeGateway/NativeGatewayPackageClient.cs:76
Agent review details

Security

Needs 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

Metric Value Why it matters
Code and test growth production +1435/-51 lines; tests +1336/-117 lines The larger production path implements a separate package runtime, with substantial focused regression coverage that still needs native behavior proof.

Merge-risk options

Maintainer options:

  1. Prove the dependency and native upgrade (recommended)
    Resolve the package check defect and capture a signed upgrade plus current-head native authorization and recovery results before merge.
  2. Revisit the profile policy
    Pause landing if manual removal of published native profiles is not an acceptable upgrade experience.

Technical review

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

Labels

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P0: The affected native onboarding path blocks a first-time user from reaching a usable local Gateway.
  • merge-risk: 🚨 compatibility: Existing same-user native records fail closed after a package contract upgrade and require manual reconfiguration.
  • merge-risk: 🚨 security-boundary: The new isolated listener attribution gates credential-bearing connections, and the package check’s read-only contract remains unresolved.
  • merge-risk: 🚨 availability: Native service restart and detach behavior changes, while real installed-service failure-path proof is outstanding.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Authority-chain proof required: the current-head terminal trace exercises a real WSL/MXC Gateway, while the changed production owner is the installed isolated Gateway package. Earlier-head native observations and a synthetic credential sink do not establish current-head allowed and unrelated or reassigned listener outcomes before real credential I/O. Native UI recovery and signed existing-record upgrade also lack current-head evidence; stored record and draft contracts change, so existing-state compatibility remains insufficient. Redact addresses, tokens, endpoints, and other private details from proof; updating the PR body should trigger re-review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

  • [medium] Resolve the package check’s read-only contract — src/OpenClaw.Connection/NativeGateway/NativeGatewayPackageClient.cs:76
    Companion calls the package’s companion prepare --check during pairing and completion. The dependency PR reports that this supposedly read-only call may alter suspicious agent configuration, so its contract must be repaired or explicitly redesigned before this consumer lands.
    Confidence: 0.9

What I checked:

Likely related people:

  • karkarl: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Linus Huang: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • natalie-aguinaldo: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Resolve and verify the package’s read-only check before consuming it.
  • Add current-head final-effect proof for the allowed listener and nearest unrelated or reassigned listener before credential I/O.
  • Capture redacted signed existing-record upgrade and visible native recovery evidence.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (6 earlier review cycles)
  • reviewed 2026-09-30T02:02:35.336Z sha 4050932 :: needs real behavior proof before merge. :: [P1] Provide an in-app way to recover an incompatible saved draft | [P1] Add an upgrade path for existing native Gateway records
  • reviewed 2026-09-30T02:18:42.332Z sha 9c65f59 :: needs real behavior proof before merge. :: [P1] Provide in-app recovery for an incompatible saved draft | [P1] Preserve an upgrade path for existing native records
  • reviewed 2026-09-30T03:01:45.560Z sha 77a7eb6 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-30T03:25:01.587Z sha 77a7eb6 :: needs real behavior proof before merge. :: [P1] Preserve the old native workspace when discarding a draft | [P1] Keep console guidance recoverable after a failed log poll | [P1] Keep the started package separate from the probed package | [P2] Roll back only a service started by this call | [P2] Make the wizard’s explicit restart restart a prestarted service | [P2] Bound status inspection during authentication recovery
  • reviewed 2026-09-30T04:06:29.417Z sha 9382941 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-30T04:23:36.703Z sha 9382941 :: needs real behavior proof before merge. :: [P2] Retry the initial Gateway console read | [P2] Normalize WinRT package inspection failures | [P2] Preserve the reason for an unknown package status

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@paulcam206

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🦞👀
Exact review queued.

Re-review progress:

@karkarl

karkarl commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Hanselman review: PR #1553

PR: fix: Companion cannot onboard with an isolated Gateway

Base: 6bcc68cde97c4a3ebd479dddbb603f46aa562357

Final evaluated head: 77a7eb64d1c8635147be24bec6bb37b84f886ace

The checkout advanced from 9c65f594 during review. Both independent reviewers evaluated the incremental recovery commit before reconciliation.

Models: claude-opus-5.5, high reasoning, default context; gpt-6-astra, high reasoning, long_context.

Both models flagged: HIGH consensus

Issue Opus 5.5 GPT-6 Astra Fix confidence
Recovery recursively deletes the old native workspace, not merely the draft LOW-MEDIUM CRITICAL 95%
A single log-tail failure permanently removes wizard console feedback LOW MEDIUM 90%
  1. Blocking data loss: src/OpenClaw.SetupEngine/NativeGatewaySetupService.cs:126-133. Completing setup deliberately retains its draft descriptor. Removing the saved connection removes only the registry record. Following the newly prescribed upgrade recovery therefore makes a previously published profile look unpublished, and accepting discard recursively deletes its configuration and default workspace. The dialog says an existing Gateway is unaffected. Retain/archive native state and replace only the descriptor; add a publish/remove/recover regression test. Both models flagged the deletion; Astra identified the stronger previously-published reproduction. Coordinator reproduced deletion using disposable state and the actual service method.
  2. Lost console recovery: src/OpenClaw.SetupEngine.UI/WizardConsoleTail.cs:204-209 and src/OpenClaw.SetupEngine.UI/Pages/WizardPage.xaml.cs:1389-1412. One failed poll terminates the tail. The warning recommends opening a terminal, but the terminal remains inside GatewayRecovery, exposed only by ShowError; the log-failure handler merely appends text. Normal wizard interaction clears that text. Preserve the unavailable state and expose terminal/retry recovery independently of wizard errors. Both models identified permanent console loss; the hidden terminal is Astra's additional evidence.

Only one model flagged: LOW consensus

Issue Opus 5.5 GPT-6 Astra Fix confidence
Probing another package transfers the stop target but not lifecycle ownership not flagged MEDIUM 90%
A later authorization failure stops a service successfully started earlier MEDIUM not flagged 95%
Explicit Restart gateway does not restart an already-running package service not flagged MEDIUM 90%
Status-probe failure escapes the authentication-failure handler under its transition lock MEDIUM not flagged 90%
  1. Wrong stop target: src/OpenClaw.Connection/NativeGateway/IsolatedGatewayRuntime.cs:48-50,103-106,143-149. Start package A, inspect already-running B during shared-token replacement, then fail token validation. _package becomes B while _startedHere still describes A. Stop/dispose terminates B and leaves A running. The manager validates before changing active records and returns early on validation failure. Track owned-start identity separately from probe metadata. Reproduced with fake package commands and snapshots.
  2. Overbroad rollback: src/OpenClaw.Connection/NativeGateway/IsolatedGatewayRuntime.cs:72-80. _startedHere survives a successful call. A subsequent status/snapshot failure runs stop even though that invocation did not start the Gateway. Scope rollback to a local startedThisCall, retaining separate lifetime ownership. Reproduced with an incomplete snapshot after successful startup.
  3. Restart no-op: src/OpenClaw.Connection/NativeGateway/IsolatedGatewayRuntime.cs:143-149, called by src/OpenClaw.SetupEngine/NativeGatewaySetupSession.cs:161-178. The explicit wizard restart uses passive Stop, which does nothing for a service running before Companion attached. Ensure then observes the same running service and does not start it. Separate explicit restart from detach semantics. Verified by source tracing and the existing prestarted-service test, not live package execution.
  4. Auth recovery exception/stall: src/OpenClaw.Connection/NativeGateway/IsolatedGatewayRuntime.cs:88-110; affected unchanged caller src/OpenClaw.Connection/GatewayConnectionManager.cs:2200-2246. The fire-and-forget auth-failure handler holds the transition semaphore while awaiting package status without cancellation. The new subprocess can throw or time out after three minutes. An exception bypasses the state transition; a hang blocks other transitions. Use bounded status inspection and explicit fail-closed exception handling in the recovery path. Source-verified; no live hang reproduction.

Reconciliation and exclusions

  • Both models' original old-draft dead-end finding is resolved by 77a7eb64; do not report it as still open.
  • Inherited host environment was not accepted as a verified defect: it depends on external package forwarding behavior not established by this review.
  • Stale console-start races, recovery-dialog exception/cancellation handling, and expected-draft identity checks remain lower-confidence robustness concerns, not additional blocking findings.
  • Removed source-guard assertions are a coverage observation, not by themselves a demonstrated product regression.
  • Fix confidence is the coordinator's estimate, not probability of occurrence or a runtime-proof substitute.

Validation and proof limits

Focused tests rerun while HEAD remained 77a7eb64:

  • Connection: 33 passed, zero failed/skipped.
  • SetupEngine: 103 passed, zero failed/skipped.
  • Tray: 34 passed, zero failed/skipped.

Selectors: IsolatedGatewayRuntimeTests, NativeGatewayPackageClientTests, NativeGatewayRecordTests, WindowsProcessSequenceSnapshotTests; NativeGatewaySetupTests, ApprovalRequestHelperTests; NativeGatewaySetupUxContractTests, WizardConsoleTailTests. Repository-root and isolated tray-data environment variables were set. Initial no-restore failed due to absent assets; restore-enabled execution succeeded, followed by successful no-restore reruns at the final head.

Disposable coordinator harness output:

Published-then-removed workspace survived discard: False
Previously running A stopped after later snapshot failure: True
Cross-record stop targeted independently running B: True
Originally owned A left running: True

Harness uses synthetic data and fake package/process commands, not the user's Gateway. Source is preserved under review-repro next to this report.

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 9c65f594, not proof of final-head UI recovery.

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 8961ca1b1af3468bb4fccd267bf417a9cecf509d, newer than the contract revision referenced in the initial Companion PR body. This review does not certify that external implementation.

@karkarl karkarl added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 30, 2026
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
@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 30, 2026
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
@karkarl

karkarl commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Implemented the six verified Hanselman findings in the requested two commits:

  1. 4f1ab424: Preserve the old Gateway's data during upgrade recovery. Discard now removes only native-setup-draft.json, never workspace/configuration/identity/credentials. Regression tests also cover a published profile removed from Connections. This uses the same metadata-only discard principle as Natalie's feat(migration): ship the Inno-to-Store migration #1519 (feat(migration): ship the Inno-to-Store migration), without reusing its Inno-specific receipt/binding machinery or importing old data into the isolated account.
  2. 93829418: The other five findings. Separate package start ownership from per-record listener proofs; restrict failed-start rollback to the current call; explicitly restart pre-existing services without acquiring detach ownership; bound/fail-close passive package inspection; retry transient console reads and retain an independently visible terminal-recovery control.

Validation on 93829418: full build passed; Shared 4,168 passed / 34 skipped; Tray 3,341 passed; Connection 1,417 passed / 1 skipped; SetupEngine 1,452 passed / 1 skipped. Strict validate-mxc-e2e.ps1 -NoBuild passed 17/17, no skips, including the real Gateway-to-Windows-node MXC execution and write-denial proofs. Disposable fixture teardown succeeded. Rubber-duck implementation review completed and verified follow-ups were addressed.

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.

@karkarl karkarl removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 30, 2026
@karkarl
karkarl marked this pull request as ready for review September 30, 2026 04:01
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Sep 30, 2026
@karkarl

karkarl commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Hanselman repeat review: 93829418

Reviewed only 77a7eb64..93829418, affected callers/tests, and remaining contract questions. Independent reviewers: Claude Opus 5.5 (high) and GPT-6 Astra (high, long context). Commits are pushed, and the PR is now ready for review at the user's request. This is not merge approval.

Both models: no verified CRITICAL or HIGH source defect in this delta. Data-destructive recovery, cross-package stop ownership, historical-start rollback, and the pre-existing-service restart no-op are resolved at source level. Console recovery and passive inspection still have narrower gaps below.

Only one model flagged: LOW consensus

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.

@karkarl karkarl added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 30, 2026
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
@karkarl

karkarl commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Follow-up fixes are pushed in dcc5556b. All three verified MEDIUM findings from the repeat Hanselman review are addressed: initial log-anchor retry, WinRT inspection failure handling, and explicit unknown-state classification. Tests use the real isolated runtime/parser and verify that failed inspection does not release additional credentials. A bounded Astra implementation review of this follow-up found no actionable new issues.

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.

@karkarl karkarl removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 30, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Sep 30, 2026
@karkarl

karkarl commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Manual verification: native setup completed with the Gateway PR 134 artifact

The developer confirms that manual testing of Companion head dcc5556bd93572cadcd7f4c9ac6062137cfd7081, using the local MSIX bundle produced from openclaw/openclaw-windows-packaging#134 (fix: Companion cannot configure an isolated Gateway), got through the setup process.

  • Host: Windows ARM64.
  • Gateway artifact: OpenClawGateway.msixbundle, version 0.1.758.1, ARM64 payload.
  • Companion ran with a separate test data directory. Its normal installed-package discovery selected the locally registered Gateway, bypassing Store installation without a source-code override.
  • Because the supplied Store-submission bundle was unsigned, its payload was unpacked with Windows SDK MakeAppx and registered in Developer Mode from a readable test location. Earlier private-path access and raw-ZIP path-decoding errors were local staging mistakes, corrected before this successful run.

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.

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

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants