fix(installer): a stopped Docker forced the admin path, and the copy said we were starting it

[LukasWodka]

Closes#740.

The trap

A Mac with Docker Desktop installed but not running — the state every machine is in after a reboot — could not be set up by a user who couldn't give an administrator password. And the installer told them it was starting Docker.

· Docker isn't running yet — starting it, then checking your environment.
Container runtime none usable as this user –
→ Install tier Tier 2 — Docker isn't running; start/install Docker Desktop (needs admin once).
[13:20:57] step b: admin check passed
[13:20:57] FAILED at .../common.sh:527 — exit 1 — command: sudo -n true

1. assess.sh printed "Docker isn't running yet — starting it" and fell through to return 0. Nothing started anything.
2. The probe found no usable runtime → macOS → Tier 2, the admin-password path.
3. preflight_sudo asked for the password. Dead end.

The circularity is the bug: the onlyopen -a Docker in the tree lives in install_docker_desktop, behindpreflight_sudo. The installer could only start Docker after taking a password it needed solely because Docker wasn't started.

With Docker running, PROBE_RUNTIME_USABLE=1 wins first and unconditionally in _classify_from_probes → Tier 0, where the admin gate and preflight_sudo never run. Tier 0 (client#704) was unreachable for exactly the machines it was written for.

The fix

_assess_handle_runtime_down either does the thing or doesn't claim it:

• macOS — open -a Docker (a GUI launch, no privileges whatsoever), wait bounded, let the probe reclassify to Tier 0. If it doesn't come up: say so, and name the cost — the fallback needs a password. That sentence is the one the old copy hid.
• Anywhere else — starting the daemon needs root, so don't claim to be starting it.

_wait_for_docker is extracted so the nudge and install_docker_desktop share one loop instead of growing a second that drifts.

The report named a healthy check

Stopped at common.sh:527 (exit 1) — command: sudo -n true

Line 527 is _real_sudo; sudo -n true is the passwordless-sudo probe, which is supposed to fail on a normal Mac — the host check prints its failure as a normal row ("sudo needs a password"). The real cause was never recorded, because error() calls exit, and exit fires no ERR trap, so TB_ERR_* still held the last benign probe.

A report that confidently names a working line is worse than one that names nothing. error() now records its own caller location and message first — BASH_SOURCE[1], not [0], which would name common.sh for every refusal in the tree — with an explicit exit code so it can't latch whatever $? happened to be.

Verified end-to-end against the failing scenario:

	last recorded cause
before	cmd=sudo -n true
after	cmd=error: Could not obtain administrator privileges (sudo authentication failed)

The coverage gap that let it ship

preflight_sudo has four branches. Three were tested — root, no sudo binary, passwordless. The ordinary Mac — an admin user whose sudo wants a password — was exercised by nothing: the tier-2 lifecycle tests stub preflight_sudo out via _tier0_mocks, and common.bats never covered it. Both branches are now tested, each asserting its specific message rather than a bare non-zero.

Tests

Full suite: 1102 passed, 0 failed (see the correction comment — my first report of this was read off a stale background run that exited 1; two failures, both mine, are fixed in the follow-up commit). shellcheck --severity=error and check-style.sh clean.

scripts/manifest.sha256 is regenerated in this PR: it carries the checksums of the installer libs edited here and is the installer's own integrity check over what it downloads, so a stale one breaks verification at install time.

Mutations, each asserted to have applied:

mutation	reddens
error() stops recording	3 tests
error() records BASH_SOURCE[0]	1
the old "starting it" copy restored	7

Three things this cost, worth recording

• One assertion passed for the wrong reason.assess.bats wasn't sourcing setup-macos.sh, so run _try_start_docker_desktop exited 127 (command not found) and a bare -ne 0 accepted it — the bare-assertRaises trap. Now run -1 asserts the one code.
• A branch was unreachable on any macOS dev box._try_start_docker_desktop's "not installed" path can't be hit on a machine that has Docker — i.e. the machine of whoever changes this code. _docker_app_installed is now its own function and the test stubs it, so it isn't a CI-only branch.
• An assertion failed against a log that plainly contained the string. bats runs a preprocessed copy (N-common.bats.src) and rewrites those paths back in its own output — so cat printed something the file does not contain, and every assertion written against what I saw on screen failed. The needle is now what is actually there, with a comment saying why.

Workaround until this ships

Start Docker Desktop, wait for "running", re-run. The installer then takes Tier 0 and never asks for a password.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes first-run classification and macOS Docker startup before the admin gate, plus fatal-error logging; scope is installer shell only with heavy test coverage, but misclassification or a wedged-daemon edge case could still affect real installs.

Overview
Fixes the case where Docker Desktop is installed but stopped on macOS: the assess gate used to print that it was “starting” Docker while doing nothing, then the probe still saw no runtime and the install fell into Tier 2 (admin password)—even though the only open -a Docker lived behind that same prompt.

Runtime-down handling adds _assess_handle_runtime_down: on Darwin, when Docker.app is present, it calls _try_start_docker_desktop (GUI open -a Docker, up to ~60s, bounded probes). Messaging either matches success or admits failure and that continuing may need an administrator password; non-macOS and Macs without Docker Desktop no longer claim to start anything.

Shared Docker liveness centralizes checks in _docker_answers (bounded docker info) and _wait_for_docker (wall-clock deadline); install-time Docker wait is guarded with || true so a timeout does not abort under set -e before user-facing guidance.

Failure reporting: error() records the caller site and message via _record_err before exit, so install logs no longer blame the last benign sudo -n true probe when sudo auth actually failed.

Bats coverage for runtime-down copy, bounded routing, error()/preflight_sudo, and updated manifest.sha256 checksums for touched installer libs.

^{Reviewed by Cursor Bugbot for commit e8cfbcb. Bugbot is set up for automated code reviews on this repo. Configure here.}

[LukasWodka]

Correction — my "1102 passed, 0 failed" line was wrong

I read that off a background suite run that had finished before the last round of test edits. A second, later run against the final tree exited 1, with two failures, both mine. Fixed in the follow-up commit; the PR body is updated.

1. scripts/manifest.sha256 was stale — the important one

gen-manifest.bats "a clean tree passes --check" failed because this PR edits common.sh, assess.sh and setup-macos.sh, and the manifest carries their checksums. That manifest is the installer's own integrity check over the libs it downloads, so merging it stale would have broken verification at install time on every platform — a considerably worse bug than the one this PR fixes.

Regenerated with scripts/gen-manifest.sh; --check now passes.

2. Two advisory assertions

bats-hygiene.bats caught two ! grep -qF "LAUNCHED" <<<"$output" lines without || return 1. Both are the last statement in their test, so they did determine the result — but the repo's rule is unconditional for good reason, and I shouldn't ship the exact class of defect this PR spends its tests arguing against. Hardened.

Worth naming the process error rather than just the two bugs: I reported a green suite from a run that predated the code it was testing. That run didn't capture its exit code in a form I actually checked — I read a tail showing the last test passing and treated it as the suite passing. Those are not the same thing, which is the entire reason the exit code exists.

[LukasWodka]

Fixed in 616e967 — correct finding, and a pointed one.

Why it mattered more than it looks:_assess_runtime_down classifies runtime-down from _bounded's 124 — i.e. from the bounded probe having already timed out on a wedged daemon. The new path then re-entered an unboundeddocker info. So it hung on exactly the input that routes to it. This branch exists because a message promised an action the code never took; unbounded, it promised the action and then froze.

Fix: one _docker_answers helper beside _bounded, defaulting to the same 10s as TB_ASSESS_DOCKER_TIMEOUT (same question, same daemon, should not disagree). Wired into all four probes on this path, including install_docker_desktop's two that bracket the extracted loop.

_wait_for_docker also moved from a poll count to a wall-clock deadline: with each probe bounded, counting iterations would make "20 polls" mean 60s against a live daemon and ~260s against the wedged one it exists to survive. $SECONDS keeps the caller's budget true either way.

Three tests, four mutations, each anchor confirmed to apply (7 passing → 6, 6, 4, 6). The fourth sets the bound to 0 — still routed through _bounded, bound disabled: covered-looking and not covered.

Two flaws the fix exposed in my own tests, worth recording:

• The first assertion read a stub's stdout, but _docker_answers redirects its whole call to /dev/null — invisible either way, and it passed regardless. Now a marker file.
• A bounded probe runs through timeout(1) as an external process, so a docker() shell-function stub stops intercepting; that broke a previously-passing test. And _bounded runs the command bare when neither timeout nor gtimeout is installed, so such a stub works on some hosts and not others — a test whose meaning depends on the machine. Stubs now sit at _docker_answers, the seam present everywhere.

Scope: 16 more unbounded probes remain (preflight.sh, setup-linux.sh, diagnose.sh, and setup-macos.sh's other branches), plus probe.sh hand-rolling its own three-branch copy of _bounded. Filed as #744 with a proposed check-style.sh guard — since Bugbot carries a learned rule for this, it recurs, and the org standard says a recurring review finding becomes tooling rather than a repeated comment.

Full suite green, manifest regenerated.

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit e872350. Configure here.}

[LukasWodka]

All four addressed in 8096970. Three were real, and worth naming rather than filing under "review feedback".

The regression this branch introduced

_wait_for_docker returns non-zero on timeout, and install_docker_desktop called it as a bare statement under set -e — so the script exited there, before the whale-icon guidance and the deliberate error() that exist precisely for "Docker didn't come up". A silent death replacing a helpful message, in a branch about messages that lie.

The old inline loop ended on printf/tput and always fell through; extracting it moved the timeout's status somewhere errexit could see. || true now guards it, with the if ! _docker_answers below remaining the verdict. Asserted by a source-level guard — driving install_docker_desktop end to end would need brew, hdiutil and a DMG.

The same defect, one layer down

Both of the assess findings are this branch's own bug reproduced inside the fix for it:

• On a Colima-only or headless Mac, _assess_handle_runtime_down announced "starting it" and only then discovered there was no Docker Desktop to start — and the failure copy went on to tell someone without Docker Desktop to open Docker Desktop. _docker_app_installed is now part of the condition, not just of the nudge.
• The off-macOS branch said "start it, then re-run this installer", then returned 0 and carried on into the privileged flow — guidance contradicting the next thing on screen. It now names its continue, as the macOS failure branch already did.

Extracting and re-routing a path is exactly where message-and-behaviour drift gets reintroduced. Good catches.

The host-dependent test

Correct, and CI had just failed on it independently. It stubbed docker(), but a bounded probe runs through timeout(1) as an external process — so on the ubuntu runner, which has a live daemon, the real probe answered and _try_start_docker_desktop returned 0 at "already up" without reaching the branch under test. It passed locally only because macOS ships no timeout(1), so _bounded fell through to the bare call and the stub worked.

Fixed with this file's existing convention, _bounded() { shift; "$@"; }, which the _assess_runtime_down tests have used all along. Verified the other way round too: Docker is running on the machine I fixed it on, so the test now passes under the same condition that failed on CI.

I had documented this exact trap in #744 and in the preceding commit message, and then shipped it anyway.

Verification

Mutations, each anchor confirmed to apply (13 passing → 12 each):

mutation	reddens
bare _wait_for_docker under set -e	1
announce without checking the app exists	1
drop the off-macOS "Continuing" line	1

Full suite 1141 passed, 0 failed. Manifest regenerated, style guard clean.

[saadqbal]

Clean — I tried to break it and could not. Verified independently on the branch rather than taking the report: shasum -a 256 -c scripts/manifest.sha256 passes, assess.bats + common.bats are 157/157 both with and without a timeout(1) on PATH (the host-dependence that bit you twice — it is genuinely gone now that the stubs sit at _docker_answers/_bounded), shellcheck --severity=error and check-style are clean, and every new path behaves identically under bash 3.2. The error() fix does exactly what the table claims: seeding TB_ERR_* with a benign sudo -n true and then refusing gives FAILED at <caller>:8 exit 1 cmd=error: Could not obtain administrator privileges — caller frame, right code, benign probe displaced.

I also checked the two places the extraction could plausibly still bite and they are fine: cmd && return 0 in _try_start_docker_desktop does not trip errexit (the left operand of an AND list is exempt), and _wait_for_docker is ERR-trap-safe in both callers because one is behind || and the other is inside an if.

Two stale-comment notes inline. Neither blocks.

[LukasWodka]

Thanks for going at it independently rather than reading the report back — the bash 3.2 pass and the with/without-timeout(1) run are the two things I could not have checked convincingly from my own machine, and the cmd && return 0 / _wait_for_docker errexit analysis is the part I would have most liked a second pair of eyes on.

Both notes fixed in e8cfbcb. Neither is a nit as far as I am concerned — they are the exact defect this branch exists to remove, a statement that no longer describes the behaviour, so they belong in this PR rather than a follow-up.

assess.sh's non-mutating header

You are right that the danger is not the stale sentence but which way the next reader resolves the contradiction — and "reads the ban as lifted and starts the k3d cluster from cluster-stopped" is the failure I would not have predicted and is plainly the worse of the two.

Restated as non-mutating with exactly ONE narrow exception, and I wrote down the four properties that make this one admissible so the boundary is checkable rather than a matter of taste:

• unprivileged — a GUI app launch as the current user, no sudo, nothing mistakable for the privileged path
• idempotent — launching a running Docker Desktop is a no-op, so a re-run cannot compound
• bounded — a wall-clock deadline, every liveness probe through _docker_answers
• load-bearing — without it a stopped-but-installed Docker classifies Tier 2 and demands a password to start a runtime that is already installed

Plus the explicit non-consequence you asked for: this does not lift the ban, cluster-stopped still only prints, and a second exception is the moment to ask whether this file is still the right home.

TB_ERR_CMD

Also right, and sharper than a nit — the comment does not merely go stale, it becomes a false safety guarantee at precisely the moment someone is writing a new error message. It now documents both producers and their different guarantees (ERR trap → BASH_COMMAND, unexpanded; error() → "error: $*", interpolated, because a deliberate refusal fires no ERR trap), notes that it reaches LOG_FILE twice, and carries the rule forward: never interpolate a credential into an error() message — name the path, not the value.

I considered making that a machine check, since the standard says a load-bearing claim should be one. Rejected, and worth saying why rather than leaving it silent: the only name-shaped hit across all 58 interpolating error() calls is error "…($cred_file)" — a path, i.e. the safe pattern the comment recommends. A name-based guard would fire solely on that false positive, and silencing it needs a hand-maintained allowlist — the restate-the-rule defect. All noise, no signal. If you would rather have it anyway, say so and I will file it.

Comments only, no behaviour change. Full suite 1141 passed, 0 failed; manifest regenerated, style guard clean.

[saadqbal]

Both notes addressed, and better than I asked for. Carving the exception out by its four properties — unprivileged, idempotent, bounded, load-bearing — and then explicitly re-stating that cluster-stopped is still banned is the version that survives the next reader; “if you are adding a second exception, question whether this file is still the right place” is the right guard rail. Same for TB_ERR_CMD: naming the two producers and turning the lost guarantee into a rule (never interpolate a credential, say “the credential file at $path”) is more useful than the blanket claim it replaces.

Re-verified at e8cfbcb: the diff is comment-only, manifest -c passes, assess.bats + common.bats 157/157, shellcheck --severity=error and check-style clean.

One cosmetic slip from the edit itself, inline. Not blocking.

[saadqbal]

Agreed on both, and your reasoning on the guard is right as far as it goes — I checked it rather than taking it: 59 interpolating error() calls, and the only credential-shaped name across all of them is $cred_file, a path. A bare name-match guard would have exactly one hit and it would be a false positive, so declining it was the right call.

One shape does answer the allowlist objection though, if you want it: exempt the safe pattern in the rule rather than in a list — match credential-ish names but drop anything ending _file/_path/_dir. That is what your comment already tells people to write, so it is a rule, not a hand-maintained set.

grep -rinE '(^|[^_[:alnum:]])error "[^"]*\$\{?[a-z_]*(cred|token|passw|secret|key|auth)[a-z0-9_]*' scripts/ \
| grep -viE '\$\{?[a-z_][a-z0-9_]*_(file|path|dir)\b'

Zero hits on the tree at e8cfbcb, and it fires on error "…$TRACEBLOC_CLIENT_PASSWORD…". Worth a follow-up ticket if you agree; genuinely fine either way and it should not hold this PR. Approved as it stands — the only thing left is the orphaned sentence on assess.sh:36.

[LukasWodka]

/fr-pass

Advanced at @LukasWodka's explicit direction. Held in the automated pass for a verification gap (interactive/TTY, GPU-only, or journey-dependent while e2e journey is red — backend#2206); Lukas is accepting that gap for this card.