diff --git a/scripts/require-bash.sh b/scripts/require-bash.sh new file mode 100644 index 000000000..82e7af3a6 --- /dev/null +++ b/scripts/require-bash.sh @@ -0,0 +1,42 @@ +# Refuse to run under a shell that is not bash, and say which one it is. +# +# Sourced as the first thing `start.sh` and `stop.sh` do, before either sets its options. Not +# executable and not a script anybody runs: it exits the shell that sourced it, which is the whole +# of what it is for. +# +# WHAT THIS IS ABOUT. `sh scripts/start.sh` overrides the `#!/usr/bin/env bash` line, and what it +# used to produce was exit 1 and not one character of output — no line number, no failing command, +# nothing to search for. On macOS `sh` IS bash, run in POSIX mode, where a failure these scripts +# survive under bash is fatal instead; the first setting read out of `.env` was enough to end the +# run. What somebody had to go on was the number 1, which reads as "this script is broken" rather +# than "run it the other way". +# +# EVERYTHING HERE IS POSIX SYNTAX ONLY, because the shell being warned about may not be bash at +# all. `set -o pipefail` is itself a bashism and a syntax error in dash, which is `sh` on most Linux +# distributions — so the refusal has to come before the `set` line in either caller, and without +# `local`, `[[` or `${!name}`. Anything bash-only written here would fail as a parse error in the +# one case it exists to explain. +# +# `SHELLOPTS` IS WHAT SEPARATES THE TWO BASHES. It is bash's own variable, absent in dash, and it +# lists `posix` exactly when bash was invoked as `sh`. A `BASH_VERSION` check alone cannot see that +# case, because bash-as-sh sets that too — which is the case on every Mac, so it is the case that +# actually happens. +openbot_wrong_shell="" +if [ -z "${BASH_VERSION:-}" ]; then + openbot_wrong_shell="a shell that is not bash" +else + case ":${SHELLOPTS:-}:" in + *:posix:*) openbot_wrong_shell="bash in POSIX mode, which is what \`sh\` is" ;; + esac +fi + +if [ -n "$openbot_wrong_shell" ]; then + # The fix and not only the fault. "Wrong shell" is not actionable to somebody who typed the only + # invocation they knew, so the line that follows is the one to retype. On stderr, so a caller + # reading the progress output still sees it. + printf '\033[31m%s\033[0m\n' "This script is bash, and it is being read by $openbot_wrong_shell." >&2 + printf '%s\n' "Run it as: bash scripts/$(basename "$0")" >&2 + exit 1 +fi + +unset openbot_wrong_shell diff --git a/scripts/start-restart-guard.test.ts b/scripts/start-restart-guard.test.ts index a450787d4..0e2bcb9fd 100644 --- a/scripts/start-restart-guard.test.ts +++ b/scripts/start-restart-guard.test.ts @@ -8,7 +8,26 @@ async function writeExecutable(path: string, contents: string) { await chmod(path, 0o755); } -async function runStartWithStaleServerProbe(status: 401 | 404) { +/** + * How this run differs from the ordinary one, which is the whole vocabulary these tests need. + * + * `shell` IS A PARAMETER BECAUSE THE INVOCATION IS PART OF WHAT IS UNDER TEST. `bash` is the + * supported one and the one the docs name; `sh` is the one somebody reaches for out of habit, and + * what it produced was exit 1 with no output at all. + * + * `omitFromEnv` IS THE OTHER HALF. Every key below has a default in the script, so a `.env` without + * one is an ordinary `.env` rather than a broken one — `.env.example` does not list `APP_PORT` at + * all. + */ +type Run = { + shell?: "bash" | "sh"; + omitFromEnv?: readonly string[]; +}; + +async function runStartWithStaleServerProbe( + status: 401 | 404, + { shell = "bash", omitFromEnv = [] }: Run = {}, +) { const root = await Bun.$`mktemp -d ${tmpdir()}/openbot-start-guard-XXXXXX`.text(); const directory = root.trim(); @@ -17,31 +36,34 @@ async function runStartWithStaleServerProbe(status: 401 | 404) { const logPath = join(directory, "pkill.log"); await mkdir(fakeBin, { recursive: true }); await mkdir(scripts, { recursive: true }); - await writeFile( - join(directory, ".env"), - [ - "APP_PORT=3010", - "SERVER_PORT=3001", - "COMPUTER_PORT=4100", - "BOT_PORT=4200", - "LANGGRAPH_PORT=4201", - "SUPERVISOR_PORT=4500", - "SUPERVISOR_TOKEN=supervisor-token", - "COMPUTER_TOKEN=computer-token", - "WORKER_SHARED_SECRET=worker-secret", - "MANAGED_AGENT_TOKEN=managed-token", - "AGENT_TOOL_TOKEN=agent-tool-token", - "MANAGED_AGENT_AG_UI_URL=http://localhost:4201/ag-ui", - "OPENBOT_ONE_COMPUTER_EACH=true", - "DATABASE_URL=postgres://openbot:openbot@localhost:5432/openbot", - "", - ].join("\n"), - ); + const environment = [ + "APP_PORT=3010", + "SERVER_PORT=3001", + "COMPUTER_PORT=4100", + "BOT_PORT=4200", + "LANGGRAPH_PORT=4201", + "SUPERVISOR_PORT=4500", + "SUPERVISOR_TOKEN=supervisor-token", + "COMPUTER_TOKEN=computer-token", + "WORKER_SHARED_SECRET=worker-secret", + "MANAGED_AGENT_TOKEN=managed-token", + "AGENT_TOOL_TOKEN=agent-tool-token", + "MANAGED_AGENT_AG_UI_URL=http://localhost:4201/ag-ui", + "OPENBOT_ONE_COMPUTER_EACH=true", + "DATABASE_URL=postgres://openbot:openbot@localhost:5432/openbot", + ].filter((line) => !omitFromEnv.some((key) => line.startsWith(`${key}=`))); + await writeFile(join(directory, ".env"), `${environment.join("\n")}\n`); await writeFile( join(scripts, "start.sh"), await readFile("scripts/start.sh"), ); await chmod(join(scripts, "start.sh"), 0o755); + // Copied beside it because `start.sh` sources it by its own directory, so a temp root without it + // would fail on a missing file rather than on whatever the test is about. + await writeFile( + join(scripts, "require-bash.sh"), + await readFile("scripts/require-bash.sh"), + ); await writeExecutable( join(fakeBin, "lsof"), @@ -91,7 +113,7 @@ exit 0 try { const child = Bun.spawn({ - cmd: ["bash", "scripts/start.sh"], + cmd: [shell, "scripts/start.sh"], cwd: directory, env: { PATH: `${fakeBin}:${process.env.PATH ?? ""}`, @@ -133,3 +155,73 @@ describe("start.sh server restart guard", () => { }, ); }); + +describe("start.sh under the wrong shell", () => { + /** + * THE WRONG INVOCATION SAYS SO, AND SAYING NOTHING IS THE DEFECT. + * + * CRITERION. `sh scripts/start.sh` exits non-zero having named bash and the command to run + * instead, on stderr. + * + * REASON. The shebang says bash and `sh` overrides it. On macOS `sh` IS bash, in POSIX mode, + * where a failed assignment inside a function is fatal instead of survivable — so the script died + * at the first setting it read out of `.env` and printed NOTHING: no line number, no failing + * command, no exit message. The whole of what somebody had to go on was `1`, which reads as "this + * script is broken" rather than "run it the other way", and it cost an afternoon. + * + * THE SENTENCE NAMES THE FIX AND NOT ONLY THE FAULT, because "wrong shell" is not actionable to + * somebody who typed the only invocation they knew. + */ + test("sh refuses with a sentence naming bash, rather than exiting in silence", async () => { + const result = await runStartWithStaleServerProbe(401, { shell: "sh" }); + + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toMatch(/bash/); + expect(result.stderr).toMatch(/bash scripts\/start\.sh/); + // On stderr rather than stdout, so it survives a caller that is reading the progress output. + expect(result.stdout).toBe(""); + }); +}); + +describe("start.sh settings with no line in .env", () => { + /** + * A KEY THIS SCRIPT HAS A DEFAULT FOR IS ALLOWED TO BE ABSENT, WHICH IS WHAT THE DEFAULT IS FOR. + * + * CRITERION. With no `APP_PORT` line in `.env` at all, the run completes exactly as it does with + * one — the app is probed on 3010, the port the script falls back to. + * + * THIS ONE PASSED BEFORE THE CHANGE IT GUARDS, AND THAT IS SAID PLAINLY RATHER THAN DRESSED UP. + * `setting` reads the key with a `grep` pipeline, `grep` finding nothing is an exit status of 1, + * and `pipefail` makes it the pipeline's — but under bash that status does not escape the command + * substitution, so the fallback won and this run was already green. It was fatal only under `sh`, + * where the sibling test now keeps anybody from arriving at all. So there is no invocation left + * that can watch this fail, and what it does instead is pin the CONTRACT: the second argument to + * `setting` is the value an absent key takes, and nothing about which shell is reading the script + * may decide otherwise. + * + * THE `|| true` IN THE SCRIPT IS WHAT MAKES THAT TRUE BY CONSTRUCTION rather than by a subtlety + * of where `set -e` applies. Both halves are worth having: the refusal stops the invocation that + * was silently fatal, and this stops the fallback depending on a detail nobody should have to + * know to add a setting. + * + * `APP_PORT` IS THE ONE OMITTED BECAUSE IT IS THE REAL CASE. `.env.example` lists `PORT` and + * `SERVER_PORT` and not this one, so every `.env` copied from it is missing exactly this key, and + * it is the first setting the script reads — which is why the silent exit under `sh` happened + * before any output at all. + * + * THE ASSERTION IS THE WHOLE RUN rather than a printed port: the faked `curl` answers the app + * probe for `http://localhost:3010/` and nothing else, so a fallback that produced any other port + * fails the run instead of quietly reporting a different number. + */ + test("a .env with no APP_PORT still starts, on the default port", async () => { + const result = await runStartWithStaleServerProbe(401, { + omitFromEnv: ["APP_PORT"], + }); + + expect({ exitCode: result.exitCode, stderr: result.stderr }).toMatchObject({ + exitCode: 0, + stderr: "", + }); + expect(result.stdout).toContain("http://localhost:3010"); + }); +}); diff --git a/scripts/start.sh b/scripts/start.sh index 1b99a6fe1..b01f676b2 100755 --- a/scripts/start.sh +++ b/scripts/start.sh @@ -3,6 +3,10 @@ # Start the local OpenBot stack and verify each service answers as OpenBot. # Safe to rerun: matching services are left running, and unrelated port holders are reported. +# Before anything else, and before the `set` line below, which is itself bash-only: this file is +# bash, and being read by `sh` used to end it with exit 1 and no output at all. See that file. +. "$(dirname "$0")/require-bash.sh" + set -euo pipefail cd "$(dirname "$0")/.." @@ -20,7 +24,20 @@ fi setting() { local name="$1" fallback="$2" value="${!1:-}" if [ -z "$value" ]; then - value="$(grep -E "^$name=" "$ROOT/.env" | tail -1 | cut -d= -f2- | sed -e 's/^"\(.*\)"$/\1/' -e "s/^'\(.*\)'$/\1/")" + # `|| true`, BECAUSE A KEY THIS FUNCTION HAS A DEFAULT FOR IS ROUTINELY ABSENT FROM `.env`. + # + # That is the whole reason the second argument exists: `.env.example` does not list `APP_PORT`, + # so a perfectly ordinary `.env` has no line for it. `grep` finding nothing is an exit status of + # 1, `pipefail` makes it the pipeline's, and `set -e` then killed the script on the way to the + # fallback that was sitting right there — before the first line of output, so the failure + # named neither the key nor the file. + # + # Under bash that status did not escape the command substitution and the fallback won, which is + # why this stood for as long as it did: the bug was invisible until somebody ran the script with + # `sh`, where the same code exits 1 in silence. The refusal at the top of this file now names + # that, and this closes the trap underneath it — an absent key takes the default in either + # shell, which is what the argument always promised. + value="$(grep -E "^$name=" "$ROOT/.env" | tail -1 | cut -d= -f2- | sed -e 's/^"\(.*\)"$/\1/' -e "s/^'\(.*\)'$/\1/" || true)" fi printf '%s' "${value:-$fallback}" } diff --git a/scripts/stop.sh b/scripts/stop.sh index 1012def1f..d7833d6f4 100755 --- a/scripts/stop.sh +++ b/scripts/stop.sh @@ -8,6 +8,10 @@ # last because they are made by the supervisor rather than by compose, so `docker compose down` # leaves them running and they are the heaviest thing here, one Chromium each. +# Before anything else, and before the `set` line below, which is itself bash-only: this file is +# bash, and being read by `sh` used to end it with exit 1 and no output at all. See that file. +. "$(dirname "$0")/require-bash.sh" + set -euo pipefail cd "$(dirname "$0")/.." @@ -41,7 +45,12 @@ done setting() { local name="$1" fallback="$2" value="${!1:-}" if [ -z "$value" ] && [ -f "$ROOT/.env" ]; then - value="$(grep -E "^$name=" "$ROOT/.env" | tail -1 | cut -d= -f2- | sed -e 's/^"\(.*\)"$/\1/' -e "s/^'\(.*\)'$/\1/")" + # `|| true`, for `start.sh`'s reason: a key with a default here is routinely absent from `.env` + # — of the two this reads, `.env.example` lists `SERVER_PORT` and not `APP_PORT` — and `grep` + # finding nothing is an exit status of 1 that `pipefail` makes the pipeline's. The fallback on + # the next line is what the second argument promises, and it must not depend on whether the key + # happened to be written down. + value="$(grep -E "^$name=" "$ROOT/.env" | tail -1 | cut -d= -f2- | sed -e 's/^"\(.*\)"$/\1/' -e "s/^'\(.*\)'$/\1/" || true)" fi printf '%s' "${value:-$fallback}" }