Deep Review: 20260728-180750-pr-5342

Round 3 closes every round-2 finding, and the delta holds up under checking. Both new sshutil tests ran and passed on windows-2025 at this SHA, the verification round 2 could not run. Four suggestions remain, none blocking. Round 3 is where this PR converged, so the suggestions below are optional.

Four reviewers ran against 244c0ca0. Codex found the one hole in the plain-host verification layers; Claude found the other three suggestions; both Gemini models returned near-clean. During consolidation I fetched this SHA's own CI job logs, reproduced the registry regex miss, read Git for Windows' installer source to settle it, counted the template symlinks in the tree, and compile-checked the Windows test file. The two plain jobs still fail at limactl copy, exactly as the PR description predicts.

Ranked by risk:

1. Which ssh.exe each job selects is asserted nowhere. S2 covers the MSYS2 half. The plain half is acknowledged in the new test's own doc comment. The Go-level selection rules are now well covered; the CI-level configuration is not.
2. Both plain jobs still fail at limactl copy pending #5299, which the PR description predicts and round 1 declined. At this SHA both are completed/failure while every other Windows job passes.
3. TestNewSSHExeFallsBackToPATH carries one inert assertion Claude Opus 5. got.Exe equals sshExe whether NewSSHExe fell through to exec.LookPath or pickCompleteSSHOnWindows wrongly returned the partial install, since both read the same PATH. The preceding pickCompleteSSHOnWindows() == "" assertion is what pins the behaviour, and it does so correctly, so the test is sound.
4. windows_plain_templates' guards and the $null guards remain unexercised, both declined in prior rounds.

The round's largest gain closes a round-2 limitation. Round 2 could not run sshutil_windows_test.go at all, because the file builds only on Windows. At this SHA job 90453290796 step 7 (go test -v ./...) reports completed/success, and its log shows --- PASS: TestPickCompleteSSHOnWindows/incomplete_native_install_disqualifies_it_too and --- PASS: TestNewSSHExeFallsBackToPATH at lines 6048 and 6050. Neither test carries a skip that fires on windows-2025.

Executed during consolidation, all at the review SHA: GOOS=windows go vet ./pkg/sshutil and GOOS=windows go test -c ./pkg/sshutil, both clean; go build ./cmd/limactl, clean. I fetched the raw job logs for 90453290796 and 90453290927, read per-step conclusions through the jobs API while the run was still going, reproduced the Layer 4 regex against Git and MSYS2 64bit, downloaded Git for Windows' install.iss, counted the template symlinks at both review SHAs, and read the pinned sshocker@v0.3.11 source. git status in all four agent worktrees showed only the supplied .scratch/, plus a sshutil.test.exe in Flash's, which corroborates its cross-compile claim.

All three description additions from this round hold up. windows_plain_build/action.yml:9-11 claims the unversioned build passes default.yaml's minimumLimaVersion, and the chain checks out end to end: pkg/version/version.go:7 defaults Version to "<unknown>", versionutil.compare returns 1 when Parse fails on it, GreaterEqual at pkg/limayaml/validate.go:46 therefore passes, and templates/default.yaml:340 is the only one of the two plain-job templates carrying the key at all. windows_msys2_prep's new sentence about the Make step matches Makefile:223. Layer 5's rewritten comment is accurate on all three arms, which I traced through pickCompleteSSHOnWindows and NewSSHExe.

The narrowed windows-plain-qemu header is the most improved of the four. Its claim that sshocker treats the failed mount confirmation as non-fatal is exactly what reversesshfs.go:252-255 does: a // not a fatal error comment, a warning, then return nil.

breaking.md's new ## v2.3.0 heading is placed correctly. v2.2.0 is the newest tag, no v2.3.0 tag exists, and the heading sits above ## v2.2.0 in the file's newest-first order. The wording is S4. The removal from environment-variables.md leaves no dangling reference anywhere in the tree.

One commit, DCO-signed, amending the round-2 SHA rather than stacking a fixup. The message still matches the tree, and name: "Windows tests (WSL2)" remains byte-identical to the merge-base, so the job-id rename leaves required status checks alone. Nothing to squash or split.

• [Environment]::SetEnvironmentVariable converts the registry PATH to REG_SZ [Gemini 3.5 Flash, rounds 2 and 3]. The author addressed this in round 3 with the "Destructive and irreversible" line at windows_plain_host/action.yml:9-11 rather than a mechanism change, which fits an ephemeral runner. The fix proposed this round does not achieve what it claims: it writes $cleaned back as ExpandString, but $cleaned derives from a value the finding itself says was read fully expanded, so the type is restored while the expanded paths stay baked in. Preserving the indirection would need a DoNotExpandEnvironmentNames read as well.
• windows_plain_templates's .. handling can underflow, guarded by the containment check that follows it.
• -ErrorAction SilentlyContinue on the Remove-Item calls is deliberate, so removal failures surface as a verification failure instead.

Declined in prior rounds ¶

