Deep Review: 20260727-171515-pr-5340

Date2026-07-27 17:15
Repolima-vm/lima
Round4 (of pr-5340)
Author@jandubois
PR#5340 — shell: measure the --sync directory depth on the host's own path
Branchshell-sync-depth
Commits174f386b shell: measure the --sync directory depth on the host's own path
Review SHA174f386b4c605f62c0731a5b8698eb72da3b37f2
ReviewersClaude Opus 5 (effort: xhigh), Codex GPT 5.6 Sol (effort: xhigh), Gemini 3.1 Pro (effort: default), Gemini 3.5 Flash (effort: default)
VerdictMerge as-is — the round-3 defect is fixed and regression-tested; what remains is comment and error-message polish
Wall-clock time25 min 27 s

Executive Summary

The backslash defect that survived three rounds is fixed and pinned by two regression rows. I re-derived both equivalence claims myself: over 1,000,000 generated strings the Windows mode never differs from the round-3 helper, and the Unix mode never scores higher than the merge-base guard, with 0 disagreements across 82,701 real directories. No agent found a correctness defect. What remains is comment and error-message polish.

All four reviewers ran this round. Gemini 3.1 Pro was quota-exhausted on launch for the third round running, but its reset was 7m22s rather than round 3's 4h27m, so a retry inside the review window restored it. Claude raised five of the six suggestions; Codex raised the one nobody else saw and corrected a stale entry in the repo context file. Gemini 3.1 Pro's single finding rests on a branch that does not do what it says, and I dropped it. I ran the suite, the vet, the Windows cross-build, and my own differential at the review SHA in a clean tree.

Critical Issues

None.

Important Issues

None.

Suggestions

S1. A wsl2 instance with mounts is told to run --mount-none, which cannot help Codex GPT 5.6 Sol
cmd/limactl/shell.go
	syncDirVal, err := flags.GetString("sync")
	if err != nil {
		return fmt.Errorf("failed to get sync flag: %w", err)
	}
	syncHostWorkdir := syncDirVal != ""
	if syncHostWorkdir && len(inst.Config.Mounts) > 0 {
		return errors.New("cannot use `--sync` when the instance has host mounts configured, start the instance with `--mount-none` to disable mounts")
	}
	// A wsl2 guest already reaches the host directory through the /mnt automount,
	// so `--sync` cannot isolate it from host files the way it does elsewhere.
	if syncHostWorkdir && inst.VMType == limatype.WSL2 {
		return errors.New("cannot use `--sync` with a wsl2 instance, the host directory is already visible in the guest")
	}

The new wsl2 refusal sits directly below this check, so a wsl2 instance that has mounts configured hits the mounts error first. Following its remedy costs an instance recreate and then produces the second error, which no flag can clear. The mounts check is remediable and conditional; the wsl2 check is neither.

templates/default.yaml:58 sets mounts: [], so the common case reaches the wsl2 error directly. That keeps this a diagnostic gap rather than a defect.

templates/default.yaml
# {{.GlobalTempDir}}, and {{.TempDir}}. The global temp dir is always "/tmp" on Unix.
# "mountPoint" can use these template variables: {{.Home}}, {{.Name}}, {{.Hostname}}, {{.UID}}, {{.User}}, and {{.Param.Key}}.
# 🟢 Builtin default: [] (Mount nothing)
# 🔵 This file: Mount the home as read-only (inherited via the `base` mechanism later in this file)
#    Until Lima v1.2, /tmp/lima was mounted too as writable.
mounts: []
# - location: "~"
#  # Configure the mountPoint inside the guest.
#  # 🟢 Builtin default: value of location
#  mountPoint: null
#  # Setting `writable` to true is discouraged when mountType is set to "reverse-sshfs".

Fix: move the wsl2 check and its comment above the mounts check, so the unconditional refusal is reported first.

