Skip to content

fix: Companion cannot configure an isolated Gateway - #134

Merged
karkarl merged 4 commits into
mainfrom
fix/companion-isolated-gateway-contract
Sep 30, 2026
Merged

karkarl merged 4 commits into
mainfrom
fix/companion-isolated-gateway-contract

Conversation

@paulcam206

@paulcam206 paulcam206 commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

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 $include settings. An absent or empty Gateway can be initialized while preserving unrelated settings; a partial Gateway without explicit gateway.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 prepare and --check call the packaged upstream plugin-sdk/health snapshot API inside the recorded isolated session, with observe: false, recoverSuspicious: false, and pluginValidation: "core-only". Core-only validation also avoids plugin-migration reads that touched SQLite shared state in the real CI payload. The public config get display CLI remains unsuitable for credential delivery because it redacts tokens.

When compatible settings are missing, the helper uses upstream plugin-sdk/config-mutation under 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's c90844ad2047f109e2ab2d02c4f62b187ebcd147.

  • .\scripts\Test-DotNetQuality.ps1 -SkipRestore — passed, zero warnings/errors; formatting and style clean. Restore had already succeeded with process-local RestoreSources=https://api.nuget.org/v3/index.json after 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.
  • 17 real-SDK scenarios passed through the production helper/Node adapter with disposable profiles and a stripped child environment. They covered missing state/config/Gateway, compatible root and included-Gateway completion, strict partial-mode rejection, implicit local/remote password inference, JSON5 includes, dotenv overrides, malformed config with a backup, and concurrent root/include edits. Passive cases compared profile paths, file hashes, and last-write timestamps. No real package, isolated session, scheduled task, certificate, or agent profile was modified.
  • Runtime SDK proof used the x64 payload from workflow run 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 revision 3a9d69db306cd7f081e06254cb89c4bcc14a7107 was 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.

  • On package head 9743772d6175742f1466f604d40526640411a5f6, a locally registered disposable x64 Developer Mode package using cached payload from run 36535899902 ran prepare/check and repeated prepare with another preferred port. Gateway start/status reported a running, listener-attributed Gateway; packaged openclaw devices list --json succeeded. 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.
  • On package head 8961ca1b1af3468bb4fccd267bf417a9cecf509d, a disposable x64 companionfix package 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 subsequent c90844a mode guard and the current review fixes have not had another installed-package run.
  • Consumer fix: Companion cannot onboard with an isolated Gateway openclaw-windows-node#1553 uses package-qualified clawctl and packaged upstream device commands. The human-confirmed onboarding/chat run used consumer head 4050932131fa3ecac4228a3e49bf31cb1511e0c9. At historical head 77a7eb64d1c8635147be24bec6bb37b84f886ace, 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.
  • With the Connection assembly committed as 77a7eb64, a disposable x64 same-port listener test exercised the real GatewayConnectionManager with 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.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@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.

@clawsweeper

clawsweeper Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

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

ClawSweeper review

