Deep Review: 20260503-123001-pr-348

Date2026-05-03 12:30
Reporancher-sandbox/rancher-desktop-daemon
Round3
Author@jandubois
PR#348 — rdd kubectl: embed kuberlr-style version resolver
Branchembed-kuberlr
Commits75346bd rdd kubectl: embed kuberlr-style version resolver
ReviewersClaude Opus 4.7 (effort: xhigh), Codex GPT 5.5 (effort: xhigh), Gemini 3.1 Pro (effort: default)
VerdictMerge with fixes — I1 (BATS 500-fall-through test cannot fire its key assertion after the Round 2 isClientOnly short-circuit) and I2 (--client=true reaches the probe path) are real correctness regressions. I3 (auth/TLS overrides not threaded into the probe) is a genuine extension of Round 2's I1 partial fix; the docstring documents the limitation, but a --server + --token invocation will silently pick the wrong kubectl version.
Wall-clock time28 min 20 s

Executive Summary

Round 2's three Important fixes (kubectl flag scanner, log-level demotion, isClientOnly short-circuit) all landed cleanly. Round 3 surfaces three new Importants on the same code paths the Round 2 fixes touched: a BATS test (apiserver returns 500 on /version) calls kubectl version --client, which the Round 2 isClientOnly short-circuit now skips before any probe — making the test's assert_file_contains "${LOG_FILE}" '^GET /version' assertion impossible to satisfy (I1, raised independently by Claude and Codex); isClientOnly recognises --client exactly but treats --client=true as a generic flag, so kubectl version --client=true reaches the probe and stalls 2 s against an unreachable cluster (I2); and the Round 2 extension of extractKubeFlags to --server/--cluster/--user did not include the auth/TLS overrides (--token, --certificate-authority, --client-certificate, --client-key, --tls-server-name, --username, --password, --insecure-skip-tls-verify), so a probe against a cluster whose access requires those flags fails 401/TLS and silently falls through to a possibly-mismatched embedded kubectl (I3).

Suggestions are localised: a config.json → config.yaml typo in the new cmd_service.md example block, missing body-size caps on streamGet (raised by both Claude and Gemini), an os.Setenv error swallowed in SkipResolver, three documentation/comment polish items.

Structure: 0 Critical · 3 Important · 8 Suggestions.

Critical Issues

None.

Important Issues

I1. BATS apiserver returns 500 test asserts a /version GET that the Round 2 isClientOnly short-circuit skips Claude Codex
bats/tests/10-cli/7-kubectl-cache.bats
    assert_output --partial 'checksum mismatch'
    refute_output --partial 'fake-kubectl:'
    assert_file_not_exists "${CACHE_DIR}/kubectl/${GOOS}-${GOARCH}/kubectl-v1.99.0${EXE}"
}

@test 'rdd kubectl falls through to embedded when the apiserver returns 500 on /version' {
    echo 500 >"${VERSION_STATUS_FILE}"

    run -0 env \
        RDD_CACHE_DIR="${CACHE_DIR}" \
        RDD_KUBECTL_MIRROR="http://127.0.0.1:${PORT}" \
        KUBECONFIG="${KUBECONFIG_PATH}" \
        rdd kubectl version --client

    # The embedded kubectl runs and prints something; the resolver
    # neither downloaded nor errored.
    assert_output
    refute_output --partial 'fake-kubectl:'
    refute_output --partial 'resolving kubectl version'
    assert_file_contains "${LOG_FILE}" '^GET /version'
    refute_file_contains "${LOG_FILE}" '/release/'
}

The Round 2 fix added isClientOnly and short-circuits Resolve for version --client (pkg/kuberlr/resolver.go:65-66). The unit test at pkg/kuberlr/resolver_test.go:96 confirms {"version --client", []string{"version", "--client"}, true}. So rdd kubectl version --client skips the probe entirely; LOG_FILE ends up empty for the /version GET; the assertion at line 204 cannot fire.