S2. The converted-path guard cannot fire off Windows, and nothing says so Claude Opus 5
cmd/limactl/shell.go
				hostCurrentDirNative, rsyncMinimumSrcDirDepth, srcWdDepth, "cd to a deeper directory")
		}
		// rsync acts on hostCurrentDir, so measure that too: cygpath can report
		// success without writing a path, and an fstab can map a deep directory
		// onto a shallow one. It is always POSIX form, even on Windows.
		if dstWdDepth := pathDepth(hostCurrentDir, false); dstWdDepth < rsyncMinimumSrcDirDepth {
			return fmt.Errorf("expected the depth of the converted host working directory (%#q) to be at least %d, only got %d",
				hostCurrentDir, rsyncMinimumSrcDirDepth, dstWdDepth)
		}
	}

	var destRsyncDir string
	workDir, err := cmd.Flags().GetString("workdir")
	if err != nil {

hostCurrentDir is reassigned only inside if runtime.GOOS == "windows" (shell.go:237), so elsewhere it holds the same string as hostCurrentDirNative, and line 257 already passes false for the mode there. Both calls then compute the same number, and this check cannot fail after line 258 passed. On macOS and Linux, the only platforms where --sync has integration coverage, it reads as an independent second guard while being inert.

cmd/limactl/shell.go
	} else {
		hostCurrentDirNative, err = os.Getwd()
	}
	if err == nil {
		hostCurrentDir = hostCurrentDirNative
		if runtime.GOOS == "windows" {
			hostCurrentDir, err = mountDirFromWindowsDir(ctx, inst, hostCurrentDirNative)
		}
	}

	if err != nil {
cmd/limactl/shell.go
			return fmt.Errorf("rsync is required for `--sync` but not found: %w", err)
		}

		// Measure the host's own path. The form hostCurrentDir carries on Windows
		// adds one component (/c/...) or two (/cygdrive/c/...).
		srcWdDepth := pathDepth(hostCurrentDirNative, runtime.GOOS == "windows")
		if srcWdDepth < rsyncMinimumSrcDirDepth {
			return fmt.Errorf("expected the depth of the host working directory (%#q) to be at least %d, only got %d (Hint: %s)",
				hostCurrentDirNative, rsyncMinimumSrcDirDepth, srcWdDepth, "cd to a deeper directory")
		}
		// rsync acts on hostCurrentDir, so measure that too: cygpath can report
		// success without writing a path, and an fstab can map a deep directory

Fix: open the comment with "On Windows" so the scope is stated. Claude proposed gating the condition on runtime.GOOS == "windows"; I would not, since the unconditional form costs one string comparison and keeps the guard if hostCurrentDir ever diverges on another platform.

S3. The converted-path error names only a path the user never typed Claude Opus 5
cmd/limactl/shell.go
		}
		// rsync acts on hostCurrentDir, so measure that too: cygpath can report
		// success without writing a path, and an fstab can map a deep directory
		// onto a shallow one. It is always POSIX form, even on Windows.
		if dstWdDepth := pathDepth(hostCurrentDir, false); dstWdDepth < rsyncMinimumSrcDirDepth {
			return fmt.Errorf("expected the depth of the converted host working directory (%#q) to be at least %d, only got %d",
				hostCurrentDir, rsyncMinimumSrcDirDepth, dstWdDepth)
		}
	}

	var destRsyncDir string
	workDir, err := cmd.Flags().GetString("workdir")

This fires on a Windows host whose cygpath prints nothing or whose fstab remaps the drive. The user ran limactl shell --sync C:\Users\jan\proj and is told about /proj or about an empty string, with no sign that a conversion happened or which tool performed it. The sibling error two lines above ends with (Hint: cd to a deeper directory); this one offers nothing, and here that hint would be wrong anyway, since going deeper cannot fix a conversion.

Fix: name both forms, and point the hint at the conversion rather than at the directory.

S4. The pathDepth docstring omits the two contracts its callers depend on Claude Opus 5
cmd/limactl/shell.go
		return err
	}
	return nil
}

