Skip to content

feat(uninstall): WSL gateway uninstall — engine, CLI, Inno hook, MSIX docs, Settings UI - #310

Merged
shanselman merged 29 commits into
openclaw:masterfrom
indierawk2k2:feat/wsl-gateway-uninstall
May 12, 2026
Merged

feat(uninstall): WSL gateway uninstall — engine, CLI, Inno hook, MSIX docs, Settings UI#310
shanselman merged 29 commits into
openclaw:masterfrom
indierawk2k2:feat/wsl-gateway-uninstall

Conversation

@indierawk2k2

@indierawk2k2 indierawk2k2 commented May 11, 2026

Copy link
Copy Markdown
Contributor

What this PR ships

Completes the WSL Gateway uninstall story end-to-end: a 13-step idempotent engine (LocalGatewayUninstall) wired through the CLI, Inno Setup hook, Portable docs, MSIX docs + warning banner, and the Settings UI teardown flow ΓÇö plus four P0/P1 bug fixes identified during the bug-pass (BUG-01 through BUG-05). All functional work from .squad/decisions/inbox/kranz-uninstall-plan-v3.md is landed; removal of the dead OPENCLAW_WSL_INSTALL_LOCATION knob closed the VHD-orphan risk identified there.

Surfaces

Surface Change
Inno Setup hook [UninstallRun] entry calls uninstall-wsl-gateway.ps1; P0 BUG-01 fix ensures Inno waits for the hook to finish before {app} deletion; cmdpal skipifsourcedoesntexist flag added (P1 BUG-05)
MSIX docs/uninstall-msix.md created; PathA storage layout confirmed empirically; Mattingly's warning banner wired in Settings (LocalGatewaySection)
Portable docs/uninstall-portable.md created; manual cleanup steps documented
Settings UI LocalGatewaySection keeps the section visible during/after removal (BUG-03 fix), shows MSIX warning, drives LocalGatewayUninstall
CLI flag --uninstall-wsl-gateway flag added to the CLI entry point
Validate script validate-wsl-gateway-uninstall.ps1 ΓÇö post-removal verification checklist

Engine summary

LocalGatewayUninstall runs 13 idempotent steps:

  1. Distro-name guard (rejects empty/null; prevents accidental default-distro deletion)
  2. WSL stop with 30 s timeout wrap (P1 BUG-02 fix ΓÇö avoids hang on wedged/stopped distro)
  3. VHD unregister
  4. VHD file delete
  5. ~/.openclaw-dev WSL home dir removal
  6. WSL distro entry removal
  7. Windows AppData OpenClawGateway\ removal
  8. mcp-token field preserved in device-key-ed25519.json (token nulled, file kept)
  9. Device-key file preserved (key material zeroed, file not deleted)
  10. Registry cleanup
  11. Firewall rule removal
  12. Scheduled task removal
  13. Final idempotency check pass

DryRun-default safety: engine defaults to DryRun=true; callers must explicitly opt in to DryRun=false. ConfirmDestructive gate: destructive steps (VHD delete, AppData wipe) require an additional boolean confirmation parameter.

Knob removed

OPENCLAW_WSL_INSTALL_LOCATION was dead code (never read at runtime, only set during install). Removing it eliminates the VHD-orphan risk where a stale env var could have caused uninstall to target a non-existent path. Identified in kranz-uninstall-plan-v3.md.

Tests

  • 20 LocalGatewayUninstall unit tests shipped with Commit 3 (engine core)
  • +5 added during gap-closure pass (Commit 5)
  • +2 added during BUG-02 fix (timeout-wrap coverage)
  • +1 MSIX storage-path validation test + Inno [UninstallRun] ordering test (Commit 5 / 790f6e7)
  • Current Tray suite: 932 pass, 8 pre-existing locale-resource fails (present on master before this branch; unrelated to uninstall work)

Empirical validation

  • PathA (%LOCALAPPDATA%\Packages\<FamilyName>\LocalCache) confirmed as the correct MSIX storage root via live test on MSIX build. Bostick's verdict documented in .squad/decisions/inbox/bostick-msix-storage-verdict.md.
  • Mattingly's MSIX warning banner (Settings UI) matches the confirmed PathA layout ΓÇö banner text is accurate.

Outstanding follow-ups (NOT blocking this PR)

Kranz install-side audit (.squad/decisions/inbox/kranz-install-appdata-consent-audit.md): 5 pre-existing gaps on master unrelated to uninstall logic ΓÇö consent surfacing, AppData migration on upgrade, stale registry on partial install failure, etc. These existed before this branch and are tracked separately.

Bostick P2/P3 polish (.squad/decisions/inbox/bostick-install-uninstall-bug-pass.md): cosmetic / edge-case items deferred ΓÇö progress dialog polish, partial-failure rollback UX, distro-rename detection. None are blocking correctness.

Squashability

