Harden companion Windows lifecycle coverage

2026-06-13 14:19:05 +08:00 · 2026-06-10 16:23:13 -07:00
parent a2e67bbd9b
commit c7d7e3550f
8 changed files with 223 additions and 30 deletions
--- a/skills/brainstorming/scripts/server.cjs
+++ b/skills/brainstorming/scripts/server.cjs
@@ -192,6 +192,16 @@ function getNewestScreen() {
  return files.length > 0 ? files[0].path : null;
 }

+function urlHostForHttp(host) {
+  const h = String(host);
+  if (h.startsWith('[') && h.endsWith(']')) return h;
+  return h.includes(':') ? '[' + h + ']' : h;
+}
+
+function companionUrl() {
+  return 'http://' + urlHostForHttp(URL_HOST) + ':' + PORT + '/?key=' + TOKEN;
+}
+
 function isRegularFileInsideContentDir(filePath) {
  let stat, realContentDir, realFilePath;
  try {
@@ -424,7 +434,7 @@ function maybeOpenBrowser() {
  if (!process.env.BRAINSTORM_OPEN) return; // opt-in: only after the user approves the companion
  if (HOST !== '127.0.0.1' && HOST !== 'localhost') return;
  if (clients.size > 0) return; // the user already opened it
-  const url = 'http://' + URL_HOST + ':' + PORT + '/?key=' + TOKEN; // must carry the key or the gate 403s it
+  const url = companionUrl(); // must carry the key or the gate 403s it
  const cp = require('child_process');
  // Operator-provided launcher: run as given (this env var is trusted operator input).
  if (process.env.BRAINSTORM_OPEN_CMD) {
@@ -572,7 +582,7 @@ function startServer() {
    }
    const info = JSON.stringify({
      type: 'server-started', port: Number(PORT), host: HOST,
-      url_host: URL_HOST, url: 'http://' + URL_HOST + ':' + PORT + '/?key=' + TOKEN,
+      url_host: URL_HOST, url: companionUrl(),
      screen_dir: CONTENT_DIR, state_dir: STATE_DIR, idle_timeout_ms: IDLE_TIMEOUT_MS
    });
    console.log(info);
--- a/skills/brainstorming/scripts/start-server.sh
+++ b/skills/brainstorming/scripts/start-server.sh
@@ -79,6 +79,21 @@ if [[ -n "$IDLE_TIMEOUT_MINUTES" ]]; then
  export BRAINSTORM_IDLE_TIMEOUT_MS=$(( IDLE_TIMEOUT_MINUTES * 60 * 1000 ))
 fi

+is_windows_like_shell() {
+  case "${OSTYPE:-}" in
+    msys*|cygwin*|mingw*) return 0 ;;
+  esac
+  if [[ -n "${MSYSTEM:-}" ]]; then
+    return 0
+  fi
+  local uname_s
+  uname_s="$(uname -s 2>/dev/null || true)"
+  case "$uname_s" in
+    MSYS*|MINGW*|CYGWIN*) return 0 ;;
+  esac
+  return 1
+}
+
 # Some environments reap detached/background processes. Auto-foreground when detected.
 if [[ -n "${CODEX_CI:-}" && "$FOREGROUND" != "true" && "$FORCE_BACKGROUND" != "true" ]]; then
  FOREGROUND="true"
@@ -86,10 +101,7 @@ fi

 # Windows/Git Bash reaps nohup background processes. Auto-foreground when detected.
 if [[ "$FOREGROUND" != "true" && "$FORCE_BACKGROUND" != "true" ]]; then
-  case "${OSTYPE:-}" in
-    msys*|cygwin*|mingw*) FOREGROUND="true" ;;
-  esac
-  if [[ -n "${MSYSTEM:-}" ]]; then
+  if is_windows_like_shell; then
    FOREGROUND="true"
  fi
 fi
@@ -139,10 +151,7 @@ fi
 # Passing a PID node cannot verify causes server to log owner-pid-invalid
 # and self-terminate at the 60-second lifecycle check. Clear it so the
 # watchdog is disabled and the idle timeout becomes the only shutdown trigger.
-case "${OSTYPE:-}" in
-  msys*|cygwin*|mingw*) OWNER_PID="" ;;
-esac
-if [[ -n "${MSYSTEM:-}" ]]; then
+if is_windows_like_shell; then
  OWNER_PID=""
 fi

