test(shell): make TestBuilder fail on expectations that cannot assert anything

[robobun]

Problem

• Several shell tests looked like they checked stdout, stderr or the exit code but checked nothing. On main, echo hi with .stdout(s => s.includes("definitely-not-present")) passes.
• Two causes: an expectation callback's return value was ignored, so a predicate callback asserted nothing; and each expectation is a single slot, so a second .stdout() in a chain replaced the first and only the last one ran.
• Affected: the 14-command "exit codes" table (no exit code was checked), the cd-in-pipeline tests, two stderr checks, four ls chains and three describe.todo chains. The table hid that bare export has always exited 0, as in bash.
• Separately, .error(...) on a command that completed ran the output checks instead of failing, and on the ShellError path a failing expectation escaped as an unhandled rejection instead of failing run().

Fix

• The builder rejects the two shapes that can never assert anything: setting the same expectation twice throws while the chain is built, and a callback that returns anything other than undefined fails the run. bun:test matchers return undefined, so existing s => expect(s)... callbacks are unaffected.
• run() fails when .error() was set but the command completed, and awaits the checks on the ShellError path, so every failing expectation now rejects run().
• The affected tests become real assertions, one per chain. export leaves the failing table and gets a test pinning its output and exit 0; a case for the cwd after a cd pipeline is added. The new expectations match current shell behaviour, so none papers over a shell bug.
• Verification: the new builder tests fail 9 of 14 against the builder on main and pass here. All 24 files that use the builder were run with the guards on and only the chains rewritten here tripped. Two ls permission-denied tests fail as root before and after, and leak.test.ts times out under a debug build regardless of this change.

Background

• TestBuilder (test/js/bun/shell/test_builder.ts) is the helper behind most shell tests: TestBuilder.command`...` followed by .stdout(), .stderr(), .exitCode() or .error(), then .run() or .runAsTest(name).
• Each expectation method takes a literal or a callback. A callback is meant to call expect() itself; the builder never looked at what it returned.
• $TEMP_DIR in a string expectation is replaced with the test's temp directory. It is not replaced for callbacks, which is why the cd predicates could never have matched.
• .error() declares that the command should throw (for example a parse error) rather than complete with an exit code.
• The ShellError path: when a command exits non-zero with Bun.$ in throwing mode, run() catches a ShellError carrying stdout, stderr and the exit code and checks the expectations against those.

Original description

What was wrong

Several shell tests built with TestBuilder (test/js/bun/shell/test_builder.ts) looked like they asserted something about the output but did not. Two builder behaviours made this possible:

1. The callback form of .stdout(fn) / .stderr(fn) / .exitCode(fn) discarded the callback's return value, so a predicate-style callback asserted nothing. doChecks() called this.expected_stdout(...) and ignored the result.
2. .stdout() (and .stderr() / .exitCode() / .error()) assign a single field, so calling one of them twice in a chain silently replaced the earlier expectation and only the last one ran.

Both are easy to demonstrate against main; these pass:

const TestBuilder = createTestBuilder(import.meta.path);
await TestBuilder.command`echo hi`.stdout(s => s.includes("definitely-not-present")).run();
await TestBuilder.command`echo hi`
  .stdout(() => { throw new Error("never runs"); })
  .stdout(s => expect(s).toBe("hi\n"))
  .run();

Call sites affected (found by temporarily instrumenting the builder and running every file that uses it, plus a scan of the chains):

• bunshell.test.ts "exit codes": .exitCode(c => c !== 0) on 14 commands, so none of them checked the exit code. One entry, bare export, has in fact always exited 0 (it prints the exported variables, like bash), which the no-op assertion hid.
• pipeline_stack.test.ts "cd builtin in pipelines": three .stdout(s => s.includes("$TEMP_DIR"))-style predicates. Doubly inert, since $TEMP_DIR is only substituted into string expectations and the third predicate expected two lines from pwd | cd / | pwd, whose first pwd goes into the pipe.
• bunshell.test.ts "nested pipeline mixed success failure": .stderr(s => s.includes("command not found")).
• bunshell-default.test.ts: two .stderr(s => s.includes("1 pass")).
• commands/ls.test.ts: four chains with 2 to 4 .stdout(...) calls (-A, -Ra, -RA, permission denied recursive). The -A test, for example, only ran toContain("regular"); the ./.. exclusion it is named for was never checked.
• bunshell.test.ts condexprs (inside describe.todo): three chains with .stdout("1").stdout("0")-style pairs.