• The plain jobs build limactl without the version ldflag — commented round 2: a cross-step version handoff is not worth it for a smoke test. I re-derived the reason rather than inheriting it, and every link holds (see Documentation Assessment). The decline stands.
• Neither new job can pass until #5299 merges — skipped round 2: the merge-ordering dependency is already stated in the PR description.
• Nothing exercises windows_plain_templates' failure paths — skipped round 2: the guards cannot fire against the current tree, and testing them needs synthetic symlink fixtures.
• The new $null guards are unreachable in CI — skipped round 2: CI always copies non-empty content and every symlink blob is non-empty.
• Adding rsync introduces a pacman error the exit-code guard misses — declined round 1 and re-derived round 2 by isolating the variable: jobs installing no rsync emit the same error.
• The plain jobs run no unit tests — declined round 1 as its own PR. Windows unit tests still run only in the MSYS2-prepared job.
• The smoke test covers single-file copies only — declined round 1: it is deliberately a smoke test.
• windows-plain-wsl2 skips windows_plain_templates — resolved round 1 by a comment. templates/experimental/wsl2.yaml still carries no base:, so the stated reason holds.

Claude Opus 5 ¶

Produced three of the four surviving suggestions and both design concerns, and its delta table cited exact CI log lines rather than summarising them. One of those citations mattered: it quoted --- PASS for both new tests at specific line numbers, and when my own grep of the same log came back empty I nearly recorded the quote as unsupported. GitHub serves that log as binary, so grep needs -a; the lines are exactly where Claude said. Two of its factual claims are wrong, both in Strengths rather than in findings. It reported 27 template symlinks where the tree holds 24 and the log prints 25 resolution lines, and it described a two-hop opensuse.yaml → opensuse-leap.yaml → opensuse-leap-16.yaml chain that the tree does not contain. Its $RUNNER_TEMP observation framed a harmless pre-checkout write as a latent bug.

Codex GPT 5.6 Sol ¶

One finding, and no other reviewer reached it. It got to the Layer 4 regex gap by fetching Git for Windows' installer source instead of reasoning about what the pattern looks like it covers, which is the same move that won it round 2's only Important finding. Both halves survived checking, including the omitted HKCU WOW6432Node root: the installer's own QueryUninstallValues probes exactly four locations and the action lists three. It marked three files clean that turned out to hold suggestions, the same cost its brevity carried last round.

Gemini 3.1 Pro ¶

Clean at every severity for the second round running. Unlike round 2 it attached real evidence to its verdicts, tracing compare() through versionutil.go and checking Layer 5's three arms against sshutil.go, and both conclusions match what I derived independently. Its Acknowledged Limitations section still restates the injected decline list item for item with no measurement attached, which is the one pattern the round-3 prompt asked agents to avoid.

Gemini 3.5 Flash ¶

Re-raised its round-2 REG_EXPAND_SZ claim, this time as a standalone suggestion with a patch. The patch is self-defeating: it restores the registry value kind while writing back a string its own premise says was read fully expanded, so the expanded paths it objects to survive the fix. Flash did do work the others skipped, cross-compiling the Windows test binary and leaving sshutil.test.exe in its worktree as evidence. Against that, its delta table marked the registry row PASS on the strength of unnamed "Registry/API research".

Summary ¶

Reconciliation. Gemini 3.5 Flash S1 REG_EXPAND_SZ: suggestion → Acknowledged Limitations. Round 3 already addressed the underlying concern with the destructive-runner warning the author added for it, and the patch offered this round does not preserve what it claims to preserve. No other severities changed.

The two Gemini models share a family, so their converging on a near-clean result is one opinion rather than two. Between them they produced nothing that survived as a finding. Claude and Codex split the round the same way they split round 2: Claude on volume, with three suggestions and the design concerns but two loose facts in its supporting prose, and Codex on a single finding reached by reading a primary source. Codex's is the one a reviewer could not have derived from the diff alone.

Skill improvements ¶

• Before concluding that a quoted log line does not exist, confirm the search tool read the file as text. A CI provider commonly serves a job log as a byte stream that file reports as data, and GNU and BSD grep then suppress every match silently until given -a; a tool writing UTF-16 produces the same empty result by a different route. Re-run with -a, and check for interleaved NULs, before recording a quote as unsupported. The failure mode is expensive in both directions: it marks an accurate reviewer as fabricating, and it hides the evidence the round most needed.
• Fetch a completed job's log directly rather than waiting for its workflow run. gh api repos/{owner}/{repo}/actions/jobs/{id}/logs returns the log as soon as that job finishes, while gh run view --log still refuses for a run with jobs outstanding. Falling back to the workflow definition plus the job's status gives up evidence that is already available, and a run whose slowest jobs take another hour would otherwise push a verifiable claim into the unverifiable column.
• When a finding arrives with a patch, check the patch against the finding's own stated mechanism before checking it against the code. A patch that contradicts the premise its own finding argues from is refutable without opening the repository at all, and that inconsistency is the cheapest signal that the reviewer matched a familiar remedy onto a mechanism it does not address. Read the finding's description of what goes wrong, then ask whether applying the diff changes that specific thing.

Repo context updates ¶

• [repo] Git for Windows registers its uninstall entry with DisplayName set to the bare string Git, from UninstallDisplayName={#APP_NAME} with #define APP_NAME 'Git' in the installer, so a pattern looking for a display name containing "git for windows" never matches it. The installer's QueryUninstallValues probes four roots for Microsoft\Windows\CurrentVersion\Uninstall\Git_is1: HKLM and HKCU, each both with and without the Software\Wow6432Node prefix. Any check that hunts for a Git for Windows installation in the registry has to match that display name and cover all four roots.