--- a/skills/brainstorming/scripts/stop-server.sh
+++ b/skills/brainstorming/scripts/stop-server.sh
@@ -20,7 +20,12 @@ PID_FILE="${STATE_DIR}/server.pid"
 # not a reused/unrelated process whose PID was recycled into a stale pid file.
 is_brainstorm_server() {
  kill -0 "$1" 2>/dev/null || return 1
-  case "$(ps -p "$1" -o command= 2>/dev/null)" in
+  local command_line
+  command_line="$(ps -p "$1" -o command= 2>/dev/null || true)"
+  if [[ -z "$command_line" ]]; then
+    command_line="$(ps -f -p "$1" 2>/dev/null | sed '1d' || true)"
+  fi
+  case "$command_line" in
    *node*server.cjs*) ;;
    *) return 1 ;;
  esac
--- a/tests/brainstorm-server/lifecycle.test.js
+++ b/tests/brainstorm-server/lifecycle.test.js
@@ -21,6 +21,7 @@ const STOP = path.join(__dirname, '../../skills/brainstorming/scripts/stop-serve
 const sleep = ms => new Promise(r => setTimeout(r, ms));

 function waitForExit(child, timeoutMs = 2000) {
+  if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve(true);
  return new Promise(resolve => {
    let settled = false;
    const finish = (exited) => {
@@ -33,10 +34,38 @@ function waitForExit(child, timeoutMs = 2000) {
  });
 }

+async function killAndWait(child, timeoutMs = 2000) {
+  if (!child || child.exitCode !== null || child.signalCode !== null) return true;
+  const exited = waitForExit(child, timeoutMs);
+  child.kill();
+  if (await exited) return true;
+
+  child.kill('SIGKILL');
+  return waitForExit(child, 500);
+}
+
+async function waitForFile(file, timeoutMs = 3000) {
+  const deadline = Date.now() + timeoutMs;
+  while (Date.now() < deadline) {
+    if (fs.existsSync(file)) return true;
+    await sleep(50);
+  }
+  return fs.existsSync(file);
+}
+
 function firstServerStarted(out) {
  return JSON.parse(out.trim().split('\n').find(l => l.includes('server-started')));
 }

+function openCaptureCommand(dir, marker) {
+  const scriptPath = path.resolve(dir, 'capture-open.cjs');
+  const markerPath = path.resolve(marker);
+  fs.writeFileSync(scriptPath,
+    "const fs = require('fs');\n" +
+    "fs.appendFileSync(process.argv[2], process.argv[3] + '\\n');\n");
+  return `node ${JSON.stringify(scriptPath)} ${JSON.stringify(markerPath)}`;
+}
+
 async function runTests() {
  let passed = 0, failed = 0;
  async function test(name, fn) {
@@ -53,7 +82,8 @@ async function runTests() {
      const info = firstServerStarted(out);
      assert.strictEqual(info.idle_timeout_ms, 1234567, 'idle_timeout_ms should reflect the env override');
    } finally {
-      srv.kill(); await sleep(100); fs.rmSync(dir, { recursive: true, force: true });
+      await killAndWait(srv);
+      fs.rmSync(dir, { recursive: true, force: true });
    }
  });

@@ -77,7 +107,7 @@ async function runTests() {
      assert(fs.existsSync(path.join(dir, 'state', 'server-stopped')), 'should write server-stopped');
    } finally {
      try { ws.close(); } catch (e) {}
-      if (!exited) srv.kill();
+      if (!exited) await killAndWait(srv);
      fs.rmSync(dir, { recursive: true, force: true });
    }
  });
@@ -95,6 +125,30 @@ async function runTests() {
    }
  });

