Skip to content

fix(setup): delete only generated uninstall children - #1486

Closed
SebTardif wants to merge 5 commits into
openclaw:mainfrom
SebTardif:fix/f023-generated-uninstall-children
Closed

SebTardif wants to merge 5 commits into
openclaw:mainfrom
SebTardif:fix/f023-generated-uninstall-children

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

What Problem

After gateway cleanup exited 0, uninstall ran DelTree on the whole {app} directory. The directory page lets the user pick any folder. Files already in that folder were removed.

Why

The default location is %LOCALAPPDATA%\OpenClawTray, but the user can choose another folder.

User Impact

Uninstall now refuses the delete unless the folder name is OpenClawTray or OpenClawTray-Dev, and then deletes only known generated children.

Current-head clarification: the author's follow-up binds generated-child cleanup to the exact generated-data root, not just its basename, and preserves sibling WSL children. The bounded maintainer repair applies that same boundary to the earlier Remove-GatewayDirectory phase. A custom or lookalike {app} folder is preserved with an ownership warning in its existing result JSON and log. Successful unregister-by-name does not establish ownership of that independently derived folder. Existing reparse points at the app root, WSL root, or configured child fail closed, including redirected ancestors whose target has no configured child. Failed unregister still preserves the directory.

The original contributor branch and author commits are preserved. Main 42c562f9 was integrated normally in 64c21245; the maintainer-only repairs are 8af2ba28 and 910784c5. No rebase, force-push, replacement PR, workflow, signing, or package-layout change.

Evidence

Red: Installer_RemovesGeneratedAppStateOnlyAfterGatewayCleanup still found DelTree of the whole {app}. Green: that test passed. .\build.ps1 passed.

The author's initial installer assertion result was 1 passed. Current-head evidence follows.

Required proof pools

  • windows-clean-installer-upgrade: uninstall delete scope changed. Not verified / blocked: exact signed installer install/upgrade/uninstall proof is not available in this shared-host run.
  • windows-wsl-gateway-e2e: native WSL cleanup and preservation behavior. Not verified / blocked: no distro start, terminate, unregister, or full uninstaller execution was authorized for this run.

Validation

Validated source tree committed as 910784c599ae3215e637eb88ed7a99c155015816, on native Windows ARM64 with the private .NET SDK 10.0.400. Fresh test projects were built first to avoid --no-restore no-ops. Test processes used this isolated worktree and private tray-data paths, with optional E2E flags off. All required commands were rerun after the one accepted autoreview correction.

Command Result
.\build.ps1 PASS, all five projects, win-arm64
dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore 4,096 passed, 35 skipped, 0 failed; 4,131 total
dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore 3,090 passed, 0 skipped, 0 failed
dotnet test .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --no-restore 1,192 passed, 1 skipped, 0 failed; 1,193 total
dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --no-build --filter FullyQualifiedName~InstallerIssAssertionTests 40 passed, including all 20 new primary-phase cases

Counts were checked from TRX results. Existing skipped tests remain unverified, not passes. The SetupEngine suite's existing wsl.exe --version decoder test is metadata-only and is not native WSL cleanup proof.

Focused rubber-duck review found no blocking issue. Initial commit-scoped autoreview found a missing-child bypass of the new ancestor check; it was reproduced, corrected, and covered by two additional tests. Final autoreview covered the entire bounded maintainer range using python .\.agents\skills\autoreview\scripts\autoreview --mode branch --base 64c2124509be0bddd40a1212d28a72ffb23cbaa1 with the verified Codex 0.156.1 executable, gpt-5.6-sol, high reasoning: exit 0, no accepted/actionable findings. No code changed after that review. Ancestor reparse rejection intentionally remains a hard failure, consistent with the existing leaf rejection, rather than proceeding with artifact cleanup through uncertain storage.

Real behavior proof

The installer script no longer contains DelTree of the whole {app} directory. Not verified: no Inno uninstall on a machine with extra files in the folder.

