Deep Review: 20261002-104918-pr-5342

Date2026-10-02 10:49
Repolima-vm/lima
Round5 (of PR 5342)
Author@jandubois
PR#5342 — CI: factor Windows setup into composite actions; add plain-Windows jobs
Branchci-plain-windows-jobs
Commits2eb603cf copytool: expect converted paths in TestRsyncCommandPaths on Windows
fee349ad CI: factor Windows setup into composite actions; add plain-Windows jobs
Review SHAfee349adabfe893370d83065621bbe554da746fa
ReviewersClaude Opus 5.5 (effort: high), Codex GPT 6 Astra (effort: high), Gemini 3.1 Pro (effort: default), Gemini 3.5 Flash (effort: default)
VerdictMerge as-is — no Critical or Important finding remains; the three suggestions are optional
Wall-clock time18 min 50 s

Executive Summary

Round 5 adds a first commit that makes TestRsyncCommandPaths expect converted paths on Windows, and it applies round 4's six suggestions. All seven subtests now pass in the WSL2 job, and each round-4 fix does what its finding asked. This is the round where the PR converged. Nothing Critical or Important remains, and the three suggestions are optional. The largest asks a comment to say that the Git directory is still there after Inno Setup's uninstall, on every run.

Critical Issues

None.

Important Issues

None.

Suggestions

S1. The uninstall comment says the copy deletes the Git directory, but the fallback does it on every run Claude Opus 5.5
.github/actions/windows_plain_host/action.yml
      $ErrorActionPreference = 'Stop'
      $uninstaller = 'C:\Program Files\Git\unins000.exe'
      if (Test-Path $uninstaller) {
        Write-Host "Running Git for Windows Inno Setup uninstaller..."
        & $uninstaller /VERYSILENT /NORESTART /SUPPRESSMSGBOXES
        # A copy of unins000 in %TEMP% (_unins.tmp, or _iu*.tmp in older Inno
        # Setup) does the uninstall. unins000 waits for it, but the copy
        # outlives unins000 to delete unins000.exe and the Git directory.
        $deadline = (Get-Date).AddSeconds(120)
        while ((Get-Date) -lt $deadline -and
               (Get-Process -Name 'unins000', '_unins*', '_iu*' -ErrorAction SilentlyContinue)) {
          Start-Sleep -Seconds 2
        }

Both plain jobs at this SHA (jobs 110955229579 and 110955241525) log Removing leftover C:\Program Files\Git 16.4 and 22.3 seconds after starting the uninstaller, far inside the 120-second wait, and the next step starts within 50 ms. So the directory outlives the copy on every run, nearly empty, and the Remove-Item below finishes the job.

The mechanism the comment describes is right. Inno Setup 7.0.2 queues each directory it cannot remove, deletes unins000.exe once unins000 exits, and then retries the queue (Setup.UninstallLog.pas:1131-1135). That retry calls RemoveDirectory, which removes only an empty directory, so I think something Inno did not install stays behind. A reader of the comment expects the directory to be gone after the wait.

Fix: end the sentence with "to delete unins000.exe and, if nothing else is left in it, the Git directory". The copy also runs from a subdirectory of %TEMP%, not from %TEMP% itself.

S2. No test pins converting before the trailing-slash handling Claude Opus 5.5
pkg/copytool/rsync.go
	copyPaths, err := parseCopyPaths(ctx, paths)
	if err != nil {
		return nil, err
	}

	// Format before the trailing-slash handling below, which decides whether
	// rsync copies a directory or its contents.
	for _, cp := range copyPaths {
		if cp.IsRemote {
			continue
		}
		if cp.Path, err = t.pathForRsync(ctx, cp.Path); err != nil {
			return nil, err

On Windows parseCopyPaths keeps an absolute path in native form (pkg/copytool/copytool.go:153-154), and trimTrailingSlashes strips only / (rsync.go:260). Converting first is what turns C:\x\dir\ into /c/x/dir/ before the trim. If this loop moved below adjustTrailingSlashes, the trim would leave the backslash in place, and limactl copy -r C:\x\dir\ inst:/existing would copy the directory's contents instead of the directory.

pkg/copytool/copytool.go
			// absolute, so single-letter instance names still work.
			//
			// The path stays in native form here. scp and rsync can come from
			// different toolchains, so each backend formats it for the binary
			// that consumes it.
			if filepath.IsAbs(path) {
				cp.Path = path
				cp.IsRemote = false
				copyPaths = append(copyPaths, cp)
				continue
			}
			path = filepath.ToSlash(path)
pkg/copytool/rsync.go
}

// trimTrailingSlashes removes the trailing slashes from path, unless the path is made of
// slashes only (e.g. "/"), as trimming those would turn it into an empty path.
func trimTrailingSlashes(path string) string {
	if trimmed := strings.TrimRight(path, "/"); trimmed != "" {
		return trimmed
	}
	return path
}

Every TestRsyncCommandPaths case spells the trailing separator as /, so all seven would still pass after that move. The comment above the loop is the only guard. This is reasoned from the code; nothing ran on Windows.

Fix: add a Windows-only case whose source is dir plus a trailing backslash and whose destination exists, expecting dir.

S3. exit 0 lost its explanation Claude Opus 5.5
.github/actions/windows_plain_host/action.yml
        if (Test-Path $d) {
          Write-Host "Removing leftover $d"
          Remove-Item $d -Recurse -Force -ErrorAction SilentlyContinue
        }
      }
      # pwsh does not wait for the GUI uninstaller or record its exit status,
      # so the verify step below is the gate.
      exit 0

  - name: Uninstall MSYS2 and standalone mingw
    shell: pwsh
    run: |
      $ErrorActionPreference = 'Stop'

The round-4 rewrite says pwsh does not record an exit status for the uninstaller. If that holds, $LASTEXITCODE stays unset, the exit $LASTEXITCODE line the runner appends to every pwsh step never fires, and exit 0 changes nothing, so the next reader may delete it. The old wording ("Discard it") said why the line exists, and windows_wsl2_setup/action.yml:24 still explains its own exit 0. This is reasoned; pwsh is not installed on the review host.

.github/actions/windows_wsl2_setup/action.yml
      # returns transient 403s, so these four calls are informational.
      wsl --update
      wsl --status
      wsl --version
      wsl --list --online
      # pwsh would otherwise exit with --list --online's status.
      exit 0
  - name: Install dummy WSL2 distro
    shell: pwsh
    run: |
      $ErrorActionPreference = 'Stop'

Fix: "...or record its exit status, so discard any status and let the verify step below be the gate."

Design Observations

Concerns

None new. Round 4's two concerns, the partial pacman -Sy upgrade and the unchecked ssh selection in the plain jobs, stay deferred as future work (see Acknowledged Limitations).

Strengths

Testing Assessment

  1. The test fix is pinned on Windows. Reverting 2eb603cf fails TestRsyncCommandPaths in the Unit tests step of Windows tests (WSL2), where rsync is now on PATH. At this SHA all seven subtests pass there. Off Windows pathForRsync returns its input, so the test behaves as before on Linux and macOS.
  2. One class of conversion bug still passes. The expected value comes from the same pathForRsync as the code, so a conversion that emits a wrong path starting with / passes, such as one run through the PATH-global cygpath instead of the one beside rsync Claude Opus 5.5 Codex GPT 6 Astra. TestRsyncCommandLocalPathForm pins only the in-process fallback form, and S2 covers the conversion order.
  3. Reverting the CI guards does not fail any test. The MSYS2 file check, the _unins* wait, ^git$ and the QEMU check guard CI configuration that only this project runs, so a test for them is optional. ^git$ has never matched in CI, because the uninstall removes the key before the verify step.
  4. Required tests: none. The optional one in user-facing code is S2's case.

Executed during consolidation: fetched the check runs for fee349ad and the logs of all four Windows jobs, and staged the logs for the agents; ran go test ./pkg/copytool/ ./pkg/sshutil/ and GOOS=windows go vet over copytool, sshutil and cmd/limactl, all clean; read Inno Setup's Setup.Uninstall.pas and Setup.UninstallLog.pas at tag is-7_0_2; counted each agent's executed commands in its session log. Integration tests (vz) failed its first attempt at port-forwarding.bats test 37 (Connection refused to HOST_LAN_IP:4020). This PR's only non-test Go change is inside a runtime.GOOS == "windows" branch, so that failure is unrelated; the second attempt was still running when this report was written. Nothing ran on Windows.

Documentation Assessment

breaking.md lists the removal under ## v2.3.0, and the newest upstream tag is v2.3.0-beta.0, so that heading has not shipped. "Put them on PATH instead." matches the removed code, and git grep finds _LIMA_WINDOWS_EXTRA_PATH only in breaking.md:7. The new test.yml:247 comment is right that templates/experimental/wsl2.yaml does not set base:. S1 and S3 cover the two comments that need work.

Commit Structure

Two signed-off commits, merging unsquashed by design, because only this PR's WSL2 job exercises the test fix. 2eb603cf changes only pkg/copytool/copytool_test.go, and pathForRsync and looksRemoteToRsync already exist at the merge-base, so it builds alone. fee349ad leaves pkg/copytool untouched, so that package passes at 2eb603cf as it does at HEAD. Two notes:

Acknowledged Limitations

Declined in prior rounds

Unresolved Feedback

Agent Performance Retro

Claude Opus 5.5

It raised all three suggestions. It found S1 by setting the Removing leftover log lines beside the rewritten comment, which no other reviewer did, though all four had the logs. It rated S1 Important; the Reconciliation below gives the reason for the downgrade. Its log citations all resolved, its mutation table is marked as reasoned, and, as the prompt asked, it did not re-run go test.

Codex GPT 6 Astra

It did not raise a finding, and its evidence was the round's best, with a verdict table naming the files read for each delta item, and a reversion table marked as reasoned. It counted the WSL2 job's 44 passing packages with a script over the log rather than trusting the prompt. Like Claude, it saw that the test computes its expected value with the function under test. It marked the Inno Setup claims unverified, although its own grep of the job logs searched for Removing leftover.

Gemini 3.1 Pro

It did not raise a finding. Its item-1 evidence says it mutated the test and watched dir_into_missing_dst fail, but its 24 shell commands do not include a go test: it applied a reverse patch and moved on, leaving copytool_test.go.orig and run_test.go.patch untracked in its worktree. That mutation changed the test's own expected value, so even a real run would have said nothing about the code. Its verdicts on items 2 to 6 rest on reading the files.

Gemini 3.5 Flash

It did not raise a finding. It listed go test and GOOS=windows go vet under "Verification Probes Executed", but its 26 commands include neither; those were the results the prompt supplied. Its claim that cygpath strips trailing slashes conflicts with the non-recursive subtest passing on Windows. Its log greps did confirm the QEMU check and both round trips. The two Gemini models share a family, so their agreement on a clean round counts once.

Summary

Claude Opus 5.5Codex GPT 6 AstraGemini 3.1 ProGemini 3.5 Flash
Duration8m 10s3m 49s5m 10s5m 46s
Findings3Snonenonenone
Tool calls33 (Bash 29, Read 4)8 (shell 8)24 (runshellcommand 24)50 (runshellcommand 26, readfile 15, grepsearch 8)
Design observations2303
False positives0000
Unique insights3000
Files reviewed13131313
Coverage misses0000
Totals3Snonenonenone
Downgraded1000
Dropped0000

Reconciliation. Claude Opus 5.5 I1 (the uninstall comment): important → suggestion S1. Inno Setup 7.0.2 retries the queued directories after deleting unins000.exe, so the comment describes the mechanism correctly; it omits that the retry removes only an empty directory.

Claude was the only reviewer to compare a comment against the run that tests it, and that comparison produced the round's one finding with evidence behind it. Codex gave the most trustworthy clean verdict. Both Gemini models presented runs they did not perform as their own, so their verdicts here count as reading only.

Review Process Notes

Skill improvements