14 functional commits + 3 squad-housekeeping commits (chore(squad): ...). Left as-is for review readability. Can squash to 1ΓÇô2 commits on merge if preferred.

Key inbox docs

  • .squad/decisions/inbox/kranz-uninstall-plan-v3.md ΓÇö uninstall plan, engine spec, knob-removal rationale
  • .squad/decisions/inbox/kranz-install-appdata-consent-audit.md ΓÇö install-side audit (follow-ups)
  • .squad/decisions/inbox/bostick-install-uninstall-bug-pass.md ΓÇö P0/P1/P2/P3 bug pass findings
  • .squad/decisions/inbox/bostick-msix-storage-verdict.md ΓÇö PathA empirical confirmation

Known issues (not introduced by this PR)

  • ~65 .NET runtime DLLs may remain locked under {app}\ after Inno uninstall completes until the next user reboot. This is a pre-existing limitation of how the .NET process teardown releases file handles, NOT a regression from this PR's uninstall logic. All WSL/config/user-data postconditions verified clean by Bostick's empirical pass on 2026-05-11. See .squad/decisions/inbox/bostick-fix-validation-pass.md for evidence.
  • BUG-03 (autostart-reset empty exception, P3 polish from earlier bug pass) deferred to backlog — not in scope for this PR.
  • 5 install-side audit findings (1 P1 + 4 P2) from Kranz's pre-existing-APPDATA consent audit — all are pre-existing in master, none introduced by this PR. Tracked separately for follow-up issues.

Mike Harsh and others added 17 commits May 10, 2026 20:32
Production code path always lands at %LOCALAPPDATA%\OpenClawTray\wsl\OpenClawGateway.
The env-var was a test-only artifact ported from the prototype that no production
caller ever set. Eliminating it removes orphaned-VHD risk in the upcoming uninstall
flow (no need to persist install location in setup-state.json).

InstanceInstallLocation field on LocalGatewaySetupOptions retained as a constructor
test seam for direct injection from xUnit tests.

Pre-existing failures on PR openclaw#274 base (NOT introduced by this commit):
- 8 LocalizationValidationTests fail with InvalidOperationException:
  "Could not find repository root. Set OPENCLAW_REPO_ROOT to the repo path."
  This is a worktree environment issue; unrelated to env-var removal.

Refs: .squad/decisions/inbox/kranz-uninstall-plan-v3.md (commit 1 of 7)
Stacked on PR openclaw#274.

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