pkg/kuberlr/resolver.go
// the cached binary closest to the server's minor.
func Resolve(ctx context.Context, args []string) (string, error) {
	if os.Getenv(envSkipResolver) != "" {
		return "", nil
	}
	if isClientOnly(args) {
		return "", nil
	}
	embedded, err := embeddedVersion()
	if err != nil {
		// `go run ./cmd/rdd kubectl ...` and IDE debug builds skip the
		// Makefile's -ldflags -X, so componentbasever returns the
pkg/kuberlr/resolver_test.go
	}{
		{"empty", nil, true},
		{"--help alone", []string{"--help"}, true},
		{"-h alone", []string{"-h"}, true},
		{"--help after subcommand", []string{"get", "pods", "--help"}, true},
		{"version --client", []string{"version", "--client"}, true},
		{"version --client with --output", []string{"version", "--client", "--output=json"}, true},
		{"version without --client", []string{"version"}, false},
		{"completion bash", []string{"completion", "bash"}, true},
		{"config view", []string{"config", "view"}, true},
		{"config get-contexts", []string{"config", "get-contexts"}, true},

The test's stated intent — verifying graceful fall-through when the apiserver returns 500 — needs a command that actually triggers the probe. The current command bypasses the probe before the 500 status matters.

Fix: pick a probe-triggering command. Drop --client so plain kubectl version runs (it probes the server for its version, then prints both client and server output; modern kubectl exits 0 with a "couldn't find server version" diagnostic on probe failure).

-        rdd kubectl version --client
+        rdd kubectl version

Verify the chosen command end-to-end before merge — if kubectl version against a 500ing apiserver exits non-zero, switch to run env ... and assert on the exit code separately.

I2. isClientOnly matches --client exactly but not --client=true — kubectl version --client=true reaches the probe and stalls 2 s against an unreachable cluster Codex
pkg/kuberlr/resolver.go
			break
		}
		if arg == "--help" || arg == "-h" {
			return true
		}
		if arg == "--client" {
			sawClient = true
			continue
		}
		if strings.HasPrefix(arg, "-") {
			if !strings.Contains(arg, "=") && kubectlGlobalFlagsTakingValue[arg] && i+1 < len(args) {
				i++
			}
			continue
		}
		if subcommand == "" {
			subcommand = arg
		}
	}

--client=true is the documented bool form kubectl accepts (along with --client=false, --client=1, --client=0). It matches the second branch (strings.HasPrefix(arg, "-")), so sawClient stays false. The walk lands at line 304 with subcommand="version" and sawClient=false, returning false. Resolve then probes the cluster — exactly the regression Round 2's I3 short-circuit was meant to prevent. --client=false should also work the other direction (force-probe), and repeated flags should follow kubectl's "last wins" semantics.

pkg/kuberlr/resolver.go
	}
	if subcommand == "" {
		return true
	}
	if subcommand == "version" {
		return sawClient
	}
	return clientOnlySubcommands[subcommand]
}

The unit test table at pkg/kuberlr/resolver_test.go:96-97 covers version --client and version --client --output=json but no equals-form case.

