Repository navigation
Classify native WSL non-directory ancestry as unsafe - #128
AviBackToBlack wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused change safely isolates invalid records and includes appropriate regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Classifies native WSL paths with non-directory ancestors as unsafe without disrupting state/GC scans.
Changes:
- Handle
ENOTDIRasUNSAFE. - Add regression coverage ensuring orphan proof is skipped.
| File | Description |
|---|---|
internal/wslstate/command.go |
Adds fail-closed ENOTDIR handling. |
internal/wslstate/command_test.go |
Tests non-directory ancestry classification. |
💡 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 head a768ab4 end to end. This is the follow-up to the 🟡 finding from my final #127 review — a recorded native WSL project path whose lstat fails ENOTDIR aborted the entire cb state / cb gc namespace scan instead of classifying UNSAFE.
Fix validation against the prior finding
ENOTDIR→UNSAFE(internal/wslstate/command.go:371-373): correct. This matches the documented contract ("a vanished default Windows-drive mount, symlinked ancestry or non-directory object isUNSAFEand never deletion input" —docs/wsl.md:360) and the outcomeinspectMissingBoundaryproduces for the identical physical state (internal/wslproject/project.go:293-295rejects a non-directory ancestor lexically).- Skipping
proveMissingonENOTDIRis sound. The non-directory ancestor is already proven; routing throughproveMissingwould only reproduceUNSAFEvia its lexical walk after an unnecessary mount-table read. The regression test correctly asserts the boundary proof is not invoked. - All three consumers behave correctly with the new classification: the
cb statereport rendersUNSAFE, the--orphansscan excludes it from candidates, and the--applyre-check refuses the stale plan if a recorded path flips toENOTDIRancestry between planning and removal (command.go:343-349). Fail-closed everywhere. - Portability:
command.gois untagged and compiled for Windows, wheresyscall.ENOTDIRis defined and unreachable in practice (Windowslstatsurfaces path-not-found, still matchingfs.ErrNotExistbelow). Windows behavior is unchanged. - Check ordering (
ENOTDIRbeforefs.ErrNotExist) is right; the two are disjoint anyway sinceErrNotExistmatchesENOENTonly.
The prior finding is resolved as intended.
Fresh review of the current revision
🟢 [nit / optional] residual of the same class — ELOOP and other non-ENOENT lstat failures still abort the whole surface.
An ancestor that is a symlink loop makes lstat(recorded) return ELOOP, which still propagates as a fatal error and disables cb state / cb gc namespace-wide — the same blast radius the ENOTDIR fix removes, though reached through a rarer filesystem state (EACCES/ENAMETOOLONG likewise). ELOOP ancestry is provably unsafe under the same taxonomy (the lexical walk would reject the symlinked component). The previous round's remediation note already flagged "possibly other provably-unsafe stat outcomes," so this is optional completeness, not a defect in this fix — hard-fail is still fail-closed and defensible under "inspection errors abort." A small allowlist (ENOTDIR, ELOOP) or an explicit comment recording that unmapped stat errors intentionally abort would close the gap.
🟢 [nit, carried over] projectWorkspace still duplicated.
/workspace/project remains a literal in both wslrun/plan.go and wslstate/command.go. Unchanged from #127; still non-blocking.
What looks good
- Minimal, targeted change with a precise regression test proving the already-established non-directory ancestry never enters the missing-boundary/orphan proof.
- Classification stays consistent with the
UNSAFE-means-never-deletion contract on every path, including the apply-time stale-plan re-check.
Note: repo CI checks were still pending at review time; nothing in this diff is platform- or toolchain-sensitive, but worth confirming the Windows builds go green.
No actionable blocking or important findings on this revision.
Summary
ENOTDIRasUNSAFEcb state/cb gcnamespace scanContext
Follow-up to the fresh CherylSnowVeil review on #127. That PR merged before this substantive finding could be committed. The merged behavior remained fail-closed, but one non-directory ancestor could disable the complete native WSL state/GC surface instead of isolating the unsafe record.
Validation
gofmt -l .go vet ./...go test ./...go test -race ./...v2.0.0-rc.review; built binary reported the injected versiongit diff --check origin/main...HEADa768ab4verified with the configured hardware-backed SSH signing key