From 2e7fb844549969917b3567d29b3fe30c90d41fc7 Mon Sep 17 00:00:00 2001 From: Andre Padez Date: Thu, 27 Aug 2026 04:01:52 +0100 Subject: [PATCH] fix three officer-setup bugs found on a general review, unrelated to macOS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit env_get() never matched its own file. write_env() always writes OFFICERDEV_-prefixed keys, but env_get() compared against the bare name — verified against a real written .env: `env_get PORT` -> [], `env_get OFFICERDEV_PORT` -> [9000]. Every one of its 5 call sites uses the bare name, so this always came back empty. Two consequences: the "recover what earlier runs already decided" block never recovers anything — its own comment says it exists to fix "Build announced PUBLIC_URL not set on a machine whose .env had been written twenty minutes earlier", and that exact bug was still live — and re-running the Environment step showed the wrong default (9000, tailnet URL) instead of the machine's actual current config. Fixed to check the prefixed key first, falling back to the bare one for an old unprefixed file, matching the runtime's own `OFFICERDEV_X ?? X` priority. pg_password_from_env_file() returned success with an empty string when the compose .env existed but had no POSTGRES_PASSWORD= line. The caller (`pg_compose_exists && PG_EXISTING_PASSWORD="$(pg_password_from_env_file)"`) would then treat that as "reuse this password" and build POSTGRES_URL with a blank credential rather than falling through to provision fresh. Only reachable via a hand-edited or corrupted file — write_pg_compose always writes the line — but "silently reuse a blank password" was the wrong direction to fail in. Now returns 1 when the line is missing or empty. SETUP_DOCKER_NETWORK was read independently by both scripts, each defaulting to "services", but never actually persisted across the machine-setup -> officer-setup boundary — not one of the three keys load_machine_answers recognised. A customised bridge name from machine-setup would silently not carry over, and Proxy would land back on "services" regardless of what Docker was actually told to use. Now saved by machine-setup's save_answers(), recognised by officer-setup's load_machine_answers(), and PROXY_NET is re-derived from it right after that load — proxy.sh's own top-level assignment runs at source time, before the answers file could have populated the variable, same ordering issue ENV_PORT/ENV_PUBLIC_URL/POSTGRES_URL already had to solve. All three verified with isolated reproductions before and after: env_get against a real prefixed .env and a legacy bare one, pg_password_from_env_file against a normal and a corrupted compose .env, and the full save/load/re-derive chain for SETUP_DOCKER_NETWORK. bash -n clean on every touched file. Co-Authored-By: Claude Opus 5 --- scripts/setup/machine-setup/lib/base.sh | 6 ++++++ scripts/setup/officer-setup.sh | 7 +++++++ scripts/setup/officer-setup/lib/env.sh | 22 ++++++++++++++++++-- scripts/setup/officer-setup/lib/postgres.sh | 12 ++++++++++- scripts/setup/officer-setup/lib/preflight.sh | 7 +++++++ 5 files changed, 51 insertions(+), 3 deletions(-) diff --git a/scripts/setup/machine-setup/lib/base.sh b/scripts/setup/machine-setup/lib/base.sh index 15f4f702..ec209b5d 100644 --- a/scripts/setup/machine-setup/lib/base.sh +++ b/scripts/setup/machine-setup/lib/base.sh @@ -100,6 +100,11 @@ fail() { ANSWERS_FILE="${ANSWERS_FILE:-}" +# DOCKER_NETWORK included so officer-setup's Proxy step lands on the SAME bridge Docker +# was told to use here, rather than each script independently defaulting to "services" and +# happening to agree only because nobody has customised either yet. lib/docker.sh sets +# DOCKER_NETWORK at source time (SETUP_DOCKER_NETWORK env var, else "services"), so it is +# always defined by the time this runs, regardless of whether the Docker step itself ran. save_answers() { [[ -n "$ANSWERS_FILE" ]] || return 0 cat >"$ANSWERS_FILE" < [], `env_get OFFICERDEV_PORT` -> [9000], before this. +# Falls back to the bare key so an old, hand-written unprefixed file still works, same as the +# runtime does. env_get() { [[ -r "$(env_file)" ]] || return 0 - awk -F= -v k="$1" ' - $1 == k { + awk -F= -v k="$1" -v pk="OFFICERDEV_$1" ' + $1 == pk { v = substr($0, index($0, "=") + 1) gsub(/^"|"$/, "", v) print v exit + } + $1 == k && !found { + v = substr($0, index($0, "=") + 1) + gsub(/^"|"$/, "", v) + bare = v + found = 1 + } + END { + if (bare != "") print bare }' "$(env_file)" } diff --git a/scripts/setup/officer-setup/lib/postgres.sh b/scripts/setup/officer-setup/lib/postgres.sh index dddd20c4..1d7e19fe 100644 --- a/scripts/setup/officer-setup/lib/postgres.sh +++ b/scripts/setup/officer-setup/lib/postgres.sh @@ -237,9 +237,19 @@ ENVF return 0 } +# Failure has to mean failure. The bare awk one-liner this replaced printed nothing and +# still exited 0 when the file existed but had no POSTGRES_PASSWORD= line — so the caller's +# `pg_compose_exists && PG_EXISTING_PASSWORD="$(pg_password_from_env_file)"` treated that as +# success with an EMPTY password, and built POSTGRES_URL with a blank credential instead of +# falling through to provision fresh. Only reachable via a hand-edited/corrupted .env, since +# write_pg_compose always writes the line — but "reuse a blank password" is the wrong +# direction to fail in even for an edge case that rare. pg_password_from_env_file() { [[ -r "$(pg_env_file)" ]] || return 1 - awk -F= '/^POSTGRES_PASSWORD=/ { print substr($0, index($0, "=") + 1); exit }' "$(pg_env_file)" + local pw + pw="$(awk -F= '/^POSTGRES_PASSWORD=/ { print substr($0, index($0, "=") + 1); exit }' "$(pg_env_file)")" + [[ -n "$pw" ]] || return 1 + printf '%s' "$pw" } pg_compose_up() { as_owner "docker compose --project-directory '$(pg_service_dir)' up -d" /; } diff --git a/scripts/setup/officer-setup/lib/preflight.sh b/scripts/setup/officer-setup/lib/preflight.sh index 1b5570f9..9e12ec1c 100644 --- a/scripts/setup/officer-setup/lib/preflight.sh +++ b/scripts/setup/officer-setup/lib/preflight.sh @@ -32,6 +32,13 @@ load_machine_answers() { MACHINE_ROLE) if [[ -z "$MACHINE_ROLE" ]]; then MACHINE_ROLE="$value"; fi ;; SETUP_USERNAME) if [[ -z "$USERNAME" ]]; then USERNAME="$value"; fi ;; OFFICER_ROOT) if [[ -z "$OFFICER_ROOT" ]]; then OFFICER_ROOT="$value"; fi ;; + # The bridge machine-setup told Docker to use — proxy.sh's PROXY_NET otherwise + # defaults to "services" independently and can only agree with this by coincidence. + # PROXY_NET is a top-level assignment evaluated when proxy.sh is SOURCED, which + # happens before this function is ever called — officer-setup.sh re-derives it from + # SETUP_DOCKER_NETWORK right after calling this, same pattern as ENV_PORT/ + # ENV_PUBLIC_URL/POSTGRES_URL below it. + SETUP_DOCKER_NETWORK) if [[ -z "${SETUP_DOCKER_NETWORK:-}" ]]; then SETUP_DOCKER_NETWORK="$value"; fi ;; esac done <"$MACHINE_ANSWERS" return 0