What this changes

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

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The branch has focused implementation and substantial validation, but configuration and listener-boundary defects plus incomplete upgrade and final-effect proof prevent merge confidence.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: Authority-chain proof required: prior-head disposable x64 runs exercised the staged config helper and reported successful onboarding, but the current-head mode guard has only supplemental tests. The paired Companion test rejected an unrelated listener before a synthetic credential sink; it did not show allowed and wrong or reassigned listeners at the credential-bearing WebSocket effect. Existing partial-config upgrade compatibility is also unverified. Provide redacted terminal traces or logs, update the PR body for re-review, and ask a maintainer to comment @clawsweeper re-review if that does not trigger.
Patch quality 🦪 silver shellfish (2/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: prior-head disposable x64 runs exercised the staged config helper and reported successful onboarding, but the current-head mode guard has only supplemental tests. The paired Companion test rejected an unrelated listener before a synthetic credential sink; it did not show allowed and wrong or reassigned listeners at the credential-bearing WebSocket effect. Existing partial-config upgrade compatibility is also unverified. Provide redacted terminal traces or logs, update the PR body for re-review, and ask a maintainer to comment @clawsweeper re-review if that does not trigger.
Evidence reviewed 13 items Introduced surface: The pinned branch adds the Companion command, agent helper, configuration protocol, and listener evidence; these are absent from the current main command tree.
Release boundary: Current main is tagged v2026.9.4-msix.5, and its documented command table has no Companion preparation command.
Release tag check: The current main commit is contained in v2026.9.4-msix.5; the PR’s integration is newer than that release.
Findings 4 actionable findings [P1] Make the check path genuinely read-only
[P1] Let prepare add an absent Gateway section
[P1] Preserve implicit password authentication
Security Needs attention Verification can restore agent config: The new check calls an upstream observed loader that can replace suspicious config with backup bytes before returning credentials.
Existing password authentication can change: The patch can select token mode for a profile whose omitted auth mode currently resolves to password, changing the Gateway credential policy.
Listener identity can mix process generations: Cached ancestry and later sequence and start-time reads can describe different processes after PID reuse, while the resulting identity is supplied to Companion for credential authorization.

How this fits together

The 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
Loading

Decision needed

Question Recommendation
Should the paired Companion integration wait for an observation-free upstream config reader and require Windows process-sequence support for credential handoff, or should its verification contract be redesigned? Wait for safe reader and approve support floor: Keep both drafts open until the upstream read is nonmutating, listener identities are generation-bound, and the Windows recovery policy is approved.

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

  • Add real behavior proof - Needs stronger real behavior proof before merge: Authority-chain proof required: prior-head disposable x64 runs exercised the staged config helper and reported successful onboarding, but the current-head mode guard has only supplemental tests. The paired Companion test rejected an unrelated listener before a synthetic credential sink; it did not show allowed and wrong or reassigned listeners at the credential-bearing WebSocket effect. Existing partial-config upgrade compatibility is also unverified. Provide redacted terminal traces or logs, update the PR body for re-review, and ask a maintainer to comment @clawsweeper re-review if that does not trigger.
  • Make the check path genuinely read-only (P1) - --check calls the pinned public loadConfig, which can restore suspicious config from backup and record observation state. The warning does not prevent those writes; use a supported observation-free effective read before Companion relies on this check.
  • Let prepare add an absent Gateway section (P1) - For an existing valid agent config with no gateway, the production reader emits {} and this guard rejects it before the patch path. Treat a wholly absent Gateway as missing settings while retaining the rejection for an incompatible partial Gateway.
  • Preserve implicit password authentication (P1) - A local Gateway with auth.password and no explicit mode currently resolves to password authentication upstream. This new patch writes auth.mode: token when the token is absent, silently changing the existing setup. This is a late finding on code unchanged since the prior reviewed head; reject or preserve that configuration before writing.
  • Bind listener ancestry and sequence to one generation (P1) - Ancestry is read from a cached process snapshot, then a later sequence and start time are attached by PID. If the former listener exits and that PID is reused between observations, the exported identity can combine the old descendant relationship with a new process generation. This is a late finding on the same reviewed head; reject a generation mismatch before publishing ownership evidence.
  • Resolve security concern: Verification can restore agent config - The new check calls an upstream observed loader that can replace suspicious config with backup bytes before returning credentials.
  • Resolve security concern: Existing password authentication can change - The patch can select token mode for a profile whose omitted auth mode currently resolves to password, changing the Gateway credential policy.
  • Resolve security concern: Listener identity can mix process generations - Cached ancestry and later sequence and start-time reads can describe different processes after PID reuse, while the resulting identity is supplied to Companion for credential authorization.
  • Resolve merge risk (P1) - Existing partial agent configurations, including JSON5 and included values, lack current-head upgrade proof; preparation can change their authentication or persisted settings.
  • Resolve merge risk (P1) - The package exports listener evidence for a credential-bearing Companion connection, but the nearest wrong or reassigned listener has not been rejected at the final network effect.
  • Resolve merge risk (P1) - Companion fails closed when Windows lacks process-sequence support, leaving affected existing users needing an OS update; the supported recovery and upgrade policy needs maintainer approval.
  • Complete next step (P2) - Fix the four source-backed defects, prove partial-config upgrades and allowed and forbidden credential effects, and obtain the Windows support and upstream-reader decision before merge.
  • Improve patch quality - Use a supported observation-free effective reader and cover absent Gateway and implicit-password existing-profile upgrades.
  • Improve patch quality - Bind listener ancestry to the same process generation as its sequence identity, then prove rejection of the nearest wrong or reassigned listener before credential I/O.
  • Improve patch quality - Record the supported Windows floor and an in-product recovery path for builds without process-sequence inspection.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

  • [P1] Make the check path genuinely read-only — src/OpenClaw.SessionHost/SessionCompanionConfig.cs:31-36
  • [P1] Let prepare add an absent Gateway section — src/OpenClaw.SessionHost/SessionCompanionConfig.cs:225-230
  • [P1] Preserve implicit password authentication — src/OpenClaw.SessionHost/SessionCompanionConfig.cs:145-148
  • [medium] Verification can restore agent config — src/OpenClaw.SessionHost/SessionCompanionConfig.cs:32
  • [medium] Existing password authentication can change — src/OpenClaw.SessionHost/SessionCompanionConfig.cs:147
  • [medium] Listener identity can mix process generations — src/OpenClaw.SessionHost/SessionInspector.cs:106
Agent review details

Security

Needs 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

Metric Value Why it matters
Code growth production +940 net lines, tests +662 net lines The new session helper, protocol, and listener inspection explain much of the growth, but create a substantial package contract to validate.

Merge-risk options

Maintainer options:

  1. Repair and prove the paired contract (recommended)
    Fix the config and listener-generation defects, then demonstrate existing-profile upgrades and allowed and forbidden credential handoffs through the real network owner.
  2. Pause the integration
    Keep the paired package and Companion work open while the upstream read API and Windows support decision remain unresolved.

Technical review

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

  • [P1] Make the check path genuinely read-only — src/OpenClaw.SessionHost/SessionCompanionConfig.cs:31-36
    --check calls the pinned public loadConfig, which can restore suspicious config from backup and record observation state. The warning does not prevent those writes; use a supported observation-free effective read before Companion relies on this check.
    Confidence: 0.98
  • [P1] Let prepare add an absent Gateway section — src/OpenClaw.SessionHost/SessionCompanionConfig.cs:225-230
    For an existing valid agent config with no gateway, the production reader emits {} and this guard rejects it before the patch path. Treat a wholly absent Gateway as missing settings while retaining the rejection for an incompatible partial Gateway.
    Confidence: 0.97
  • [P1] Preserve implicit password authentication — src/OpenClaw.SessionHost/SessionCompanionConfig.cs:145-148
    A local Gateway with auth.password and no explicit mode currently resolves to password authentication upstream. This new patch writes auth.mode: token when the token is absent, silently changing the existing setup. This is a late finding on code unchanged since the prior reviewed head; reject or preserve that configuration before writing.
    Confidence: 0.94
    Late finding: first raised on code an earlier review cycle already covered.
  • [P1] Bind listener ancestry and sequence to one generation — src/OpenClaw.SessionHost/SessionInspector.cs:98-111
    Ancestry is read from a cached process snapshot, then a later sequence and start time are attached by PID. If the former listener exits and that PID is reused between observations, the exported identity can combine the old descendant relationship with a new process generation. This is a late finding on the same reviewed head; reject a generation mismatch before publishing ownership evidence.
    Confidence: 0.86
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against abf9e91d0885.

Labels

Label changes:

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

Label justifications:

  • P0: A first-time Companion user can finish setup without a usable isolated Gateway, and current recovery requires technical agent-profile work.
  • merge-risk: 🚨 compatibility: Preparation can change an existing implicit-password setup and lacks complete partial-config upgrade proof.
  • merge-risk: 🚨 security-boundary: The PR returns an agent token and listener identities across the Companion boundary, while generation binding and final-effect rejection remain unresolved.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦐 gold shrimp and patch quality is 🦪 silver shellfish.
  • 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: prior-head disposable x64 runs exercised the staged config helper and reported successful onboarding, but the current-head mode guard has only supplemental tests. The paired Companion test rejected an unrelated listener before a synthetic credential sink; it did not show allowed and wrong or reassigned listeners at the credential-bearing WebSocket effect. Existing partial-config upgrade compatibility is also unverified. Provide redacted terminal traces or logs, update the PR body for re-review, and ask a maintainer to comment @clawsweeper re-review if that does not trigger.

Evidence

Security concerns:

  • [medium] Verification can restore agent config — src/OpenClaw.SessionHost/SessionCompanionConfig.cs:32
    The new check calls an upstream observed loader that can replace suspicious config with backup bytes before returning credentials.
    Confidence: 0.98
  • [medium] Existing password authentication can change — src/OpenClaw.SessionHost/SessionCompanionConfig.cs:147
    The patch can select token mode for a profile whose omitted auth mode currently resolves to password, changing the Gateway credential policy.
    Confidence: 0.94
  • [medium] Listener identity can mix process generations — src/OpenClaw.SessionHost/SessionInspector.cs:106
    Cached ancestry and later sequence and start-time reads can describe different processes after PID reuse, while the resulting identity is supplied to Companion for credential authorization.
    Confidence: 0.86

What I checked:

  • Introduced surface: The pinned branch adds the Companion command, agent helper, configuration protocol, and listener evidence; these are absent from the current main command tree. (src/OpenClaw.Launcher/ClawCtlCommandLine.cs:344, c90844ad2047)
  • Release boundary: Current main is tagged v2026.9.4-msix.5, and its documented command table has no Companion preparation command. (README.md:141, abf9e91d0885)
  • Release tag check: The current main commit is contained in v2026.9.4-msix.5; the PR’s integration is newer than that release. (abf9e91d0885)
  • Dependency signal: The introduced helper imports the packaged upstream plugin SDK config runtime and calls loadConfig, so that loader’s read behavior is part of this change. (src/OpenClaw.SessionHost/SessionCompanionConfig.cs:28, c90844ad2047)
  • Pinned upstream owner: The package release policy identifies openclaw/openclaw and pins its approved payload commit. (release-policy.json:7, abf9e91d0885)
  • Observation-enabled read: At the pinned upstream commit, public loadConfig constructs config I/O without observe:false; the separate policy-check reader explicitly disables observation and suspicious recovery. (src/config/io.runtime.ts:98, 3a9d69db306c)

Likely related people:

  • paulcam206: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Anna Koenig: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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 (4 earlier review cycles)
  • reviewed 2026-09-30T02:01:07.237Z sha 9743772 :: blocked before merge. :: [P1] Read supported Gateway config through its effective owner | [P1] Preserve comments before repeating the config patch | [P2] Align the documented Companion flow with its consumer
  • reviewed 2026-09-30T02:18:05.445Z sha 9743772 :: needs real behavior proof before merge. :: [P1] Read supported Gateway config through its effective owner | [P1] Preserve comments before repeating the config patch | [P2] Align listener recovery guidance with the actual response
  • reviewed 2026-09-30T03:02:51.244Z sha 8961ca1 :: needs real behavior proof before merge. :: [P1] Require a startable local mode before returning ready | [P1] Make the check path genuinely read-only | [P3] Describe the SDK config read accurately
  • reviewed 2026-09-30T03:25:24.613Z sha c90844a :: needs real behavior proof before merge. :: [P1] Make the check path genuinely read-only | [P1] Let prepare add an absent Gateway section

@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: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 30, 2026
paulcam206 and others added 2 commits September 29, 2026 19:49
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@karkarl

karkarl commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Hanselman review verdict: hold merge for the reviewed revision

Reviewed head: 8961ca1b1af3468bb4fccd267bf417a9cecf509d
Base: abf9e91d0885488b9c31a2e769cd8a07b35cb183
Independent reviewers: Claude Opus 5.5 and GPT-6 Astra, both at high reasoning; Astra used long context.

Revision caveat: The PR now points to c90844ad2047f109e2ab2d02c4f62b187ebcd147. That newer revision was not reviewed in this pass. The findings below describe the reviewed head, not a claim that all six remain in the current head.

Both models flagged: HIGH consensus

Finding Opus 5.5 GPT-6 Astra Fix confidence
Preparation/check can report success with a port and token but no gateway.mode; upstream then refuses Gateway startup. MEDIUM MEDIUM 95%
Authentication with an implicit password mode can be silently converted to token authentication. MEDIUM MEDIUM 90%
Override checks miss upstream-loaded dotenv settings and config-selection inputs, risking a wrong reported port or a write to another selected config. MEDIUM MEDIUM 90%
Cached process ancestry can be combined with a reused PID's new sequence identity. The listener and its ancestry must be bound to the same process generation. LOW MEDIUM 75%

The configuration findings are in src/OpenClaw.SessionHost/SessionCompanionConfig.cs; the process-generation finding is in src/OpenClaw.SessionHost/SessionInspector.cs:85-111 at the reviewed revision. The race is supported by source inspection; no Windows exploit or credential disclosure was demonstrated.

Only one formal finding: LOW consensus

Finding Opus 5.5 GPT-6 Astra Fix confidence
--check calls upstream loadConfig, which can recover a suspicious config by replacing it with backup contents. not flagged* HIGH 90%
A concurrent config update between the initial read and the later patch can be overwritten without detecting that update. not flagged MEDIUM 75%

*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 validation

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

  • Canonical scripts/Test-DotNetQuality.ps1 -SkipRestore: passed after an explicit nuget.org restore, avoiding conflicting machine-level feeds without changing machine settings.
  • Full Release solution tests: 1,267 passed.
  • scripts/Test-NativeAotCli.Tests.ps1: 28 scenarios passed, including the new config snapshot serialization scenario, and the expected wrong-alias negative check passed. The script nevertheless exited with a temporary-file cleanup error; both task-owned output directories were subsequently removed manually.
  • scripts/Test-DocReferences.ps1 and git diff --check: passed.

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.

@paulcam206

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🦞👀
Exact review queued.

Re-review progress:

@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
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>
@karkarl
karkarl marked this pull request as ready for review September 30, 2026 05:51
@karkarl
karkarl merged commit 30c3cbe into main Sep 30, 2026
3 of 4 checks passed

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);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants