From 97a49782aa60ec10b975ef359b3150475ea6f0b9 Mon Sep 17 00:00:00 2001 From: Maxim Date: Tue, 15 Sep 2026 21:26:56 +0200 Subject: [PATCH] Say which shell is reading these scripts, instead of exiting 1 with nothing printed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sh scripts/start.sh` produced exit 1 and not one character of output. No line number, no failing command, no message — the whole of what somebody had to go on was the number, which reads as "this script is broken" rather than "run it the other way". Two things were behind it. The shebang says bash and `sh` overrides it. On macOS `sh` IS bash, run in POSIX mode, where a failure these scripts survive under bash is fatal instead. So the run ended at the first setting read out of `.env`, before any output existed to say so. That setting reads its key with a `grep` pipeline, and `grep` finding nothing is an exit status of 1 that `pipefail` makes the pipeline's. `set -e` then ended the script on its way to the fallback sitting on the next line — the fallback that is the entire reason `setting` takes a second argument. `.env.example` does not list `APP_PORT`, so every `.env` copied from it is missing exactly the first key the script reads. Both scripts now source `scripts/require-bash.sh` before they set any options, which names the shell it found and prints the command to retype. It is POSIX syntax throughout, because the shell it is warning 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. `SHELLOPTS` carrying `posix` is what separates bash-as-sh from bash; a `BASH_VERSION` check alone cannot see that case, and that case is every Mac. `stop.sh` had both defects identically and is fixed with the same two changes. The refusal has a test that was watched failing. The absent-key test passes either way and says so in its own docblock: under bash the fallback already won, and with the refusal in place there is no longer an invocation that can watch it fail — so what it pins is the contract rather than the fix. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/require-bash.sh | 42 +++++++++ scripts/start-restart-guard.test.ts | 136 +++++++++++++++++++++++----- scripts/start.sh | 19 +++- scripts/stop.sh | 11 ++- 4 files changed, 184 insertions(+), 24 deletions(-) create mode 100644 scripts/require-bash.sh 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}" }