pkg/kuberlr/resolver_test.go
	}{
		{"empty", nil, true},
		{"--help alone", []string{"--help"}, true},
		{"-h alone", []string{"-h"}, true},
		{"--help after subcommand", []string{"get", "pods", "--help"}, true},
		{"version --client", []string{"version", "--client"}, true},
		{"version --client with --output", []string{"version", "--client", "--output=json"}, true},
		{"version without --client", []string{"version"}, false},
		{"completion bash", []string{"completion", "bash"}, true},
		{"config view", []string{"config", "view"}, true},
		{"config get-contexts", []string{"config", "get-contexts"}, true},
		{"options", []string{"options"}, true},

Fix: parse --client=<bool> and let later occurrences overwrite earlier ones, matching kubectl's flag-parser behaviour.

+import "strconv"
+
 if arg == "--client" {
     sawClient = true
     continue
 }
+if strings.HasPrefix(arg, "--client=") {
+    if v, err := strconv.ParseBool(strings.TrimPrefix(arg, "--client=")); err == nil {
+        sawClient = v
+    }
+    continue
+}

Add unit-test rows for version --client=true, version --client=false, and version --client --client=false (last wins).

I3. Round 2's extractKubeFlags extension covered cluster-selection but not auth/TLS overrides; rdd kubectl --server X --token Y makes the probe fail 401 and silently exec mismatched embedded kubectl Codex
pkg/kuberlr/resolver.go

// loadClientConfig builds a rest.Config that honors KUBECONFIG plus the
// cluster-selection flags kubectl will pass to its own clientcmd. The
// resolver and the kubectl child must aim at the same cluster so the
// version skew check matches the cluster kubectl actually contacts.
func loadClientConfig(args []string) (*rest.Config, error) {
	flags := extractKubeFlags(args)
	rules := clientcmd.NewDefaultClientConfigLoadingRules()
	rules.ExplicitPath = flags.kubeconfig
	overrides := &clientcmd.ConfigOverrides{
		CurrentContext: flags.context,
		ClusterInfo:    clientapi.Cluster{Server: flags.server},
		Context:        clientapi.Context{Cluster: flags.cluster, AuthInfo: flags.user},
	}
	return clientcmd.NewNonInteractiveDeferredLoadingClientConfig(rules, overrides).ClientConfig()
}

// kubeFlags holds the kubectl cluster-selection flags Resolve tracks. A
// missing flag yields an empty string, which clientcmd treats as "use
// the default."
type kubeFlags struct {

Round 2's I1 fix added --server, --cluster, --user to extractKubeFlags. The Round 2 finding text named the rest of kubectl's connection-override surface (--token, --certificate-authority, --client-certificate, --client-key, --tls-server-name, --username, --password, --insecure-skip-tls-verify) and said "If the full flag set is too much code for this PR, document the gap in the Resolve doc." The author chose the minimum fix and updated the docstring to enumerate the five handled flags (pkg/kuberlr/resolver.go:41-47). The auth/TLS surface stayed out.

pkg/kuberlr/resolver.go
// serverProbeTimeout caps the discovery call. A reachable cluster answers
// in under 100 ms; an unreachable one would otherwise stall every kubectl
// invocation.
const serverProbeTimeout = 2 * time.Second

// Resolve returns the path to a kubectl binary compatible with the cluster
// the user's kubeconfig points at. An empty path means "use the embedded
// kubectl"; a non-empty path means "exec this binary instead". args holds
// the kubectl-style arguments the caller will pass through; Resolve scans
// them for the kubectl cluster-selection flags (--kubeconfig, --context,
// --server, --cluster, --user) so the version probe targets the same
// cluster the kubectl child will contact.
//
// When the cluster probe fails for any reason — unreachable, missing or
// malformed kubeconfig, unparseable server version — or when the embedded
// kubectl is within ±1 minor of the server, Resolve returns "". Resolve
// also returns "" without probing for invocations the embedded kubectl

kubectlGlobalFlagsTakingValue (pkg/kuberlr/resolver.go:216-248) already lists the auth/TLS flags as value-taking — isClientOnly knows to skip them when locating the subcommand — but extractKubeFlags never records them, so they never reach clientcmd.ConfigOverrides. A user running

pkg/kuberlr/resolver.go
// and its value when it walks args looking for the subcommand. The
// list covers what kubectl exposes via genericclioptions.ConfigFlags
// and the klog flags. An unknown flag falls through as no-value;
// kubectl rejects unknown globals anyway, so the realistic miss set
// is empty.
var kubectlGlobalFlagsTakingValue = map[string]bool{
	"--kubeconfig":            true,
	"--context":               true,
	"--cluster":               true,
	"--user":                  true,
	"--namespace":             true,
	"-n":                      true,
	"--server":                true,
	"-s":                      true,
	"--token":                 true,
	"--certificate-authority": true,
	"--client-certificate":    true,
	"--client-key":            true,
	"--tls-server-name":       true,
	"--cache-dir":             true,
	"--request-timeout":       true,
	"--password":              true,
	"--username":              true,
	"--as":                    true,
	"--as-group":              true,
	"--as-uid":                true,
	"--profile":               true,
	"--profile-output":        true,
	"--log-backtrace-at":      true,
	"--log-dir":               true,
	"--log-file":              true,
	"--log-file-max-size":     true,
	"--log-flush-frequency":   true,
	"--stderrthreshold":       true,
	"-v":                      true,
	"--v":                     true,
	"--vmodule":               true,
}

// clientOnlySubcommands lists kubectl subcommands that never contact
// a cluster. config and completion manipulate local state only;
// options prints kubectl's global flags; help prints help text.
var clientOnlySubcommands = map[string]bool{
rdd kubectl --server=https://api.staging:6443 --token=ey... --insecure-skip-tls-verify get pods

makes the resolver probe staging without --token and without --insecure-skip-tls-verify. The probe fails (401 or TLS error), serverVersion returns ok=false, Resolve returns "", and the embedded kubectl runs against staging with the full flag set. If staging is at v1.34 and embedded is at v1.30, the user gets a silent skew violation — the very case the resolver exists to prevent.

Fix: thread the standard auth/TLS overrides through clientcmd.ConfigOverrides. The minimum is --token → AuthInfo.Token, --certificate-authority → ClusterInfo.CertificateAuthority, --client-certificate/--client-key → AuthInfo.ClientCertificate/ClientKey, --tls-server-name → ClusterInfo.TLSServerName, --insecure-skip-tls-verify → ClusterInfo.InsecureSkipTLSVerify. The durable fix delegates to genericclioptions.ConfigFlags so resolver and kubectl child cannot drift by construction:

import "k8s.io/cli-runtime/pkg/genericclioptions"

func loadClientConfig(args []string) (*rest.Config, error) {
    flags := genericclioptions.NewConfigFlags(false)
    fs := pflag.NewFlagSet("rdd-kubectl", pflag.ContinueOnError)
    flags.AddFlags(fs)
    _ = fs.Parse(stripPositional(args))
    return flags.ToRESTConfig()
}

If the durable fix is too much for this PR, extend extractKubeFlags and loadClientConfig to cover the auth/TLS minimum above. Either way, add TestExtractKubeFlags rows for --token, --certificate-authority, and --insecure-skip-tls-verify.

Suggestions

S1. docs/design/cmd_service.md example shows config.json; implementation reports config.yaml Codex
docs/design/cmd_service.md
Examples:

```console
$ rdd svc paths
args_file      /path/to/rancher-desktop-default/rdd.args
config         /path/to/rancher-desktop-default/config.json
dir            /path/to/rancher-desktop-default
kubectl_cache  /path/to/Caches/rancher-desktop/kubectl/darwin-arm64
lima_home      /path/to/.rd2/lima
log_dir        /path/to/rancher-desktop-default/log
pid_file       /path/to/rancher-desktop-default/rdd.pid

instance.Config() returns filepath.Join(Dir(), "config.yaml") (pkg/instance/instance.go:91). git blame shows the example block was rewritten by this PR's commit (75346bd), so the typo is a regression introduced here, not a pre-existing doc drift.

pkg/instance/instance.go
	return filepath.Join(Dir(), "args.json")
})

// Config returns the path to the kubeconfig file.
var Config = sync.OnceValue(func() string {
	return filepath.Join(Dir(), "config.yaml")
})

// PIDFile returns the path to the service PID file.
var PIDFile = sync.OnceValue(func() string {
	return filepath.Join(Dir(), "rdd.pid")

Fix:

-config         /path/to/rancher-desktop-default/config.json
+config         /path/to/rancher-desktop-default/config.yaml
S2. streamGet has no body-size cap; a misconfigured mirror returning the binary URL for the .sha512 path streams ~50 MB into a strings.Builder Claude Gemini
pkg/kuberlr/downloader.go

// fetchSha512 downloads the sha512 hex digest at url. dl.k8s.io serves
// the bare digest, but a mirror populated with `sha512sum` writes the
// digest, two spaces, and the filename — fields() takes the digest and
// drops the trailing filename so both formats verify.
func fetchSha512(ctx context.Context, url string) (string, error) {
	var sb strings.Builder
	if err := streamGet(ctx, url, &sb); err != nil {
		return "", err
	}
	fields := strings.Fields(sb.String())
	if len(fields) == 0 {
		return "", fmt.Errorf("empty checksum response from %s", url)
	}
	return fields[0], nil
}

// streamGet performs a GET on url and copies the response body into w.
// streamGet returns an error on any non-200 status.
func streamGet(ctx context.Context, url string, w io.Writer) error {
	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody)
	if err != nil {
		return err
	}
	resp, err := httpClient.Do(req)
	if err != nil {
		return err
	}
	defer resp.Body.Close()
	if resp.StatusCode != http.StatusOK {
		return fmt.Errorf("GET %s: %s", url, resp.Status)
	}
	_, err = io.Copy(w, resp.Body)
	return err
}

streamGet does io.Copy(w, resp.Body) with no io.LimitReader. The 5-minute httpClient.Timeout bounds total wall-time, but at modest LAN speeds 5 minutes is gigabytes. The realistic trigger is a misconfigured mirror that serves the binary at the .sha512 URL (or any mirror error page returned with status 200) — fetchSha512 allocates the whole response in memory, then strings.Fields(sb.String()) returns gibberish that fails the digest comparison with a misleading checksum mismatch error.

Default mirror (dl.k8s.io) is trusted, so this matters only with operator-configured mirrors. Severity is Suggestion, not Important — Gemini raised it as Important; reconciled to Suggestion (see Reconciliation).

Fix: bound both endpoints. The sha512 response is at most a few hundred bytes; the kubectl binary is ~50 MB but bounding to e.g. 200 MB gives headroom.

-func streamGet(ctx context.Context, url string, w io.Writer) error {
+func streamGet(ctx context.Context, url string, w io.Writer, maxBytes int64) error {
     ...
-    _, err = io.Copy(w, resp.Body)
+    _, err = io.Copy(w, io.LimitReader(resp.Body, maxBytes))
     return err
 }

Pass 1<<10 from fetchSha512, 256<<20 from download. Belt-and-suspenders: validate fields[0] is exactly 128 hex characters in fetchSha512 (sha512.Size*2) so a non-digest response surfaces with a clearer error than checksum mismatch.

S3. SkipResolver swallows os.Setenv error; the surrounding ctlAction propagates the same call's error for KUBECONFIG Claude
pkg/kuberlr/resolver.go

// SkipResolver short-circuits Resolve for the rest of this process.
// rdd ctl calls this before kubectlAction because it always targets
// the embedded apiserver, whose version matches the embedded kubectl
// by construction — the probe would always fall through anyway.
func SkipResolver() {
	os.Setenv(envSkipResolver, "1")
}

// serverProbeTimeout caps the discovery call. A reachable cluster answers
// in under 100 ms; an unreachable one would otherwise stall every kubectl
// invocation.
const serverProbeTimeout = 2 * time.Second

ctlAction two lines up does if err := os.Setenv("KUBECONFIG", instance.Config()); err != nil { return ... }. The inconsistency reads as an oversight rather than a deliberate choice; the constant name and value "1" cannot trigger the EINVAL path in practice, but for stylistic alignment with the sibling call, surface the error.

Fix:

-func SkipResolver() {
-    os.Setenv(envSkipResolver, "1")
+func SkipResolver() error {
+    if err := os.Setenv(envSkipResolver, "1"); err != nil {
+        return fmt.Errorf("setting %s: %w", envSkipResolver, err)
+    }
+    return nil
 }

Then in ctlAction:

-    kuberlr.SkipResolver()
+    if err := kuberlr.SkipResolver(); err != nil {
+        return err
+    }
S4. isClientOnly lacks a comment explaining why bare version is cluster-bound Claude
pkg/kuberlr/resolver.go
		}
	}
	if subcommand == "" {
		return true
	}
	if subcommand == "version" {
		return sawClient
	}
	return clientOnlySubcommands[subcommand]
}

The unit test {"version without --client", []string{"version"}, false} covers the behaviour; the rationale ("kubectl version probes the server for its version, so we want a version-matched binary doing that probe") is non-obvious from the code. A one-line comment locks the intent.

S5. Document httpClient.Timeout as a backstop, not the primary deadline Claude
pkg/kuberlr/downloader.go
// envKubeMirror lets tests (and operators) point the resolver at an
// alternate mirror. Useful for offline mirrors and for the BATS cache
// lifecycle test, which serves a fake binary from a local HTTP server.
const envKubeMirror = "RDD_KUBECTL_MIRROR"

// downloadTimeout caps each mirror request. Five minutes turns a hung
// mirror into a bounded failure and still covers a ~50 MB kubectl
// binary on slow links (~170 kB/s).
const downloadTimeout = 5 * time.Minute

// httpClient enforces downloadTimeout on every kuberlr request.
// http.DefaultClient sets no Timeout, and the inherited cobra context
// carries no deadline.
var httpClient = &http.Client{Timeout: downloadTimeout}

// mirrorURL returns the mirror base URL the resolver downloads from.
//
// TODO(offline): pair this with a "may we download?" toggle so air-gapped
// users can pre-populate CacheDir() and forbid network fetches.
func mirrorURL() string {

If a future caller adds a deadline to the cobra context, the request honours both — whichever fires first. The current comment explains why the cap exists today but does not name the composition rule. A one-line addition prevents a future maintainer from concluding "context deadline must be wrong; the client wins."

Fix: end the httpClient comment with "If the request context carries a shorter deadline, that deadline wins."

S6. Cache the server-version probe across invocations to remove RTT from interactive rdd kubectl loops Claude

Every non-client-only invocation pays a fresh /version round trip. For an interactive workflow running many short rdd kubectl commands in sequence, that is RTT × N of latency that a (server-URL, server-version, expires-at) cache keyed on the resolved flags.server would amortise away. Out of scope for this PR; flagging as a follow-up.

S7. Move package doc-comment from cache.go to resolver.go (or a dedicated doc.go) for findability Claude
pkg/kuberlr/cache.go
// SPDX-License-Identifier: Apache-2.0
// SPDX-FileCopyrightText: SUSE LLC
// SPDX-FileCopyrightText: The Rancher Desktop Authors

// Package kuberlr resolves a kubectl binary compatible with the cluster
// targeted by the user's kubeconfig. When the kubectl embedded in rdd falls
// within the supported version skew, rdd execs the embedded binary;
// otherwise rdd fetches a matching kubectl from dl.k8s.io into a shared
// cache and execs it in place. Modeled on github.com/flavio/kuberlr.
package kuberlr

import (
	"fmt"
	"os"
	"path/filepath"

The package doc on cache.go is the convention-breaker — most readers open resolver.go first. Cosmetic.

S8. write_kubectl_sha512 could drop the is_macos branch — shasum -a 512 ships on Linux too Claude
bats/tests/10-cli/7-kubectl-cache.bats
    # Republish the kubectl checksum every test so the sha-mismatch test
    # cannot leak its zeroed fixture into a later run.
    write_kubectl_sha512
}

write_kubectl_sha512() {
    if is_macos; then
        shasum -a 512 "${MIRROR_BIN_DIR}/kubectl${EXE}" | awk '{print $1}' >"${MIRROR_BIN_DIR}/kubectl${EXE}.sha512"
    else
        sha512sum "${MIRROR_BIN_DIR}/kubectl${EXE}" | awk '{print $1}' >"${MIRROR_BIN_DIR}/kubectl${EXE}.sha512"
    fi
}

@test 'rdd kubectl downloads a version-matched kubectl on cache miss' {
    cached=${CACHE_DIR}/kubectl/${GOOS}-${GOARCH}/kubectl-v1.99.0${EXE}
    assert_file_not_exists "${cached}"

shasum is part of Perl's standard distribution and present on most Linuxes by default. Picking one tool removes the conditional. Trivial; do not block the merge.

Design Observations

Concerns

Strengths

Testing Assessment

Highest-risk untested scenarios in Round 3 (excluding items declined in prior rounds):

  1. --client=true and --client=false equals-form (I2). No test asserts kubectl version --client=true is client-only.
  2. --token / --certificate-authority / --insecure-skip-tls-verify overrides (I3). No TestExtractKubeFlags row covers these.
  3. End-to-end exec of the downloaded binary on Windows. pkg/kuberlr/exec_windows.go has no automated coverage; declined in prior rounds (Windows BATS runner deferred), surfacing again because I3's fix interacts with TLS code paths that diverge between platforms.
  4. info.GitVersion from a vendor cluster (EKS, GKE, AKS — v1.30.0-eks-1234, v1.30.7-gke.500). semver.ParseTolerant should handle suffixed forms; a TestCompatible-adjacent unit test would lock the contract.

Verification runs: Codex ran make build-rdd and go test ./pkg/kuberlr in its worktree (both passed). Claude verified isClientOnly(["version", "--client"]) == true against the implementation directly (relevant to I1).

Documentation Assessment

Acknowledged Limitations

Declined in prior rounds

Agent Performance Retro

Claude

Claude's strongest move this round was independent verification of I1 — running the args through isClientOnly against the implementation, then cross-referencing the unit test row that confirms version --client returns true. Codex caught the same test bug at the same severity. Beyond I1, Claude's contribution was breadth: seven Suggestions covering body-size caps (also caught by Gemini), SkipResolver error handling, comment polish, and a server-version probe cache. Two of Claude's initial Importants (I4 httpClient.Timeout composition, I5 every-invocation probe) were withdrawn within the review and downgraded to Suggestions — visible self-correction is a positive signal, not a knock. Claude's I2 (Windows os.Rename race) and S7 (CacheDir panic) re-raised findings declined in Round 1 and Round 2; both dropped per the resolutions gate.

Codex

Codex carried this round's correctness work. I2 (--client=true) and I3 (auth/TLS overrides) are both genuine follow-ups to Round 2's fixes that Claude and Gemini missed; Codex was the only agent to spot the equals-form gap and the only one to enumerate the auth/TLS flag set against kubectlGlobalFlagsTakingValue. S2 (config.json typo in cmd_service.md) required reading the example block against the implementation — instance.Config() returns config.yaml, the example was rewritten in this PR's commit, no other agent caught it. Tight three Importants, one Suggestion, one declined-prior-rounds Suggestion (Windows rename); zero false positives. Best severity calibration of the round.

Gemini

Gemini ran successfully this round (R1 and R2 had auth/trust failures). Of three findings, two (I1 Windows os.Rename race, S1 CacheDir panic) were declined-in-prior-rounds and dropped per the resolutions gate. The surviving I2 (unbounded read in fetchSha512) overlapped with Claude's S4 — Gemini called it Important, Claude called it Suggestion; reconciled to Suggestion (S2 in this report) because the trigger requires an operator-configured malicious or misconfigured mirror. Gemini's design observation on double execution of client-go exec auth plugins (AWS IAM Authenticator, gcloud, kubelogin) is a genuinely new architectural insight neither Round 1 nor Round 2 surfaced; it is the kind of cross-component reasoning that justifies running three agents.

Summary

Claude Opus 4.7Codex GPT 5.5Gemini 3.1 Pro
Duration21m 02s8m 39s—
Findings1I 7S3I 1S1S
Tool calls39 (Read 19, Bash 17, Grep 3)71 (shell 70, stdin 1)—
Design observations111
False positives000
Unique insights641
Files reviewed181818
Coverage misses000
Totals1I 7S3I 1S1S
Downgraded2 (I→S)01 (I→S)
Dropped312

#### Reconciliation

Claude provided the most volume; Codex provided the most value. The two complement each other — Codex tight on correctness, Claude broad on polish and forward-looking suggestions. Gemini contributed one architecturally interesting observation that the other agents did not surface.

Net Round 3 contribution after the resolutions gate: 3 Important findings (all on Round 2's recently-touched code paths), 8 Suggestions, 3 design observations.

Review Process Notes

Skill improvements

None. The prior-round resolutions gate did its job: nine declined-in-prior-rounds re-raises were dropped without consuming clarification rounds or finding slots. The agents' recurring re-raises of the Windows os.Rename race (now declined in three consecutive rounds across all three agents) suggest the gate is the right mechanism but the agents lack visibility into prior decisions; that is by design — they review the code, not the prior reviews.

Repo context updates