Executed production-first-phase filesystem proof, with modeled transport: Windows PowerShell 5.1 runs the actual production top-level cleanup try AST and allowlisted production functions. The deletion guard, control sequence, filesystem operations, warning collection, and result/log writes are real. Only WSL transport, unrelated Windows-artifact cleanup, and delays are substituted. The harness never invokes registry/task cleanup or a real WSL command. Every sentinel VHD file, junction target, log, and fixture path is owned temporary test data.

  • Custom and same-basename lookalike install roots retain their sentinel VHDs; warnings persist in the result JSON and log. The separate generated-root sentinel is also preserved by this primary pass.
  • The exact generated root removes only the configured child after modeled absent/successful-unregister outcomes, retaining default-name and sibling distro sentinels. Failed unregister retains the child and skips subsequent artifact cleanup.
  • Root, WSL-root, and child junctions are rejected. Empty-target ancestor junctions also fail closed before the missing-child return.
  • Original author source 889bf119: 10 failures and 8 passes across the initial 18 new primary-phase cases, demonstrating the custom-root and ancestor-junction gaps.
  • Initial maintainer source 8af2ba28: exactly the two empty-target ancestor cases fail, with 18 passes. Final source: all 20 primary-phase cases pass; all 40 installer assertions/runtime cases pass.

Not verified / blocked: clean signed-installer pool behavior, real native WSL lifecycle cleanup, and full Inno/GUI uninstall remain untested. A real temporary-filesystem pass with modeled transport is not clean-installer or native-WSL pool proof. Primary custom-root warnings are retained in the preserved folder; the existing secondary pass still logs in Inno's temporary directory, and its post-uninstall log durability was not changed or proven.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Sep 23, 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 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. 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 23, 2026
@clawsweeper

clawsweeper Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 25, 2026, 7:40 PM ET / 23:40 UTC (Revision 9).

ClawSweeper review

What this changes

The branch replaces whole-install-folder deletion during Inno uninstall with named-child cleanup, adds generated-root checks to the PowerShell gateway cleanup, and adds documentation and Windows filesystem tests.

Merge readiness

⛔ Blocked before merge - 12 items remain

The uninstall safety fix is still absent from current main. A maintainer-owned replacement, #1521, addresses the same work, but it is draft and explicitly held for signed-installer proof. This branch also retains unresolved deletion-boundary concerns and conflicts with main's newer migration safeguards, so it is not ready to land or safe to close as superseded.

Priority: P0
Reviewed head: 910784c599ae3215e637eb88ed7a99c155015816

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The primary cleanup has meaningful Windows filesystem coverage, but the final deletion boundary and current-main integration remain unsafe to approve.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: Authority-chain proof required: current-head Windows cases exercise the production PowerShell first-phase filesystem cleanup with temporary sentinels and modeled WSL transport, reporting preservation and deletion outcomes. They do not exercise the secondary Inno DelTree against the nearest unowned or redirected target, nor the full signed-installer upgrade and uninstall path. No stored-data format changes are introduced. Redacted terminal output or logs from the exact signed-installer scenario would address the gap; redact private paths, endpoints, and credentials. 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 🧂 unranked krab (1/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: current-head Windows cases exercise the production PowerShell first-phase filesystem cleanup with temporary sentinels and modeled WSL transport, reporting preservation and deletion outcomes. They do not exercise the secondary Inno DelTree against the nearest unowned or redirected target, nor the full signed-installer upgrade and uninstall path. No stored-data format changes are introduced. Redacted terminal output or logs from the exact signed-installer scenario would address the gap; redact private paths, endpoints, and credentials. 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 8 items Introduced installer boundary: The introduced secondary phase checks the app root against the generated root, then recursively deletes named directories without checking whether the root or child is redirected or whether content inside a named child is owned.
First-phase safeguard: The primary PowerShell phase compares the app root with the generated-data root and checks the app, WSL, and distro paths for reparse points before recursive deletion.
Current main still deletes the whole app folder: Current main retains the post-uninstall DelTree call on the entire user-selectable {app} directory, so the central fix is not implemented there.
Findings 3 actionable findings [P1] Reject redirected roots before secondary deletion
[P1] Preserve unowned content inside named children
[P2] Keep secondary cleanup warnings in a durable location
Security Needs attention Unverified final deletion target: String equality to the expected root does not establish where a reparse point or named child resolves when Inno performs recursive deletion.

How this fits together

The Inno uninstaller asks whether to remove the local WSL Gateway, then calls a PowerShell helper to unregister it and clean generated files. Its path decisions determine which files survive uninstall.

flowchart TD
  A[Uninstall choice] --> B[Inno uninstaller]
  B --> C[PowerShell Gateway cleanup]
  C --> D{Gateway removal succeeded?}
  D -->|No| E[Preserve generated state]
  D -->|Yes| F[Check generated root and children]
  F --> G[Delete allowed state or retain uncertain files]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: Authority-chain proof required: current-head Windows cases exercise the production PowerShell first-phase filesystem cleanup with temporary sentinels and modeled WSL transport, reporting preservation and deletion outcomes. They do not exercise the secondary Inno DelTree against the nearest unowned or redirected target, nor the full signed-installer upgrade and uninstall path. No stored-data format changes are introduced. Redacted terminal output or logs from the exact signed-installer scenario would address the gap; redact private paths, endpoints, and credentials. 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.
  • Reject redirected roots before secondary deletion (P1) - The new Inno cleanup compares path strings, then calls recursive DelTree without checking whether the generated root or a named child is a reparse point. A redirected path can send this final deletion outside the intended state tree; check the path chain at the deletion boundary.
  • Preserve unowned content inside named children (P1) - The new helper deletes entire generic directories such as Logs, canvas, and WebView2 after matching only the root path. Existing user files in those directories are still removed. Require ownership evidence for their contents or leave uncertain directories in place.
  • Keep secondary cleanup warnings in a durable location (P2) - The added second PowerShell call uses Inno's temporary folder as AppRoot, where its warning log is written. If cleanup preserves uncertain WSL children, the warning may disappear with that folder; retain the result in the uninstall log or another durable location.
  • Resolve security concern: Unverified final deletion target - String equality to the expected root does not establish where a reparse point or named child resolves when Inno performs recursive deletion.
  • Resolve merge risk (P1) - The secondary Inno phase can recursively delete unowned files inside a named child and has no demonstrated final-deletion check for a redirected generated-data root.
  • Resolve merge risk (P1) - The signed installer and upgrade proof pool has not exercised custom-folder sentinels, redirected paths, and preserved sibling WSL data through the complete uninstall.
  • Resolve merge risk (P1) - This head conflicts with main's newer Store-migration uninstall admission; a resolution must preserve migration-owned Gateway state.
  • Complete next step (P2) - Resolve the final-deletion findings and main migration conflict, then provide exact-head signed-installer proof before merge or defer to the proven replacement.
  • Improve patch quality - Prove that the secondary installer path preserves the nearest unowned child and redirected target before final deletion.
  • Improve patch quality - Resolve the current-main migration conflict while retaining its Gateway preservation checks.
  • Improve patch quality - Run the signed-installer clean-install, upgrade, and uninstall proof pool on the resulting head.

Findings

  • [P1] Reject redirected roots before secondary deletion — installer.iss:310-313
  • [P1] Preserve unowned content inside named children — installer.iss:311-314
  • [P2] Keep secondary cleanup warnings in a durable location — installer.iss:335-342
  • [high] Unverified final deletion target — installer.iss:313
Agent review details

Security

Needs attention: The introduced recursive child deletion lacks final-effect checks for redirected paths and unowned contents.

Review metrics

Metric Value Why it matters
Code and test growth production +197/-11 lines; tests +311/-1 lines The added test coverage is substantial, but most executable cases cover PowerShell rather than the final Inno deletion.

Merge-risk options

Maintainer options:

  1. Validate the replacement (recommended)
    Complete the signed-installer proof and review the current-main migration and deletion boundaries in fix(setup): delete only generated uninstall children #1521 before choosing it as the landing path.
  2. Repair this branch
    Resolve the main conflict while preserving migration admission, add final-deletion ownership checks, and prove the resulting installer behavior before merge.
  3. Close after a proven replacement lands
    Keep this contribution linked for provenance, then close it when a viable replacement has merged.

Technical review

Best possible solution:

Land one current-main-compatible deletion boundary that verifies ownership at each final filesystem removal, preserves uncertain content and durable diagnostics, and passes the exact signed-installer upgrade and uninstall proof pool.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main calls recursive DelTree on the user-selectable install folder after successful Gateway cleanup; the PR's Windows filesystem cases also exercise the narrower primary cleanup, though they do not run the signed installer.

Is this the best way to solve the issue?

No. Narrowing deletion is the right direction, but this branch still needs final-effect ownership safeguards and integration with current main's migration admission.

Full review comments:

  • [P1] Reject redirected roots before secondary deletion — installer.iss:310-313
    The new Inno cleanup compares path strings, then calls recursive DelTree without checking whether the generated root or a named child is a reparse point. A redirected path can send this final deletion outside the intended state tree; check the path chain at the deletion boundary.
    Confidence: 0.86
  • [P1] Preserve unowned content inside named children — installer.iss:311-314
    The new helper deletes entire generic directories such as Logs, canvas, and WebView2 after matching only the root path. Existing user files in those directories are still removed. Require ownership evidence for their contents or leave uncertain directories in place.
    Confidence: 0.9
  • [P2] Keep secondary cleanup warnings in a durable location — installer.iss:335-342
    The added second PowerShell call uses Inno's temporary folder as AppRoot, where its warning log is written. If cleanup preserves uncertain WSL children, the warning may disappear with that folder; retain the result in the uninstall log or another durable location.
    Confidence: 0.82

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 5a59535216ee.

Labels

Label changes:

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

Label justifications:

  • P0: An incorrect uninstall deletion boundary can irreversibly remove files from a user-selected folder.
  • merge-risk: 🚨 compatibility: This branch has not integrated main's later Store-migration uninstall preservation contract.
  • merge-risk: 🚨 security-boundary: The final recursive deletion still depends on unproven path and child ownership at the filesystem boundary.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦐 gold shrimp and patch quality is 🧂 unranked krab.
  • 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: current-head Windows cases exercise the production PowerShell first-phase filesystem cleanup with temporary sentinels and modeled WSL transport, reporting preservation and deletion outcomes. They do not exercise the secondary Inno DelTree against the nearest unowned or redirected target, nor the full signed-installer upgrade and uninstall path. No stored-data format changes are introduced. Redacted terminal output or logs from the exact signed-installer scenario would address the gap; redact private paths, endpoints, and credentials. 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:

  • [high] Unverified final deletion target — installer.iss:313
    String equality to the expected root does not establish where a reparse point or named child resolves when Inno performs recursive deletion.
    Confidence: 0.86

What I checked:

  • Introduced installer boundary: The introduced secondary phase checks the app root against the generated root, then recursively deletes named directories without checking whether the root or child is redirected or whether content inside a named child is owned. (installer.iss:313, 910784c599ae)
  • First-phase safeguard: The primary PowerShell phase compares the app root with the generated-data root and checks the app, WSL, and distro paths for reparse points before recursive deletion. (scripts/Uninstall-LocalGateway.ps1:711, 910784c599ae)
  • Current main still deletes the whole app folder: Current main retains the post-uninstall DelTree call on the entire user-selectable {app} directory, so the central fix is not implemented there. (installer.iss:594, 5a59535216ee)
  • New main migration boundary: Main added Store-migration admission and preservation checks to the same uninstall sequence after this PR branched; the reviewed head lacks those integrated guards and is reported as conflicting. (installer.iss:425, 5a59535216ee)
  • Proof scope: The PR body reports current-head Windows PowerShell filesystem cases with modeled WSL transport and passing build and tests. It explicitly says the signed Inno uninstall, native WSL lifecycle, and full GUI uninstall were not verified; this supports the first phase but does not observe the secondary Inno deletion. (tests/OpenClaw.Tray.Tests/InstallerIssAssertionTests.cs:153, 910784c599ae)
  • Required installer proof pool: The repository defines windows-clean-installer-upgrade as clean install, upgrade, repair, and uninstall with exact signed artifacts. (docs/PROOF_POOLS.md:19, 910784c599ae)

Likely related people:

  • shanselman: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • karkarl: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • ranjeshj: 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 (8 earlier review cycles)
  • reviewed 2026-09-23T02:23:30.379Z sha d7364a1 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-23T20:10:55.231Z sha 889bf11 :: needs real behavior proof before merge. :: [P1] Verify ownership before deleting the named distro directory | [P1] Preserve pre-existing content in named child directories
  • reviewed 2026-09-23T21:50:56.328Z sha 889bf11 :: needs real behavior proof before merge. :: [P1] Check ownership before deleting the named WSL child | [P1] Preserve unowned files in generic named children
  • reviewed 2026-09-23T22:30:30.687Z sha 889bf11 :: needs real behavior proof before merge. :: [P1] Verify ownership before deleting the named distro child | [P1] Preserve unowned files in generic named children
  • reviewed 2026-09-24T00:05:28.670Z sha 889bf11 :: needs real behavior proof before merge. :: [P1] Guard the primary distro-directory deletion | [P1] Require ownership proof for named child removal | [P1] Preserve unowned content inside generic children | [P2] Keep leftover diagnostics in a durable location
  • reviewed 2026-09-24T00:40:11.199Z sha 910784c :: needs real behavior proof before merge. :: [P1] Reject a redirected generated-data root before secondary deletion | [P2] Persist secondary leftover diagnostics
  • reviewed 2026-09-24T01:16:20.081Z sha 910784c :: needs real behavior proof before merge. :: [P1] Reject a redirected generated-data root before secondary deletion | [P1] Preserve unowned content inside named generated children | [P2] Persist secondary leftover diagnostics
  • reviewed 2026-09-25T22:57:08.970Z sha 910784c :: needs real behavior proof before merge. :: [P1] Reject a redirected generated-data root before secondary deletion | [P1] Preserve unowned content inside named generated children | [P2] Persist secondary leftover diagnostics

@shanselman

Copy link
Copy Markdown
Collaborator

Thanks, Seb. Removing the recursive delete of the entire install directory is worthwhile. I would like to finish the remaining ownership edge on this PR.

Reviewed head: d7364a1f2333b1cbc7ec7d477737ccdef6524d19.

An accepted folder basename plus a child name does not establish that the child is generated state. A user-selected directory named OpenClawTray can already contain unrelated Logs, canvas, or WebView2 content, and the new helper still recursively deletes those children. This is narrower than the old behavior, but it does not yet fully support the data-preservation claim.

The bounded repair I suggest:

  • Match the actual generated-data root for this build, not merely either accepted basename. Preserve unowned/pre-existing children when ownership cannot be established.
  • Do not recursively delete the entire wsl child after cleanup of only the configured distro. Preserve other distro children and any VHD whose registration/removal was not confirmed.
  • Add disposable installer proof covering a custom directory, sentinel files in generic-name children, another distro child, and failed gateway cleanup. Preserve all unrelated sentinels.

If reliable ownership cannot be established cheaply, leaving generated leftovers with a clear log is preferable to recursively deleting uncertain content. No broad installer redesign is needed; the goal is a small, defensible deletion boundary.

@karkarl

karkarl commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Global triage: HOLD_FOR_AUTHOR. Take confidence 15%; recommendation confidence 92%; effort moderate; risk high/P0.

Reviewed exact head d7364a1f2333. Replacing blanket deletion is right, but {app}\wsl contains distro VHDX children and is still deleted recursively. A sibling distro not unregistered by gateway cleanup can lose its VHD while remaining registered. Delete only the confirmed configured distro child and derive ownership from the actual generated-data root, not a generic basename. Source-text tests do not prove deletion safety. Require windows-clean-installer-upgrade with custom-directory sentinels, a second distro, failed gateway cleanup, plus windows-11-sac-on. Prefer logged leftovers over uncertain deletion.

Recursive deletion of the wsl directory could remove a sibling distro VHD. Uninstall now deletes the configured child only and leaves an uncertain path in the log.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. 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 23, 2026
@shanselman shanselman added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 23, 2026
@shanselman

Copy link
Copy Markdown
Collaborator

Thanks, 889bf11938f824f001e88a5e43f6215918f385da now limits the generated-state phase to the actual generated root and no longer recursively deletes the whole wsl parent. Those are useful corrections.

The real uninstall sequence still has an earlier deletion that the new second-phase guard cannot protect: primary Remove-GatewayDirectory derives $AppRoot\wsl\$DistroName and recursively deletes it after a missing-distro result or unregister success. Inno passes {app} as that first invocation's AppRoot. A custom install folder containing an unrelated wsl\OpenClawGateway child therefore loses it before the guarded RemoveConfirmedDistroChild phase runs.

Please apply the same ownership restriction to the primary phase, or leave uncertain custom-root content in place. Bind any recursive distro-directory deletion to a defensible owned path, rather than treating unregister-by-name as proof of ownership of a separately derived filesystem path. The smallest conservative repair is preferable to a new cleanup framework.

Extend the test to run the actual first-phase sequence with a custom/lookalike AppRoot and a sentinel VHD, not only the -RemoveConfirmedDistroChild entry point. Also keep any “leftover preserved” diagnostics in a durable uninstall log; the second phase currently writes under Inno's temporary directory.

These are incomplete protections in the existing cleanup sequence, not a claim that your narrowing newly introduced the broad deletion.

@shanselman shanselman added status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. and removed status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. labels Sep 23, 2026
shanselman and others added 3 commits September 24, 2026 01:59
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa19a99c-5eea-4942-a104-a82d6fb6c12a
Bind primary filesystem deletion to the generated local-data root, preserve uncertain custom roots with durable warnings, and reject redirected ancestors. Exercise the real PowerShell first-phase AST with modeled transport and owned filesystem fixtures.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa19a99c-5eea-4942-a104-a82d6fb6c12a
Reject redirected app and WSL roots even when their target lacks the configured distro child. Preserve the missing-directory no-op and cover empty junction targets through the actual first-phase PowerShell path.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa19a99c-5eea-4942-a104-a82d6fb6c12a
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Sep 24, 2026
@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 24, 2026
@karkarl karkarl added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 25, 2026
@karkarl

karkarl commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator

A maintainer-owned current-main replacement is now available as #1521. It preserves this contributor branch unchanged, includes #1515 and #1518, and contains the narrow generated-child deletion behavior plus current-main migration/reparse safeguards.

Exact replacement head: 5e5bd4bc0a3e2f2fe97c9ac958084a5353da733e. Full build and required Shared/Tray/focused tests pass; isolated dry-run and confirmed task-owned uninstall proof pass. It remains draft/HOLD because windows-clean-installer-upgrade pool proof is unavailable and structured Codex autoreview was blocked by repeated API 401 Unauthorized responses. No merge requested.

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

Copy link
Copy Markdown
Collaborator

Closing in favor of #1521 (fix(setup): delete only generated uninstall children). The maintainer replacement preserves this work on current main and includes the later migration, reparse-point, and primary cleanup-path ownership safeguards. It remains draft until signed installer/uninstall proof is complete.

@shanselman shanselman closed this Sep 28, 2026
@SebTardif
SebTardif deleted the fix/f023-generated-uninstall-children branch September 28, 2026 18:33
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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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