Repository navigation
fix: Companion cannot configure an isolated Gateway - #134
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🦞👀 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. |
|
Codex review: needs real behavior proof before merge. Reviewed September 30, 2026, 12:20 AM ET / 04:20 UTC (Revision 5). ClawSweeper reviewWhat this changesAdds a package command that prepares the isolated agent’s Gateway settings for Windows Companion and exposes listener identity in Gateway status. Merge readiness⛔ Blocked before merge - 16 items remain Keep this paired Companion integration open. The released package has no equivalent command, and the current branch still has source-backed configuration and listener-identity defects. The earlier x64 runs establish useful progress, but they do not prove the current credential handoff or existing-profile upgrade path. Priority: P0 Review scores
Verification
How this fits togetherThe Windows package runs OpenClaw in a recorded isolated agent session. Companion asks the package for the agent’s Gateway port, token, and listener evidence before connecting. flowchart LR
A[Windows Companion] --> B[Package command]
B --> C[Recorded agent session]
C --> D[Effective config reader]
D --> E{Usable Gateway settings?}
E -->|yes| F[Port and token]
E -->|no| G[Upstream config patch]
G --> F
C --> H[Listener identity status]
F --> A
H --> A
Decision needed
Why: The current check can mutate agent config, while the proposed ownership contract intentionally blocks older Windows builds; choosing an acceptable cross-product security and recovery boundary requires maintainer intent. Before merge
Findings
Agent review detailsSecurityNeeds attention: The check can mutate credential-bearing config, preparation can change authentication mode, and listener evidence is not bound to one observed process generation. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep upstream as the config owner, provide a supported nonmutating effective read, preserve existing authentication and partial config, bind listener ancestry to one process generation, and give Companion an in-product recovery path for unsupported Windows builds. Do we have a high-confidence way to reproduce the issue? Yes for the introduced preparation defects: an existing config without a Gateway section reaches the throwing mode guard, and an implicit-password config reaches the token-mode patch. These paths are established from source; this read-only review did not execute them. Is this the best way to solve the issue? No. Using the recorded agent session and upstream config owner fits the architecture, but the current read is mutating-capable and the patch must preserve existing authentication before this contract can ship. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against abf9e91d0885. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Hanselman review verdict: hold merge for the reviewed revisionReviewed head: Revision caveat: The PR now points to Both models flagged: HIGH consensus
The configuration findings are in Only one formal finding: LOW consensus
*Opus mentioned backup recovery as a proof concern rather than a formal finding. Coordinator verification confirmed the recovery path in the pinned upstream source. Consensus describes reviewer overlap, not severity; fix confidence is the coordinator's implementation assessment, not runtime proof. Local remediation and validationAll six findings were addressed in the review worktree, with regression tests and documentation updates. Those edits remain uncommitted and were not pushed by this review session. Their validation is not evidence for the current PR head.
The local fix prevents config backup recovery during checks; upstream config-health observations can still occur and are documented. Node adapter tests execute the production expression against synthetic SDK modules, not the packaged upstream payload. Remaining proof gates: current-head review and validation, actual packaged SDK behavior with JSON5/includes and config preservation, live MXC/Companion rejection before credential I/O, ARM64 execution, and MSIX build/upgrade validation for the fixes. The reviewed head's green CI does not substitute for these runtime checks or validate unpublished edits. |
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
Address the seven review findings with observation-free upstream snapshots, conditional configuration writes, authentication and environment validation, stable listener ancestry, and regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
|
||
| public SessionCompanionConfigNodeTests() | ||
| { | ||
| Directory.CreateDirectory(Path.GetDirectoryName(ConfigPath)!); |
| { | ||
| Directory.CreateDirectory(Path.GetDirectoryName(ConfigPath)!); | ||
| string sdk = Path.Combine(Application, "dist", "plugin-sdk"); | ||
| Directory.CreateDirectory(sdk); |
| Directory.CreateDirectory(Path.GetDirectoryName(ConfigPath)!); | ||
| string sdk = Path.Combine(Application, "dist", "plugin-sdk"); | ||
| Directory.CreateDirectory(sdk); | ||
| File.WriteAllText(Path.Combine(Application, "package.json"), """{"type":"module"}"""); |
| File.WriteAllText(Path.Combine(Application, "package.json"), """{"type":"module"}"""); | ||
| foreach (string module in new[] { "health", "config-mutation", "config-runtime" }) | ||
| { | ||
| File.WriteAllText(Path.Combine(sdk, module + ".js"), SdkFixture); |
Related: openclaw/openclaw-windows-node#1553 is the Companion consumer; review the two changes together. Historical consumer evidence below is pinned to its tested commits, not presented as proof of its latest head.
What Problem This Solves
Fixes: Windows Companion setup can finish without a usable Gateway when it writes configuration under the interactive user's profile rather than the Gateway MSIX's isolated agent profile.
User Impact
Companion prepares and rechecks the agent-owned Gateway without replacing an already usable local token configuration, including valid JSON5 and
$includesettings. An absent or empty Gateway can be initialized while preserving unrelated settings; a partial Gateway without explicitgateway.mode: "local"still requires deliberate recovery. Implicit password authentication and unsupported agent-side profile/Gateway overrides fail rather than silently changing credentials. Concurrent root or included-Gateway edits are not overwritten.Checks use an observation-free, core-only upstream snapshot reader: they do not restore backups, rewrite configuration, or record config-health observations. A running Gateway reports listener ownership evidence only when its listener and every ancestor used for attribution have stable process generations. Ordinary Gateway health remains available when process-sequence evidence is unavailable.
Why This Change Was Made
clawctl companion prepareand--checkcall the packaged upstreamplugin-sdk/healthsnapshot API inside the recorded isolated session, withobserve: false,recoverSuspicious: false, andpluginValidation: "core-only". Core-only validation also avoids plugin-migration reads that touched SQLite shared state in the real CI payload. The publicconfig getdisplay CLI remains unsuitable for credential delivery because it redacts tokens.When compatible settings are missing, the helper uses upstream
plugin-sdk/config-mutationunder its mutation lock, carrying the original config path, root hash, and effective Gateway as preconditions. Upstream retains configuration, include handling, and write ownership. Environment overrides are checked before and after upstream loading, and the selected path must match the agent profile. Tokens stay out of status and argv. The launcher/helper protocol version and release/runtime pins are unchanged.Guest process-sequence snapshots bracket listener, ancestry, and creation-time observations. Any missing, zero, or changed generation on the attribution chain withholds ownership evidence without removing the ordinary health fallback. Regression tests assert forbidden patch attempts after the production error boundary, rather than relying on exceptions that the boundary can catch.
Evidence
Head:
7d8d87a7577cd9d3aecd2854699a67af6017e5a3, containing all seven accepted review fixes on top of Paul'sc90844ad2047f109e2ab2d02c4f62b187ebcd147..\scripts\Test-DotNetQuality.ps1 -SkipRestore— passed, zero warnings/errors; formatting and style clean. Restore had already succeeded with process-localRestoreSources=https://api.nuget.org/v3/index.jsonafter the project changes.dotnet test .\OpenClaw.Gateway.MSIX.slnx --configuration Release --no-build --no-restore --verbosity quiet— passed, 1,275 tests, zero failed/skipped..\scripts\Test-NativeAotCli.Tests.ps1— final invocation passed 28 scenarios, including the helper snapshot/conditional-patch serialization path, the expected wrong-alias negative check, and cleanup. Earlier scratch-path/cleanup failures were investigated and their task-owned outputs removed; the final standard-path gate completed successfully.36661652346, identifying upstream OpenClaw 2026.9.5 /ec9c1a13db8938e5a3eaa51fca2e981cde2395a9, with its selected Node 24.20.0. The Node archive matched its official SHA-256. The approved-release upstream revision3a9d69db306cd7f081e06254cb89c4bcc14a7107was source-checked for the same public API contract; it was not the runtime payload used in these tests. Proof source, metadata, and logs are retained locally; disposable payloads, runtime, profiles, and probe build outputs were removed..\scripts\Test-DocReferences.ps1— passed, zero findings;git diff --check— passed.Historical installed-package and consumer evidence
These runs predate the current review-fix commit and do not substitute for a current-head installed-package test.
9743772d6175742f1466f604d40526640411a5f6, a locally registered disposable x64 Developer Mode package using cached payload from run36535899902ran prepare/check and repeated prepare with another preferred port. Gateway start/status reported a running, listener-attributed Gateway; packagedopenclaw devices list --jsonsucceeded. In a fresh isolated Companion profile, the package approved only that Companion's operator device; the user finished onboarding, separately approved the node role, and confirmed the first chat message worked. The test tray stopped and the package session was torn down without warning; package unregistration and disposable-profile deletion were left to the user at their request.8961ca1b1af3468bb4fccd267bf417a9cecf509d, a disposable x64companionfixpackage exercised the actual staged agent helper against synthetic JSON5 with nested$include. Prepare/check returned the literal included token and effective port without changing config SHA-256 or comments. Credentials were withheld from the report. The task-created patch session was torn down and its registration removed. The subsequentc90844amode guard and the current review fixes have not had another installed-package run.clawctland packaged upstream device commands. The human-confirmed onboarding/chat run used consumer head4050932131fa3ecac4228a3e49bf31cb1511e0c9. At historical head77a7eb64d1c8635147be24bec6bb37b84f886ace, localized draft replacement and published-profile recovery guidance were added; build, Shared (4,168 passed/34 skipped), Tray (3,336), SetupEngine (1,449 passed/1 skipped), and Connection (1,402 passed/1 skipped) checks were recorded. That is historical source/test proof, not a live WinGet installation claim.77a7eb64, a disposable x64 same-port listener test exercised the realGatewayConnectionManagerwith a synthetic credential sink. An unrelated listener produced zero sink calls and no TCP accept; after it stopped, ownership verification reached the sink exactly once. The managed package session was torn down. No real credential or WebSocket was sent; this does not substitute for current-head visible UI proof.Remaining Risk and Unrun Lanes
The previous observation-enabled-reader finding is addressed in this commit, not merely documented. The remaining release risk is deployment/end-to-end coverage: not run on this head are installed-package/Companion onboarding and credential handoff, ARM64 runtime execution, new MSIX composition, signing/upgrade lanes, and runtime tests against the approved-release payload. Prior live proofs and the real-SDK fixtures do not establish Store/WinGet acquisition or current-head wizard behavior. Marking this PR ready for review is not a claim that those release gates have passed.