+  await test('server-started URL brackets IPv6 URL hosts', async () => {
+    const dir = fs.mkdtempSync('/tmp/bs-ipv6-url-');
+    const srv = spawn('node', [SERVER], {
+      env: {
+        ...process.env,
+        BRAINSTORM_PORT: 3421,
+        BRAINSTORM_HOST: '127.0.0.1',
+        BRAINSTORM_URL_HOST: '::1',
+        BRAINSTORM_TOKEN: 'ipv6token',
+        BRAINSTORM_DIR: dir,
+        BRAINSTORM_LIFECYCLE_CHECK_MS: 100000
+      }
+    });
+    let out = ''; srv.stdout.on('data', d => out += d.toString());
+    try {
+      for (let i = 0; i < 60 && !out.includes('server-started'); i++) await sleep(50);
+      const info = firstServerStarted(out);
+      assert.strictEqual(info.url, 'http://[::1]:3421/?key=ipv6token');
+    } finally {
+      await killAndWait(srv);
+      fs.rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
  await test('persists the bound port AND key, and restores both on restart', async () => {
    const dir = fs.mkdtempSync('/tmp/bs-port-');
    const portFile = path.join(dir, '.last-port');
@@ -116,7 +170,8 @@ async function runTests() {
    for (let i = 0; i < 60 && !outB.includes('server-started'); i++) await sleep(50);
    const infoB = firstServerStarted(outB);
    const keyB = new URL(infoB.url).searchParams.get('key');
-    b.kill(); await sleep(100); fs.rmSync(dir, { recursive: true, force: true });
+    await killAndWait(b);
+    fs.rmSync(dir, { recursive: true, force: true });

    assert.strictEqual(infoB.port, infoA.port, 'restart should reuse the same port');
    // Same key too — otherwise the open tab's cookie would 403 against the restart.
@@ -159,9 +214,8 @@ async function runTests() {
      assert(opened, 'stored key should authenticate WS after restart');
    } finally {
      try { if (ws) ws.close(); } catch (e) {}
-      try { if (a) a.kill(); } catch (e) {}
-      try { if (b) b.kill(); } catch (e) {}
-      await sleep(100);
+      await killAndWait(a);
+      await killAndWait(b);
      fs.rmSync(dir, { recursive: true, force: true });
    }
  });
@@ -181,7 +235,9 @@ async function runTests() {
    const portB = firstServerStarted(outB).port;
    const persisted = fs.readFileSync(portFile, 'utf8').trim();

-    a.kill(); b.kill(); await sleep(100); fs.rmSync(dir, { recursive: true, force: true });
+    await killAndWait(a);
+    await killAndWait(b);
+    fs.rmSync(dir, { recursive: true, force: true });

    assert.notStrictEqual(portB, 3415, 'must not bind the already-taken port');
    assert(portB >= 49152, 'should fall back to a random high port');
@@ -193,14 +249,14 @@ async function runTests() {
  await test('auto-opens the browser once, on the first screen', async () => {
    const dir = fs.mkdtempSync('/tmp/bs-open-');
    const marker = path.join(dir, 'opened.log');
-    const openCmd = `sh -c 'echo "$0" >> ${marker}'`; // capture the launch instead of opening a browser
+    const openCmd = openCaptureCommand(dir, marker); // capture the launch instead of opening a browser
    const srv = spawn('node', [SERVER], { env: { ...process.env, BRAINSTORM_PORT: 3417, BRAINSTORM_DIR: dir, BRAINSTORM_OPEN: '1', BRAINSTORM_OPEN_CMD: openCmd, BRAINSTORM_LIFECYCLE_CHECK_MS: 100000 } });
    let out = ''; srv.stdout.on('data', d => out += d.toString());
    for (let i = 0; i < 60 && !out.includes('server-started'); i++) await sleep(50);

    // First screen, with no browser connected -> should auto-open.
    fs.writeFileSync(path.join(dir, 'content', 'first.html'), '<h2>First</h2>');
-    await sleep(700);
+    await waitForFile(marker);
    // Second screen -> must NOT open again.
    fs.writeFileSync(path.join(dir, 'content', 'second.html'), '<h2>Second</h2>');
    await sleep(700);
@@ -211,7 +267,7 @@ async function runTests() {
    if (lines[0]) {
      status = await new Promise(r => require('http').get(lines[0], res => { res.resume(); r(res.statusCode); }).on('error', () => r(0)));
    }
-    srv.kill(); await sleep(100);
+    await killAndWait(srv);
    fs.rmSync(dir, { recursive: true, force: true });

    assert.strictEqual(lines.length, 1, 'should open exactly once');
@@ -223,14 +279,14 @@ async function runTests() {
  await test('does NOT auto-open unless approved (BRAINSTORM_OPEN unset)', async () => {
    const dir = fs.mkdtempSync('/tmp/bs-open-');
    const marker = path.join(dir, 'opened.log');
-    const openCmd = `sh -c 'echo "$0" >> ${marker}'`;
+    const openCmd = openCaptureCommand(dir, marker);
    // BRAINSTORM_OPEN intentionally NOT set — auto-open must stay off.
    const srv = spawn('node', [SERVER], { env: { ...process.env, BRAINSTORM_PORT: 3418, BRAINSTORM_DIR: dir, BRAINSTORM_OPEN_CMD: openCmd, BRAINSTORM_LIFECYCLE_CHECK_MS: 100000 } });
    let out = ''; srv.stdout.on('data', d => out += d.toString());
    for (let i = 0; i < 60 && !out.includes('server-started'); i++) await sleep(50);
    fs.writeFileSync(path.join(dir, 'content', 'first.html'), '<h2>First</h2>');
    await sleep(700);
-    srv.kill(); await sleep(100);
+    await killAndWait(srv);
    const opened = fs.existsSync(marker);
    fs.rmSync(dir, { recursive: true, force: true });
    assert(!opened, 'must not open the browser without explicit approval');
@@ -248,7 +304,7 @@ async function runTests() {
    const hammer = setInterval(() => { require('http').get('http://localhost:3419/', r => r.resume()).on('error', () => {}); }, 60);
    for (let i = 0; i < 40 && !exited; i++) await sleep(100);
    clearInterval(hammer);
-    if (!exited) srv.kill();
+    if (!exited) await killAndWait(srv);
    fs.rmSync(dir, { recursive: true, force: true });

    assert(exited, 'idle shutdown must still fire despite a flood of unauthenticated requests');
--- a/tests/brainstorm-server/package.json
+++ b/tests/brainstorm-server/package.json
@@ -2,7 +2,7 @@
  "name": "brainstorm-server-tests",
  "version": "1.0.0",
  "scripts": {
-    "test": "node ws-protocol.test.js && node helper.test.js && node auth.test.js && node server.test.js && node lifecycle.test.js && bash stop-server.test.sh"
+    "test": "node ws-protocol.test.js && node helper.test.js && node auth.test.js && node server.test.js && node lifecycle.test.js && bash start-server.test.sh && bash stop-server.test.sh"
  },
  "dependencies": {
    "ws": "^8.19.0"
--- a/tests/brainstorm-server/start-server.test.sh
+++ b/tests/brainstorm-server/start-server.test.sh
@@ -0,0 +1,95 @@
+#!/usr/bin/env bash
+# Fast tests for start-server.sh shell-only platform decisions.
+set -uo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)"
+START_SCRIPT="$REPO_ROOT/skills/brainstorming/scripts/start-server.sh"
+
+TEST_DIR="${TMPDIR:-/tmp}/brainstorm-start-test-$$"
+passed=0
+failed=0
+
+cleanup() {
+  rm -rf "$TEST_DIR"
+}
+trap cleanup EXIT
+
+pass() {
+  echo "  PASS: $1"
+  passed=$((passed + 1))
+}
+
+fail() {
+  echo "  FAIL: $1"
+  echo "    $2"
+  failed=$((failed + 1))
+}
+
+make_fake_uname() {
+  local fake_bin="$1"
+  cat > "$fake_bin/uname" <<'EOF'
+#!/usr/bin/env bash
+if [[ "${1:-}" == "-s" ]]; then
+  echo "MINGW64_NT-10.0"
+else
+  /usr/bin/uname "$@"
+fi
+EOF
+  chmod +x "$fake_bin/uname"
+}
+
+echo ""
+echo "--- start-server.sh platform detection ---"
+
+mkdir -p "$TEST_DIR/fake-bin" "$TEST_DIR/project"
+make_fake_uname "$TEST_DIR/fake-bin"
+
+cat > "$TEST_DIR/fake-bin/node" <<'EOF'
+#!/usr/bin/env bash
+echo "CAPTURED_OWNER_PID=${BRAINSTORM_OWNER_PID:-__UNSET__}"
+exit 0
+EOF
+chmod +x "$TEST_DIR/fake-bin/node"
+
+captured=$(
+  PATH="$TEST_DIR/fake-bin:$PATH" \
+    MSYSTEM="" \
+    bash "$START_SCRIPT" --project-dir "$TEST_DIR/project" --foreground 2>/dev/null || true
+)
+owner_pid_value=$(echo "$captured" | grep "CAPTURED_OWNER_PID=" | head -1 | sed 's/CAPTURED_OWNER_PID=//')
+
+if [[ "$owner_pid_value" == "" || "$owner_pid_value" == "__UNSET__" ]]; then
+  pass "clears BRAINSTORM_OWNER_PID when uname reports a Windows-like shell"
+else
+  fail "clears BRAINSTORM_OWNER_PID when uname reports a Windows-like shell" \
+       "expected empty or unset, got '$owner_pid_value'"
+fi
+
+rm -rf "$TEST_DIR/project"/*
+
+cat > "$TEST_DIR/fake-bin/node" <<'EOF'
+#!/usr/bin/env bash
+echo "FOREGROUND_MODE=true"
+exit 0
+EOF
+chmod +x "$TEST_DIR/fake-bin/node"
+
+captured=$(
+  PATH="$TEST_DIR/fake-bin:$PATH" \
+    MSYSTEM="" \
+    bash "$START_SCRIPT" --project-dir "$TEST_DIR/project" 2>/dev/null || true
+)
+
+if echo "$captured" | grep -q "FOREGROUND_MODE=true"; then
+  pass "auto-foregrounds when uname reports a Windows-like shell"
+else
+  fail "auto-foregrounds when uname reports a Windows-like shell" \
+       "expected foreground node path, got: $captured"
+fi
+
+echo ""
+echo "--- Results: $passed passed, $failed failed ---"
+if [[ $failed -gt 0 ]]; then
+  exit 1
+fi
--- a/tests/brainstorm-server/stop-server.test.sh
+++ b/tests/brainstorm-server/stop-server.test.sh
@@ -13,11 +13,13 @@ SERVER="$SCRIPT_DIR/../../skills/brainstorming/scripts/server.cjs"
 PASS=0; FAIL=0
 ok() { echo "  PASS: $1"; PASS=$((PASS + 1)); }
 bad() { echo "  FAIL: $1"; echo "    $2"; FAIL=$((FAIL + 1)); }
+reap_job() { wait "$1" 2>/dev/null || true; }

 # --- Test 1: an unrelated, reused PID must NOT be killed ---
 SESS="$(mktemp -d)"; mkdir -p "$SESS/state"
 sleep 600 &
 UNRELATED=$!
+disown "$UNRELATED" 2>/dev/null || true
 echo "$UNRELATED" > "$SESS/state/server.pid"
 OUT="$("$STOP" "$SESS")"
 if kill -0 "$UNRELATED" 2>/dev/null; then
@@ -29,12 +31,14 @@ else
  bad "unrelated reused PID was KILLED" "$OUT"
 fi
 kill -9 "$UNRELATED" 2>/dev/null
+reap_job "$UNRELATED"
 rm -rf "$SESS"

 # --- Test 2: a real brainstorm server IS stopped ---
 SESS="$(mktemp -d)"; mkdir -p "$SESS/content" "$SESS/state"
 BRAINSTORM_DIR="$SESS" BRAINSTORM_PORT=3399 node "$SERVER" > /dev/null 2>&1 &
 SRV=$!
+disown "$SRV" 2>/dev/null || true
 for _ in $(seq 1 40); do kill -0 "$SRV" 2>/dev/null && break; sleep 0.1; done
 sleep 0.4
 echo "$SRV" > "$SESS/state/server.pid"
@@ -43,7 +47,9 @@ sleep 0.3
 if kill -0 "$SRV" 2>/dev/null; then
  bad "real brainstorm server still running after stop" "$OUT"
  kill -9 "$SRV" 2>/dev/null
+  reap_job "$SRV"
 else
+  reap_job "$SRV"
  case "$OUT" in
    *stopped*) ok "real brainstorm server is stopped" ;;
    *) bad "server stopped but status was not 'stopped'" "$OUT" ;;
@@ -66,6 +72,7 @@ if command -v lsof > /dev/null 2>&1; then
  echo '{"type":"server-started","port":3499}' > "$SESS/state/server-info" # nothing listens on 3499
  ( exec -a "node server.cjs" sleep 600 ) &
  IMPOSTOR=$!
+  disown "$IMPOSTOR" 2>/dev/null || true
  echo "$IMPOSTOR" > "$SESS/state/server.pid"
  OUT="$("$STOP" "$SESS")"
  if kill -0 "$IMPOSTOR" 2>/dev/null; then
@@ -77,6 +84,7 @@ if command -v lsof > /dev/null 2>&1; then
    bad "killed a node server.cjs that was NOT on our recorded port" "$OUT"
  fi
  kill -9 "$IMPOSTOR" 2>/dev/null
+  reap_job "$IMPOSTOR"
  rm -rf "$SESS"
 else
  echo "  SKIP: lsof unavailable — port cross-check test"
--- a/tests/brainstorm-server/windows-lifecycle.test.sh
+++ b/tests/brainstorm-server/windows-lifecycle.test.sh
@@ -80,14 +80,23 @@ get_port_from_info() {
  grep -o '"port":[0-9]*' "$1/state/server-info" | head -1 | sed 's/"port"://'
 }

+get_key_from_info() {
+  grep -o '"url":"[^"]*key=[^"]*' "$1/state/server-info" | head -1 | sed 's/.*key=//'
+}
+
 http_check() {
  local port="$1"
-  node -e "
+  local key="${2:-}"
+  node - "$port" "$key" <<'NODE'
    const http = require('http');
-    http.get('http://localhost:$port/', (res) => {
+    const port = Number(process.argv[2]);
+    const key = process.argv[3] || '';
+    const path = key ? '/?key=' + encodeURIComponent(key) : '/';
+    http.get({ hostname: '127.0.0.1', port, path }, (res) => {
+      res.resume();
      process.exit(res.statusCode === 200 ? 0 : 1);
    }).on('error', () => process.exit(1));
-  " 2>/dev/null
+NODE
 }

 # ========== Platform Detection ==========
@@ -227,6 +236,7 @@ else
  pass "Server starts successfully with empty OWNER_PID"

  SERVER_PORT=$(get_port_from_info "$TEST_DIR/survival")
+  SERVER_KEY=$(get_key_from_info "$TEST_DIR/survival")

  sleep 75

@@ -237,11 +247,11 @@ else
         "Server died. Log tail: $(tail -5 "$TEST_DIR/survival/.server.log" 2>/dev/null)"
  fi

-  if http_check "$SERVER_PORT"; then
+  if http_check "$SERVER_PORT" "$SERVER_KEY"; then
    pass "Server responds to HTTP after lifecycle check window"
  else
    fail "Server responds to HTTP after lifecycle check window" \
-         "HTTP request to port $SERVER_PORT failed"
+         "Authenticated HTTP request to port $SERVER_PORT failed"
  fi

  if grep -q "owner process exited" "$TEST_DIR/survival/.server.log" 2>/dev/null; then