Fix

test_builder.ts:

• Setting the same expectation twice on one builder throws, naming the expectation. This fires at chain construction, so a bad runAsTest chain fails the file immediately.
• A .stdout() / .stderr() / .exitCode() callback that returns anything other than undefined fails the run with a message saying the callback has to call expect() itself. All bun:test matchers return undefined, so the existing s => expect(s).toBe(...) style keeps working.
• run() now fails when .error(...) was set but the command completed (previously the output checks ran instead, and with .error() and no argument any failure was swallowed), and it awaits doChecks() on the ShellError path, where a failing expectation used to escape as an unhandled rejection instead of rejecting run(). The output checks also moved out of the try, so an assertion failure can no longer be reinterpreted as the thrown error.

Call sites: the predicates are replaced with real assertions (exact $TEMP_DIR\n strings for the cd pipeline tests, expect(c).not.toBe(0) for the exit code table, exact stderr for the nested pipeline test) and the chained .stdout() calls are folded into single exact toEqual assertions, matching the neighbouring ls tests. export is dropped from the failing-commands table and env variables gets a test pinning that bare export prints the exported variables sorted and exits 0. A cd foo | pwd; pwd case is added to the pipeline describe since none of the existing cases covered the cwd after the pipeline.

test_builder.test.ts pins the builder behaviour: each of the set-twice guards, each return-value guard, .error() with and without a throw, and the ShellError path both passing and failing.

Why this is the right fix

Both broken patterns were written independently several times (seven predicate callbacks across four files, seven chains with repeated calls across two files), so the API itself invites them; fixing only the call sites would leave the next one just as silent. The guards reject exactly the two shapes that can never assert anything, and nothing in the suite relies on either: with the guards in place, every file that uses TestBuilder was run and the only chains that tripped are the ones rewritten here. The rewritten expectations were checked against the current shell behaviour (cd in a pipeline stage does not change the cwd of the other stages or of the commands after the pipeline, same as bash), so none of them is papering over a shell bug. The two shell bugs the exit-code table happens to exercise (echo hi > /dev/full exiting with 65508, and export foo-bar succeeding) already have open PRs (#32278, #32298); the table only asserts non-zero, so it is unaffected by either.

Verification

• test_builder.test.ts with the builder from main: 9 of 14 fail (the four set-twice cases, the three return-value cases, .error() without a throw, and the failing ShellError-path expectation). With this change: 14 pass.
• All 24 TestBuilder-based files under test/js/bun/shell with a debug build: everything passes except the two ls permission-denied tests, which fail as root both before and after this change (they pass when run as an unprivileged user, including the rewritten recursive one), and the leak.test.ts iteration tests, which time out under the debug build here regardless of this change and pass with a release build.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 12 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ff7b1b8c-06ce-4192-abb5-7ae82bcaf809

📥 Commits

Reviewing files that changed from the base of the PR and between f59f705 and 2a0be52.

📒 Files selected for processing (6)

• test/js/bun/shell/bunshell-default.test.ts
• test/js/bun/shell/bunshell.test.ts
• test/js/bun/shell/commands/ls.test.ts
• test/js/bun/shell/pipeline_stack.test.ts
• test/js/bun/shell/test_builder.test.ts
• test/js/bun/shell/test_builder.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: ready for review.

Reproduced on main by instrumenting test_builder.ts and running every file under test/js/bun/shell that uses TestBuilder: 7 predicate callbacks whose return value was ignored and 7 chains that set the same expectation twice, all listed in the PR description. With the guards in this PR the same run only trips on those sites, which are rewritten here.

test_builder.test.ts fails 9 of 14 cases against the builder from main and passes with this change; the rest of the shell suite passes with a debug build apart from the two ls permission-denied tests (fail as root before and after, pass as an unprivileged user) and the leak.test.ts iteration tests (debug-build timeouts here, pass with a release build).

[claude]

LGTM — test-only hardening of the shell TestBuilder helper plus fixes for the vacuous call sites it exposed.

What was reviewed:

• _setExpectation guard and checkCallbackReturnedNothing: verified all bun:test matchers return undefined, so existing s => expect(s).toBe(...) chains keep working; grep confirms no existing chain calls .error() twice.
• run() restructure: doChecks moved out of the try so assertion failures aren't swallowed by the catch, and now awaited on the ShellError path; the new .error()-but-completed check correctly excludes .error(false).
• Rewritten ls -R level1 expectation: checked src/runtime/shell/builtin/ls.rs — print_directory is false for a single path arg and the header is written only after the dir fd opens, so the ["file1","file2","file3","level2"] exact match is correct.
• export removed from the failing-commands table with a replacement test pinning its actual behavior; condexpr double-.stdout() chains inside describe.todo were fixed so construction-time throws don't break the file.

Extended reasoning...

Overview

Test-only PR touching six files under test/js/bun/shell/. It hardens test_builder.ts (the shared helper used by ~24 shell test files) so two classes of vacuous assertion fail loudly: (1) setting the same expectation twice on one builder now throws at chain-construction time, and (2) an expectation callback that returns a value (predicate style) now fails the run instead of being silently discarded. run() is restructured so doChecks runs outside the try (assertion failures no longer land in the catch), is awaited on the ShellError path, and a set .error() now fails when the command completes instead of throwing. Every call site that tripped the new guards is rewritten with a real assertion, and a new test_builder.test.ts pins each guard.

Security risks

None. No runtime code is touched — only test files and a test helper.

Level of scrutiny

Medium-low. The change is test-only, so the worst-case regression is a CI failure rather than a shipped bug. The parts warranting a closer look were the tightened exact-match assertions (which could be platform- or implementation-sensitive) and the shared-helper API change (which affects 24 files). I checked:

• Double-set guard reach: grepped for .error( across test/js/bun/shell/ — no chain calls it twice outside the new test file. The three condexpr double-.stdout() chains sit inside describe.todo, whose body still runs at collection time, so they had to be fixed (and were).
• ls -R level1 exact assertion: read src/runtime/shell/builtin/ls.rs. The level1: header is printed only when print_directory is true, which is set from task_count > 1 for the initial task (false here — single arg) and forced true only for recursive subtasks; the level1/level2: subtask header is written after fd is opened, which fails with EACCES, so it never reaches stdout. sortedLsOutput therefore yields exactly ["file1","file2","file3","level2"].
• run() restructure: the new expected_error !== undefined && expected_error !== false check correctly leaves .error(false) (explicit expect-no-error) on the success path, and doChecks being awaited on the ShellError branch fixes the previously fire-and-forget async.

Other factors

• The PR description documents exhaustive verification: all 24 TestBuilder-based files were run with the guards in place and only the sites rewritten here tripped; test_builder.test.ts fails 9/14 against main and passes 14/14 with the change.
• The export removal from failing_cmds follows REVIEW.md's rule that every deletion needs a stated reason or replacement — bare export exits 0, and a new test pins that it prints the exported vars sorted.
• The bug-hunting system found nothing.
• No prior human reviews or outstanding comments on the PR.

[robobun]

^{Updated 3:14 AM PT - Aug 12th, 2026}

✅ @robobun, your commit 2a0be52f45b4c0db61f67b8ca48ec750c789af78 passed in Build #92818! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37737

That installs a local version of the PR into your bun-37737 executable, so you can run:

bun-37737 --bun