fix: Bugbot findings from promotion PR #397

internal/cli/prepare_host.go

@@ -20,29 +20,41 @@ import (
 // early with a clear error rather than a confusing failure deep in the installer).
 var prepareHostUserRe = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]{0,31}$`)
 
-// prepareHostInstallerCmd runs the official installer's admin-only prepare-host
-// step. Like `tracebloc upgrade`, this deliberately delegates to the verified
-// installer (cosign-checked) instead of re-implementing any privileged host prep
-// in the CLI — the privileged surface stays in one audited place.
+// installerRunScript builds the bash program that downloads the cosign-verified
+// installer to a temp file and runs THAT — optionally with a subcommand (e.g.
+// "prepare-host"). Shared by `tracebloc upgrade` (no subcommand, full install)
+// and `tracebloc prepare-host`, so both stay on one download-then-execute idiom.
 //
-// We download the installer to a temp file and run THAT, rather than
-// `curl | bash -s`. Two reasons, both Bugbot #394:
-//   - stdin: with `curl | bash -s`, the inner bash reads its *program* from the
+// We run a downloaded FILE rather than `curl … | bash`. Two reasons, both Bugbot
+// (#394, #397):
+//   - stdin: with `curl | bash`, the inner bash reads its *program* from the
 //     pipe, so the installer's stdin is no longer the terminal. Any interactive
-//     prompt in prepare-host (e.g. which non-admin user gets runtime access)
-//     would get EOF. Running a downloaded file leaves stdin on the TTY.
+//     prompt (sign-in, or which non-admin user gets runtime access) would get
+//     EOF. Running a downloaded file leaves stdin on the TTY.
 //   - fail-closed: `set -e` + `curl -o` makes a failed download (network/DNS/HTTP
 //     error) abort with a non-zero status instead of silently running nothing.
 //     (`curl | bash` swallowed this — bash read empty stdin and exited 0.)
 //
 // The temp file is removed on exit. The URL comes from installerURL (doctor.go)
-// so the automated download can't drift from the manual hint / other bootstrap
-// copy (Bugbot #394).
-const prepareHostInstallerCmd = `set -e
+// so every installer path shares one source and can't drift (Bugbot #394/#397).
+func installerRunScript(subcommand string) string {
+	run := `bash "$tmp"`
+	if subcommand != "" {
+		run += " " + subcommand
+	}
+	return `set -e
 tmp="$(mktemp)"
 trap 'rm -f "$tmp"' EXIT
 curl -fsSL ` + installerURL + ` -o "$tmp"
-bash "$tmp" prepare-host`
+` + run
+}
+
+// prepareHostInstallerCmd runs the official installer's admin-only prepare-host
+// step. Like `tracebloc upgrade`, this deliberately delegates to the verified
+// installer (cosign-checked) instead of re-implementing any privileged host prep
+// in the CLI — the privileged surface stays in one audited place. See
+// installerRunScript for why we download-then-execute rather than pipe.
+var prepareHostInstallerCmd = installerRunScript("prepare-host")
 
 // prepareHostManualHint is the copy-pasteable command we show if the automated
 // run fails. Built from installCmd (doctor.go) — the single shared bootstrap
@@ -100,13 +112,13 @@ func prepareHostCmd(ctx context.Context) *exec.Cmd {
 	return c
 }
 
-// prepareHostInterrupted reports whether the installer run ended because the user
-// aborted, so the caller can exit quietly (130) instead of framing it as a failed
-// install. ctx.Err() catches a cancel the signal handler already propagated — but
-// on a terminal Ctrl-C the child can die and c.Run() can return BEFORE
-// NotifyContext flips ctx.Err() (a race), so also treat bash's 130 (128+SIGINT)
-// exit as an interrupt (Bugbot #394).
-func prepareHostInterrupted(ctx context.Context, runErr error) bool {
+// installerRunInterrupted reports whether an installer run (upgrade or
+// prepare-host) ended because the user aborted, so the caller can exit quietly
+// (130) instead of framing it as a failed install. ctx.Err() catches a cancel
+// the signal handler already propagated — but on a terminal Ctrl-C the child can
+// die and c.Run() can return BEFORE NotifyContext flips ctx.Err() (a race), so
+// also treat bash's 130 (128+SIGINT) exit as an interrupt (Bugbot #394/#397).
+func installerRunInterrupted(ctx context.Context, runErr error) bool {
 	if ctx.Err() != nil {
 		return true
 	}
@@ -189,7 +201,7 @@ host, so it's safe to run on a shared machine. Safe to re-run.`,
 				// User aborted (Ctrl-C) or the parent context was cancelled: exit
 				// quietly with 130 like the other cancellable paths, not a scary
 				// "prepare-host didn't complete — retry" (Bugbot #394).
-				if prepareHostInterrupted(ctx, err) {
+				if installerRunInterrupted(ctx, err) {
 					return &exitError{code: exitInterrupted}
 				}
 				return &exitError{code: exitFailure, err: fmt.Errorf("prepare-host didn't complete (%w). You can run the installer directly:\n    %s", err, prepareHostManualHint(user))}

internal/cli/prepare_host_test.go

@@ -54,11 +54,11 @@ func TestPrepareHostCmdStaysInForegroundGroup(t *testing.T) {
 // A user abort must be detected as an interrupt even when NotifyContext hasn't
 // flipped ctx.Err() yet — bash exits 130 on SIGINT and c.Run() can return first
 // (Bugbot #394). A genuine failure (exit 1) must NOT be treated as an interrupt.
-func TestPrepareHostInterrupted(t *testing.T) {
+func TestInstallerRunInterrupted(t *testing.T) {
 	// Cancelled context → interrupt regardless of the run error.
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel()
-	if !prepareHostInterrupted(ctx, errors.New("boom")) {
+	if !installerRunInterrupted(ctx, errors.New("boom")) {
 		t.Error("a cancelled context must be treated as an interrupt")
 	}
 
@@ -68,13 +68,13 @@ func TestPrepareHostInterrupted(t *testing.T) {
 	// Live context + bash exit 130 (128+SIGINT) → interrupt (the Ctrl-C race).
 	if err := exec.Command("bash", "-c", "exit 130").Run(); err == nil {
 		t.Fatal("expected a non-nil error from exit 130")
-	} else if !prepareHostInterrupted(context.Background(), err) {
+	} else if !installerRunInterrupted(context.Background(), err) {
 		t.Error("exit 130 with a live context must be treated as an interrupt")
 	}
 	// Live context + a normal failure (exit 1) → NOT an interrupt.
 	if err := exec.Command("bash", "-c", "exit 1").Run(); err == nil {
 		t.Fatal("expected a non-nil error from exit 1")
-	} else if prepareHostInterrupted(context.Background(), err) {
+	} else if installerRunInterrupted(context.Background(), err) {
 		t.Error("exit 1 must NOT be treated as an interrupt")
 	}
 }

internal/cli/upgrade.go

@@ -18,17 +18,18 @@ const upgradeCmdName = "upgrade"
 // installer ourselves: it re-downloads + cosign-verifies the release, replaces
 // the CLI, and upgrades the secure environment's services to match — so we never
 // re-implement (and risk diverging from) the installer's signature verification.
-// The short `curl … | bash` form (not `bash <(curl …)`) works from any shell
-// (see the shell-safety fix in the docs).
+// We download-then-execute the installer (installerRunScript, shared with
+// prepare-host) rather than `curl … | bash`: piping makes the inner bash read
+// its program from the pipe, stealing the installer's stdin so its interactive
+// prompts (sign-in, etc.) can't read the TTY. The URL is derived from
+// installerURL (doctor.go) so it can't drift from the other installer paths
+// (Bugbot #397).
 //
 // Windows is different: we do NOT self-exec there. A running .exe is locked, so
 // install.ps1's Move-Item can't overwrite the very binary we're running, and
 // install.ps1 is CLI-only (no environment). So on Windows we print the command
 // for the user to run in a fresh shell instead of pretending to self-update.
-const (
-	upgradeInstallerCmdUnix    = "curl -fsSL https://tracebloc.io/i.sh | bash"
-	upgradeInstallerCmdWindows = "irm https://github.com/tracebloc/cli/releases/latest/download/install.ps1 | iex"
-)
+const upgradeInstallerCmdWindows = "irm https://github.com/tracebloc/cli/releases/latest/download/install.ps1 | iex"
 
 // upgradePlan is how `upgrade` proceeds on a given OS: either exec the installer
 // (Unix) or just show the user a command to run (Windows). manual is the
@@ -46,14 +47,16 @@ func upgradePlanFor(goos string) upgradePlan {
 	if goos == "windows" {
 		return upgradePlan{exec: false, manual: upgradeInstallerCmdWindows}
 	}
-	// `-o pipefail` so a failed `curl` fails the whole pipeline: without it the
-	// trailing `bash` exits 0 on empty stdin and we'd report a successful upgrade
-	// having installed nothing.
+	// Download-then-execute the verified installer (installerRunScript, shared
+	// with prepare-host): its `set -e`+`curl -o` fails closed on a bad download,
+	// and running a file (not a pipe) keeps the installer's stdin on the TTY. The
+	// manual hint reuses installCmd (doctor.go), the shared bootstrap idiom, so
+	// the URL has a single source.
 	return upgradePlan{
 		exec:   true,
 		name:   "bash",
-		args:   []string{"-o", "pipefail", "-c", upgradeInstallerCmdUnix},
-		manual: upgradeInstallerCmdUnix,
+		args:   []string{"-c", installerRunScript("")},
+		manual: installCmd,
 	}
 }
 
@@ -102,9 +105,16 @@ Safe to run anytime; safe to re-run.`,
 
 			// Stream the installer straight to the user's terminal, and keep
 			// stdin wired so its interactive prompts (sign-in, etc.) still work.
-			c := exec.CommandContext(cmd.Context(), plan.name, plan.args...)
+			ctx := cmd.Context()
+			c := exec.CommandContext(ctx, plan.name, plan.args...)
 			c.Stdin, c.Stdout, c.Stderr = os.Stdin, os.Stdout, os.Stderr
 			if err := c.Run(); err != nil {
+				// User aborted (Ctrl-C) or the parent context was cancelled: exit
+				// quietly with 130 like prepare-host, not a scary "upgrade didn't
+				// complete — retry" (Bugbot #397).
+				if installerRunInterrupted(ctx, err) {
+					return &exitError{code: exitInterrupted}
+				}
 				return &exitError{code: exitFailure, err: fmt.Errorf(
 					"upgrade didn't complete (%w). You can run the installer directly:\n    %s",
 					err, plan.manual)}

internal/cli/upgrade_test.go

@@ -49,7 +49,9 @@ func TestUpgradeCmd_HelpMentionsVerified(t *testing.T) {
 
 // TestUpgradePlanFor_PerOS: Windows must NOT self-exec (a running .exe is locked
 // and install.ps1 is CLI-only) — it only prints the manual command. Unix runs
-// the installer with pipefail so a failed curl fails the whole pipeline.
+// the verified installer via the shared download-then-execute script, never
+// `curl | bash` (which would steal the installer's stdin), and reuses installCmd
+// for the manual hint so the URL has one source (Bugbot #397).
 func TestUpgradePlanFor_PerOS(t *testing.T) {
 	win := upgradePlanFor("windows")
 	if win.exec {
@@ -68,14 +70,22 @@ func TestUpgradePlanFor_PerOS(t *testing.T) {
 			t.Errorf("%s upgrade should exec bash, got exec=%v name=%q", goos, p.exec, p.name)
 		}
 		joined := strings.Join(p.args, " ")
-		if !strings.Contains(joined, "-o pipefail") {
-			t.Errorf("%s upgrade must set pipefail so a failed curl fails the run: %q", goos, joined)
+		// Download-then-execute, not `curl | bash`: piping steals the installer's
+		// stdin so its interactive prompts can't read the TTY (Bugbot #397).
+		if strings.Contains(joined, "| bash") || strings.Contains(joined, "|bash") {
+			t.Errorf("%s upgrade must not pipe the installer into bash (steals its stdin): %q", goos, joined)
+		}
+		// `set -e` + `curl -o <file>` fails closed on a bad download instead of
+		// running an empty script and reporting a phantom success.
+		if !strings.Contains(joined, "set -e") || !strings.Contains(joined, "curl") || !strings.Contains(joined, "-o ") {
+			t.Errorf("%s upgrade must download the installer to a file (set -e + curl -o): %q", goos, joined)
 		}
 		if !strings.Contains(joined, "i.sh") {
 			t.Errorf("%s upgrade must run i.sh: %q", goos, joined)
 		}
-		if p.manual != upgradeInstallerCmdUnix {
-			t.Errorf("%s manual hint = %q, want %q", goos, p.manual, upgradeInstallerCmdUnix)
+		// Manual hint reuses installCmd (single URL source), not a re-hardcoded URL.
+		if p.manual != installCmd {
+			t.Errorf("%s manual hint = %q, want installCmd %q", goos, p.manual, installCmd)
 		}
 	}
 }