From 8eb21a113601eb682a62f3da21357f328498e704 Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Tue, 28 Jul 2026 14:31:12 +0200 Subject: [PATCH 1/4] fix(installer): execute-gate installed tools so a broken binary fails Step 1 (#411) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only post-install "verification" was a log-only, failure-masking interpolation, so a corrupt or wrong-architecture binary (winget shims / partial installs skip checksum verify; brew delivers with no checksum of ours) still printed "System tools" and only died at cluster-create in Step 2. All three OSes had the gap. - New shared execute-gate helpers: assert_tool_runs (common.sh, bash) and Assert-ToolRuns (install-k8s.ps1, PowerShell). Each runs the tool's self-check; on non-zero exit or an exception it removes the located binary and fails loudly with an arch-aware remedy, so the tool step fails instead of the cluster step. - Gate kubectl / k3d / helm after install on Linux (setup-linux.sh), macOS (setup-macos.sh), and Windows (install-k8s.ps1), on both the fresh-install and already-present paths. "System tools" success only prints once all gates pass. - kubectl is gated with `version --client` (NOT --short — removed in kubectl 1.28+, which would false-fail the gate). - New check-drift.sh parity check (_drift_execute_gates) so no installer can silently drop a gate for a tool. Tests: +2 bats (common — working tool passes / broken tool errors + removes the binary), +2 bats (drift parity), +7 Pester (exit-nonzero / exception / binary-removal / arch remedy / static gates). Co-Authored-By: Claude Opus 4.8 --- scripts/install-k8s.ps1 | 36 ++++++++++++++++++++-- scripts/lib/common.sh | 21 +++++++++++++ scripts/lib/setup-linux.sh | 11 ++++--- scripts/lib/setup-macos.sh | 9 ++++-- scripts/manifest.sha256 | 8 ++--- scripts/tests/check-drift.bats | 15 +++++++++ scripts/tests/check-drift.sh | 28 +++++++++++++++++ scripts/tests/common.bats | 22 ++++++++++++++ scripts/tests/install-k8s.Tests.ps1 | 47 +++++++++++++++++++++++++++++ 9 files changed, 182 insertions(+), 15 deletions(-) diff --git a/scripts/install-k8s.ps1 b/scripts/install-k8s.ps1 index d5fbbb2e..0b832885 100644 --- a/scripts/install-k8s.ps1 +++ b/scripts/install-k8s.ps1 @@ -142,6 +142,33 @@ function Invoke-WithRetry { } } +# Execute-gate a freshly-installed tool (#411). The old post-install "check" was a +# Log interpolation whose failure is non-terminating, so a corrupt or wrong-arch +# binary (winget shims / partial installs skip the direct path's checksum verify) +# still reached "System tools" and only died at cluster-create. Actually RUN the +# tool's self-check; on a non-zero exit or an exception, remove the binary we +# dropped and Err with an arch-aware remedy so Step 1 fails loudly. NOTE: kubectl +# uses `version --client` (NOT --short — removed in kubectl 1.28+, would +# false-fail the gate). +function Assert-ToolRuns { + param( + [Parameter(Mandatory)][string]$Name, + [Parameter(Mandatory)][string[]]$VersionArgs, + [string]$BinPath + ) + $ok = $false; $out = $null + try { + $out = (& $Name @VersionArgs 2>&1) + $ok = ($LASTEXITCODE -eq 0) + } catch { $ok = $false } + if (-not $ok) { + if ($BinPath -and (Test-Path $BinPath)) { Remove-Item $BinPath -Force -ErrorAction SilentlyContinue } + $arch = Get-WindowsArch + Err "$Name was installed but won't run -- a corrupt or wrong-architecture binary (this machine is $arch). Re-run this script to re-download it; if it recurs, remove any $Name installed via a package manager (winget/choco) first, then re-run." + } + Log "$Name OK: $(($out | Select-Object -First 1))" +} + # Sanitize workspace name to comply with DNS-1123 function ConvertTo-WorkspaceName { param([string]$Input_) @@ -712,7 +739,9 @@ echo "NCT installed successfully." # ============================================================================= function Install-Kubectl { - if (Has "kubectl") { Log "kubectl: $(cmd /c 'kubectl version --client 2>&1' | Select-Object -First 1)"; return } + # Execute-gate on both paths (#411): a present-but-broken kubectl is as fatal as + # a bad fresh install, and this runs in Step 1, before the cluster step. + if (Has "kubectl") { Assert-ToolRuns -Name "kubectl" -VersionArgs @("version","--client"); return } $arch = Get-WindowsArch $kVer = Invoke-WithRetry -Label "version check" -ScriptBlock { @@ -735,6 +764,7 @@ function Install-Kubectl { } RefreshPath Log "kubectl $kVer installed." + Assert-ToolRuns -Name "kubectl" -VersionArgs @("version","--client") -BinPath $kubectlDest } # ── Pinned tool versions (#382 / #410) ────────────────────────────────────── @@ -861,7 +891,7 @@ function Install-K3dAndHelm { RefreshPath } } - Log "k3d: $(k3d version | Select-Object -First 1)" + Assert-ToolRuns -Name "k3d" -VersionArgs @("version") -BinPath "$TOOL_DIR\k3d.exe" # -- Helm -- if (-not (Has "helm")) { @@ -898,7 +928,7 @@ function Install-K3dAndHelm { if (-not (Has "helm")) { Err "Helm could not be installed. Install manually from https://helm.sh/docs/intro/install/ and re-run." } } - Log "helm: $(cmd /c 'helm version --short 2>&1')" + Assert-ToolRuns -Name "helm" -VersionArgs @("version","--short") -BinPath "$TOOL_DIR\helm.exe" Ok "System tools" } diff --git a/scripts/lib/common.sh b/scripts/lib/common.sh index 5ef52314..d1dc086e 100755 --- a/scripts/lib/common.sh +++ b/scripts/lib/common.sh @@ -140,6 +140,27 @@ step_header() { echo -e " ${TB_HEADING}$1) $2${RESET}"; echo ""; } # ── Utility ────────────────────────────────────────────────────────────────── has() { command -v "$1" &>/dev/null; } +# Execute-gate a freshly-installed tool (#411). The old post-install "check" was a +# log-only interpolation (`... 2>/dev/null || echo present`) that masked failure, +# so a corrupt or wrong-architecture binary — a partial pkg/brew install, or a +# download no checksum path guarded — sat on PATH and failed only later, at +# cluster-create, after a green "System tools". Actually RUN the tool's cheapest +# self-check; on failure remove the binary we can locate and error() with an +# arch-aware remedy so the tool step fails loudly instead. NOTE: kubectl is gated +# with `version --client` (NOT --short, which was removed in kubectl 1.28+ and +# would false-fail the gate). Usage: assert_tool_runs ... +assert_tool_runs() { + local name="$1"; shift + local out + if out="$("$name" "$@" 2>&1)"; then + log "$name OK: $(printf '%s\n' "$out" | head -1)" + return 0 + fi + local path; path="$(command -v "$name" 2>/dev/null || true)" + [[ -n "$path" && -w "$path" ]] && rm -f "$path" 2>/dev/null || true + error "$name was installed but won't run — a corrupt or wrong-architecture binary (this machine is ${ARCH:-$(uname -m)}). Re-run the installer to re-download it; if it recurs, remove ${path:-the $name on your PATH} (and any package-manager copy) first." +} + # Sanitize a minutes-valued env override to a base-10 integer, else . # The 10# base prefix matters: bash arithmetic reads a leading zero as octal, # so 08/09 would ABORT $(( … )) under set -e (mid-create, leaving a partial diff --git a/scripts/lib/setup-linux.sh b/scripts/lib/setup-linux.sh index a675ccb5..6ddcc3be 100644 --- a/scripts/lib/setup-linux.sh +++ b/scripts/lib/setup-linux.sh @@ -401,9 +401,10 @@ install_kubectl() { || error "Couldn't resolve the kubectl version from dl.k8s.io/release/stable.txt — check network connectivity to dl.k8s.io and re-run." spin_cmd "Installing system tools…" _fetch_kubectl "$KUBE_VER" "$ARCH_DL" log "kubectl $KUBE_VER installed." - else - log "kubectl: $(kubectl version --client --short 2>/dev/null || echo present)" fi + # Execute-gate on both paths (#411): a present-but-broken kubectl is as fatal + # as a bad fresh install, and this runs before the cluster step either way. + assert_tool_runs kubectl version --client } # ── k3d ────────────────────────────────────────────────────────────────────── @@ -447,7 +448,7 @@ _fetch_k3d_release() { install_k3d() { if has k3d; then - log "k3d: $(k3d version | head -1)" + assert_tool_runs k3d version return 0 fi @@ -484,7 +485,7 @@ install_k3d() { error "System tool installation completed but not found on PATH." fi - log "k3d: $(k3d version | head -1)" + assert_tool_runs k3d version } # ── Helm ───────────────────────────────────────────────────────────────────── @@ -646,7 +647,7 @@ install_helm() { fi fi _ensure_helm_executable - log "helm: $(helm version --short 2>/dev/null || echo installed)" + assert_tool_runs helm version --short success "System tools" } diff --git a/scripts/lib/setup-macos.sh b/scripts/lib/setup-macos.sh index 4e472767..c1059dd0 100755 --- a/scripts/lib/setup-macos.sh +++ b/scripts/lib/setup-macos.sh @@ -253,20 +253,23 @@ install_docker_desktop() { } install_macos_cli_tools() { + # brew delivers these binaries with no checksum of our own (#429), so the + # execute-gate (#411) is the only thing standing between a partial/wrong-arch + # install and a green "System tools" that dies at cluster-create. if ! has kubectl; then spin_cmd "Installing system tools…" brew install kubectl fi - log "kubectl: $(kubectl version --client --short 2>/dev/null || echo installed)" + assert_tool_runs kubectl version --client if ! has k3d; then spin_cmd "Installing system tools…" brew install k3d fi - log "k3d: $(k3d version | head -1 2>/dev/null || echo installed)" + assert_tool_runs k3d version if ! has helm; then spin_cmd "Installing system tools…" brew install helm fi - log "helm: $(helm version --short 2>/dev/null || echo installed)" + assert_tool_runs helm version --short success "System tools ready" } diff --git a/scripts/manifest.sha256 b/scripts/manifest.sha256 index 9ee827c5..c0fe6521 100644 --- a/scripts/manifest.sha256 +++ b/scripts/manifest.sha256 @@ -1,11 +1,11 @@ c29c4fafe6691bfbfb6f90d761aa0650d79d0a5962a599926284d9e3e6a10006 scripts/install-k8s.sh -944e980e796852f152601e929ea8391f7208b104eadbb34de7ab47841e77d7a2 scripts/lib/common.sh +9d1eb92d1edd0dd97917a8953dabd082671601e652031a25d3496a575bdefa49 scripts/lib/common.sh de18e2c38363e6296fd9e9ed89748cc87c0949bde019383b8ee11b7ee9bdb212 scripts/lib/preflight.sh 19be2771df0e1a41b4fa9678e1cf6a77492304f66f73cf705a2ae42b1dac2ba3 scripts/lib/detect-gpu.sh 7977ef12a16fc6aee1c2824cae13bd1450f7fe1c1345323292e4a14decbfe83f scripts/lib/gpu-nvidia.sh b569eec2d8ffb9673da287a2a59d249a7dbc7236c98ab6a5062136bcc69a942c scripts/lib/gpu-amd.sh -cdc27b7929c42713e84006010aee06585d95102fa34798ee5a80ff7f05e33799 scripts/lib/setup-macos.sh -74c9228617baaafa407a5f03876412c3fd7bf9858cb11239a083e5e6675d84f0 scripts/lib/setup-linux.sh +9ced488b35d06e84355efc06d08248eb419eaad9ec02f807d54a9b747950ac9c scripts/lib/setup-macos.sh +05f2744c26cf8ddc41a0793f3c676d79ae63c4707a74206b7d3f5a1077509038 scripts/lib/setup-linux.sh 0d864b857e81d0bfde0c0ed9cbf663ca71741e1db0de4ccfbaee774164876e0b scripts/lib/cluster.sh 045caf6efeb583e5005d881d9281edae6a6aedf8f318b0a4021964b3b4b29cc5 scripts/lib/gpu-plugins.sh e673941dd9d63b2fcbb2051a1c3a86ae9a7ef6b780b9f52b34f37bfdefd66948 scripts/lib/install-client-helm.sh @@ -15,4 +15,4 @@ e2ea63d844e6649f1d3aaae9fd4733845a1a39df37d68abbaeda00330f9e1c7e scripts/lib/as b6b903a97872925ad2c0189292a81b7ebf8f0c6d4e93ac7b1e103e15afd5df68 scripts/lib/probe.sh cf095d9a92d6f4099ad19c8783d908a5952c7cbe7851043c90279724f7767dff scripts/lib/summary.sh 77e03332ebfab1ef759c6148a57afcf479c02c5dc6cc7b0e0e680f58e20cd364 scripts/lib/diagnose.sh -cdb6afa84c5d81c390399dc2de0c158238a95e91eb90796a8ccac799412b381f scripts/install-k8s.ps1 +2fd800a51c36714ed9f5ae91fa3fab2ce598ee8e48299adbf36ec010a9ec8944 scripts/install-k8s.ps1 diff --git a/scripts/tests/check-drift.bats b/scripts/tests/check-drift.bats index 47f14c4d..69e41fc0 100644 --- a/scripts/tests/check-drift.bats +++ b/scripts/tests/check-drift.bats @@ -88,3 +88,18 @@ YAML command() { if [[ "${2:-}" == helm ]]; then return 1; fi; builtin command "$@"; } _drift=0; _drift_workload_names >/dev/null 2>&1; [ "$_drift" -eq 0 ] } + +# ── Check 4: tool execute-gate parity (#411) ───────────────────────────────── +@test "execute-gates: all installers gate all tools -> no drift (#411)" { + printf 'assert_tool_runs kubectl version --client\nassert_tool_runs k3d version\nassert_tool_runs helm version --short\n' > "$ROOT/scripts/lib/setup-linux.sh" + printf 'assert_tool_runs kubectl version --client\nassert_tool_runs k3d version\nassert_tool_runs helm version --short\n' > "$ROOT/scripts/lib/setup-macos.sh" + printf 'Assert-ToolRuns -Name "kubectl" -VersionArgs @("version","--client")\nAssert-ToolRuns -Name "k3d" -VersionArgs @("version")\nAssert-ToolRuns -Name "helm" -VersionArgs @("version","--short")\n' >> "$ROOT/scripts/install-k8s.ps1" + _drift=0; _drift_execute_gates >/dev/null; [ "$_drift" -eq 0 ] +} + +@test "execute-gates: an installer missing a tool gate -> drift (#411)" { + printf 'assert_tool_runs kubectl version --client\nassert_tool_runs k3d version\nassert_tool_runs helm version --short\n' > "$ROOT/scripts/lib/setup-linux.sh" + printf 'assert_tool_runs kubectl version --client\nassert_tool_runs k3d version\n' > "$ROOT/scripts/lib/setup-macos.sh" # no helm gate + printf 'Assert-ToolRuns -Name "kubectl" -VersionArgs @("version","--client")\nAssert-ToolRuns -Name "k3d" -VersionArgs @("version")\nAssert-ToolRuns -Name "helm" -VersionArgs @("version","--short")\n' >> "$ROOT/scripts/install-k8s.ps1" + _drift=0; _drift_execute_gates >/dev/null 2>&1; [ "$_drift" -ge 1 ] +} diff --git a/scripts/tests/check-drift.sh b/scripts/tests/check-drift.sh index 11130a98..6e7a6d8d 100755 --- a/scripts/tests/check-drift.sh +++ b/scripts/tests/check-drift.sh @@ -186,12 +186,40 @@ _drift_cli_contract() { fi } +# ── Check 4: every installer execute-gates every tool (#411) ───────────────── +# The post-install "check" used to be a log-only interpolation that masked a +# corrupt/wrong-arch binary until cluster-create. All installers now RUN each tool +# (assert_tool_runs on bash, Assert-ToolRuns on PowerShell). If an installer drops +# the gate for a tool, the "green System tools then dies in Step 2" bug reopens on +# that OS — catch it here rather than in the field. +_drift_execute_gates() { + echo "▸ Tool execute-gate parity (setup-linux.sh · setup-macos.sh · install-k8s.ps1)" + local before=$_drift f t + local bash_files=( scripts/lib/setup-linux.sh scripts/lib/setup-macos.sh ) + for f in "${bash_files[@]}"; do + if [[ ! -f "$DRIFT_ROOT/$f" ]]; then _note "$f is missing"; continue; fi + for t in kubectl k3d helm; do + grep -qE "assert_tool_runs $t\b" "$DRIFT_ROOT/$f" || \ + _note "$f: no execute-gate for '$t' (assert_tool_runs $t ...) — #411" + done + done + local ps1="scripts/install-k8s.ps1" + if [[ ! -f "$DRIFT_ROOT/$ps1" ]]; then _note "$ps1 is missing"; fi + for t in kubectl k3d helm; do + grep -qE "Assert-ToolRuns -Name \"$t\"" "$DRIFT_ROOT/$ps1" || \ + _note "$ps1: no execute-gate for '$t' (Assert-ToolRuns -Name \"$t\") — #411" + done + if [[ "$_drift" -eq "$before" ]]; then _ok "all installers execute-gate kubectl / k3d / helm"; fi + return 0 +} + main() { set -uo pipefail echo "── source-of-truth drift checks ─────────────────────────────" _drift_backend_hosts _drift_workload_names _drift_cli_contract + _drift_execute_gates echo "─────────────────────────────────────────────────────────────" if [[ "$_drift" -gt 0 ]]; then echo "DRIFT: $_drift divergence(s) above. Update both sides (or the contract in check-drift.sh) and re-run." >&2 diff --git a/scripts/tests/common.bats b/scripts/tests/common.bats index 6b0277bc..1c2f3af7 100644 --- a/scripts/tests/common.bats +++ b/scripts/tests/common.bats @@ -486,3 +486,25 @@ setup() { [ "$status" -eq 124 ] [[ "$output" == *"timed out after 1s"* ]] } + +# ── assert_tool_runs (execute-gate, #411) ──────────────────────────────────── +@test "assert_tool_runs: a working tool passes, binary untouched (#411)" { + mkdir -p "$BATS_TEST_TMPDIR/bin" + printf '#!/usr/bin/env bash\necho "k3d version v5.9.0"\n' > "$BATS_TEST_TMPDIR/bin/k3d" + chmod +x "$BATS_TEST_TMPDIR/bin/k3d" + PATH="$BATS_TEST_TMPDIR/bin:$PATH" + run assert_tool_runs k3d version + [ "$status" -eq 0 ] + [ -f "$BATS_TEST_TMPDIR/bin/k3d" ] +} + +@test "assert_tool_runs: a broken tool errors and removes the binary (#411)" { + mkdir -p "$BATS_TEST_TMPDIR/bin" + printf '#!/usr/bin/env bash\nexit 1\n' > "$BATS_TEST_TMPDIR/bin/k3d" + chmod +x "$BATS_TEST_TMPDIR/bin/k3d" + PATH="$BATS_TEST_TMPDIR/bin:$PATH" + run assert_tool_runs k3d version + [ "$status" -ne 0 ] + [[ "$output" == *"won't run"* ]] + [ ! -f "$BATS_TEST_TMPDIR/bin/k3d" ] +} diff --git a/scripts/tests/install-k8s.Tests.ps1 b/scripts/tests/install-k8s.Tests.ps1 index 0a00e0dd..e62ca05d 100644 --- a/scripts/tests/install-k8s.Tests.ps1 +++ b/scripts/tests/install-k8s.Tests.ps1 @@ -1468,3 +1468,50 @@ Describe "Docker engine wait calibration (#413)" { $raw | Should -Match 'Get-Process "Docker Desktop"' } } + +Describe "Assert-ToolRuns execute-gate (#411)" { + BeforeEach { + Mock Err { throw "ERR: $args" } + Mock Get-WindowsArch { "amd64" } + } + + It "passes when the tool runs (exit 0), no throw" { + Mock k3d { $global:LASTEXITCODE = 0; "k3d version v5.9.0" } + { Assert-ToolRuns -Name "k3d" -VersionArgs @("version") } | Should -Not -Throw + } + + It "a non-zero exit -> hard fail (Err)" { + Mock k3d { $global:LASTEXITCODE = 1; "boom" } + { Assert-ToolRuns -Name "k3d" -VersionArgs @("version") } | Should -Throw "*ERR:*" + } + + It "an unrunnable binary (exception) -> hard fail (Err)" { + Mock k3d { throw "is not a valid application for this OS platform" } + { Assert-ToolRuns -Name "k3d" -VersionArgs @("version") } | Should -Throw "*ERR:*" + } + + It "removes the dropped binary on failure" { + Mock k3d { $global:LASTEXITCODE = 1 } + $bin = Join-Path $TestDrive "k3d.exe"; "x" | Set-Content $bin + { Assert-ToolRuns -Name "k3d" -VersionArgs @("version") -BinPath $bin } | Should -Throw + Test-Path $bin | Should -BeFalse + } + + It "the arch-aware remedy names the machine architecture" { + Mock k3d { $global:LASTEXITCODE = 1 } + { Assert-ToolRuns -Name "k3d" -VersionArgs @("version") } | Should -Throw "*amd64*" + } +} + +Describe "System-tool installs are execute-gated (#411)" { + BeforeAll { $script:raw = Get-Content "$PSScriptRoot/../install-k8s.ps1" -Raw } + It "gates kubectl, k3d, and helm" { + $script:raw | Should -Match 'Assert-ToolRuns -Name "kubectl"' + $script:raw | Should -Match 'Assert-ToolRuns -Name "k3d"' + $script:raw | Should -Match 'Assert-ToolRuns -Name "helm"' + } + It "no longer masks a broken tool with a non-terminating version Log interpolation" { + $script:raw | Should -Not -Match 'Log "k3d: \$\(' + $script:raw | Should -Not -Match 'Log "helm: \$\(' + } +} From c08f40eff7092c6a42536eca3ae42a5f45d602c1 Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Tue, 28 Jul 2026 14:41:45 +0200 Subject: [PATCH 2/4] test(installer): make setup-linux tool mocks runnable for the execute-gate (#411) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The execute-gate (#411) runs ` version` after install, but the setup-linux.bats harness only marked tools "present" via has() — it never provided a RUNNABLE k3d/kubectl/helm. On the toolless CI runner the gate's probe found no binary and failed 4 install_k3d tests; locally it false-passed because a real k3d was on PATH. Add silent (non-recording) runnable stubs to setup() so the gate has something to execute and they shadow any real tool on a dev host. Test-only; no production change. --- scripts/tests/setup-linux.bats | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/scripts/tests/setup-linux.bats b/scripts/tests/setup-linux.bats index b131e2e9..6de585a0 100644 --- a/scripts/tests/setup-linux.bats +++ b/scripts/tests/setup-linux.bats @@ -20,6 +20,14 @@ setup() { id() { echo "testuser docker"; } curl() { record "curl $*"; return 0; } + # The execute-gate (#411) runs ` version` after install, so the tool mocks + # must be RUNNABLE, not merely "present" via has(). Silent (no record) so the + # mock_calls assertions stay about the install/fetch, not the gate's probe — and + # so these shadow any real tool on the dev host (the toolless CI runner has none). + k3d() { echo "k3d version v5.9.0"; } + kubectl() { echo "Client Version: v1.29.4"; } + helm() { echo "v4.2.3"; } + # macOS has no /etc/os-release, and a bash `[[ -f ]]` file-test can't be mocked # the way `grep` can — so install_docker_engine's amzn/RHEL-clone branches # short-circuited off-Linux and fell through to get.docker.com. Write a real From 3dc866f8add5e5e0782e434899d5c51d820581e1 Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Wed, 29 Jul 2026 07:25:33 +0200 Subject: [PATCH 3/4] fix(installer): gate only removes binaries we placed; helm bare version; drift extracts calls (review #411) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings on #411 (saadqbal + Bugbot): - Execute-gate deleted the wrong binary on the already-present path. Removal is now OPT-IN via `assert_tool_runs --rm ` / PS `-BinPath`, passed ONLY by the fresh-install callers that placed the binary. On the present / brew / winget path we don't pass it, so a broken pkg-managed binary is left in place (deleting a brew symlink just wedges the re-run, and the bad copy may be elsewhere on PATH). bash: --rm on Linux fresh-install, none on present or macOS/brew. ps1: -BinPath tracks the direct-download dest ($null for winget/present). - helm was gated with `version --short`; --short can be dropped like kubectl's was, which would false-fail the gate and (previously) delete a good binary. Use bare `helm version` on all three OSes. - _drift_execute_gates used a whole-file grep that could match comments. It now strips comment lines and matches the actual call (`assert_tool_runs … version` / `Assert-ToolRuns -Name ""`), handling the new --rm form. (no grep -q under pipefail — SIGPIPE would false-fail.) Tests: common.bats split into --rm-removes vs no-rm-leaves; check-drift.bats adds --rm-form + commented-out-gate cases. Co-Authored-By: Claude Opus 4.8 --- scripts/install-k8s.ps1 | 11 +++++++++-- scripts/lib/common.sh | 22 +++++++++++++++------- scripts/lib/setup-linux.sh | 19 ++++++++++++++----- scripts/lib/setup-macos.sh | 4 +++- scripts/manifest.sha256 | 8 ++++---- scripts/tests/check-drift.bats | 20 ++++++++++++++++++-- scripts/tests/check-drift.sh | 18 ++++++++++++++---- scripts/tests/common.bats | 17 +++++++++++++++-- 8 files changed, 92 insertions(+), 27 deletions(-) diff --git a/scripts/install-k8s.ps1 b/scripts/install-k8s.ps1 index 519f6db9..338e0a23 100644 --- a/scripts/install-k8s.ps1 +++ b/scripts/install-k8s.ps1 @@ -907,6 +907,10 @@ function Resolve-ToolVersion { function Install-K3dAndHelm { # -- k3d -- + # $k3dDest is set ONLY on the direct-download path below; it stays $null when k3d + # came from winget or was already present, so the gate's -BinPath removes only a + # binary WE placed — never a winget/pre-existing copy elsewhere on PATH (#411 review). + $k3dDest = $null if (-not (Has "k3d")) { if (Has "winget") { Log "Installing k3d via winget..." @@ -962,9 +966,11 @@ function Install-K3dAndHelm { RefreshPath } } - Assert-ToolRuns -Name "k3d" -VersionArgs @("version") -BinPath "$TOOL_DIR\k3d.exe" + Assert-ToolRuns -Name "k3d" -VersionArgs @("version") -BinPath $k3dDest # -- Helm -- + # $helmDest set only on the direct-download path; gate removes only what we placed. + $helmDest = $null if (-not (Has "helm")) { if (Has "winget") { Log "Installing Helm via winget..." @@ -992,6 +998,7 @@ function Install-K3dAndHelm { if (Test-Path $helmExtract) { Remove-Item $helmExtract -Recurse -Force } Expand-Archive -Path $helmZip -DestinationPath $helmExtract -Force Copy-Item "$helmExtract\windows-$arch\helm.exe" "$TOOL_DIR\helm.exe" -Force + $helmDest = "$TOOL_DIR\helm.exe" Remove-Item $helmZip -Force -ErrorAction SilentlyContinue Remove-Item $helmExtract -Recurse -Force -ErrorAction SilentlyContinue RefreshPath @@ -999,7 +1006,7 @@ function Install-K3dAndHelm { if (-not (Has "helm")) { Err "Helm could not be installed. Install manually from https://helm.sh/docs/intro/install/ and re-run." } } - Assert-ToolRuns -Name "helm" -VersionArgs @("version","--short") -BinPath "$TOOL_DIR\helm.exe" + Assert-ToolRuns -Name "helm" -VersionArgs @("version") -BinPath $helmDest Ok "System tools" } diff --git a/scripts/lib/common.sh b/scripts/lib/common.sh index d1dc086e..7a7398d5 100755 --- a/scripts/lib/common.sh +++ b/scripts/lib/common.sh @@ -145,20 +145,28 @@ has() { command -v "$1" &>/dev/null; } # so a corrupt or wrong-architecture binary — a partial pkg/brew install, or a # download no checksum path guarded — sat on PATH and failed only later, at # cluster-create, after a green "System tools". Actually RUN the tool's cheapest -# self-check; on failure remove the binary we can locate and error() with an -# arch-aware remedy so the tool step fails loudly instead. NOTE: kubectl is gated -# with `version --client` (NOT --short, which was removed in kubectl 1.28+ and -# would false-fail the gate). Usage: assert_tool_runs ... +# self-check; on failure error() with an arch-aware remedy so the tool step fails +# loudly instead. NOTE: kubectl is gated with `version --client` (NOT --short, +# removed in kubectl 1.28+); helm with bare `version` (—short may go the same way). +# +# Removal is OPT-IN via a leading `--rm `: pass it ONLY from a fresh-install +# caller that placed the binary at , so we delete only what WE own. On the +# already-present / brew / pkg-manager path we don't pass it — deleting a +# brew-managed symlink just yields a re-run that won't relink (a stuck loop), and +# the broken copy may live elsewhere on PATH anyway (reviewer). There we leave the +# binary and let the remedy tell the user which copy to remove. +# Usage: assert_tool_runs [--rm ] ... assert_tool_runs() { + local rm_path="" + if [[ "${1:-}" == "--rm" ]]; then rm_path="$2"; shift 2; fi local name="$1"; shift local out if out="$("$name" "$@" 2>&1)"; then log "$name OK: $(printf '%s\n' "$out" | head -1)" return 0 fi - local path; path="$(command -v "$name" 2>/dev/null || true)" - [[ -n "$path" && -w "$path" ]] && rm -f "$path" 2>/dev/null || true - error "$name was installed but won't run — a corrupt or wrong-architecture binary (this machine is ${ARCH:-$(uname -m)}). Re-run the installer to re-download it; if it recurs, remove ${path:-the $name on your PATH} (and any package-manager copy) first." + [[ -n "$rm_path" && -f "$rm_path" ]] && rm -f "$rm_path" 2>/dev/null || true + error "$name was installed but won't run — a corrupt or wrong-architecture binary (this machine is ${ARCH:-$(uname -m)}). Re-run the installer to re-download it; if it recurs, remove ${rm_path:-the $name on your PATH} (and any package-manager copy) first." } # Sanitize a minutes-valued env override to a base-10 integer, else . diff --git a/scripts/lib/setup-linux.sh b/scripts/lib/setup-linux.sh index 6ddcc3be..86d6c30e 100644 --- a/scripts/lib/setup-linux.sh +++ b/scripts/lib/setup-linux.sh @@ -401,10 +401,12 @@ install_kubectl() { || error "Couldn't resolve the kubectl version from dl.k8s.io/release/stable.txt — check network connectivity to dl.k8s.io and re-run." spin_cmd "Installing system tools…" _fetch_kubectl "$KUBE_VER" "$ARCH_DL" log "kubectl $KUBE_VER installed." + # Fresh install: we placed it, so --rm lets a corrupt/wrong-arch download be + # cleared. Present path (else): don't remove a binary we didn't install (#411 review). + assert_tool_runs --rm "$TB_TOOLS_DIR/kubectl" kubectl version --client + else + assert_tool_runs kubectl version --client fi - # Execute-gate on both paths (#411): a present-but-broken kubectl is as fatal - # as a bad fresh install, and this runs before the cluster step either way. - assert_tool_runs kubectl version --client } # ── k3d ────────────────────────────────────────────────────────────────────── @@ -485,7 +487,7 @@ install_k3d() { error "System tool installation completed but not found on PATH." fi - assert_tool_runs k3d version + assert_tool_runs --rm "$TB_TOOLS_DIR/k3d" k3d version } # ── Helm ───────────────────────────────────────────────────────────────────── @@ -647,7 +649,14 @@ install_helm() { fi fi _ensure_helm_executable - assert_tool_runs helm version --short + # bare `helm version` (not --short: it may be dropped like kubectl's was, which + # would false-fail the gate). --rm only when helm sits in OUR tools dir; a + # pre-existing / pkg-managed helm elsewhere on PATH is not ours to delete (#411 review). + if [[ -f "$TB_TOOLS_DIR/helm" ]]; then + assert_tool_runs --rm "$TB_TOOLS_DIR/helm" helm version + else + assert_tool_runs helm version + fi success "System tools" } diff --git a/scripts/lib/setup-macos.sh b/scripts/lib/setup-macos.sh index c1059dd0..230450f0 100755 --- a/scripts/lib/setup-macos.sh +++ b/scripts/lib/setup-macos.sh @@ -269,7 +269,9 @@ install_macos_cli_tools() { if ! has helm; then spin_cmd "Installing system tools…" brew install helm fi - assert_tool_runs helm version --short + # bare `helm version` (not --short: may be dropped like kubectl's). No --rm: + # brew owns these binaries; deleting a formula's symlink just wedges the re-run. + assert_tool_runs helm version success "System tools ready" } diff --git a/scripts/manifest.sha256 b/scripts/manifest.sha256 index 4f41f9e6..488828aa 100644 --- a/scripts/manifest.sha256 +++ b/scripts/manifest.sha256 @@ -1,11 +1,11 @@ c29c4fafe6691bfbfb6f90d761aa0650d79d0a5962a599926284d9e3e6a10006 scripts/install-k8s.sh -9d1eb92d1edd0dd97917a8953dabd082671601e652031a25d3496a575bdefa49 scripts/lib/common.sh +f6dc6630ee2176f3a63a4bce1af881cbcd682de92ef20c484ad56f0cf9770e50 scripts/lib/common.sh 5ecbc741b61bfdbbdae14123d12328d786aa1a7d4e16bc3c31ce256eb4a01933 scripts/lib/preflight.sh 19be2771df0e1a41b4fa9678e1cf6a77492304f66f73cf705a2ae42b1dac2ba3 scripts/lib/detect-gpu.sh 7977ef12a16fc6aee1c2824cae13bd1450f7fe1c1345323292e4a14decbfe83f scripts/lib/gpu-nvidia.sh b569eec2d8ffb9673da287a2a59d249a7dbc7236c98ab6a5062136bcc69a942c scripts/lib/gpu-amd.sh -9ced488b35d06e84355efc06d08248eb419eaad9ec02f807d54a9b747950ac9c scripts/lib/setup-macos.sh -05f2744c26cf8ddc41a0793f3c676d79ae63c4707a74206b7d3f5a1077509038 scripts/lib/setup-linux.sh +76f0d4230d4aff510114968a477ef60e28860d1827c8ff36853a3f59d30be617 scripts/lib/setup-macos.sh +c6f4480fde612206f505bc207201ca70c3da49e6220b76400aef2f1c1806099e scripts/lib/setup-linux.sh 0d864b857e81d0bfde0c0ed9cbf663ca71741e1db0de4ccfbaee774164876e0b scripts/lib/cluster.sh 045caf6efeb583e5005d881d9281edae6a6aedf8f318b0a4021964b3b4b29cc5 scripts/lib/gpu-plugins.sh e673941dd9d63b2fcbb2051a1c3a86ae9a7ef6b780b9f52b34f37bfdefd66948 scripts/lib/install-client-helm.sh @@ -15,4 +15,4 @@ e2ea63d844e6649f1d3aaae9fd4733845a1a39df37d68abbaeda00330f9e1c7e scripts/lib/as b6b903a97872925ad2c0189292a81b7ebf8f0c6d4e93ac7b1e103e15afd5df68 scripts/lib/probe.sh cf095d9a92d6f4099ad19c8783d908a5952c7cbe7851043c90279724f7767dff scripts/lib/summary.sh 77e03332ebfab1ef759c6148a57afcf479c02c5dc6cc7b0e0e680f58e20cd364 scripts/lib/diagnose.sh -1c96831725adfd4b887577fed4e6a4d5b3212c4417c02e800d379999a97750ea scripts/install-k8s.ps1 +ddf12f45415607a9cd82e57f994d42bf554efced3bae994bcb0b068e2cc3a1e3 scripts/install-k8s.ps1 diff --git a/scripts/tests/check-drift.bats b/scripts/tests/check-drift.bats index 6d3efa65..4f40b8b3 100644 --- a/scripts/tests/check-drift.bats +++ b/scripts/tests/check-drift.bats @@ -98,9 +98,25 @@ YAML } @test "execute-gates: an installer missing a tool gate -> drift (#411)" { - printf 'assert_tool_runs kubectl version --client\nassert_tool_runs k3d version\nassert_tool_runs helm version --short\n' > "$ROOT/scripts/lib/setup-linux.sh" + printf 'assert_tool_runs kubectl version --client\nassert_tool_runs k3d version\nassert_tool_runs helm version\n' > "$ROOT/scripts/lib/setup-linux.sh" printf 'assert_tool_runs kubectl version --client\nassert_tool_runs k3d version\n' > "$ROOT/scripts/lib/setup-macos.sh" # no helm gate - printf 'Assert-ToolRuns -Name "kubectl" -VersionArgs @("version","--client")\nAssert-ToolRuns -Name "k3d" -VersionArgs @("version")\nAssert-ToolRuns -Name "helm" -VersionArgs @("version","--short")\n' >> "$ROOT/scripts/install-k8s.ps1" + printf 'Assert-ToolRuns -Name "kubectl" -VersionArgs @("version","--client")\nAssert-ToolRuns -Name "k3d" -VersionArgs @("version")\nAssert-ToolRuns -Name "helm" -VersionArgs @("version")\n' >> "$ROOT/scripts/install-k8s.ps1" + _drift=0; _drift_execute_gates >/dev/null 2>&1; [ "$_drift" -ge 1 ] +} + +@test "execute-gates: the --rm form is recognized as a gate (#411 review)" { + printf 'assert_tool_runs --rm "$TB_TOOLS_DIR/kubectl" kubectl version --client\nassert_tool_runs --rm "$TB_TOOLS_DIR/k3d" k3d version\nassert_tool_runs helm version\n' > "$ROOT/scripts/lib/setup-linux.sh" + printf 'assert_tool_runs kubectl version --client\nassert_tool_runs k3d version\nassert_tool_runs helm version\n' > "$ROOT/scripts/lib/setup-macos.sh" + printf 'Assert-ToolRuns -Name "kubectl" -VersionArgs @("version","--client")\nAssert-ToolRuns -Name "k3d" -VersionArgs @("version")\nAssert-ToolRuns -Name "helm" -VersionArgs @("version")\n' >> "$ROOT/scripts/install-k8s.ps1" + _drift=0; _drift_execute_gates >/dev/null; [ "$_drift" -eq 0 ] +} + +@test "execute-gates: a COMMENTED-OUT gate does NOT count -> drift (Bugbot #411)" { + # A gate that lingers only in a comment must not satisfy the check (structured + # call extraction, not whole-file grep). + printf 'assert_tool_runs kubectl version --client\nassert_tool_runs k3d version\n# assert_tool_runs helm version\n' > "$ROOT/scripts/lib/setup-linux.sh" + printf 'assert_tool_runs kubectl version --client\nassert_tool_runs k3d version\nassert_tool_runs helm version\n' > "$ROOT/scripts/lib/setup-macos.sh" + printf 'Assert-ToolRuns -Name "kubectl" -VersionArgs @("version","--client")\nAssert-ToolRuns -Name "k3d" -VersionArgs @("version")\n# Assert-ToolRuns -Name "helm" -VersionArgs @("version")\n' >> "$ROOT/scripts/install-k8s.ps1" _drift=0; _drift_execute_gates >/dev/null 2>&1; [ "$_drift" -ge 1 ] } diff --git a/scripts/tests/check-drift.sh b/scripts/tests/check-drift.sh index 5e6b7130..0b734359 100755 --- a/scripts/tests/check-drift.sh +++ b/scripts/tests/check-drift.sh @@ -192,6 +192,16 @@ _drift_cli_contract() { # (assert_tool_runs on bash, Assert-ToolRuns on PowerShell). If an installer drops # the gate for a tool, the "green System tools then dies in Step 2" bug reopens on # that OS — catch it here rather than in the field. +# Match the actual gate CALL, never a whole-file grep: strip comment lines first, +# then require the tool's own version-check invocation — `assert_tool_runs … +# version` (the optional `--rm ` sits between and is ignored) on bash, +# `Assert-ToolRuns -Name ""` on ps1. A commented-out or merely-mentioned gate no +# longer satisfies the check (reviewer: parity checks must extract structured calls). +# NOTE: no `grep -q` — under main()'s `set -o pipefail` a -q that closes the pipe +# early makes the upstream grep exit on SIGPIPE (141), failing the pipeline even on +# a match. Read to EOF and redirect instead. +_drift_gate_bash() { grep -vE '^[[:space:]]*#' "$1" 2>/dev/null | grep -F assert_tool_runs | grep -E "(^| )$2 version" >/dev/null 2>&1; } +_drift_gate_ps1() { grep -vE '^[[:space:]]*#' "$1" 2>/dev/null | grep -E "Assert-ToolRuns -Name \"$2\"" >/dev/null 2>&1; } _drift_execute_gates() { echo "▸ Tool execute-gate parity (setup-linux.sh · setup-macos.sh · install-k8s.ps1)" local before=$_drift f t @@ -199,15 +209,15 @@ _drift_execute_gates() { for f in "${bash_files[@]}"; do if [[ ! -f "$DRIFT_ROOT/$f" ]]; then _note "$f is missing"; continue; fi for t in kubectl k3d helm; do - grep -qE "assert_tool_runs $t\b" "$DRIFT_ROOT/$f" || \ - _note "$f: no execute-gate for '$t' (assert_tool_runs $t ...) — #411" + _drift_gate_bash "$DRIFT_ROOT/$f" "$t" || \ + _note "$f: no execute-gate call for '$t' (assert_tool_runs … $t version) — #411" done done local ps1="scripts/install-k8s.ps1" if [[ ! -f "$DRIFT_ROOT/$ps1" ]]; then _note "$ps1 is missing"; fi for t in kubectl k3d helm; do - grep -qE "Assert-ToolRuns -Name \"$t\"" "$DRIFT_ROOT/$ps1" || \ - _note "$ps1: no execute-gate for '$t' (Assert-ToolRuns -Name \"$t\") — #411" + _drift_gate_ps1 "$DRIFT_ROOT/$ps1" "$t" || \ + _note "$ps1: no execute-gate call for '$t' (Assert-ToolRuns -Name \"$t\") — #411" done if [[ "$_drift" -eq "$before" ]]; then _ok "all installers execute-gate kubectl / k3d / helm"; fi return 0 diff --git a/scripts/tests/common.bats b/scripts/tests/common.bats index 1c2f3af7..c62da57f 100644 --- a/scripts/tests/common.bats +++ b/scripts/tests/common.bats @@ -498,7 +498,20 @@ setup() { [ -f "$BATS_TEST_TMPDIR/bin/k3d" ] } -@test "assert_tool_runs: a broken tool errors and removes the binary (#411)" { +@test "assert_tool_runs: a broken tool with --rm errors and removes the binary WE placed (#411)" { + mkdir -p "$BATS_TEST_TMPDIR/bin" + printf '#!/usr/bin/env bash\nexit 1\n' > "$BATS_TEST_TMPDIR/bin/k3d" + chmod +x "$BATS_TEST_TMPDIR/bin/k3d" + PATH="$BATS_TEST_TMPDIR/bin:$PATH" + run assert_tool_runs --rm "$BATS_TEST_TMPDIR/bin/k3d" k3d version + [ "$status" -ne 0 ] + [[ "$output" == *"won't run"* ]] + [ ! -f "$BATS_TEST_TMPDIR/bin/k3d" ] # the binary we placed was removed +} + +@test "assert_tool_runs: a broken tool WITHOUT --rm errors but leaves the binary (#411 review)" { + # Already-present / pkg-managed path: we didn't place it, so we must not delete it + # (deleting a brew symlink just wedges the re-run — reviewer). mkdir -p "$BATS_TEST_TMPDIR/bin" printf '#!/usr/bin/env bash\nexit 1\n' > "$BATS_TEST_TMPDIR/bin/k3d" chmod +x "$BATS_TEST_TMPDIR/bin/k3d" @@ -506,5 +519,5 @@ setup() { run assert_tool_runs k3d version [ "$status" -ne 0 ] [[ "$output" == *"won't run"* ]] - [ ! -f "$BATS_TEST_TMPDIR/bin/k3d" ] + [ -f "$BATS_TEST_TMPDIR/bin/k3d" ] # NOT removed — we didn't place it } From f0f2626fb91ab4f713a7c8e10cfa8ffb3d102ddf Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Wed, 29 Jul 2026 07:41:48 +0200 Subject: [PATCH 4/4] fix(installer): execute-gate removes a broken binary only if WE placed it AND it ran (Bugbot #411 r2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 made removal opt-in but only on the fresh-install path, so a broken installer-placed kubectl/k3d on the ALREADY-PRESENT path (left by a prior run) stayed → `has` true → re-run couldn't self-heal (Bugbot). helm already handled it. Unify all three: callers pass --rm "$TB_TOOLS_DIR/" (bash) / -BinPath "$TOOL_DIR\.exe" (ps1) on EVERY path, and the gate removes that path only when the binary that actually ran resolves to it — `command -v … -ef ` on bash, `(Get-Command).Source -eq $BinPath` on ps1. So a broken copy we own (fresh or prior-run) self-heals, while a brew/winget/pkg-manager copy elsewhere on PATH is never touched (satisfies the earlier reviewer guard too). Dropped the now-moot $k3dDest/$helmDest null-tracking and helm's -f branch. Tests: common.bats +decoy-copy case; install-k8s.Tests.ps1 removal split into ran-here (removed) vs resolved-elsewhere (left); check-drift already covers --rm. Co-Authored-By: Claude Opus 4.8 --- scripts/install-k8s.ps1 | 31 +++++++++++++++-------------- scripts/lib/common.sh | 20 ++++++++++++------- scripts/lib/setup-linux.sh | 23 +++++++++------------ scripts/manifest.sha256 | 6 +++--- scripts/tests/common.bats | 12 +++++++++++ scripts/tests/install-k8s.Tests.ps1 | 11 +++++++++- 6 files changed, 63 insertions(+), 40 deletions(-) diff --git a/scripts/install-k8s.ps1 b/scripts/install-k8s.ps1 index 338e0a23..3d3cd653 100644 --- a/scripts/install-k8s.ps1 +++ b/scripts/install-k8s.ps1 @@ -180,10 +180,15 @@ function Invoke-WithRetry { # Log interpolation whose failure is non-terminating, so a corrupt or wrong-arch # binary (winget shims / partial installs skip the direct path's checksum verify) # still reached "System tools" and only died at cluster-create. Actually RUN the -# tool's self-check; on a non-zero exit or an exception, remove the binary we -# dropped and Err with an arch-aware remedy so Step 1 fails loudly. NOTE: kubectl -# uses `version --client` (NOT --short — removed in kubectl 1.28+, would -# false-fail the gate). +# tool's self-check; on failure Err with an arch-aware remedy so Step 1 fails +# loudly. NOTE: kubectl uses `version --client` (NOT --short — removed in 1.28+); +# helm uses bare `version` (--short may go the same way). +# +# -BinPath is our own install location. On failure we remove it ONLY when the +# binary that actually ran resolves to it — so a broken copy WE placed (fresh or +# left by a prior run) self-heals on re-run, while a winget/choco/pre-existing copy +# elsewhere on PATH is never deleted (reviewer + Bugbot). Callers may pass -BinPath +# on every path; the resolved-source guard sorts out ownership. function Assert-ToolRuns { param( [Parameter(Mandatory)][string]$Name, @@ -196,7 +201,10 @@ function Assert-ToolRuns { $ok = ($LASTEXITCODE -eq 0) } catch { $ok = $false } if (-not $ok) { - if ($BinPath -and (Test-Path $BinPath)) { Remove-Item $BinPath -Force -ErrorAction SilentlyContinue } + if ($BinPath -and (Test-Path $BinPath)) { + $resolved = (Get-Command $Name -ErrorAction SilentlyContinue).Source + if ($resolved -and ($resolved -eq $BinPath)) { Remove-Item $BinPath -Force -ErrorAction SilentlyContinue } + } $arch = Get-WindowsArch Err "$Name was installed but won't run -- a corrupt or wrong-architecture binary (this machine is $arch). Re-run this script to re-download it; if it recurs, remove any $Name installed via a package manager (winget/choco) first, then re-run." } @@ -812,7 +820,7 @@ echo "NCT installed successfully." function Install-Kubectl { # Execute-gate on both paths (#411): a present-but-broken kubectl is as fatal as # a bad fresh install, and this runs in Step 1, before the cluster step. - if (Has "kubectl") { Assert-ToolRuns -Name "kubectl" -VersionArgs @("version","--client"); return } + if (Has "kubectl") { Assert-ToolRuns -Name "kubectl" -VersionArgs @("version","--client") -BinPath "$TOOL_DIR\kubectl.exe"; return } $arch = Get-WindowsArch $kVer = Invoke-WithRetry -Label "version check" -ScriptBlock { @@ -907,10 +915,6 @@ function Resolve-ToolVersion { function Install-K3dAndHelm { # -- k3d -- - # $k3dDest is set ONLY on the direct-download path below; it stays $null when k3d - # came from winget or was already present, so the gate's -BinPath removes only a - # binary WE placed — never a winget/pre-existing copy elsewhere on PATH (#411 review). - $k3dDest = $null if (-not (Has "k3d")) { if (Has "winget") { Log "Installing k3d via winget..." @@ -966,11 +970,9 @@ function Install-K3dAndHelm { RefreshPath } } - Assert-ToolRuns -Name "k3d" -VersionArgs @("version") -BinPath $k3dDest + Assert-ToolRuns -Name "k3d" -VersionArgs @("version") -BinPath "$TOOL_DIR\k3d.exe" # -- Helm -- - # $helmDest set only on the direct-download path; gate removes only what we placed. - $helmDest = $null if (-not (Has "helm")) { if (Has "winget") { Log "Installing Helm via winget..." @@ -998,7 +1000,6 @@ function Install-K3dAndHelm { if (Test-Path $helmExtract) { Remove-Item $helmExtract -Recurse -Force } Expand-Archive -Path $helmZip -DestinationPath $helmExtract -Force Copy-Item "$helmExtract\windows-$arch\helm.exe" "$TOOL_DIR\helm.exe" -Force - $helmDest = "$TOOL_DIR\helm.exe" Remove-Item $helmZip -Force -ErrorAction SilentlyContinue Remove-Item $helmExtract -Recurse -Force -ErrorAction SilentlyContinue RefreshPath @@ -1006,7 +1007,7 @@ function Install-K3dAndHelm { if (-not (Has "helm")) { Err "Helm could not be installed. Install manually from https://helm.sh/docs/intro/install/ and re-run." } } - Assert-ToolRuns -Name "helm" -VersionArgs @("version") -BinPath $helmDest + Assert-ToolRuns -Name "helm" -VersionArgs @("version") -BinPath "$TOOL_DIR\helm.exe" Ok "System tools" } diff --git a/scripts/lib/common.sh b/scripts/lib/common.sh index 7a7398d5..3a4eef20 100755 --- a/scripts/lib/common.sh +++ b/scripts/lib/common.sh @@ -149,12 +149,14 @@ has() { command -v "$1" &>/dev/null; } # loudly instead. NOTE: kubectl is gated with `version --client` (NOT --short, # removed in kubectl 1.28+); helm with bare `version` (—short may go the same way). # -# Removal is OPT-IN via a leading `--rm `: pass it ONLY from a fresh-install -# caller that placed the binary at , so we delete only what WE own. On the -# already-present / brew / pkg-manager path we don't pass it — deleting a -# brew-managed symlink just yields a re-run that won't relink (a stuck loop), and -# the broken copy may live elsewhere on PATH anyway (reviewer). There we leave the -# binary and let the remedy tell the user which copy to remove. +# Removal is OPT-IN via a leading `--rm `: pass it with the path where the +# installer PLACES the binary (TB_TOOLS_DIR/). On failure we remove that path +# ONLY when the binary that actually ran is that exact file (same inode, `-ef`). +# So: a broken binary WE installed there (fresh OR left by a prior run) is cleared, +# letting a re-run self-heal (Bugbot: otherwise `has` stays true → stuck loop); +# but a brew/pkg-manager copy that lives elsewhere on PATH is never deleted — the +# resolved binary won't match our path (reviewer). Callers may pass --rm on every +# path; the `-ef` guard sorts out ownership. macOS/brew callers pass no --rm. # Usage: assert_tool_runs [--rm ] ... assert_tool_runs() { local rm_path="" @@ -165,7 +167,11 @@ assert_tool_runs() { log "$name OK: $(printf '%s\n' "$out" | head -1)" return 0 fi - [[ -n "$rm_path" && -f "$rm_path" ]] && rm -f "$rm_path" 2>/dev/null || true + # Remove only the file we placed AND only if it's the binary that just failed. + if [[ -n "$rm_path" && -f "$rm_path" ]]; then + local resolved; resolved="$(command -v "$name" 2>/dev/null || true)" + [[ -n "$resolved" && "$resolved" -ef "$rm_path" ]] && rm -f "$rm_path" 2>/dev/null || true + fi error "$name was installed but won't run — a corrupt or wrong-architecture binary (this machine is ${ARCH:-$(uname -m)}). Re-run the installer to re-download it; if it recurs, remove ${rm_path:-the $name on your PATH} (and any package-manager copy) first." } diff --git a/scripts/lib/setup-linux.sh b/scripts/lib/setup-linux.sh index 86d6c30e..a61beab2 100644 --- a/scripts/lib/setup-linux.sh +++ b/scripts/lib/setup-linux.sh @@ -401,12 +401,11 @@ install_kubectl() { || error "Couldn't resolve the kubectl version from dl.k8s.io/release/stable.txt — check network connectivity to dl.k8s.io and re-run." spin_cmd "Installing system tools…" _fetch_kubectl "$KUBE_VER" "$ARCH_DL" log "kubectl $KUBE_VER installed." - # Fresh install: we placed it, so --rm lets a corrupt/wrong-arch download be - # cleared. Present path (else): don't remove a binary we didn't install (#411 review). - assert_tool_runs --rm "$TB_TOOLS_DIR/kubectl" kubectl version --client - else - assert_tool_runs kubectl version --client fi + # Gate on both paths (fresh + already-present). --rm removes our TB_TOOLS_DIR copy + # only if IT is the binary that failed (assert_tool_runs' -ef guard), so a broken + # installer-placed kubectl self-heals on re-run while a pkg copy elsewhere is safe. + assert_tool_runs --rm "$TB_TOOLS_DIR/kubectl" kubectl version --client } # ── k3d ────────────────────────────────────────────────────────────────────── @@ -450,7 +449,7 @@ _fetch_k3d_release() { install_k3d() { if has k3d; then - assert_tool_runs k3d version + assert_tool_runs --rm "$TB_TOOLS_DIR/k3d" k3d version return 0 fi @@ -649,14 +648,10 @@ install_helm() { fi fi _ensure_helm_executable - # bare `helm version` (not --short: it may be dropped like kubectl's was, which - # would false-fail the gate). --rm only when helm sits in OUR tools dir; a - # pre-existing / pkg-managed helm elsewhere on PATH is not ours to delete (#411 review). - if [[ -f "$TB_TOOLS_DIR/helm" ]]; then - assert_tool_runs --rm "$TB_TOOLS_DIR/helm" helm version - else - assert_tool_runs helm version - fi + # bare `helm version` (not --short: it may be dropped like kubectl's was). --rm + # removes our TB_TOOLS_DIR copy only if IT is the binary that failed (-ef guard), + # never a pre-existing / pkg-managed helm elsewhere on PATH (#411 review). + assert_tool_runs --rm "$TB_TOOLS_DIR/helm" helm version success "System tools" } diff --git a/scripts/manifest.sha256 b/scripts/manifest.sha256 index 488828aa..a1ba21e2 100644 --- a/scripts/manifest.sha256 +++ b/scripts/manifest.sha256 @@ -1,11 +1,11 @@ c29c4fafe6691bfbfb6f90d761aa0650d79d0a5962a599926284d9e3e6a10006 scripts/install-k8s.sh -f6dc6630ee2176f3a63a4bce1af881cbcd682de92ef20c484ad56f0cf9770e50 scripts/lib/common.sh +c64c2bcf00cbaffc00ae54a9149436a375ee5052e3e882fd689bc19a7b4a1620 scripts/lib/common.sh 5ecbc741b61bfdbbdae14123d12328d786aa1a7d4e16bc3c31ce256eb4a01933 scripts/lib/preflight.sh 19be2771df0e1a41b4fa9678e1cf6a77492304f66f73cf705a2ae42b1dac2ba3 scripts/lib/detect-gpu.sh 7977ef12a16fc6aee1c2824cae13bd1450f7fe1c1345323292e4a14decbfe83f scripts/lib/gpu-nvidia.sh b569eec2d8ffb9673da287a2a59d249a7dbc7236c98ab6a5062136bcc69a942c scripts/lib/gpu-amd.sh 76f0d4230d4aff510114968a477ef60e28860d1827c8ff36853a3f59d30be617 scripts/lib/setup-macos.sh -c6f4480fde612206f505bc207201ca70c3da49e6220b76400aef2f1c1806099e scripts/lib/setup-linux.sh +bf4f299ad501a90184dd3dc9dadee9f82f839351c44f0b2542c2020b9c0cf0b7 scripts/lib/setup-linux.sh 0d864b857e81d0bfde0c0ed9cbf663ca71741e1db0de4ccfbaee774164876e0b scripts/lib/cluster.sh 045caf6efeb583e5005d881d9281edae6a6aedf8f318b0a4021964b3b4b29cc5 scripts/lib/gpu-plugins.sh e673941dd9d63b2fcbb2051a1c3a86ae9a7ef6b780b9f52b34f37bfdefd66948 scripts/lib/install-client-helm.sh @@ -15,4 +15,4 @@ e2ea63d844e6649f1d3aaae9fd4733845a1a39df37d68abbaeda00330f9e1c7e scripts/lib/as b6b903a97872925ad2c0189292a81b7ebf8f0c6d4e93ac7b1e103e15afd5df68 scripts/lib/probe.sh cf095d9a92d6f4099ad19c8783d908a5952c7cbe7851043c90279724f7767dff scripts/lib/summary.sh 77e03332ebfab1ef759c6148a57afcf479c02c5dc6cc7b0e0e680f58e20cd364 scripts/lib/diagnose.sh -ddf12f45415607a9cd82e57f994d42bf554efced3bae994bcb0b068e2cc3a1e3 scripts/install-k8s.ps1 +3a01d1f17ea4f257dfa37c31dde0166bbdf1f9a020b027f8f1bf4362645fba86 scripts/install-k8s.ps1 diff --git a/scripts/tests/common.bats b/scripts/tests/common.bats index c62da57f..9ff4dd6b 100644 --- a/scripts/tests/common.bats +++ b/scripts/tests/common.bats @@ -521,3 +521,15 @@ setup() { [[ "$output" == *"won't run"* ]] [ -f "$BATS_TEST_TMPDIR/bin/k3d" ] # NOT removed — we didn't place it } + +@test "assert_tool_runs: --rm removes ONLY the binary that actually ran, not a decoy copy (#411 Bugbot)" { + # The failing k3d resolves to bin/; --rm points at a different (installer-dir) + # copy that did NOT run. The -ef guard must leave that copy alone. + mkdir -p "$BATS_TEST_TMPDIR/bin" "$BATS_TEST_TMPDIR/tools" + printf '#!/usr/bin/env bash\nexit 1\n' > "$BATS_TEST_TMPDIR/bin/k3d"; chmod +x "$BATS_TEST_TMPDIR/bin/k3d" + : > "$BATS_TEST_TMPDIR/tools/k3d" + PATH="$BATS_TEST_TMPDIR/bin:$PATH" + run assert_tool_runs --rm "$BATS_TEST_TMPDIR/tools/k3d" k3d version + [ "$status" -ne 0 ] + [ -f "$BATS_TEST_TMPDIR/tools/k3d" ] # NOT removed — it isn't the binary that ran +} diff --git a/scripts/tests/install-k8s.Tests.ps1 b/scripts/tests/install-k8s.Tests.ps1 index f7429e9a..1eb09740 100644 --- a/scripts/tests/install-k8s.Tests.ps1 +++ b/scripts/tests/install-k8s.Tests.ps1 @@ -1490,13 +1490,22 @@ Describe "Assert-ToolRuns execute-gate (#411)" { { Assert-ToolRuns -Name "k3d" -VersionArgs @("version") } | Should -Throw "*ERR:*" } - It "removes the dropped binary on failure" { + It "removes the dropped binary on failure when it's the one that ran" { Mock k3d { $global:LASTEXITCODE = 1 } $bin = Join-Path $TestDrive "k3d.exe"; "x" | Set-Content $bin + Mock Get-Command { [pscustomobject]@{ Source = $bin } } -ParameterFilter { $Name -eq 'k3d' } { Assert-ToolRuns -Name "k3d" -VersionArgs @("version") -BinPath $bin } | Should -Throw Test-Path $bin | Should -BeFalse } + It "does NOT remove BinPath when the failing binary resolved elsewhere (winget/present)" { + Mock k3d { $global:LASTEXITCODE = 1 } + $bin = Join-Path $TestDrive "k3d.exe"; "x" | Set-Content $bin + Mock Get-Command { [pscustomobject]@{ Source = "C:\winget\k3d.exe" } } -ParameterFilter { $Name -eq 'k3d' } + { Assert-ToolRuns -Name "k3d" -VersionArgs @("version") -BinPath $bin } | Should -Throw + Test-Path $bin | Should -BeTrue # left alone — it isn't the binary that ran + } + It "the arch-aware remedy names the machine architecture" { Mock k3d { $global:LASTEXITCODE = 1 } { Assert-ToolRuns -Name "k3d" -VersionArgs @("version") } | Should -Throw "*amd64*"