Deep Review: 20260726-235250-pr-5299

Date2026-07-26 23:52
Repolima-vm/lima
Round5 (of target)
Author@jandubois
PR#5299 — copytool: support native Windows OpenSSH
Branchcopytool-native-windows-openssh
Commits628fa44a copytool: support native Windows OpenSSH
Review SHA628fa44ac889b5907fb4dc1f926d963f18d550e4
ReviewersClaude Opus 5 (effort: xhigh), Codex GPT 5.6 Sol (effort: xhigh), Gemini 3.1 Pro (effort: default, quota-failed), Gemini 3.5 Flash (effort: default)
VerdictMerge as-is — four optional cleanups, none blocking; both Important findings failed verification
Wall-clock time24 min 32 s

Executive Summary

Round 5 reviews the recut commit that closes all seven round-4 findings, and every fix holds. Both agent-raised Important findings failed verification: executing the merge with #5337 shows copytool.go auto-merges and only the test file conflicts, exactly as the PR description says, and the CommonOpts signature break is real but matches how this repo already treats pkg/sshutil. What remains are four small cleanups in sshutil and rsync.

Three reviewers produced output. Gemini 3.1 Pro hit a terminal quota error and contributed nothing, which costs less than the count suggests, since the two Gemini models share a family and have converged on near-empty output for three rounds running. Claude raised every surviving finding. The round's most useful work was adjudication rather than discovery: two Important claims arrived with confident write-ups, and both dissolved once I ran the merge and read the release history.

Critical Issues

None.

Important Issues

None. Both candidates were refuted or downgraded; see the Reconciliation paragraph in the retro.

Suggestions

S1. cygpathForSSH keeps an ssh-specific name after its contract widened Claude Opus 5
pkg/sshutil/sshutil.go
// symlinks) and whether toolExe is Cygwin-based. PathForTool passes non-ssh
// binaries here too. Callers pass the returned path to exec.Command so
// conversions run through that toolchain's own cygpath, even when $SSH points
// outside PATH. It returns ("", false) on non-Windows or empty input; results
// are cached per resolved path.
func cygpathForSSH(toolExe SSHExe) (string, bool) {
	if runtime.GOOS != "windows" {
		return "", false
	}
	path, ok := resolvedToolPath(toolExe)
	if !ok {

The same pass renamed resolvedSSHPath to resolvedToolPath and both parameters to toolExe, and the doc at line 185 now says "PathForTool passes non-ssh binaries here too". The function name is the last ssh-specific token left. CompanionForSSH and SftpServerForSSH genuinely take an ssh and should keep their names.

pkg/sshutil/sshutil.go
	}
	return companion
}

// cygpathForSSH returns the cygpath.exe beside toolExe (resolved through
// symlinks) and whether toolExe is Cygwin-based. PathForTool passes non-ssh
// binaries here too. Callers pass the returned path to exec.Command so
// conversions run through that toolchain's own cygpath, even when $SSH points
// outside PATH. It returns ("", false) on non-Windows or empty input; results
// are cached per resolved path.
func cygpathForSSH(toolExe SSHExe) (string, bool) {

Fix: rename to cygpathForTool, and rename TestCygpathForSSH (pkg/sshutil/sshutil_windows_test.go:58) with it. This finishes the rename round 4's S6 started.

S2. PathForTool builds an SSHExe to pass one string Claude Opus 5
pkg/sshutil/sshutil.go
// this returns as a fallback would be wrong for them.
func PathForTool(ctx context.Context, toolPath, orig string) (string, error) {
	if runtime.GOOS != "windows" {
		return orig, nil
	}
	if cygpathExe, ok := cygpathForSSH(SSHExe{Exe: toolPath}); ok {
		return fsutil.WindowsSubsystemPathWithCygpath(ctx, cygpathExe, orig)
	}
	return filepath.ToSlash(orig), nil
}

Neither cygpathForSSH nor resolvedToolPath reads SSHExe.Args; both use .Exe alone. The wrapper struct exists to satisfy a signature, and it invites the reader to ask whether Args affects path conversion. It does not.

Fix: give both helpers a string parameter, then PathForSSH and SftpServerForSSH pass sshExe.Exe. Pairs naturally with S1.

S3. SSHOptsWithoutMultiplexing's doc omits its most surprising caller Claude Opus 5
pkg/sshutil/sshutil.go
		opts = append(opts, "ForwardX11Trusted=yes")
	}
	return opts, nil
}