// pathDepth counts the separator-delimited fields of an absolute path,
// collapsing repeated separators. A trailing separator adds no field unless the
// path is a bare root. Pass windows for a path in Windows form: elsewhere a
// backslash is an ordinary filename character, and counting it as a separator
// would score a directory just below the root deep enough to pass the guard.
func pathDepth(path string, windows bool) int {
	slashed := path
	if windows {
		slashed = strings.ReplaceAll(path, `\`, "/")
		// An extended-length or device prefix spells a path the plain form also

Two properties live only in the test table. A drive letter occupies the root field, so C:\Users\jan and /Users/jan both score 3, and that equivalence is the reason one threshold is portable at all, which is this change's headline claim. An empty path returns 1, and shell.go:265 relies on that to refuse a cygpath result that came back empty; the docstring instead scopes itself to "an absolute path" and so disclaims the case the guard needs.

cmd/limactl/shell.go
				hostCurrentDirNative, rsyncMinimumSrcDirDepth, srcWdDepth, "cd to a deeper directory")
		}
		// rsync acts on hostCurrentDir, so measure that too: cygpath can report
		// success without writing a path, and an fstab can map a deep directory
		// onto a shallow one. It is always POSIX form, even on Windows.
		if dstWdDepth := pathDepth(hostCurrentDir, false); dstWdDepth < rsyncMinimumSrcDirDepth {
			return fmt.Errorf("expected the depth of the converted host working directory (%#q) to be at least %d, only got %d",
				hostCurrentDir, rsyncMinimumSrcDirDepth, dstWdDepth)
		}
	}

Fix: state the drive-letter equivalence and the empty-path result.

S5. The comment justifying the native measurement does not hold for a UNC source Claude Opus 5
cmd/limactl/shell.go
	if syncHostWorkdir {
		if _, err := exec.LookPath(string(copytool.BackendRsync)); err != nil {
			return fmt.Errorf("rsync is required for `--sync` but not found: %w", err)
		}

		// Measure the host's own path. The form hostCurrentDir carries on Windows
		// adds one component (/c/...) or two (/cygdrive/c/...).
		srcWdDepth := pathDepth(hostCurrentDirNative, runtime.GOOS == "windows")
		if srcWdDepth < rsyncMinimumSrcDirDepth {
			return fmt.Errorf("expected the depth of the host working directory (%#q) to be at least %d, only got %d (Hint: %s)",
				hostCurrentDirNative, rsyncMinimumSrcDirDepth, srcWdDepth, "cd to a deeper directory")
		}

This is the stated reason for measuring hostCurrentDirNative at line 257: the converted form scores higher, so measuring it would be too generous. A UNC source adds nothing. I measured \\server\share\proj at 4 in Windows mode and cygpath's //server/share/proj at 4 in POSIX mode, because the collapse loop at line 790 folds the two leading separators into one. The guard stays correct, since equal is not more generous; only the justification is narrower than written.

cmd/limactl/shell.go
			return fmt.Errorf("rsync is required for `--sync` but not found: %w", err)
		}

		// Measure the host's own path. The form hostCurrentDir carries on Windows
		// adds one component (/c/...) or two (/cygdrive/c/...).
		srcWdDepth := pathDepth(hostCurrentDirNative, runtime.GOOS == "windows")
		if srcWdDepth < rsyncMinimumSrcDirDepth {
			return fmt.Errorf("expected the depth of the host working directory (%#q) to be at least %d, only got %d (Hint: %s)",
				hostCurrentDirNative, rsyncMinimumSrcDirDepth, srcWdDepth, "cd to a deeper directory")
		}
		// rsync acts on hostCurrentDir, so measure that too: cygpath can report
cmd/limactl/shell.go
		}
	}
	// A separator run delimits one field. The two leading separators of a UNC
	// path enclose one root, so counting both would clear the minimum depth for
	// a share root.
	for strings.Contains(slashed, "//") {
		slashed = strings.ReplaceAll(slashed, "//", "/")
	}
	// filepath.Clean keeps the trailing separator of a root, so `\\server\share`
	// and `\\server\share\` both reach here. Trim one that leaves a separator
	// behind, which spares the bare roots "/" and `C:\`.

Fix: bound the claim to drive-letter paths and say a UNC path converts to the same depth.

S6. The commit body does not account for two hunks Claude Opus 5 
			return fmt.Errorf("expected the depth of the host working directory (%#q) to be at least %d, only got %d (Hint: %s)",

The merge-base spells this "to be more than %d" (acdea753:cmd/limactl/shell.go, line 243). Correcting it to "at least" fixes prose that contradicted the srcWdDepth < rsyncMinimumSrcDirDepth test, which accepts 4. The change is user-visible and the body does not mention it. Neither does the body mention the website/content/en/docs/examples/ai.md bullet.

Round 2 asked for a body that accounts for every hunk, and round 3 confirmed it did. Two hunks now fall outside it.

Fix: add one clause covering the error-wording correction and the docs bullet.

Design Observations

Concerns

Strengths

Testing Assessment

I ran everything below myself from the lead worktree at 174f386b with a clean tree. go test ./cmd/limactl/ passes, go vet ./cmd/limactl/ exits 0, and GOOS=windows GOARCH=amd64 go build ./cmd/limactl/ succeeds. TestPathDepth runs 20 subtests and all pass. All four agents report the same, and Codex also ran go test ./... -count=1.

I wrote my own differential rather than inheriting round 3's numbers. Over 1,000,000 generated strings drawn from / \ ? . : C U a b and space: pathDepth(p, true) never differs from the round-3 helper (0 mismatches), and pathDepth(p, false) never scores higher than the merge-base len(strings.Split(p, "/")) (0 cases). The second result is stronger than round 3 claimed and provable rather than sampled: collapsing separator runs and trimming one trailing separator can only lower the count, so the Unix mode is uniformly at least as strict as the guard it replaces. Across 82,701 real directories reached through filepath.Abs there were 0 disagreements with the merge-base.

Gaps, in rough risk order. All were declined in earlier rounds and are listed for completeness, not raised again.

  1. The call site is unpinned. Nothing fails if line 257 reverts to pathDepth(hostCurrentDir, ...) or if the mode hard-codes true. TestPathDepth proves the formula and pins nothing about which variable it measures, which is the exact defect this PR fixes.
  2. The converted-path guard at line 265 is unreached by any test, and on the platforms CI runs --sync on it cannot fire at all (S2).
  3. The wsl2 refusal and the conversion-failure branch have no coverage. hack/bats/helpers/limactl.bash hard-codes the template per instance name with no vm-type hook.
  4. No BATS test asserts a depth rejection, and the bats job is runs-on: ubuntu-24.04 (.github/workflows/test.yml:358).
  5. No Windows end-to-end --sync. I confirmed hack/test-templates.sh contains no --sync invocation at all.
  6. pathDepth("", true) is uncovered. The table exercises the empty path only in Unix mode. Both modes return 1, so this is a one-row gap.

Documentation Assessment

The new ai.md bullet records the wsl2 restriction and matches the code at shell.go:218. The docstring gained the trailing-separator rule and the UNC token note that round 3 asked for; S4 covers what it still omits.

Every other factual claim in the commit body checks out. The merge-base split the converted path on os.PathSeparator and scored every Windows path at 1; the empty string scored 1 and was the only thing refusing a failed conversion; an empty path reaches rsync as / through pkg/copytool/rsync.go:166; --tty defaults to whether stdout is a terminal (cmd/limactl/main.go:127) and shell.go:569 calls rsyncBack() unprompted when it is false, with --delete set at shell.go:451. S6 covers the two hunks the body omits.

The --sync flag help at shell.go:76 still describes only the copy behaviour, with no pointer to the mounts, depth, or wsl2 restrictions. Pre-existing.

Commit Structure

One commit, DCO signed off, subject matching the diff. The body explains intent rather than mechanics and justifies bundling the wsl2 refusal, which would otherwise read as an unrelated behaviour change: without it the depth fix would newly admit a configuration that cannot work. Splitting the two would ship a window in which Windows --sync reaches rsync with a /mnt/c/... path. S6 is the only accuracy gap.

Acknowledged Limitations

Declined in prior rounds

Agent Performance Retro

[Claude Opus 5] — It raised five of the six suggestions, and every one survived verification. Its strongest work came from reading the comments against the code rather than reading the code alone: S2, S4, and S5 each name a sentence that is true of the case its author had in mind and false of a neighbouring one. It was also the only reviewer to diff the commit body against the diff hunks and notice the error-wording correction goes unmentioned. I declined its proposed fix for S2, which gates the check on runtime.GOOS == "windows" and trades a free guard for clarity a comment buys more cheaply. It left an untracked limactl.exe in its worktree from the Windows cross-build; no tracked file was modified.

[Codex GPT 5.6 Sol] — One finding, and it is the only one this round that no other reviewer reached: a wsl2 instance with mounts is handed a remedy that cannot work. Reaching it needed the two guard clauses read in order and then the user's next step imagined, which is a different move from checking either clause alone. It was also the only agent to contradict its own injected context, and it was right to: the repo context file claims both Windows jobs run go test, and only Windows tests (WSL2) does. Its per-change verdict table answered the round-4 prompt directly, and its observation that --start boots the instance before either refusal became a design observation.

[Gemini 3.1 Pro] — Quota-exhausted on launch for the third consecutive round, but the reset was 7m22s rather than round 3's 4h27m, so a retry inside the review window recovered it. Its one finding does not hold: it calls changeDirCmd = "false" dead because the switch at line 291 overwrites the variable "in every branch, including the default: branch", and that branch only calls logrus.Debug. The line is also pre-existing and reaches the diff as context. The assignment is arguably redundant, though for a reason the finding never names, the if changeDirCmd == "" fallback at lines 307 to 309. Its per-change verification section is accurate but restates the prompt without measuring anything.

[Gemini 3.5 Flash] — It returned in about two minutes with one suggestion that restates a testing gap the prompt listed as declined in all three prior rounds, so it cost a slot and added nothing. It gave no verdict on the five named changes, which the prompt required of every agent. For the third round running it leaked tool narration above its header, this time two lines. Because both Gemini reviewers share a model family, their agreement would count as one opinion; here neither produced a surviving finding, so the question does not arise.

Summary

Claude Opus 5Codex GPT 5.6 SolGemini 3.1 ProGemini 3.5 Flash
Duration13m 25s13m 55s5m 43s2m 24s
Findings5S1Snonenone
Tool calls29 (Bash 21, Read 8)41 (shell 32, stdin 6, plan 3)10 (runshellcommand 10)16 (readfile 7, grepsearch 4, runshellcommand 2)
Design observations4423
False positives0010
Unique insights5200
Files reviewed3333
Coverage misses0000
Totals5S1Snonenone
Downgraded0000
Dropped0011

Codex gave the most value per token: one finding, unique, plus the only challenge to the injected repo context that turned out to be correct. Claude gave the most value overall, supplying five of six suggestions and the only reading of the comments as claims to be checked. Gemini 3.1 Pro's recovered run was worth the retry mainly as evidence that the quota rule can be relaxed, not for its output. Flash cost little and added nothing for the second round running.

Review Process Notes

Skill improvements

Repo context updates