Repository navigation
Wire native WSL state and GC lifecycle - #127
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Orphan cleanup has unresolved path-safety gaps, and current-project cleanup rejects the Python state-group filter.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds native WSL state inventory and cleanup for the v2 lifecycle while keeping managed-tool activation gated on integration coverage and real WSL qualification.
Changes:
- Adds native
cb stateand dry-run-by-defaultcb gc. - Shares Python volume identity planning between runtime and state commands.
- Updates tests, support boundaries, and roadmap documentation.
| File | Description |
|---|---|
| README.md | Documents native state commands and activation gates. |
| main.go | Dispatches native WSL state commands. |
| main_test.go | Tests dispatch before the general host gate. |
| internal/wslvolume/wslvolume.go | Updates package documentation. |
| internal/wslvolume/python.go | Centralizes Python volume planning. |
| internal/wslvolume/python_test.go | Tests Python identities and project proofs. |
| internal/wslstate/command.go | Implements inventory, classification, and cleanup. |
| internal/wslstate/command_test.go | Tests state and cleanup safeguards. |
| internal/wslrun/plan.go | Uses shared Python planning. |
| internal/wslrun/plan_test.go | Updates planning test dependencies. |
| internal/wslinstall/install.go | Updates installation status text. |
| internal/wslinstall/install_test.go | Updates status assertions. |
| internal/hostenv/hostenv.go | Clarifies the remaining activation gate. |
| docs/wsl.md | Documents native state lifecycle behavior. |
| docs/wsl-process-contract.md | Updates runtime activation status. |
| docs/security-model.md | Describes cleanup safety guarantees. |
| docs/roadmap-implementation-requirements.md | Updates WSL delivery requirements. |
| docs/roadmap-decisions.md | Records state lifecycle progress. |
| docs/architecture.md | Adds state orchestration and dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Reviewed the current head 0e27959 end to end. One critical piece of context first:
The claimed fixes are not in this revision. The PR discussion asserts fixes in 0189a316c ("Harden WSL orphan deletion proofs") and a683c169d ("Honor Python state-group cleanup filter"). Both commits exist on the server with 0e27959 as their (grand)parent — but the current branch head is 0e27959 itself, i.e. the branch was force-pushed/rebased back to the pre-fix commit and both fixes were dropped. Verified directly: wslproject.ProveMissingProject does not exist in the tree, inspectProjectPath still walks only to the first existing ancestor, and expectedVolumes still discards the resolved state group. All prior findings therefore remain live on the mergeable revision:
🔴 [blocking] internal/wslstate/command.go — inspectProjectPath treats a vanished /mnt/<drive> mount as project absence.
The orphan walk climbs only while lstat returns ErrNotExist and stops at the first existing directory. wslproject.Classify legitimately accepts /mnt/<drive> project roots (DrvFs-backed), so a recorded cb.project_path like /mnt/c/Work/app is valid. If that drive mount is absent (automount off, drive unmounted), lstat fails up to the still-existing /mnt, which is accepted as safe ancestry → ORPHAN → cb gc --orphans --apply deletes the project's persistent volume even though the project still exists on the unmounted drive. cb state misreports it the same way. The dropped commit addressed exactly this by proving the missing path's storage/mount boundary (ProveMissingProject). A /mnt/<drive> path whose mount is gone must classify UNSAFE, not ORPHAN.
🟡 [important] command.go:351 — symlinked ancestry above the nearest existing ancestor escapes detection.
os.Lstat resolves intermediate components, so the loop only inspects the final component of the first existing ancestor. Recorded /work/link/sub/app, with /work/link now a symlink to /data, /data/sub a real directory, /data/sub/app absent → lstat("/work/link/sub") succeeds through the symlink → ORPHAN; /work/link itself is never inspected. That contradicts the documented contract ("symlink or non-directory ancestry ⇒ UNSAFE, never deletion input" — docs/wsl.md, docs/security-model.md) and the boundary wslproject.classify enforces. Every lexical ancestor must be checked without following symlinks.
🟡 [important] command.go:338 — orphan status is never re-proven before removal.
The apply loop deletes on the stale plan-time classification; wslvolume.Remove re-proves only the Docker-side identity. A project directory recreated between classification and its deletion turn still loses its volume. Re-run the path/absence proof immediately before each removal and abort that candidate if it is no longer ORPHAN (not fully atomic, but it closes the stale-plan window — again, what the dropped commit did).
🟡 [important] command.go:386 — cb gc python313 selects nothing in current-project mode.
expectedVolumes discards resolveFilter's state-group result (resolved, _ :=) and the predicate only special-cases the literal filter python. Built-in Python profiles (python, python3, pip, pip3) carry no state_group, while their volumes persist under owner python313. So cb gc python313 in a project directory errors with "no project-scoped state matches the current directory/filter" even though the venv volume exists — while cb gc python313 --orphans does match via the owner prefix. Same filter, inconsistent across modes, and the documented [TOOL|STATE_GROUP] surface is broken for the actual Python state group. (The same predicate shape exists in internal/state on Windows, so this is a ported asymmetry rather than a new regression — still a real defect here.)
What looks good
- The
wslrun→wslvolume.PlanPythonVolumesextraction is faithful: identical proof requirements and identical constructed identities, so runtime and state/GC agree on naming. - Dispatch in
main.gois correct:cb state/cb gcreach the native path before the fail-closed general gate only onWSL2Native, and Windows behavior is unchanged. - Validation ordering mirrors the
wslrunfrontend (layout → registry recovery → registry presence → policy → authenticated registry), whole-namespace proof precedes all output/mutation, and removal stays non-force with absence re-verification. - Suggestion (non-blocking):
projectWorkspace(/workspace/project) is now duplicated betweenwslrun/plan.goandwslstate/command.go; sharing the constant avoids divergent destination-collision planning if it ever changes.
The false-orphan deletion path in particular needs the previously written fix restored (or re-applied) before merge.
|
The re-review appears to have inspected a stale tree. GitHub currently points refs/heads/codex/wsl-state-integration to a683c16, and this review is itself attached to that commit, but the review body says it inspected 0e27959. The remote a683c16 tree contains the fixes the review says are absent:
The remote branch history is a683c16 -> 0189a31 -> 0e27959; it has not been reset to 0e27959. All checks on a683c16 are green and all six earlier threads are resolved. Please re-run the review from a fresh checkout of the current remote head a683c16. |
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Re-review of head a683c16. The branch now contains the previously dropped fix commits, and the earlier review's claim that they were missing no longer applies to this revision.
Validation of previous findings
/mnt/<drive>vanished mount → ORPHAN (blocking): fixed.wslproject.ProveMissingProjectrequires the nearest existing ancestor of a/mnt/<drive>-rooted path to sit at or below that exact mount point, and requires the live mount table to show a proven DrvFs mount whose device backs the nearest dentry (internal/wslproject/project.go:250-261). A vanished drive now yieldsUNSAFE, never a deletion candidate. Covered by the "live default drive"/"vanished default drive" cases inTestProveMissingProjectRequiresLiveSupportedStorageBoundary.- Symlinked ancestry escaping detection: fixed.
inspectMissingBoundaryLstats every lexical component from/down and rejects any symlink or non-directory component before absence is accepted (internal/wslproject/project.go:286-304). The example from last round (/work/linksymlink →/data) now classifiesUNSAFE. Covered by the "symlinked ancestor" case. - Stale orphan plan reused at removal: fixed. Apply mode re-runs
inspectProjectPathimmediately before each non-force removal and aborts the plan when the status is no longerORPHAN(internal/wslstate/command.go:339-349). Covered byTestGCRechecksOrphanImmediatelyBeforeRemoval. The residual re-check-to-docker rmwindow is inherent and is honestly documented as non-atomic. cb gc python313selected nothing in current-project mode: fixed.toolMatchesFilternow matches the persisted Python state group for python providers, and the dry-run test exercises bothpythonandpython313filters (internal/wslstate/command.go:444,command_test.go:74-84).
All four findings are resolved in the current head with appropriate regression coverage.
Fresh review of the current revision
🟡 [important] internal/wslstate/command.go:370 — a non-directory ancestor aborts the whole command instead of classifying UNSAFE.
inspectProjectPath routes only fs.ErrNotExist into the boundary proof. When any ancestor of the recorded path is replaced by a non-directory, lstat(recorded) returns ENOTDIR (not matched by errors.Is(err, fs.ErrNotExist)), so the error propagates and aborts the entire cb state report and any cb gc run for the whole namespace until the stray object is removed manually. Yet the identical condition reached through inspectMissingBoundary — which Lstats each ancestor individually — is classified UNSAFE, matching the documented contract ("non-directory objects are unsafe rather than guessed to be orphans"). The result is inconsistent: /work/app missing while /work is a file is provably unsafe ancestry, but the command hard-fails rather than reporting the volume UNSAFE and continuing. Consequence is fail-closed (nothing is wrongly deleted), but a single mundane filesystem state disables the state/GC surface namespace-wide. Mapping ENOTDIR (and possibly other provably-unsafe stat outcomes) to the UNSAFE classification — e.g. by routing the recorded path through proveMissing for ENOTDIR as well — would align it with the taxonomy while staying fail-closed. Defensible as written under "inspection errors fail closed", but the blast radius (whole report/orphan sweep) makes it worth reconsidering.
🟢 [nit] projectWorkspace still duplicated.
/workspace/project remains a separate literal in wslrun/plan.go:25 and wslstate/command.go:24. Carried over from the previous round; sharing the constant still avoids divergent destination-collision planning if the workspace ever moves. Non-blocking.
What looks good
- The
wslrun→wslvolume.PlanPythonVolumesextraction is faithful: identical proof requirements and identical constructed identities, so runtime and state/GC agree on naming;python313is now a single shared constant. - Dispatch in
main.gois correct:cb state/cb gcreach the native path before the fail-closed general gate only onWSL2Native; Windows behavior is unchanged (management dispatch, read-only registry load, no shim/binary proof needed for the management surface — consistent withcb wsl). - Validation ordering mirrors the
wslrunfrontend (layout → registry recovery → registry presence → policy → authenticated registry), whole-namespace proof precedes all output and mutation, removal stays non-force with absence re-verification, and shared volumes are structurally excluded from every GC path. - The double ancestry walk around the mount-table read in
proveMissingProjectcorrectly bounds mount/replacement races, including unmount-during-proof via the device/mode comparison.
Notes
- CI is green (Go/Linux tests, Windows + ARM64 builds, CodeQL, govulncheck, zizmor, reproducible release bundle).
- Pre-existing and out of scope here: the Windows
internal/statepredicate still lacks thepython313state-group match in current-project mode, so the platform asymmetry noted last round remains on the Windows side — a reasonable follow-up now that the WSL contract is fixed.


Implements the v2-required native WSL state-management subset from issue #2. Adds fixed-layout and authenticated-registry native cb state/cb gc dispatch; reconstructs and exactly proves every namespaced volume before output or mutation; keeps GC dry-run by default; excludes shared volumes; requires a missing recorded Linux project path with safe existing ancestry for orphan cleanup; and uses proof-bound non-force removal with absence verification. Refactors Python volume planning into the shared WSL volume identity layer so runtime and state commands construct identical identities. Updates the WSL support boundary and roadmap docs; native backup/restore remains deferred, while integration-corpus and real WSL2 + Docker Desktop qualification remain release blockers. Validation: go test -count=50 ./internal/wslstate ./internal/wslvolume ./internal/wslrun; go test -race ./...; go vet ./...; release-style Windows/Linux amd64/arm64 builds; canonical Windows cb.exe version check; git diff --check.