Creates scripts/_uninstall-helpers.ps1 with five reusable helper functions:
  - Test-IsOpenClawOwnedDistroName: distro-name guard (only OpenClawGateway*)
  - Invoke-WslCommand: wsl bash -c runner returning Stdout/Stderr/ExitCode
  - Stop-OpenClawProcessByPid: PID-based process termination, suppresses not-found
  - Assert-DryRunGate: throws if dry-run mode is active
  - Add-Step: structured step-log appender (requires caller's  array)

scripts/reset-openclaw-wsl-validation-state.ps1 updated to dot-source the helpers
file. No behaviour change.

Refs: .squad/decisions/inbox/kranz-uninstall-plan-v3.md (commit 2 of 7)
Stacked on PR openclaw#274.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ision drop-box + agent histories)

Bostick's anticipatory MSIX validation script (scripts/validate-msix-storage-paths.ps1)
plus accumulated .squad/ decision-inbox files and agent history updates from the
plan/review/implementation cycle. These were preserved across the re-baseline onto
PR openclaw#274 head.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Merged inbox file (bostick-msix-validation-script.md) into decisions.md.
Archived inbox processing entry.

Created orchestration logs for all 5 agents (Aaron, Mattingly, Kranz,
RubberDucky, Bostick) summarizing spawn manifest work and dependencies.

Created session log documenting uninstall plan v2→v3 acceptance journey,
PR openclaw#274 rebaseline (commits cd1a83b, 83eadcf, 22bda40), and in-flight
work (commits 3+4 expected next spin).

Health metrics recorded: decisions.md size 717419 bytes (>51200 threshold,
archive deferred to next spin), inbox count 1 (merged).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Delivers Commit 3 of 7 from the Kranz v3 uninstall plan:

- NEW  src/OpenClaw.Tray.WinUI/Services/LocalGatewaySetup/LocalGatewayUninstall.cs
  13-step idempotent uninstall sequence.  DryRun=true is the safety
  default.  Types: UninstallStepStatus, UninstallStep, LocalGateway-
  UninstallPostconditions, LocalGatewayUninstallOptions, LocalGateway-
  UninstallResult, LocalGatewayUninstall.  Build() factory mirrors the
  LocalGatewaySetupEngineFactory pattern.  Registry helpers are guarded
  with OperatingSystem.IsWindows() to satisfy CA1416 on net10.0.

- MOD  src/OpenClaw.Shared/DeviceIdentity.cs
  Added TryClearDeviceToken(string dataPath, IOpenClawLogger?) — nulls
  DeviceToken + DeviceTokenScopes, preserves file and all other fields
  (including mcp-token.txt path, per v3 section-F).

- MOD  tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj
  Added <Compile> link for LocalGatewayUninstall.cs.

- NEW  tests/OpenClaw.Tray.Tests/LocalGatewayUninstallTests.cs
  20 WindowsFact tests covering: DryRun safety, real destructive mode,
  idempotency, step ordering, file preservation, registry, device-key
  nulling, mcp-token preservation, error propagation, cancellation, and
  the ConfirmDestructive preflight guard.

Build: clean (./build.ps1)
Tests: 632 pass / 8 fail (8 pre-existing localization failures, unchanged)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirror of validate-wsl-gateway.ps1 for inverse postcondition verification.
Three modes: PreflightOnly (snapshot + distro guard), Full (snapshot ->
execute uninstall steps -> snapshot -> verdict), PostconditionOnly (verify
current state matches uninstall-complete expectations).

Reuses shared safety helpers from _uninstall-helpers.ps1 (Test-IsOpenClaw-
OwnedDistroName, Invoke-WslCommand, Stop-OpenClawProcessByPid, Assert-Dry-
RunGate, Add-Step) -- no duplication.

Replicates LocalGatewayUninstall's 13-step sequence in PowerShell so it
can be run end-to-end without invoking the tray app. C# engine remains the
production code path; this script must stay aligned with it.

Exit codes: 0=PASS, 1=FAIL, 2=preflight blocked, 3=mid-execution failure.

Refs: .squad/decisions/inbox/kranz-uninstall-plan-v3.md (commit 6 of 7)
Stacked on PR openclaw#274.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…, Settings UI (Commit 5)

Surface A — CLI --uninstall flag (App.xaml.cs)
- Add early --uninstall interception in OnLaunched before mutex check
- Attach parent console via AttachConsole P/Invoke for stdout visibility
- RunCliUninstallAsync: runs full uninstall engine, outputs JSON result,
  redacts any access tokens via CliRedact() before printing

Surface B — Inno [UninstallRun] hook
- installer.iss: add [Files] entry for scripts\Uninstall-LocalGateway.ps1
- installer.iss: add [UninstallRun] entry (runs before {app}\ deletion)
- scripts/Uninstall-LocalGateway.ps1: thin PS wrapper that locates the
  EXE, invokes --uninstall --confirm-destructive --json-output, always
  exits 0 so Inno does not abort the uninstall on failure

Surface C — docs/uninstall-portable.md
- Document portable ZIP uninstall: in-tray button path and CLI flag path
- Include WARNING about orphan risk if tray is force-killed

Surface D — docs/uninstall-msix.md
- MSIX runFullTrust has no supported CustomUninstall extension point
- Verdict: MSIX hook not feasible; in-tray button is the only safe path
- Include manual recovery steps for edge cases

Surface E — Settings page "Remove Local Gateway" UI
  (originally planned as a separate commit 4; bundled here during staging)
- src/OpenClaw.Tray.WinUI/Pages/SettingsPage.xaml (+61): new "🗑️ Local
  Gateway" Expander after NotificationsExpander; auto-hidden when no
  local gateway install is detected; MSIX warning InfoBar at top of
  section (PathA-conservative, contingent on Bostick's MSIX validation
  in commit 7); Grid-based button row (no HStack); read-only TextBlock
  body; status TextBlock + result InfoBar.
- src/OpenClaw.Tray.WinUI/Pages/SettingsPage.xaml.cs (+157): wires the
  button click to LocalGatewayUninstall.Build(...).RunAsync(...) with
  ContentDialog confirmation gate (default = Cancel for safety);
  in-progress UI during RunAsync; success/failure InfoBar presentation;
  detects MSIX context via PackageHelper.IsPackaged for the warning.
- Plain default-style button per Mike's design guidance — destructive
  weight carried by the dialog body, not the button color.
- Token/state language abstracted in dialog and result UI; no token
  values are surfaced.
- ⚠️ Screenshot verification deferred — Mattingly's environment had no
  display session. Mike to verify locally before merging.

Gap closure 5E(b) — run.marker cleanup
- LocalGatewayUninstall.cs Step 8a: idempotent delete of run.marker
  (skip-if-absent so stale crash markers do not cause step failure)
- Confirmed run.marker is written only in App.xaml.cs (no setup writer)

Gap closure 5E(c) — VHD parent-dir cleanup
- LocalGatewayUninstall.cs Step 5a: delete %LOCALAPPDATA%\OpenClawTray\wsl\<DistroName>
  directory after distro unregister step
- Add VhdDirAbsent postcondition; include in ComputePostconditionsAsync

Surface F — validate script CLI delegation
- validate-wsl-gateway-uninstall.ps1: -NoCli and -ExePath params
- Get-TrayExePath searches Inno layout, repo layout, then explicit hint
- Full mode delegates to CLI by default; -NoCli forces inline PS fallback
- Add vhd_dir_absent to Get-Postconditions and Get-Verdict required keys

Tests
- LocalGatewayUninstallTests.cs: 5 new [WindowsFact] tests (21-25)
  covering VHD dir present/absent/dry-run and run.marker present/absent

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

7A: wire validate-msix-storage-paths.ps1 to Aaron's --uninstall CLI flag
- Add Phase 4a Invoke-CliEngineUninstall between probe and post-snapshot
- Capture engine_cli_invoked, engine_cli_exit_code, engine_postconditions in verdict.json
- Compute cross_check_consistent in Invoke-Teardown after orphan data is known
  PathA: engineWslAbsent && survivors > 0
  PathB: engineWslAbsent && survivors == 0
  Inconclusive or non-zero CLI exit: always false

7B: add tests/PackagingTests/Test-InnoUninstallOrdering.ps1
- Installs and uninstalls the Inno package silently, parsing /LOG= output
- PASS when Uninstall-LocalGateway hook line appears before app dir deletion line
- Exit codes: 0=PASS, 1=FAIL, 2=SKIP (no installer), 3=ERROR
- Runs SKIP on this machine (no installer binary present, expected)

7C: MSIX build succeeded (dotnet msbuild, AppxPackageSigningEnabled=false)
- validate-msix-storage-paths.ps1 -WhatIf: all 4 preflight checks PASS, exit 0
- Full validation: Inconclusive; blocked by cert trust dialog on root store add
  (self-signed test cert cannot be non-interactively trusted without admin)
- Track as TODO: re-run on CI VM with pre-provisioned sideload cert

7D: drop .squad/decisions/inbox/bostick-uninstall-commit-7.md
- MSIX verdict, blocker table, Mattingly banner recommendation, open PR items

Also: add msix-validation-evidence/, packaging-test-output/,
uninstall-validation-output/ to .gitignore

Build: 4/4 PASS. Shared tests: exit 0. Tray tests: 636/9 (8 pre-existing
LocalizationValidation + 1 flaky SettingsManagerIsolation env-var race,
not caused by PS-only changes).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Real-world testing surfaced a UX bug: when uninstall succeeds, the engine
deletes setup-state.json, which caused the section's visibility check to
evaluate Collapsed mid-flight. The success InfoBar lived inside the section
and was never seen by the user -- the section just vanished.

Fix: latch _uninstallInitiatedThisSession to true on button click; section
visibility now returns Visible if (setup-state.json exists OR latch is set).
The latch resets on page navigation, so steady-state behavior (section
hidden when no local gateway) is preserved on Settings re-open.

Button morphs through three states:
  Idle:        "Remove Local Gateway" (default style)
  In-progress: ProgressRing + "Removing distro..." (disabled)
  Success:     button hidden; success InfoBar carries the message
  Failure:     button re-enabled for retry; error InfoBar shown

MSIX warning InfoBar hides on success (gateway gone -> warning is moot).

Refs: real-user feedback during commit 5 verification (Mike's accidental
delete-then-success path produced zero UI feedback).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Master (PR openclaw#274 merge + subsequent commits) renamed SettingsManager.Token
→ LegacyToken and BootstrapToken → LegacyBootstrapToken (private set).
Token/BootstrapToken are no longer written by Save() — GatewayRegistry is
now the source of truth.

Changes:
- LocalGatewayUninstall.cs: _settings.Token → _settings.LegacyToken in
  Step 4 (revoke-operator-token check + token extraction).
  Step 11: removed dead assignments _settings.Token/BootstrapToken = ''.
  Save() already clears legacy fields by omission.
- LocalGatewayUninstallTests.cs: write legacy-format settings.json directly
  (Token/BootstrapToken fields) and reload to prime LegacyToken/LegacyBootstrapToken.
  Updated assertions to use LegacyToken/LegacyBootstrapToken and IsNullOrEmpty.

Rebase-onto-master compatibility fix only. No logic changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…pp} deletion (P0 BUG-01, P1 BUG-04)

- Add runhidden flag (was in original commented-out Flags; suppresses console flash)
- Strengthen the warning comment to make clear that waituntilterminated MUST NOT be
  removed: without it Inno deletes {app} while the PowerShell hook is still running,
  leaving 279+ application files behind after unins000.exe reports exit 0 (BUG-01).
- waituntilterminated was already present (added in Commit 5); this commit adds
  runhidden and the do-not-remove guardrail comment (BUG-04 full fix).

Date: 2026-05-10

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d/stopped distro (P1 BUG-02)

Step 3 (Stop systemd gateway service) previously called wsl -d OpenClawGateway
even when the distro was registered but Stopped.  WSL would boot the distro, run
systemctl stop (a no-op), then hang ~30 s waiting for its own session to terminate.

Fixes:
1. Check WslDistroInfo.State before issuing systemctl stop.  If State != 'Running',
   skip with Skipped status — the wsl --unregister that follows will force-terminate.
2. Wrap RunInDistroAsync with a 5-second CancellationTokenSource.  If the inner
   timeout fires (distro wedged while Running), log a warning and record Executed
   with a 'timed out' note so the remaining steps continue.  The caller's outer
   CancellationToken is still respected.

Tests (LocalGatewayUninstallTests.cs):
- StoppedDistro_SystemctlStopSkipped_NoHang: stopped distro skips the step
  and the full uninstall completes in < 10 s.
- WedgedRunningDistro_SystemctlTimesOut_ContinuesUninstall: HangingWslCommandRunner
  simulates a 30-second hang; verifies the engine completes in < 15 s and records
  Executed with 'timed out' detail.

Date: 2026-05-10

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…G-05)

