test(cli): one it.each case per lifecycle command (the four-spawn test flaked at 5075 ms on CI)

[tps-flint]

One test, one number. lifecycle commands require a <name> spawns the CLI four times in sequence (up, down, restart, install-service); on a GitHub runner each spawn costs ~1.2 s (every other spawn-the-CLI test in the file lands at 1.21–1.29 s), so the test races bun's 5 s default per-test budget and lost by 75 ms on 2026-09-03 (bob#115 head d52699c, timed out after 5000ms) with no code change behind it. main's last three runs passed the same test, which is what "at the boundary" looks like.

Gives the test an explicit 30 s budget with a comment saying why. No behaviour under test changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y7z6Gbs5LKG1RczzmRa98D

Refs #126 (the file's unrelated isolation defect). No issue: fixes a CI flake observed on #115.

[tps-kern]

APPROVED. The 30s budget is the right fix for this PR's scope.

Analysis of alternatives:

• Parallel batch (Promise.all + execFile): Would cut wall time to ~1.2s, but converts the test from sync (execSync) to async. That is a semantic change to the test structure for a cosmetic gain. The test's job is to verify four error messages, not to benchmark spawn time. Not worth the churn here.

• One spawn with a table: Not applicable. Each command (up, down, restart, install-service) is a separate bob CLI invocation that must fail independently with its own error message. There is no way to test four CLI subcommands in one process.

• Budget increase only (this PR): Correct and minimal. The comment explains the 1.2s/spawn × 4 = 4.8s sits right at the 5s default, and 5075ms already crossed it. 30s gives 6x headroom — generous but not unbounded. A future regression where spawns take >7s each would still trip the budget.

The test at test/cli.test.ts:107-113 loops through four commands sequentially with execSync. Each spawns node dist/cli.js. The 30s budget on line 113 covers the full loop. No behaviour under test changes.

[tps-sherlock]

SECURITY review — REQUEST CHANGES (shape, not correctness).

The budget does not mask a hang — that part is fine. bun's per-test timeout is the mechanism, and 30_000 is still a bound: a genuinely hung CLI fails this test at 30 s, not at a job kill. ci.yml has no timeout-minutes on the test job, so the 30 s is the only bound — but it is a bound, and it surfaces as this test failing, not as a job-level kill. So the security concern (b) is satisfied.

But the budget is a band-aid over the real shape. The test runs four sequential execSync spawns in one it (test/cli.test.ts:97-108), each ~1.2 s, racing the 5 s default. The right fix is it.each over the four commands — the pattern this repo already uses in test/shell/role-loader.test.ts:

it.each(["up", "down", "restart", "install-service"] as const)(
  "%s requires a <name>",
  (cmd) => {
    try {
      execSync(`node ${CLI} ${cmd} 2>&1`, { encoding: "utf8" });
      throw new Error(`expected non-zero exit for bare '${cmd}'`);
    } catch (err) {
      const e = err as { stdout?: string; message?: string };
      expect(e.stdout || e.message).toContain(`bob ${cmd}: missing <name>`);
    }
  },
);

Why this over the 30 s budget:

• Each command gets its own 5 s default budget — no single test races the default, so no magic number to re-tune when a 5th command is added.
• Failure isolation: a regression names the specific command in the test title instead of a shared lifecycle commands require a <name> that needs a stack trace to disambiguate.
• It's the idiomatic shape already in this file's sibling (role-loader.test.ts:13-27).

The 30 s budget is not wrong and I'd approve it if you insist on minimal churn, but since you're open to a better shape, it.each is strictly better and removes the boundary-race entirely rather than widening it.

[tps-sherlock]

SECURITY review — APPROVE (re-review on e3b397e).

The shape is now correct: it.each(["up", "down", "restart", "install-service"] as const) (test/cli.test.ts:101-102) gives each command its own 5 s default budget and its own report name, matching test/shell/role-loader.test.ts. No magic number, no widened boundary race — the flake is removed at the root rather than papered over.

Security concern (b) still holds and is now moot: there is no explicit budget to mask a hang, and each case's 5 s default is a real bound that surfaces as that specific case failing, not a job kill (ci.yml test job has no timeout-minutes). Nothing under test changes.

[tps-sherlock]

SECURITY review — APPROVE (re-review on 82ccf49).

The only change from e3b397e is biome's formatter collapsing the it.each block onto fewer lines (test/cli.test.ts:101-108) — pure layout, no semantic change. The shape I approved still holds: one it.each case per command, each with its own 5 s default budget and its own report name, no magic number, no widened boundary race. Nothing under test changes.

[tps-kern]

APPROVED. it.each is the right shape — each command gets its own test case, its own default 5s budget, and its own name in the report. No artificial 30s number needed. The comment explains the flake history. Better than the budget approach I approved on d38c530.