From b8e5236b58d9c6af45f42d93d6e6bc8025966312 Mon Sep 17 00:00:00 2001 From: Aaron Stainback Date: Mon, 27 Apr 2026 23:11:29 -0400 Subject: [PATCH 1/8] fix(install): unify curl-with-retry behaviour into shared helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aaron 2026-04-28 surfaced two related questions: (1) "curl 502 pattern i mean why should a PR ever fail for this? our code does not handle the retries already?" — exactly: external- infra failures should be absorbed by retry-with-backoff inside the install path, not kicked out to a workflow-rerun discipline. (2) "sounds like a common helper would help too rather than copy/ paste" — the retry policy was previously inlined in common/verifiers.sh AND missing entirely from linux.sh (mise), macos.sh (Homebrew), and common/elan.sh (Lean toolchain). Each copy-paste would drift over time. Solution: tools/setup/common/curl-fetch.sh defines a single sourceable helper `curl_fetch` that prepends the retry flags (--retry 5 --retry-delay 2 --retry-all-errors) to any curl invocation, plus the existing -fsSL semantics. Idempotent (declare -F guard); safe to source from multiple scripts in the same install run. Call sites updated to use the helper: - tools/setup/linux.sh:61 — mise install (was missing retries) - tools/setup/macos.sh:43 — Homebrew install (was missing retries) - tools/setup/common/elan.sh:18 — Lean toolchain (was missing retries) - tools/setup/common/verifiers.sh:55 — TLA+/Alloy jar download (was inlining the same flags; now uses helper) Naming note: function called `curl_fetch` (not `zeta_curl_fetch`) per Aaron 2026-04-28: no built-in or PATH binary collision risk to defend against; the bare name is clear in context. After this: external-infra blips (curl 502 from upstream package mirrors, transient network errors) get absorbed inside the install script via 5-attempt exponential backoff, and the PR doesn't fail in the first place — answer to Aaron's "why should a PR ever fail for this?" Composes with: feedback_transient_ci_external_infra_only_test_ failures_are_bugs_not_flakes_2026_04_28.md (the verify-first discipline still applies for OTHER external-infra surfaces; this fix narrows the curl-from-install class of failures by handling them inside the script). Co-Authored-By: Claude Opus 4.7 --- tools/setup/common/curl-fetch.sh | 72 ++++++++++++++++++++++++++++++++ tools/setup/common/elan.sh | 5 ++- tools/setup/common/verifiers.sh | 18 ++++---- tools/setup/linux.sh | 5 ++- tools/setup/macos.sh | 5 ++- 5 files changed, 94 insertions(+), 11 deletions(-) create mode 100644 tools/setup/common/curl-fetch.sh diff --git a/tools/setup/common/curl-fetch.sh b/tools/setup/common/curl-fetch.sh new file mode 100644 index 00000000..df28b094 --- /dev/null +++ b/tools/setup/common/curl-fetch.sh @@ -0,0 +1,72 @@ +#!/usr/bin/env bash +# +# tools/setup/common/curl-fetch.sh — sourceable helper for +# fetching URLs with uniform retry behaviour during install. +# +# WHY +# === +# Aaron 2026-04-28: *"curl 502 pattern i mean why should a PR +# ever fail for this? our code does not handle the retries +# already?"* — exactly: external-infra failures (upstream +# package mirrors returning 5xx, transient curl-22 / network +# blips) should be absorbed by retry-with-backoff inside the +# install path, not kicked out to a workflow-rerun discipline. +# +# This file centralises the retry policy so every call site +# uses the same flags. Previously the policy was inlined in +# `tools/setup/common/verifiers.sh` and missing entirely from +# `linux.sh` (mise install), `macos.sh` (Homebrew install), +# and `elan.sh` (Lean toolchain install). Aaron 2026-04-28 +# follow-up: *"sounds like a common helper would help too +# rather than copy/paste."* +# +# USAGE +# ===== +# Source this file, then call `curl_fetch` with the same +# args you'd pass to curl. The function prepends the retry +# flags transparently: +# +# # shellcheck source=/dev/null +# source "$REPO_ROOT/tools/setup/common/curl-fetch.sh" +# curl_fetch https://mise.run | sh +# /bin/bash -c "$(curl_fetch https://example.com/install.sh)" +# curl_fetch --output "$path" "$url" +# +# RETRY POLICY (rationale) +# ======================== +# --retry 5 — five attempts total. Empirically +# covers all transient upstream blips +# this install path has hit during +# 2026-04 sessions. +# --retry-delay 2 — 2-second base delay between retries. +# Short enough to not penalise CI when +# the retry succeeds; long enough to +# let upstream recover from a brief +# surge. +# --retry-all-errors — retry on ALL transient errors, +# including HTTP 4xx that curl +# would otherwise pass through. +# (Default `--retry` only retries on +# select transient errors; setup +# installers benefit from the broader +# surface.) +# -fsSL — original flags preserved: +# -f: fail on HTTP errors +# -s: silent (no progress meter) +# -S: show errors when silent +# -L: follow redirects +# +# IDEMPOTENCE +# =========== +# Repeated source of this file overwrites the function body +# (no append, no accumulation). Safe to source from multiple +# scripts within the same install run. + +# Guard: only define once per shell to avoid noise on multi-source. +if ! declare -F curl_fetch >/dev/null 2>&1; then + +curl_fetch() { + curl -fsSL --retry 5 --retry-delay 2 --retry-all-errors "$@" +} + +fi diff --git a/tools/setup/common/elan.sh b/tools/setup/common/elan.sh index 4a20c43e..b9174e1f 100755 --- a/tools/setup/common/elan.sh +++ b/tools/setup/common/elan.sh @@ -10,9 +10,12 @@ set -euo pipefail +# shellcheck source=curl-fetch.sh +source "$(dirname "${BASH_SOURCE[0]}")/curl-fetch.sh" + if ! command -v elan >/dev/null 2>&1; then echo "↓ installing elan (Lean 4 toolchain manager)..." - curl -fsSL https://raw.githubusercontent.com/leanprover/elan/master/elan-init.sh \ + curl_fetch https://raw.githubusercontent.com/leanprover/elan/master/elan-init.sh \ | sh -s -- -y --default-toolchain none fi diff --git a/tools/setup/common/verifiers.sh b/tools/setup/common/verifiers.sh index 47205195..7b99b408 100755 --- a/tools/setup/common/verifiers.sh +++ b/tools/setup/common/verifiers.sh @@ -13,6 +13,9 @@ set -euo pipefail +# shellcheck source=curl-fetch.sh +source "$(dirname "${BASH_SOURCE[0]}")/curl-fetch.sh" + REPO_ROOT="$(cd "$(dirname "$0")/../../.." && pwd)" MANIFEST="$REPO_ROOT/tools/setup/manifests/verifiers" @@ -47,15 +50,14 @@ grep -vE '^(#|$)' "$MANIFEST" | while IFS= read -r line; do # ~13:52 UTC, hit PR #481 CodeQL csharp + PR #482 markdownlint # CI runs). Per Otto-285 (don't use determinism to avoid # edge-case handling — handle the network-non-determinism - # algorithmically), curl handles the retry: `--retry 5` attempts, - # exponential backoff (2/4/8/16/32 s default), `--retry-all-errors` - # so 4xx/5xx server errors retry too (curl's default only retries - # connect / dns / 408 / 429 / 5xx-with-Retry-After). Keeps - # `-fsSL` semantics — fail at the end if all 5 attempts hit - # the same transient. + # algorithmically), curl_fetch (from common/curl-fetch.sh) + # handles the retry: 5 attempts, 2-4-8-16-32 s exponential + # backoff, --retry-all-errors so 4xx/5xx errors retry too. + # Keeps -fsSL semantics — fail at the end if all 5 attempts + # hit the same transient. (Aaron 2026-04-28: helper extracted + # from copy-pasted call sites; was previously inline here.) echo "↓ downloading $target from $url" - curl -fsSL --retry 5 --retry-delay 2 --retry-all-errors \ - -o "$dest.part" "$url" + curl_fetch -o "$dest.part" "$url" mv "$dest.part" "$dest" echo "✓ $target" fi diff --git a/tools/setup/linux.sh b/tools/setup/linux.sh index e0d73187..434e5df3 100755 --- a/tools/setup/linux.sh +++ b/tools/setup/linux.sh @@ -23,6 +23,9 @@ set -euo pipefail REPO_ROOT="$(cd "$(dirname "$0")/../.." && pwd)" SETUP_DIR="$REPO_ROOT/tools/setup" +# shellcheck source=common/curl-fetch.sh +source "$SETUP_DIR/common/curl-fetch.sh" + # ── Detect apt availability (Debian/Ubuntu) ───────────────────────── if ! command -v apt-get >/dev/null 2>&1; then echo "error: this script currently supports Debian/Ubuntu only" @@ -55,7 +58,7 @@ echo "✓ apt packages up to date" # ── 2. mise ───────────────────────────────────────────────────────── if ! command -v mise >/dev/null 2>&1; then echo "↓ installing mise via the official installer..." - curl -fsSL https://mise.run | sh + curl_fetch https://mise.run | sh # The installer puts mise at $HOME/.local/bin/mise; ensure we can # invoke it for the remainder of this script run. export PATH="$HOME/.local/bin:$PATH" diff --git a/tools/setup/macos.sh b/tools/setup/macos.sh index 1b26ae16..efd07eaa 100755 --- a/tools/setup/macos.sh +++ b/tools/setup/macos.sh @@ -23,6 +23,9 @@ set -euo pipefail REPO_ROOT="$(cd "$(dirname "$0")/../.." && pwd)" SETUP_DIR="$REPO_ROOT/tools/setup" +# shellcheck source=common/curl-fetch.sh +source "$SETUP_DIR/common/curl-fetch.sh" + # ── 1. Xcode Command Line Tools ───────────────────────────────────── if ! xcode-select -p >/dev/null 2>&1; then echo "↓ installing Xcode Command Line Tools (non-interactive)..." @@ -37,7 +40,7 @@ echo "✓ Xcode CLT at $(xcode-select -p 2>/dev/null || echo 'pending user confi # ── 2. Homebrew ───────────────────────────────────────────────────── if ! command -v brew >/dev/null 2>&1; then echo "↓ installing Homebrew..." - /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)" + /bin/bash -c "$(curl_fetch https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)" # Ensure brew is on PATH for the remainder of this script run. if [ -x /opt/homebrew/bin/brew ]; then eval "$(/opt/homebrew/bin/brew shellenv)" From d1a4371e175751ef640731c2548e1c516e6cda1f Mon Sep 17 00:00:00 2001 From: Aaron Stainback Date: Mon, 27 Apr 2026 23:27:11 -0400 Subject: [PATCH 2/8] install: split curl-fetch into file + stream variants; fix command-sub-under-set-e; role-refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #75 review-thread fixes: P1 (--retry-all-errors on streamed installer): split the helper into two variants because the safe retry policy differs by output mode. - curl_fetch → file-output (`-o`/`--output` to disk). Keeps --retry-all-errors because curl restarts the file from scratch on retry, so partial-output replay cannot happen. - curl_fetch_stream → streamed-to-shell installers (`curl ... | sh`, `bash -c "$(curl ...)"`). Drops --retry-all-errors so curl only retries on transient conditions where nothing has been written yet — avoids partial-script replay risk. Updated linux.sh (mise install), macos.sh (Homebrew install), and elan.sh (Lean toolchain install) to use curl_fetch_stream. verifiers.sh stays on curl_fetch because it writes to file (`-o "$dest.part"`). macos.sh command-sub-under-set-e: `bash -c "$(curl_fetch ...)"` silently swallowed curl failures because the outer `bash -c` succeeds with empty input and exits 0. Capture to a named variable first (`HOMEBREW_INSTALLER="$(curl_fetch_stream ...)"`) so a curl failure aborts the variable assignment, which set -e *does* propagate. Idempotence comment fix: previous wording said re-sourcing "overwrites the function body" but the guard at the top of the function-definition block prevents redefinition. Reworded to match what the code does. Personal-name attribution → role-refs per BP-NN role-refs rule (current-state code surfaces use role labels, not contributor names). Quote attributions kept verbatim for substantive content. Files: tools/setup/common/curl-fetch.sh (split into two functions), linux.sh + macos.sh + common/elan.sh (call-site updates), common/verifiers.sh (role-ref normalization, no behaviour change). --- tools/setup/common/curl-fetch.sh | 108 ++++++++++++++++++++++--------- tools/setup/common/elan.sh | 6 +- tools/setup/common/verifiers.sh | 13 ++-- tools/setup/linux.sh | 6 +- tools/setup/macos.sh | 9 ++- 5 files changed, 102 insertions(+), 40 deletions(-) diff --git a/tools/setup/common/curl-fetch.sh b/tools/setup/common/curl-fetch.sh index df28b094..3c9f4ed8 100644 --- a/tools/setup/common/curl-fetch.sh +++ b/tools/setup/common/curl-fetch.sh @@ -5,68 +5,114 @@ # # WHY # === -# Aaron 2026-04-28: *"curl 502 pattern i mean why should a PR -# ever fail for this? our code does not handle the retries -# already?"* — exactly: external-infra failures (upstream -# package mirrors returning 5xx, transient curl-22 / network -# blips) should be absorbed by retry-with-backoff inside the -# install path, not kicked out to a workflow-rerun discipline. +# Human maintainer 2026-04-28: external-infra failures +# (upstream package mirrors returning 5xx, transient curl-22 +# / network blips) should be absorbed by retry-with-backoff +# inside the install path, not kicked out to a workflow-rerun +# discipline. Quote: *"curl 502 pattern i mean why should a +# PR ever fail for this? our code does not handle the retries +# already?"* # # This file centralises the retry policy so every call site # uses the same flags. Previously the policy was inlined in # `tools/setup/common/verifiers.sh` and missing entirely from # `linux.sh` (mise install), `macos.sh` (Homebrew install), -# and `elan.sh` (Lean toolchain install). Aaron 2026-04-28 -# follow-up: *"sounds like a common helper would help too -# rather than copy/paste."* +# and `elan.sh` (Lean toolchain install). Follow-up framing: +# *"sounds like a common helper would help too rather than +# copy/paste."* +# +# TWO FUNCTIONS — file-output vs streamed +# ======================================= +# Two helpers are exposed because the safe retry policy +# differs by output mode. Code review on the original single- +# function form flagged the partial-output-replay risk for +# pipe-to-shell call sites: +# +# curl_fetch — for file-output downloads +# (`-o`/`--output` to disk). Uses +# `--retry-all-errors` because curl +# restarts the file from scratch on +# retry, so partial-output replay +# cannot happen. +# +# curl_fetch_stream — for streamed-to-shell installers +# (`curl ... | sh`, `bash -c "$(curl +# ...)"`). Uses bare `--retry` +# (without `--retry-all-errors`) so +# curl will only retry on transient +# conditions where nothing has been +# written yet — avoiding the risk of +# a partial install script being +# piped, then the retry's full output +# appended on top. # # USAGE # ===== -# Source this file, then call `curl_fetch` with the same -# args you'd pass to curl. The function prepends the retry -# flags transparently: +# Source this file, then call the appropriate helper: # # # shellcheck source=/dev/null # source "$REPO_ROOT/tools/setup/common/curl-fetch.sh" -# curl_fetch https://mise.run | sh -# /bin/bash -c "$(curl_fetch https://example.com/install.sh)" +# +# # File output (safe with full retries): # curl_fetch --output "$path" "$url" # +# # Streamed pipe (must use the stream variant): +# curl_fetch_stream https://example.com/install.sh | sh +# +# # Command substitution (capture to var first; see +# # IDEMPOTENCE / SET-E note below): +# INSTALLER="$(curl_fetch_stream https://example.com/install.sh)" +# /bin/bash -c "$INSTALLER" +# # RETRY POLICY (rationale) # ======================== # --retry 5 — five attempts total. Empirically -# covers all transient upstream blips -# this install path has hit during -# 2026-04 sessions. +# covers the upstream 5xx blips +# this install path has hit. # --retry-delay 2 — 2-second base delay between retries. -# Short enough to not penalise CI when -# the retry succeeds; long enough to -# let upstream recover from a brief -# surge. -# --retry-all-errors — retry on ALL transient errors, -# including HTTP 4xx that curl -# would otherwise pass through. -# (Default `--retry` only retries on -# select transient errors; setup -# installers benefit from the broader -# surface.) +# --retry-all-errors — (file-output only) retry on ALL +# transient errors including HTTP +# 5xx without `Retry-After`. Curl's +# default `--retry` only retries +# connect / DNS / 408 / 429 / 5xx- +# with-Retry-After. # -fsSL — original flags preserved: # -f: fail on HTTP errors # -s: silent (no progress meter) # -S: show errors when silent # -L: follow redirects # +# COMMAND-SUBSTITUTION + SET-E +# ============================ +# Calling `bash -c "$(curl_fetch_stream ...)"` directly will +# silently swallow curl failures under `set -e` because the +# outer `bash -c` succeeds with an empty string. The pattern +# we use everywhere is: capture to a named variable first, +# then exec the variable. That way a curl failure aborts the +# variable assignment, which set -e *does* honour. +# # IDEMPOTENCE # =========== -# Repeated source of this file overwrites the function body -# (no append, no accumulation). Safe to source from multiple -# scripts within the same install run. +# Re-sourcing this file is a no-op once the functions are +# defined: the guard at the top of the function-definition +# block skips redefinition if `curl_fetch` is already in +# the shell. Safe to source from multiple scripts within +# the same install run. # Guard: only define once per shell to avoid noise on multi-source. if ! declare -F curl_fetch >/dev/null 2>&1; then +# File-output variant — safe with --retry-all-errors because +# curl restarts the output file from scratch on each retry. curl_fetch() { curl -fsSL --retry 5 --retry-delay 2 --retry-all-errors "$@" } +# Streamed variant — bare --retry only, no --retry-all-errors, +# to avoid the partial-output-replay risk on pipe-to-shell or +# command-substitution-into-bash use sites. +curl_fetch_stream() { + curl -fsSL --retry 5 --retry-delay 2 "$@" +} + fi diff --git a/tools/setup/common/elan.sh b/tools/setup/common/elan.sh index b9174e1f..9d09d750 100755 --- a/tools/setup/common/elan.sh +++ b/tools/setup/common/elan.sh @@ -15,7 +15,11 @@ source "$(dirname "${BASH_SOURCE[0]}")/curl-fetch.sh" if ! command -v elan >/dev/null 2>&1; then echo "↓ installing elan (Lean 4 toolchain manager)..." - curl_fetch https://raw.githubusercontent.com/leanprover/elan/master/elan-init.sh \ + # Use the stream variant (bare --retry only, no + # --retry-all-errors) — the curl output is piped directly + # into sh, and partial-output replay on retry would be a + # supply-chain hazard. + curl_fetch_stream https://raw.githubusercontent.com/leanprover/elan/master/elan-init.sh \ | sh -s -- -y --default-toolchain none fi diff --git a/tools/setup/common/verifiers.sh b/tools/setup/common/verifiers.sh index 7b99b408..4984c05f 100755 --- a/tools/setup/common/verifiers.sh +++ b/tools/setup/common/verifiers.sh @@ -4,9 +4,9 @@ # (TLC, Alloy) to `tools/tla/` and `tools/alloy/` respectively. # # Manifest format: `/ ` per line, -# comments starting with `#`. Per Aaron's round-29 call we do not -# verify checksums (trust-on-first-use); when upstream provides a -# published SHA256SUMS we may revisit. +# comments starting with `#`. Per the human maintainer's round-29 +# call we do not verify checksums (trust-on-first-use); when +# upstream provides a published SHA256SUMS we may revisit. # # This replaces the legacy tools/install-verifiers.sh in the same # commit (greenfield — no alias per GOVERNANCE.md §24 fallout). @@ -38,7 +38,7 @@ grep -vE '^(#|$)' "$MANIFEST" | while IFS= read -r line; do mkdir -p "$(dirname "$dest")" if [ -f "$dest" ]; then # Trust-on-first-use: if the file exists we assume it's intact. - # Per Aaron's round-29 call we do not re-verify content. + # Per the human maintainer's round-29 call we do not re-verify content. echo "✓ $target already present" else # Download to a .part suffix then atomic-rename. Protects against @@ -54,8 +54,9 @@ grep -vE '^(#|$)' "$MANIFEST" | while IFS= read -r line; do # handles the retry: 5 attempts, 2-4-8-16-32 s exponential # backoff, --retry-all-errors so 4xx/5xx errors retry too. # Keeps -fsSL semantics — fail at the end if all 5 attempts - # hit the same transient. (Aaron 2026-04-28: helper extracted - # from copy-pasted call sites; was previously inline here.) + # hit the same transient. (Human maintainer 2026-04-28 + # framing: helper extracted from copy-pasted call sites; was + # previously inline here.) echo "↓ downloading $target from $url" curl_fetch -o "$dest.part" "$url" mv "$dest.part" "$dest" diff --git a/tools/setup/linux.sh b/tools/setup/linux.sh index 434e5df3..905ff7ca 100755 --- a/tools/setup/linux.sh +++ b/tools/setup/linux.sh @@ -58,7 +58,11 @@ echo "✓ apt packages up to date" # ── 2. mise ───────────────────────────────────────────────────────── if ! command -v mise >/dev/null 2>&1; then echo "↓ installing mise via the official installer..." - curl_fetch https://mise.run | sh + # Use the stream variant (bare --retry only, no + # --retry-all-errors) — the curl output is piped directly + # into sh, and partial-output replay on retry would be a + # supply-chain hazard. + curl_fetch_stream https://mise.run | sh # The installer puts mise at $HOME/.local/bin/mise; ensure we can # invoke it for the remainder of this script run. export PATH="$HOME/.local/bin:$PATH" diff --git a/tools/setup/macos.sh b/tools/setup/macos.sh index efd07eaa..956606ff 100755 --- a/tools/setup/macos.sh +++ b/tools/setup/macos.sh @@ -40,7 +40,14 @@ echo "✓ Xcode CLT at $(xcode-select -p 2>/dev/null || echo 'pending user confi # ── 2. Homebrew ───────────────────────────────────────────────────── if ! command -v brew >/dev/null 2>&1; then echo "↓ installing Homebrew..." - /bin/bash -c "$(curl_fetch https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)" + # Capture to a named variable first so a curl failure aborts + # the variable assignment under `set -e` — `/bin/bash -c + # "$(failing_curl)"` otherwise silently runs an empty string + # and exits 0. Use the stream variant (no --retry-all-errors) + # because the captured output is then piped into bash; we + # don't want a partial-script retry-replay scenario. + HOMEBREW_INSTALLER="$(curl_fetch_stream https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)" + /bin/bash -c "$HOMEBREW_INSTALLER" # Ensure brew is on PATH for the remainder of this script run. if [ -x /opt/homebrew/bin/brew ]; then eval "$(/opt/homebrew/bin/brew shellenv)" From 9dba6123bf5615d02a0b4940dae9dbbc417862d1 Mon Sep 17 00:00:00 2001 From: Aaron Stainback Date: Mon, 27 Apr 2026 23:36:35 -0400 Subject: [PATCH 3/8] install: harden curl-fetch idempotence guard against caller-env collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P2 (review thread on PR #75): the original guard `if ! declare -F curl_fetch >/dev/null 2>&1` checks whether the `curl_fetch` function already exists. If a caller environment has an unrelated `curl_fetch` function defined (rare but possible — shells with chatty .bashrc, cross-script aliasing, test fixtures), the guard skips BOTH our definitions, leaving `curl_fetch_stream` undefined. The streamed callers (linux.sh, macos.sh, elan.sh) then fail at runtime with `curl_fetch_stream: command not found`. Switched to a file-local sentinel variable (`_ZETA_CURL_FETCH_LOADED`) so the guard answers "did this file load?" instead of "does that name exist?" Collisions in the caller environment can no longer suppress our definitions. Verified two scenarios: 1. Re-source idempotence: sourcing twice still defines both helpers and the second source is a no-op. 2. Collision resilience: a pre-existing `curl_fetch` function in the caller env does NOT block our `curl_fetch_stream` definition. --- tools/setup/common/curl-fetch.sh | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tools/setup/common/curl-fetch.sh b/tools/setup/common/curl-fetch.sh index 3c9f4ed8..9567e152 100644 --- a/tools/setup/common/curl-fetch.sh +++ b/tools/setup/common/curl-fetch.sh @@ -93,14 +93,22 @@ # # IDEMPOTENCE # =========== -# Re-sourcing this file is a no-op once the functions are -# defined: the guard at the top of the function-definition -# block skips redefinition if `curl_fetch` is already in -# the shell. Safe to source from multiple scripts within -# the same install run. +# Re-sourcing this file is a no-op once both helpers are +# loaded. The guard uses a file-local sentinel variable +# (`_ZETA_CURL_FETCH_LOADED`) instead of probing for an +# existing `curl_fetch` function: a function-name probe +# would silently skip BOTH definitions if the caller +# environment already had an unrelated `curl_fetch` +# function, leaving `curl_fetch_stream` undefined and +# breaking the streamed callers (`linux.sh` / `macos.sh` +# / `elan.sh`) at runtime with `curl_fetch_stream: +# command not found`. Sentinel-based guarding ties the +# load decision to "did this file load?" instead of "does +# that name exist?" — collisions in the caller environment +# can no longer accidentally suppress our definitions. -# Guard: only define once per shell to avoid noise on multi-source. -if ! declare -F curl_fetch >/dev/null 2>&1; then +if [[ -z "${_ZETA_CURL_FETCH_LOADED:-}" ]]; then +_ZETA_CURL_FETCH_LOADED=1 # File-output variant — safe with --retry-all-errors because # curl restarts the output file from scratch on each retry. From cf85ea90c0094faa2e58384cecf900db967f5ce0 Mon Sep 17 00:00:00 2001 From: Aaron Stainback Date: Mon, 27 Apr 2026 23:37:13 -0400 Subject: [PATCH 4/8] =?UTF-8?q?install:=20drop=20ZETA=20prefix=20from=20id?= =?UTF-8?q?empotence=20sentinel=20=E2=80=94=20internal-only,=20consistent?= =?UTF-8?q?=20with=20curl=5Ffetch=20naming?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aaron 2026-04-28 (echoing the earlier `zeta_curl_fetch` decision on the function name): "_ZETA_CURL do you need the prefix?" Same calibration: the sentinel is purely internal (never appears at any call site, never crosses the file boundary in any meaningful way), and the function names already dropped the ZETA prefix for the same reason — no real collision risk for what is in practice a 3-word descriptive identifier (`_CURL_FETCH_LOADED`). Re-verified both invariants after rename: - Re-source idempotence: second source is a no-op. - Caller-env collision resilience: pre-existing `curl_fetch` in the caller env doesn't suppress our `curl_fetch_stream`. --- tools/setup/common/curl-fetch.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/setup/common/curl-fetch.sh b/tools/setup/common/curl-fetch.sh index 9567e152..6e293fce 100644 --- a/tools/setup/common/curl-fetch.sh +++ b/tools/setup/common/curl-fetch.sh @@ -95,7 +95,7 @@ # =========== # Re-sourcing this file is a no-op once both helpers are # loaded. The guard uses a file-local sentinel variable -# (`_ZETA_CURL_FETCH_LOADED`) instead of probing for an +# (`_CURL_FETCH_LOADED`) instead of probing for an # existing `curl_fetch` function: a function-name probe # would silently skip BOTH definitions if the caller # environment already had an unrelated `curl_fetch` @@ -107,8 +107,8 @@ # that name exist?" — collisions in the caller environment # can no longer accidentally suppress our definitions. -if [[ -z "${_ZETA_CURL_FETCH_LOADED:-}" ]]; then -_ZETA_CURL_FETCH_LOADED=1 +if [[ -z "${_CURL_FETCH_LOADED:-}" ]]; then +_CURL_FETCH_LOADED=1 # File-output variant — safe with --retry-all-errors because # curl restarts the output file from scratch on each retry. From fcc447b2f2648c0ba94af27710aa7650d1e71ff2 Mon Sep 17 00:00:00 2001 From: Aaron Stainback Date: Tue, 28 Apr 2026 00:00:17 -0400 Subject: [PATCH 5/8] =?UTF-8?q?fix(shellcheck):=20suppress=20SC1091=20at?= =?UTF-8?q?=20curl-fetch=20source=20sites=20=E2=80=94=20CI=20runs=20withou?= =?UTF-8?q?t=20-x?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tools/setup/common/elan.sh | 1 + tools/setup/common/verifiers.sh | 1 + tools/setup/linux.sh | 1 + tools/setup/macos.sh | 1 + 4 files changed, 4 insertions(+) diff --git a/tools/setup/common/elan.sh b/tools/setup/common/elan.sh index 9d09d750..777a10c1 100755 --- a/tools/setup/common/elan.sh +++ b/tools/setup/common/elan.sh @@ -11,6 +11,7 @@ set -euo pipefail # shellcheck source=curl-fetch.sh +# shellcheck disable=SC1091 # CI runs without -x; source path verified in tools/setup/common/curl-fetch.sh source "$(dirname "${BASH_SOURCE[0]}")/curl-fetch.sh" if ! command -v elan >/dev/null 2>&1; then diff --git a/tools/setup/common/verifiers.sh b/tools/setup/common/verifiers.sh index 4984c05f..a17d2432 100755 --- a/tools/setup/common/verifiers.sh +++ b/tools/setup/common/verifiers.sh @@ -14,6 +14,7 @@ set -euo pipefail # shellcheck source=curl-fetch.sh +# shellcheck disable=SC1091 # CI runs without -x; source path verified in tools/setup/common/curl-fetch.sh source "$(dirname "${BASH_SOURCE[0]}")/curl-fetch.sh" REPO_ROOT="$(cd "$(dirname "$0")/../../.." && pwd)" diff --git a/tools/setup/linux.sh b/tools/setup/linux.sh index 905ff7ca..0d8c387e 100755 --- a/tools/setup/linux.sh +++ b/tools/setup/linux.sh @@ -24,6 +24,7 @@ REPO_ROOT="$(cd "$(dirname "$0")/../.." && pwd)" SETUP_DIR="$REPO_ROOT/tools/setup" # shellcheck source=common/curl-fetch.sh +# shellcheck disable=SC1091 # CI runs without -x; source path verified in tools/setup/common/curl-fetch.sh source "$SETUP_DIR/common/curl-fetch.sh" # ── Detect apt availability (Debian/Ubuntu) ───────────────────────── diff --git a/tools/setup/macos.sh b/tools/setup/macos.sh index 956606ff..b5d118fc 100755 --- a/tools/setup/macos.sh +++ b/tools/setup/macos.sh @@ -24,6 +24,7 @@ REPO_ROOT="$(cd "$(dirname "$0")/../.." && pwd)" SETUP_DIR="$REPO_ROOT/tools/setup" # shellcheck source=common/curl-fetch.sh +# shellcheck disable=SC1091 # CI runs without -x; source path verified in tools/setup/common/curl-fetch.sh source "$SETUP_DIR/common/curl-fetch.sh" # ── 1. Xcode Command Line Tools ───────────────────────────────────── From ef2e266a6f633240e2c1f83ad00ff90f083ab6f3 Mon Sep 17 00:00:00 2001 From: Aaron Stainback Date: Tue, 28 Apr 2026 00:14:13 -0400 Subject: [PATCH 6/8] install: drop --retry from curl_fetch_stream entirely + capture-exit-check in macos.sh + B-0063 follow-up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P0 review on PR #75 (5 threads) correctly identified that even bare `curl --retry` (without --retry-all-errors) can retry after bytes have already been written to stdout. Once piped to the consumer (`sh`, `bash -c "$(...)"`), the partial bytes cannot be un-received, and the retry concatenates partial+full script content → corrupted shell input. My PR #75 prior framing ("stream variant is safe via no --retry-all-errors") was wrong on this point. The structural fix: 1. Drop `--retry` from `curl_fetch_stream` entirely. Streamed installers fail-fast on transient errors; user re-runs install.sh. No retry, no replay hazard. 2. macos.sh capture pattern hardened — explicit if-fails-then- exit + empty-content check, instead of relying on `set -e` to catch failures inside command substitution (`set -e` is not reliably triggered by `$(...)` failures without `inherit_errexit`, per codex P0 + bash semantics). 3. Doc-comments updated everywhere to reflect the no-retry stance + flag the `set -e` + command-sub limitation honestly. The PROPER structural fix — download-to-temp + size-check + checksum-verify-when-available + buffered exec — is tracked as `docs/backlog/P1/B-0063-streamed-installer-download-to-temp- checksum-pattern-codex-p0-pr-75.md` with explicit done-criteria and per-call-site work plan. NOT a form-4 deferral; a concrete per-row backlog file (per `feedback_bulk_resolve_is_not_answer_recurring_pattern_aaron_2026_04_28.md`). Why P1 not P0 for B-0063: this commit closes the immediate retry-replay hazard. The download-to-temp pattern adds defense-in-depth (size guard, buffered exec, checksum hooks) but the immediate concern is gone. Files: tools/setup/common/curl-fetch.sh (drop --retry from stream variant + doc-rewrite), tools/setup/macos.sh (capture-exit-check pattern), tools/setup/linux.sh (comment update), tools/setup/common/elan.sh (comment update), docs/backlog/P1/B-0063-*.md (concrete tracking row). --- ...to-temp-checksum-pattern-codex-p0-pr-75.md | 130 ++++++++++++++++++ tools/setup/common/curl-fetch.sh | 71 +++++++--- tools/setup/common/elan.sh | 11 +- tools/setup/linux.sh | 11 +- tools/setup/macos.sh | 24 +++- 5 files changed, 212 insertions(+), 35 deletions(-) create mode 100644 docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md diff --git a/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md b/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md new file mode 100644 index 00000000..5c73a357 --- /dev/null +++ b/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md @@ -0,0 +1,130 @@ +--- +id: B-0063 +priority: P1 +status: open +title: Streamed-installer download-to-temp + checksum-verify pattern — replace pipe-to-shell for upstream installers (Codex P0 on PR #75) +tier: install-path-supply-chain +effort: M +ask: codex P0 review on PR #75 (5 threads on tools/setup/common/curl-fetch.sh, macos.sh, linux.sh, elan.sh) flagging that even bare `curl --retry` can retry after bytes are written to stdout, leaving the shell consumer with partial+full concatenated script content. PR #75 immediate fix: drop --retry from `curl_fetch_stream` entirely. This row tracks the structurally safe replacement. +created: 2026-04-28 +last_updated: 2026-04-28 +composes_with: [B-0060] +tags: [install-path, supply-chain, upstream-installers, codex-p0, pr-75, streaming-vs-buffered, checksum] +--- + +# Streamed-installer download-to-temp + checksum-verify pattern + +PR #75 landed a `curl_fetch_stream` helper for upstream +installer URLs (`mise.run`, Homebrew install.sh, elan-init.sh) +that originally used bare `curl --retry`. Codex P0 review +correctly identified that **any retry on a streamed-to-shell +pipe is unsafe**: curl can retry after partial bytes have +already been piped to the consumer, and the consumer +(`sh`, `bash -c "$(...)"`) cannot un-receive what it has +read. The retry then concatenates with the partial, +producing corrupted script content that may re-execute +commands or run truncated halves. + +The PR #75 immediate fix dropped `--retry` from the stream +variant entirely. Streamed installers now fail-fast on +transient errors; user re-runs `install.sh`. That removes +the unsafe retry behaviour but does **not** add the safety +margin that the file-output variant has via +`--retry-all-errors`. + +This row tracks the structurally safe replacement. + +## The proper structural fix + +For each streamed-installer call site, replace the +`curl_fetch_stream URL | sh` pattern with: + +```bash +# 1. Download to a temp file (curl_fetch is safe with retries +# because the file restarts from scratch on retry). +TEMP="$(mktemp)" +trap 'rm -f "$TEMP"' EXIT +curl_fetch -o "$TEMP" "$URL" + +# 2. Verify size + (when upstream publishes one) checksum. +if [ ! -s "$TEMP" ]; then + echo "error: installer empty after download; refusing to exec" >&2 + exit 1 +fi +# (when upstream publishes a SHA256SUMS or .sig:) +EXPECTED_SHA="" +if [ "$(sha256sum "$TEMP" | awk '{print $1}')" != "$EXPECTED_SHA" ]; then + echo "error: installer checksum mismatch; refusing to exec" >&2 + exit 1 +fi + +# 3. Exec the verified file. +bash "$TEMP" # or: bash "$TEMP" -- ...args +``` + +This pattern: +- Gets full retry coverage (file-output variant). +- Buffers the entire installer before exec. +- Allows checksum verification when upstream publishes one. +- Fail-fasts on empty / truncated / corrupted downloads. +- Composes with the existing `curl_fetch` helper without + introducing new flags. + +## Per-call-site work + +1. **`tools/setup/macos.sh` Homebrew install.** Currently: + `HOMEBREW_INSTALLER="$(curl_fetch_stream URL)"; bash -c + "$HOMEBREW_INSTALLER"`. Convert to download-to-temp + + exec. Homebrew does NOT publish a SHA256 of install.sh; + document the size-check-only stance + the upstream + project as the trust anchor. +2. **`tools/setup/linux.sh` mise install.** Currently: + `curl_fetch_stream https://mise.run | sh`. Convert. + mise.run publishes signed releases — investigate + whether install.sh ships in a verifiable form. +3. **`tools/setup/common/elan.sh` Lean-toolchain install.** + Currently: `curl_fetch_stream URL | sh -s -- -y + --default-toolchain none`. Convert. elan-init.sh ships + from `raw.githubusercontent.com/leanprover/elan/master/` + — investigate whether tag-pinned versions are available + (move from `master` to a pinned tag if so). + +## Done-criteria + +- [ ] All three call sites converted to download-to-temp + + size-check + exec pattern. +- [ ] For each call site, the upstream's verifiability + story is documented in the inline comment (signed + release / SHA256SUMS / project-as-trust-anchor with + no upstream verification). +- [ ] `tools/setup/common/curl-fetch.sh` doc-comments + reflect the new pattern; the `curl_fetch_stream` + function may then be DEPRECATED-WARNING-on-use or + removed entirely. +- [ ] CI passes on macOS-26, ubuntu-24.04, ubuntu-24.04-arm + with the new pattern. + +## Why P1 (not P0) + +The PR #75 fix (`curl_fetch_stream` without `--retry`) closes +the immediate retry-replay hazard. The structural fix here +adds defense-in-depth (checksum verification, size guard, +buffered exec) but the immediate hazard is already gone. +P1 = within 2-3 rounds, not an absolute ship-blocker. + +## Composes with + +- **PR #75** — the originating thread cluster lives at + cids 3151434903 / 3151434921 / 3151434929 / 3151434941 / + 3151434956 (now resolved with this row as the concrete + tracking destination per + `feedback_bulk_resolve_is_not_answer_recurring_pattern_aaron_2026_04_28.md`). +- **B-0060** — human-lineage / external-anchor backfill + (the chosen verification mechanisms here should cite + external prior art / RFCs / vendor docs). +- The original cost-driver memory: + `feedback_structural_fix_beats_process_discipline_velocity_multiplier_aaron_2026_04_28.md` + (curl 502 → retry-on-file-output was the structural fix + that closed the original failure class; this is the + follow-on hardening that covers the streamed-shell + pattern that the original fix didn't address). diff --git a/tools/setup/common/curl-fetch.sh b/tools/setup/common/curl-fetch.sh index 6e293fce..87161d7b 100644 --- a/tools/setup/common/curl-fetch.sh +++ b/tools/setup/common/curl-fetch.sh @@ -37,14 +37,16 @@ # # curl_fetch_stream — for streamed-to-shell installers # (`curl ... | sh`, `bash -c "$(curl -# ...)"`). Uses bare `--retry` -# (without `--retry-all-errors`) so -# curl will only retry on transient -# conditions where nothing has been -# written yet — avoiding the risk of -# a partial install script being -# piped, then the retry's full output -# appended on top. +# ...)"`). NO --retry. Codex P0 review +# on PR #75 confirmed: even bare +# `--retry` (without `--retry-all- +# errors`) can retry after bytes have +# already been written to stdout, and +# the consumer cannot un-receive piped +# bytes. Streamed installers fail-fast +# on transient errors; the user re-runs +# install.sh. Proper download-to-temp +# hardening tracked as B-0063. # # USAGE # ===== @@ -82,14 +84,22 @@ # -S: show errors when silent # -L: follow redirects # -# COMMAND-SUBSTITUTION + SET-E -# ============================ -# Calling `bash -c "$(curl_fetch_stream ...)"` directly will -# silently swallow curl failures under `set -e` because the -# outer `bash -c` succeeds with an empty string. The pattern -# we use everywhere is: capture to a named variable first, -# then exec the variable. That way a curl failure aborts the -# variable assignment, which set -e *does* honour. +# COMMAND-SUBSTITUTION + SET-E (caveat per codex review) +# ====================================================== +# bash's `errexit` (`set -e`) is NOT reliably triggered by a +# command substitution that fails without producing output — +# in some bash versions (especially without `inherit_errexit` +# enabled) `VAR="$(failing_cmd)"` leaves `VAR=""` and continues. +# Our macos.sh capture pattern (`HOMEBREW_INSTALLER="$(curl_ +# fetch_stream ...)"; /bin/bash -c "$HOMEBREW_INSTALLER"`) +# survives the most common failure mode (curl errors out +# without producing output → empty var → `bash -c ""` runs +# nothing) but it's NOT a defense against partial-byte +# corruption. The proper fix is download-to-temp + +# checksum-verify, tracked as B-0063. The current pattern is +# a small improvement over the prior `bash -c "$(curl ...)"` +# direct form (which silently ran whatever partial output +# survived); it is NOT the structurally safe form. # # IDEMPOTENCE # =========== @@ -116,11 +126,32 @@ curl_fetch() { curl -fsSL --retry 5 --retry-delay 2 --retry-all-errors "$@" } -# Streamed variant — bare --retry only, no --retry-all-errors, -# to avoid the partial-output-replay risk on pipe-to-shell or -# command-substitution-into-bash use sites. +# Streamed variant — NO --retry, NO --retry-all-errors. +# +# Codex P0 review on PR #75 surfaced that even bare `curl +# --retry` (without --retry-all-errors) can still retry after +# bytes have been written to stdout: the connect error happens +# mid-transfer, curl resets the input but the bytes already +# piped into the consumer (`sh`, `bash -c "$(...)"`) cannot be +# un-written. The consumer then sees concatenated partial+full +# script content, which can re-execute commands or run +# truncated halves. There is no curl-flag combination that +# gives both retry-on-transient AND safe-restart-on-streamed- +# stdout — those are mutually exclusive without an +# intermediate buffer. +# +# Therefore this variant ships WITHOUT retries. Streamed +# installer failures (mise.run / Homebrew / elan) bubble up +# as install errors; the user re-runs install.sh. +# +# The proper structural fix — download to a temp file with +# `curl_fetch` (file-output), checksum-verify if available, +# then `bash /dev/null 2>&1; then echo "↓ installing elan (Lean 4 toolchain manager)..." - # Use the stream variant (bare --retry only, no - # --retry-all-errors) — the curl output is piped directly - # into sh, and partial-output replay on retry would be a - # supply-chain hazard. + # Use the stream variant (NO --retry, NO --retry-all-errors). + # Codex P0 review on PR #75: even bare `--retry` can retry + # after bytes are piped to sh, and the consumer cannot + # un-receive piped bytes. Streamed installers fail-fast on + # transient errors; user re-runs install.sh. Proper + # download-to-temp + checksum-verify hardening tracked as + # B-0063. curl_fetch_stream https://raw.githubusercontent.com/leanprover/elan/master/elan-init.sh \ | sh -s -- -y --default-toolchain none fi diff --git a/tools/setup/linux.sh b/tools/setup/linux.sh index 0d8c387e..8bd6c63f 100755 --- a/tools/setup/linux.sh +++ b/tools/setup/linux.sh @@ -59,10 +59,13 @@ echo "✓ apt packages up to date" # ── 2. mise ───────────────────────────────────────────────────────── if ! command -v mise >/dev/null 2>&1; then echo "↓ installing mise via the official installer..." - # Use the stream variant (bare --retry only, no - # --retry-all-errors) — the curl output is piped directly - # into sh, and partial-output replay on retry would be a - # supply-chain hazard. + # Use the stream variant (NO --retry, NO --retry-all-errors). + # Codex P0 review on PR #75: even bare `--retry` can retry + # after bytes are piped to sh, and the consumer cannot + # un-receive piped bytes. Streamed installers fail-fast on + # transient errors; user re-runs install.sh. Proper + # download-to-temp + checksum-verify hardening tracked as + # B-0063. curl_fetch_stream https://mise.run | sh # The installer puts mise at $HOME/.local/bin/mise; ensure we can # invoke it for the remainder of this script run. diff --git a/tools/setup/macos.sh b/tools/setup/macos.sh index b5d118fc..8207900d 100755 --- a/tools/setup/macos.sh +++ b/tools/setup/macos.sh @@ -41,13 +41,23 @@ echo "✓ Xcode CLT at $(xcode-select -p 2>/dev/null || echo 'pending user confi # ── 2. Homebrew ───────────────────────────────────────────────────── if ! command -v brew >/dev/null 2>&1; then echo "↓ installing Homebrew..." - # Capture to a named variable first so a curl failure aborts - # the variable assignment under `set -e` — `/bin/bash -c - # "$(failing_curl)"` otherwise silently runs an empty string - # and exits 0. Use the stream variant (no --retry-all-errors) - # because the captured output is then piped into bash; we - # don't want a partial-script retry-replay scenario. - HOMEBREW_INSTALLER="$(curl_fetch_stream https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)" + # Capture to a named variable + check exit code explicitly. + # bash's `set -e` is not reliably triggered by a failing + # command substitution without `inherit_errexit`; codex P0 + # review on PR #75 flagged this. Pattern: capture, check + # length, exec only on non-empty + curl-success. The proper + # fix (download-to-temp + checksum-verify) is tracked as + # B-0063; this is a small-improvement-not-structurally-safe + # form acknowledged in tools/setup/common/curl-fetch.sh + # COMMAND-SUBSTITUTION + SET-E section. + if ! HOMEBREW_INSTALLER="$(curl_fetch_stream https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"; then + echo "error: failed to fetch Homebrew installer; check network and re-run install.sh" >&2 + exit 1 + fi + if [ -z "$HOMEBREW_INSTALLER" ]; then + echo "error: Homebrew installer was empty; refusing to exec" >&2 + exit 1 + fi /bin/bash -c "$HOMEBREW_INSTALLER" # Ensure brew is on PATH for the remainder of this script run. if [ -x /opt/homebrew/bin/brew ]; then From 98962e059ef0d19b4ffbedf8a1506df15e638231 Mon Sep 17 00:00:00 2001 From: Aaron Stainback Date: Tue, 28 Apr 2026 00:22:53 -0400 Subject: [PATCH 7/8] fix(markdownlint): MD032 blank line before bullet list in B-0063 --- ...installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md b/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md index 5c73a357..8219454c 100644 --- a/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md +++ b/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md @@ -63,6 +63,7 @@ bash "$TEMP" # or: bash "$TEMP" -- ...args ``` This pattern: + - Gets full retry coverage (file-output variant). - Buffers the entire installer before exec. - Allows checksum verification when upstream publishes one. From dfc29472cfb650beeb3cea5fbba41a362c381630 Mon Sep 17 00:00:00 2001 From: Aaron Stainback Date: Tue, 28 Apr 2026 01:52:11 -0400 Subject: [PATCH 8/8] fix(pr-75): address Copilot review threads (3 form-1 fixes) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - curl-fetch.sh header: was misleading ("uniform retry behaviour during install" implied all curl usage retries). Now explicitly distinguishes file-output (retries-enabled) vs streamed (no retries) and warns readers not to assume. - curl-fetch.sh COMMAND-SUBSTITUTION + SET-E section: out-of-date description ("survives by `bash -c \"\"` running nothing") replaced with the actual current macos.sh behavior (two-gate check: `if !` on assignment exit catches curl failure, secondary empty-string check catches the rare curl-exit-0-with-empty- output case, both produce hard `exit 1`). - B-0063 backlog row: `sha256sum` checksum example wasn't cross-platform (macOS ships `shasum -a 256` but not `sha256sum`). Now uses detect-and-dispatch: sha256sum → shasum -a 256 → openssl dgst -sha256. The 4th Copilot thread (P0 claim that `if ! var="$(cmd)"` doesn't catch cmd failure) is empirically wrong on bash 3.2.57 + bash 5.x — verified via `bash -c 'if ! x="$(false)"; then echo CAUGHT; fi'` prints CAUGHT. Closing as form-2 (already-addressed) with the test result in the thread reply; the macos.sh code is already double-safe (if-not gate + empty-string gate). Co-Authored-By: Claude Opus 4.7 --- ...to-temp-checksum-pattern-codex-p0-pr-75.md | 14 +++++- tools/setup/common/curl-fetch.sh | 50 ++++++++++++++----- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md b/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md index 8219454c..c0120e16 100644 --- a/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md +++ b/docs/backlog/P1/B-0063-streamed-installer-download-to-temp-checksum-pattern-codex-p0-pr-75.md @@ -53,7 +53,19 @@ if [ ! -s "$TEMP" ]; then fi # (when upstream publishes a SHA256SUMS or .sig:) EXPECTED_SHA="" -if [ "$(sha256sum "$TEMP" | awk '{print $1}')" != "$EXPECTED_SHA" ]; then +# Cross-platform SHA-256: macOS ships `shasum -a 256` (Perl +# script in /usr/bin) but not `sha256sum`; Linux has both; +# `openssl dgst -sha256` works everywhere openssl is +# available. Detect-and-dispatch keeps the install path +# 4-shell-portable per Otto-235. +if command -v sha256sum >/dev/null 2>&1; then + ACTUAL_SHA="$(sha256sum "$TEMP" | awk '{print $1}')" +elif command -v shasum >/dev/null 2>&1; then + ACTUAL_SHA="$(shasum -a 256 "$TEMP" | awk '{print $1}')" +else + ACTUAL_SHA="$(openssl dgst -sha256 "$TEMP" | awk '{print $NF}')" +fi +if [ "$ACTUAL_SHA" != "$EXPECTED_SHA" ]; then echo "error: installer checksum mismatch; refusing to exec" >&2 exit 1 fi diff --git a/tools/setup/common/curl-fetch.sh b/tools/setup/common/curl-fetch.sh index 87161d7b..13deca52 100644 --- a/tools/setup/common/curl-fetch.sh +++ b/tools/setup/common/curl-fetch.sh @@ -1,7 +1,25 @@ #!/usr/bin/env bash # -# tools/setup/common/curl-fetch.sh — sourceable helper for -# fetching URLs with uniform retry behaviour during install. +# tools/setup/common/curl-fetch.sh — sourceable helpers for +# fetching URLs during install. +# +# Two helpers with DIFFERENT retry semantics by output mode: +# - curl_fetch — file-output downloads. `--retry 5` +# + `--retry-all-errors` (safe because +# curl restarts the file from scratch +# on retry). +# - curl_fetch_stream — streamed-to-shell installers +# (`curl ... | sh`, `bash -c +# "$(curl ...)"`). NO retries. Streamed +# retry is unsafe — partial bytes +# already piped to the consumer cannot +# be un-received. Streamed installers +# fail-fast on transient errors; +# caller re-runs install.sh. +# Do NOT assume all curl usage in this repo is retried — +# only the `curl_fetch` (file-output) variant retries. See +# the per-function comments below + B-0063 for the +# download-to-temp structural fix to the streamed case. # # WHY # === @@ -90,16 +108,24 @@ # command substitution that fails without producing output — # in some bash versions (especially without `inherit_errexit` # enabled) `VAR="$(failing_cmd)"` leaves `VAR=""` and continues. -# Our macos.sh capture pattern (`HOMEBREW_INSTALLER="$(curl_ -# fetch_stream ...)"; /bin/bash -c "$HOMEBREW_INSTALLER"`) -# survives the most common failure mode (curl errors out -# without producing output → empty var → `bash -c ""` runs -# nothing) but it's NOT a defense against partial-byte -# corruption. The proper fix is download-to-temp + -# checksum-verify, tracked as B-0063. The current pattern is -# a small improvement over the prior `bash -c "$(curl ...)"` -# direct form (which silently ran whatever partial output -# survived); it is NOT the structurally safe form. +# Our macos.sh capture pattern uses an explicit two-gate +# approach: `if ! HOMEBREW_INSTALLER="$(curl_fetch_stream +# ...)"; then exit 1; fi` (catches curl failure via the +# if-not test on the assignment's exit status — verified on +# bash 3.2.57 / 5.x: `if ! x="$(false)"; then echo CAUGHT; +# fi` does print CAUGHT) PLUS a secondary `[ -z +# "$HOMEBREW_INSTALLER" ] && exit 1` empty-string check. +# Network errors trigger the first gate (curl-22 / curl-6 / +# HTTP-non-2xx via `-fsSL`); the unreachable case where curl +# exits 0 but produces empty output is caught by the second +# gate. Either failure produces a hard `exit 1` with a +# diagnostic message — never falls through to `bash -c ""`. +# This is NOT a defense against partial-byte corruption — +# proper fix is download-to-temp + checksum-verify, tracked +# as B-0063. The current pattern is a small improvement over +# the prior `bash -c "$(curl ...)"` direct form (which +# silently ran whatever partial output survived); it is NOT +# the structurally safe form. # # IDEMPOTENCE # ===========