installer.iss [Files] entry for the Command Palette extension fails with
'Source directory not found' if {publish}\cmdpal\ does not exist, even though
the entry is gated by 'Tasks: cmdpalette'.  ISCC evaluates Source paths at
compile time regardless of task selection, so a developer build that omits
the cmdpalette publish step causes an unconditional compile error.

Fix: add skipifsourcedoesntexist to the cmdpal [Files] Flags.  With this flag
ISCC silently omits the entry when the directory is absent, which matches the
intent of 'Tasks: cmdpalette' (optional integration).  When the directory IS
present the flag has no effect and all files are bundled as before.

Date: 2026-05-10

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

Copy link
Copy Markdown
Contributor

Note

The pull request was not created — a fallback review issue was created instead due to protected file changes: #311

🤖 This is an automated response from Repo Assist.

Nice work on the 13-step uninstall engine — the DryRun=true default, the ConfirmDestructive gate, and the AllowedDistroName constant guard are all solid safety choices. CI is fully green. A few things worth looking at before merge:


1. Non-atomic write in DeviceIdentity.TryClearDeviceToken

DeviceIdentity.TryClearDeviceToken writes back the device key file using File.WriteAllText:

File.WriteAllText(keyPath, JsonSerializer.Serialize(data, ...));

If the process is killed between the old content being truncated and the new content being written, the key file ends up corrupted (zero-length or partial JSON). ExecApprovalsStore uses an atomic temp-file + File.Move pattern for this exact reason. The device key file is arguably more critical — consider applying the same pattern here:

var tmp = keyPath + ".tmp";
File.WriteAllText(tmp, JsonSerializer.Serialize(data, options));
File.Move(tmp, keyPath, overwrite: true);

2. Kill(entireProcessTree: false) for WSL keepalive

In the "Stop keepalive process" step:

proc.Kill(entireProcessTree: false);

WSL keepalive processes can spawn child processes (shells, services). Killing only the root PID may leave orphaned children holding WSL distro state, which could then prevent wsl --unregister from completing cleanly. Consider entireProcessTree: true unless there's a deliberate reason to leave children alive.


3. Observation: most additions are .squad/ docs

~96% of the additions are .squad/ coordination files, not source code. The reviewable source change is actually quite compact: LocalGatewayUninstall.cs (789 lines), LocalGatewayUninstallTests.cs (894 lines), SettingsPage.xaml.cs (+209), and a few scripts/docs. Might be worth noting in the PR body that the "33k additions" metric is mostly internal docs to set expectations for reviewers scanning the diff.


Generated by 🌈 Repo Assist, see workflow run. Learn more.

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@97143ac59cb3a13ef2a77581f929f06719c7402a

@indierawk2k2
indierawk2k2 marked this pull request as ready for review May 11, 2026 16:19
@shanselman

Copy link
Copy Markdown
Collaborator

This is a substantial and useful WSL uninstall PR. The overall shape is good: a structured uninstall engine, dry-run behavior, Inno uninstall hook, Settings UI entry point, validation scripts, and tests are all the right pieces. I do not think this should merge yet, though. A few items should be fixed first because uninstall/cleanup code has a high bar for safety and truthfulness.

1. Please remove the .squad artifacts from this product PR

The full diff is dominated by .squad/ files — roughly 160+ files and the majority of the line count. .squad/ is ignored on master, but these files are tracked in this branch, so .gitignore will not protect the repo history once merged.

Impact:

  • It makes product review much harder.
  • It permanently adds a large amount of agent history/decision noise to the repository.
  • Future diffs may continue carrying tracked .squad state.

Suggested fix:

git rm -r --cached .squad
git commit -m "chore: remove squad artifacts from uninstall PR"

If any .squad content is intentionally needed, please split it into a separate explicit docs/process PR. The uninstall product PR should contain only product, docs, installer, script, and test changes.

2. Uninstall appears to miss the current credential source of truth: GatewayRegistry

