Say which shell is reading start.sh and stop.sh, instead of exiting 1 with nothing printed

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

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");
+  });
+});

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}"
 }

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}"
 }