Conversation
Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 25, 2026, 7:40 PM ET / 23:40 UTC (Revision 9). ClawSweeper reviewWhat this changesThe 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 Review scores
Verification
How this fits togetherThe 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]
Before merge
Findings
Agent review detailsSecurityNeeds attention: The introduced recursive child deletion lacks final-effect checks for redirected paths and unowned contents. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest 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:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 5a59535216ee. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (8 earlier review cycles)
|
|
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: An accepted folder basename plus a child name does not establish that the child is generated state. A user-selected directory named The bounded repair I suggest:
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. |
|
Global triage: HOLD_FOR_AUTHOR. Take confidence 15%; recommendation confidence 92%; effort moderate; risk high/P0. Reviewed exact head |
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>
|
Thanks, The real uninstall sequence still has an earlier deletion that the new second-phase guard cannot protect: primary 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 These are incomplete protections in the existing cleanup sequence, not a claim that your narrowing newly introduced the broad deletion. |
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
|
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: |
|
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. |
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-GatewayDirectoryphase. 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
42c562f9was integrated normally in64c21245; the maintainer-only repairs are8af2ba28and910784c5. 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-restoreno-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..\build.ps1win-arm64dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --no-build --filter FullyQualifiedName~InstallerIssAssertionTestsCounts were checked from TRX results. Existing skipped tests remain unverified, not passes. The SetupEngine suite's existing
wsl.exe --versiondecoder 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 64c2124509be0bddd40a1212d28a72ffb23cbaa1with 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
tryAST 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.889bf119: 10 failures and 8 passes across the initial 18 new primary-phase cases, demonstrating the custom-root and ancestor-junction gaps.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.