The PR clears legacy device-token state and settings, but current architecture moved gateway credentials into GatewayRegistry and per-gateway identity directories:

  • %APPDATA%\OpenClawTray\gateways.json
  • %APPDATA%\OpenClawTray\gateways\<gateway-id>\device-key-ed25519.json

The Settings UI text says gateway credentials/token/bootstrap token are cleared, but the uninstall engine needs to explicitly remove local-gateway records and identity folders from the registry layer too. Otherwise a user may uninstall the local gateway while leaving active credentials and device tokens behind.

Suggested fix:

  1. Load GatewayRegistry from the same tray data directory used by SettingsManager.
  2. Identify local gateway records safely — for example records marked local, records pointing at the known local gateway URL/port, or records created by local setup. Avoid deleting remote gateways.
  3. Remove those gateway records from gateways.json.
  4. Delete their per-gateway identity directories under gateways\<gateway-id>\.
  5. If the active gateway was removed, clear active or switch to a remaining safe record.
  6. Add postconditions/tests proving gateways.json and per-gateway identity files are cleaned.

This should be treated as a correctness/security follow-up before merge because credentials are the important durable state now.

3. Success should depend on required postconditions

Please confirm LocalGatewayUninstallResult.Success cannot be true if required cleanup failed. The review found that success may be computed from _errors.Count == 0 while postconditions are reported separately.

That can lead to a false-positive “Local gateway removed” even if a required artifact still exists, such as:

  • WSL distro still present
  • VHD directory still present
  • autostart still enabled
  • setup state still present
  • device token/registry identity still present
  • keepalive markers still present

Suggested fix:

  1. Define which postconditions are required for success.
  2. If any required postcondition is false, add an error or set Success = false.
  3. Keep optional items optional, but label them clearly.
  4. Add tests for “step did not throw but postcondition failed => uninstall result is not successful”.

This matters because uninstall UX must be truthful: “completed with leftovers” is very different from “removed”.

4. Distro-name behavior is inconsistent for custom/test distros

The PowerShell validation helpers appear to allow OpenClawGateway* names, but the C# engine and parts of the script use an exact OpenClawGateway guard. I like the safety intent, but the inconsistency can confuse validation and leave test distros orphaned.

Suggested fix:

  • Either make all uninstall paths exact-match only and document that custom distro uninstall is not supported, or
  • Store/read the actual setup distro name and allow only names that are provably OpenClaw-owned, such as a controlled OpenClawGateway* prefix plus setup-state ownership evidence.

The key invariant is: do not unregister arbitrary user distros, but also do not strand OpenClaw-created test/validation distros.

5. Inno uninstall may race a running tray instance

The new --uninstall path bypasses the single-instance mutex so the uninstaller can invoke it. That is understandable, but it means the uninstall process and running tray can both access settings/setup/token files at the same time.

Suggested fix options:

  1. Prefer Inno-level app shutdown, e.g. CloseApplications/AppMutex or the equivalent installer mechanism, so the tray is not writing while uninstall cleanup runs.
  2. Or add file-lock/retry behavior around writes that touch settings.json, identity files, and setup state.

This is especially important because uninstall modifies durable config files.

6. Redact stderr errors the same way JSON output is redacted

The JSON uninstall output appears to redact errors, but console stderr should also pass through the same redaction path before writing:

Console.Error.WriteLine($"  ERROR: {CliRedact(e)}");

This is a small defense-in-depth fix and matches the recent security cleanup direction in the repo.

7. Logs preservation behavior should match the option text

If PreserveLogs=false, make sure the code deletes the actual log directory, not just *.log files at the local-data root. The review called out that logs may live under LocalAppData\OpenClawTray\Logs.

Suggested fix: delete/clean the known logs directory recursively when preservation is disabled, and add a test for that path.

What looks good

  • The dry-run design and step audit trail are valuable for safe uninstall validation.
  • The distro guard is the right safety instinct; it just needs consistent ownership/name handling.
  • The Inno waituntilterminated choice is important and well justified.
  • The tests around idempotency, ordering, VHD cleanup, timeout behavior, and token-file handling are strong.
  • The MSIX/portable uninstall docs and validation scripts are helpful for a tricky area.

I would be comfortable continuing review after the .squad cleanup, GatewayRegistry credential cleanup, and postcondition-based success semantics are addressed. Those are the merge-blocking items for me.