// SSHOptsWithoutMultiplexing returns CommonOpts plus an explicit User, adding
// neither multiplexing nor forwarding options. Use it for invocations that
// cannot share a control socket: native Windows OpenSSH has no multiplexing,
// and Cygwin ssh's is unreliable. It builds no control path, so the caller also
// escapes the socket length limit SSHOpts enforces.
// Path options take toolPath's form; pass sshExe.Exe unless another binary
// receives the options, as described on CommonOpts.
func SSHOptsWithoutMultiplexing(ctx context.Context, sshExe SSHExe, toolPath, username string, useDotSSH bool) ([]string, error) {

Two of the three callers match that description. The third is SSHOpts itself at line 598, which calls it as a base and then appends the very control socket the doc says its callers cannot have. That matters because the User= option moved into this function, so its position relative to ControlMaster is now set here.

pkg/sshutil/sshutil.go
func SSHOpts(ctx context.Context, sshExe SSHExe, instDir, username string, useDotSSH, forwardAgent, forwardX11, forwardX11Trusted bool) ([]string, error) {
	controlSock := filepath.Join(instDir, filenames.SSHSock)
	if len(controlSock) >= osutil.UnixPathMax {
		return nil, fmt.Errorf("socket path %#q is too long: >= UNIX_PATH_MAX=%d", controlSock, osutil.UnixPathMax)
	}
	opts, err := SSHOptsWithoutMultiplexing(ctx, sshExe, sshExe.Exe, username, useDotSSH)
	if err != nil {
		return nil, err
	}
	controlPath := fmt.Sprintf(`ControlPath="%s"`, controlSock)
	if runtime.GOOS == "windows" {

Fix: add a sentence noting that SSHOpts builds on it, layering the multiplexing and forwarding options on top.

S4. rsyncTool re-resolves ssh per call while scpTool now pins it Claude Opus 5
pkg/copytool/rsync.go
	}

	return true
}

func checkRsyncOnGuest(ctx context.Context, inst *limatype.Instance) bool {
	sshExe, err := sshutil.NewSSHExe()
	if err != nil {
		logrus.Debugf("failed to create SSH executable: %v", err)
		return false
	}
	sshOpts, err := sshOptsForInstance(ctx, sshExe, sshExe.Exe, inst)

This PR's principle is that a tool and the path form it receives come from one resolved binary, which is why scpTool gained an sshExe field set once at scp.go:28. The rsync side still resolves independently in the probe and again in Command (pkg/copytool/rsync.go:156), each re-walking PATH. At merge-base both backends did this, so rsync is no worse than before; the PR fixed one side and left the other.

pkg/copytool/scp.go
}

func newSCPTool(opts *Options) (*scpTool, error) {
	// scp must come from the same toolchain as the ssh whose path form it
	// receives, and NewSSHExe can select an ssh that PATH would not.
	sshExe, err := sshutil.NewSSHExe()
	if err != nil {
		return nil, fmt.Errorf("ssh not found on host: %w", err)
	}
	path, err := exec.LookPath(sshutil.CompanionForSSH(sshExe, "scp"))
	if err != nil {
pkg/copytool/rsync.go

	for _, cp := range copyPaths {
		if cp.IsRemote {
			if remoteInstance == nil {
				remoteInstance = cp.Instance
				sshExe, err := sshutil.NewSSHExe()
				if err != nil {
					return nil, err
				}
				sshOpts, err := sshOptsForInstance(ctx, sshExe, sshExe.Exe, cp.Instance)
				if err != nil {

Fix: store sshExe on rsyncTool in newRsyncTool, mirroring newSCPTool.

S5. The commit body omits the exported signature change Codex GPT 5.6 Sol Claude Opus 5
pkg/sshutil/sshutil.go
// The result never contains the Port option.
//
// Path options take toolPath's form, for the binary that ends up reading them.
// scp hands its options to the ssh beside itself, which need not be sshExe.
// Version detection still runs against sshExe, since only ssh reports a version.
func CommonOpts(ctx context.Context, sshExe SSHExe, toolPath string, useDotSSH bool) ([]string, error) {
	configDir, err := dirnames.LimaConfigDir()
	if err != nil {
		return nil, err
	}
	privateKeyPath := filepath.Join(configDir, filenames.UserPrivateKey)

CommonOpts shipped in v2.2.0 taking three arguments. This commit widens it to four and deletes the old wrapper, which is the largest single edit in sshutil.go and the one an archaeologist is most likely to search for. The body describes the path-form and multiplexing changes but never mentions it.

Repo practice makes the break itself acceptable: IsSSHCygwin was exported, shipped in v2.2.0, and removed outright in d32c9a4b with no breaking.md entry, and that file tracks user-facing config and the external-driver plugin interface rather than arbitrary pkg/ symbols. Recording the change in the body still costs one sentence.

Fix: add a sentence to the commit body naming the widened CommonOpts signature.

Design Observations

Concerns

Strengths

Testing Assessment

I ran the suite myself from the main checkout, which sits at 628fa44a with no modified tracked files: go test -count=1 ./pkg/copytool/... ./pkg/sshutil/... passes, GOOS=windows GOARCH=amd64 go vet over copytool, sshutil, limactl, and hostagent is clean, and gofmt -l reports nothing. A repo-wide grep confirms no stale references to the renamed CommonOptsForTool or resolvedSSHPath survive. Claude reports the same from its worktree plus golangci-lint and shellcheck; Codex adds a Windows test-binary compile and its CommonOpts compatibility probe.

The Windows unit-test step does reach the new Windows-only branches, and only one of the two Windows jobs runs unit tests. That job is the only thing standing behind these assertions.

Untested, highest risk first:

  1. Native Windows OpenSSH end to end. No CI job runs Lima against a host without a Cygwin toolchain, since _LIMA_WINDOWS_EXTRA_PATH steers ssh selection to Git for Windows in both Windows jobs. Deferred to #5342 by design.
  2. The whole rsync backend on Windows. Neither Windows job installs rsync, so the command -v rsync guard short-circuits and the new hack/test-templates.sh hunk never executes on this branch. Round 4 raised this as I2; it was fixed on the CI branch in c1f83823, not here.
  3. pathForRsync's real cygpath branch. Only the no-sibling fallback runs. Declined in round 3.
  4. The non-Windows arms of the new tests are inert. On Linux and macOS they assert paths are unchanged and three mux options are present, both true before this change. Only the Windows arms can fail on pre-fix code.
  5. Mixed-toolchain scp resolution. CompanionForSSH falling back to a PATH scp is the only case where the two keyings differ, and no test builds it.

Documentation Assessment

The round-4 doc corrections all hold. All five sites that claimed scp parses IdentityFile now say scp hands its options to the ssh beside itself, and no stale copy survives. Claude verified the surviving comment at pkg/hostagent/hostagent.go:302 against SSHOptsRemovingControlPath, which clones before deleting, so the caller's slice reaching sshConfig really is untouched.

S3 is the one wording gap: a doc that describes a contract two of its three callers satisfy. S5 concerns the commit body rather than a doc comment. No user-facing documentation describes Windows copy path forms, so none needs updating.

Commit Structure

One commit, signed off, subject matching the diff, and a body that gives the failure, the constraint, and the consequence without narrating mechanics. Assisted-by sits in the PR description where the project's rules put it. The one omission is the widened CommonOpts signature (S5).

Claude read the duplicated New() error-message hunk as a second, undeclared fix and asked for it to be dropped. I disagree: the duplication is what makes the merge clean, and the merge test below confirms it.

Acknowledged Limitations

Unresolved Feedback

None. The PR carries no review comments from other reviewers.

Agent Performance Retro

[Claude Opus 5] It raised every surviving finding and was the only reviewer to trace call graphs rather than read the diff, which is where S3 and S4 came from: both require knowing who calls a function, not what the function says. Its prior-round re-verification table is the most useful artifact any agent produced this round, and it caught the one place round 4's rename stopped short (S1). Against that, its single Important finding is the round's clearest false positive, and it is instructive: it fetched #5337's diff, saw byte-identical text, and reasoned from there to a merge conflict without running the merge. The evidence it gathered was real and its conclusion did not follow. It also flagged the same hunk as an undeclared second fix, which reads the deliberate synchronization backwards.

[Codex GPT 5.6 Sol] Its one finding is the most interesting claim of the round and the best-evidenced: it noticed the exported CommonOpts signature widened, checked that the three-argument form shipped, and ran an actual compile probe that failed with not enough arguments. Every fact in it holds. What it lacked was the repo's own history, which shows an exported sshutil symbol removed outright one week earlier with no ceremony, so the finding landed two severity levels above where this project would put it. Its Testing Assessment was the only one to report on the current SHA's CI run, including that WSL2 skipped rsync before an unrelated port-forwarding failure. It marked eight of nine files clean and missed S4 in rsync.go.

[Gemini 3.1 Pro] It produced no review. The CLI got 15 shell calls into the work and then died with TerminalQuotaError and a 429, with the quota resetting in roughly 17 hours, so I dropped it rather than spending the retry budget on an error the skill classifies as terminal. Whatever it had learned by then went with it, and no conclusion in this report rests on it.

[Gemini 3.5 Flash] Its S1 is real and correctly cited: all three PATH-global converter call sites exist where it said. The write-up overstated their availability, since one is already being reworked by sibling PR #5300 and another was round 4's design observation, leaving defaults.go as its genuine contribution. It opened by announcing 25 verification steps, including "I will run the go tests", and reported no result from any of them, which repeats its round-4 pattern. Its Coverage Summary attributes S1 to pkg/hostagent/hostagent.go, a file it never cited in the finding itself, and marks rsync.go clean.

With Gemini 3.1 Pro absent, this round has three opinions rather than four, and the two that raised Important findings disagreed with each other about nothing, since neither read the other's area.

Summary

Claude Opus 5Codex GPT 5.6 SolGemini 3.1 ProGemini 3.5 Flash
Duration18m 13s11m 40s3m 25s3m 37s
Findings5S1Snonenone
Tool calls45 (Bash 31, Read 14)37 (shell 33, plan 3, stdin 1)15 (runshellcommand 15)27 (readfile 14, grepsearch 10, runshellcommand 2)
Design observations6203
False positives1000
Unique insights4101
Files reviewed9909
Coverage misses0191
Totals5S1Snonenone
Downgraded01 (I→S)01 (I→S)
Dropped1000

Reconciliation. Two severity changes, both downward, and they are the substance of this round. Codex's I1 (CommonOpts API break) went from Important to S5: the facts survive scrutiny, but breaking.md covers user-facing configuration and the external-driver plugin interface rather than arbitrary pkg/ symbols, and IsSSHCygwin was removed from this same package in d32c9a4b after shipping in v2.2.0 with no entry. Reverting as Codex proposed would also undo round 4's S2 fix, which deliberately collapsed the two spellings so the PR adds no net exported symbol. What remains is the commit-message omission, which Claude raised independently.

Claude's I1 (#5337 overlap) was dropped as a false positive. I fetched #5337, merged it into 628fa44a in a scratch worktree, and got Auto-merging pkg/copytool/copytool.go with a conflict only in copytool_test.go, which is exactly what the PR description predicts. Git merges the duplicated three lines cleanly because both branches make the identical change, so the verbatim copy is what prevents the collision rather than causing it. Round 4's decision was correct and the PR description is accurate as written.

Gemini 3.5 Flash's S1 was downgraded to a design observation, since two of its three call sites are spoken for and none is modified by this PR.

Review Process Notes

Skill improvements

Repo context updates