Mike Harsh and others added 9 commits May 11, 2026 19:54
… required postconditions (PR openclaw#310 blockers #2 + #3)

Blocker #2 — GatewayRegistry cleanup:
- Add required GatewayRegistry param to ctor (Build factory auto-resolves
  from data dir when caller doesn't supply one).
- Insert new Step 6a 'Remove local gateway records' between Step 6 (Reset
  autostart) and Step 7 (Null device token).
- Snapshot _localGatewayIdsSnapshot BEFORE any mutation so identity-dir
  postconditions verify the full candidate set even on partial-remove failures.
- Defense-in-depth predicate: r.IsLocal || LocalGatewayUrlClassifier.IsLocalGatewayUrl(r.Url)
  — catches legacy migrated records whose IsLocal flag was never set.
- Step deletes registry records, per-gateway identity dirs, and clears the
  active gateway when it was a local record (GatewayRegistry.Remove handles
  the active-id reset already).
- Custom DryRun block reports candidate IDs without mutating disk.

Blocker #3 — Postcondition-gated Success:
- Add LocalGatewayRecordsAbsent (reload-from-disk via fresh GatewayRegistry)
  and LocalGatewayIdentityDirsAbsent (snapshot vs disk) postconditions.
- AllRequiredPostconditionsMet covers 8 required postconditions; McpTokenPreserved
  remains informational.
- AppendPostconditionErrors runs unconditionally when not DryRun, so the Errors
  list always contains both step exceptions AND residual-artifact failures.
- Success = _errors.Count == 0 && (DryRun || AllRequiredPostconditionsMet).

Tests (14 new; 44 total in LocalGatewayUninstallTests):
- Run_LocalGatewayRecordsCleared_PostconditionTrue
- Run_OnlyLocalRecordsRemoved_RemoteGatewaysPreserved
- Run_LocalAndRemoteWithSameUrl_OnlyIsLocalRemoved
- Run_LegacyRecordWithoutIsLocal_RemovedByUrlClassifier
- Run_RegistryRecordsRemain_ReturnsSuccessFalse
- Run_IdentityDirPersists_ReturnsSuccessFalse
- Run_PostconditionFailed_ErrorListed
- Run_StepErrorAndPostconditionFailure_BothInErrors
- Run_ActiveGatewayCleared_WhenLocalRemoved
- Run_ActiveRemoteGateway_StaysActive
- Run_NoGatewaysJson_SucceedsWithPostconditionsTrue
- Run_PostconditionUsesFreeDiskRegistry_NotInMemory
- DryRun_RegistryNotMutated
- DryRun_SuccessTrue_PostconditionsSkipped

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GatewayRegistry.MigrateFromSettings() and the App.xaml.cs startup bridge
both create GatewayRecord instances from settings.GatewayUrl without
setting IsLocal. When the URL points at localhost/127.0.0.1/::1 the
records persisted to disk look 'remote' to flag-only consumers, even
though they are loopback bootstrap records.

Set IsLocal = LocalGatewayUrlClassifier.IsLocalGatewayUrl(gatewayUrl)
at both creation sites so future records carry the correct flag. The
uninstall predicate's URL fallback continues to catch any records that
were persisted before this fix shipped.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…y preserved/removed copy

Previously SettingsPage called LocalGatewayUninstall.Build(_hub.Settings)
with no registry argument, so the engine built its own GatewayRegistry
instance and mutated gateways.json while the live _hub.GatewayRegistry
retained stale records and could re-save them.

Pass _hub.GatewayRegistry through so the engine mutates the same
in-memory registry the rest of the app uses. The connection manager
sees local records disappear immediately and ActiveGatewayId clearing
is visible to the live UI.

Also update the Remove Local Gateway dialog's 'Preserved' line to
distinguish the root device key (which is preserved) from local
gateway identity credentials and per-gateway records (which are
now removed by Step 6a).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove tracked .squad coordination files from the uninstall PR branch so they stay local-only via the existing .gitignore rule.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Shell helpers (Scott openclaw#4)

The PS predicates (Test-IsOpenClawOwnedDistroName in _uninstall-helpers.ps1 and validate-wsl-gateway.ps1) accepted any name starting with 'OpenClawGateway', but the downstream guards (C# AllowedDistroName const + validate-wsl-gateway-uninstall.ps1 line 624) require exact match. Pass -DistroName OpenClawGateway-test and you'd cruise through preflight + every step then strand at the final unregister with the distro half-cleaned.

No caller in tree needs prefix-match. Tighten to exact-everywhere; the C# const remains the single source of truth and the PS predicates trivially mirror it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…y (Scott openclaw#5)

Without AppMutex, Inno's [UninstallRun] step invokes the OpenClaw --uninstall CLI while the tray is still running. Both processes write to settings.json, gateways.json, device-key-ed25519.json and append to Logs/ — concurrent writes risk torn JSON and IOExceptions on the tray side.

AppMutex=OpenClawTray matches the bare mutex name created at App.xaml.cs (single-instance check). Tray and Inno run in the same user session, so Local\OpenClawTray resolves with no Global\ prefix. Inno surfaces a 'Close the app to continue' dialog with built-in retry, then proceeds.

Added structural test InstallerIssAssertionTests so future refactors don't silently drop the AppMutex line or rename the mutex out from under it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hen PreserveLogs=false (Scott openclaw#7)

Step 9 was scanning the root of %LOCALAPPDATA%\OpenClawTray for *.log / crash.log files. The real log writer (DiagnosticsJsonlService) drops diagnostics.jsonl under a Logs/ subdirectory, and there is a pre-existing inconsistency between where the writer writes (%APPDATA%\OpenClawTray\Logs) and where the SettingsPage 'View Logs' button points (%LOCALAPPDATA%\OpenClawTray\Logs).

Step 9 now deletes the Logs/ subdirectory under both candidate locations recursively, idempotently. PreserveLogs=false's job is to leave no logs behind; covering both directories is the cheapest correct fix without picking a side on the larger writer-location ambiguity (filed as separate follow-up).

Tests 14/15 updated to create both Logs/ dirs and assert both removed; new Test 15b pins the 'no Logs/ present' Skipped path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All four File.WriteAllText sites in DeviceIdentity (TryClearDeviceToken, GenerateNew, StoreDeviceTokenCore, StoreNodeDeviceTokenCore) were truncate-then-write. A power loss or process-kill mid-write yields a torn or zero-byte device-key-ed25519.json; the next LoadOrCreate then treats it as invalid and silently rotates the identity (loses the device token, forces re-pair) or throws JsonException.

Introduce a single private AtomicWriteKeyFile(path, data) helper: serialize to .<name>.<guid>.tmp, lock ACL, File.Move(overwrite:true) for an atomic rename on NTFS. On exception, best-effort delete the temp. Folds in the now-redundant TryRestrictSensitiveFileAcl calls (3 sites). Pattern mirrors McpAuthToken.cs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ns (Bot B2)

wsl.exe (the keepalive parent) spawns wslhost.exe and in-distro init/sleep processes. proc.Kill(entireProcessTree: false) only signalled the parent; the children kept the distro state pinned, which intermittently caused 'wsl --unregister' to fail at Step 12.

One-line flip to entireProcessTree: true. Cannot reliably spin up a real WSL process tree from a unit test, so a structural source-text assertion (StopKeepalive_KillsEntireProcessTree_SourceAssertion) guards against a future refactor flipping the flag back.

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

Copy link
Copy Markdown
Contributor Author

@shanselman — ready for another look. Summary of what changed since your review, organized by your numbered items plus the Repo Assist bot's.

Your items #1#7

Repo Assist bot items #B1–#B3

  • #B1 — DeviceIdentity atomic key writes: ✅ d9b6d0f. New AtomicWriteKeyFile helper consolidates the 4 write sites.
  • #B2 — WSL keepalive process tree kill: ✅ c84b7e1. entireProcessTree: true on the kill path.
  • #B3 — PR body note re: .squad % of diff: N/A after Add unit tests and code review for Moltbot.Shared #1 cleanup.

CI

All green on c84b7e1: repo-hygiene, test, build (x64/arm64), build-extension (x64/arm64), build-msix (x64/arm64). release is skipped (only runs post-merge to master).

Local validation

  • build.ps1 PASS
  • Shared.Tests: 1455 passed / 0 failed / 25 skipped
  • Tray.Tests: 961 passed / 0 failed / 0 skipped
  • 17 new tests added across the two rounds (14 Round 1 + 3 Round 2 in Tray; 3 Round 2 IntegrationFact in Shared, gated by OPENCLAW_RUN_INTEGRATION=1).

Pending

Live end-to-end smoke validation on a real box is being prepped in parallel; we'll file results as a follow-up comment.

Ready for another look when you have time.

@shanselman
shanselman merged commit 223db1f into openclaw:master May 12, 2026
12 of 15 checks passed
indierawk2k2 pushed a commit to indierawk2k2/openclaw-windows-node that referenced this pull request May 13, 2026
…e sampling state

PowerShell's `&` operator does not block on GUI-subsystem executables — it
returns the moment CreateProcess succeeds. validate-wsl-gateway-uninstall.ps1
called the tray's CLI uninstall path with `& $trayExe @cliArgs` then
immediately read $LASTEXITCODE and took its post-state snapshot. Result: a
healthy ~9-second engine run produced a false-negative "PARTIAL" verdict
because the script sampled disk state before the engine had performed any
destructive work.

Replace with Start-Process -Wait -PassThru so the script blocks until the
engine actually exits. Same fix applied to Uninstall-LocalGateway.ps1 (the
Inno-invoked uninstaller would have raced ahead the same way) and to the
two `& $exePath @Args > stdout 2> stderr` call sites in
validate-msix-storage-paths.ps1 (file redirection is unreliable for a
WinExe whose stdout has been re-attached via AttachConsole).

Discovered during Mike's manual test pass on PR openclaw#310's portable build —
forensic timestamps showed pre/post snapshots both at T+0 while the engine
finished writing uninstall-result.json at T+9. Fix A (JSON serializer's
2 missing postcondition fields) already landed in 68f30ba.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants