Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .claude/rules/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,19 @@
turbo test # Run tests across all packages
```

Each package's `test` script passes `--parallel=N` (issue #261): test files
run in worker processes and **every file gets a fresh module registry** - a
fresh `db` singleton with its own database file. Two consequences for suite
authors: a suite that touches the DB must migrate it itself (server/api:
`@/__tests__/helpers/test-database.js`) and may assume neither an empty DB
nor a sibling file's boot; and runner-wide cleanup cannot live in a preload
`afterAll` (those fire per file under `--parallel`), it belongs in the
package's `test` script. A bare `bun test` still runs serially - bun ignores
a `parallel` key in `bunfig.toml [test]` (measured on 1.4.0 and 1.4.2), so
the flag lives in the scripts only. `SUBSHELL_TEST_PARALLEL` overrides a
package's default (CI pins 4 for its 4-core runners; 12 starved the web
suite's timeouts there).

## Guidelines

**Always write tests for new features.** Every new service, repository, route, or significant function should have corresponding tests. Tests should cover:
Expand Down
10 changes: 10 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ on:
branches:
- main

# CI runs every suite SERIALLY inside its package (issue #261's scripts
# still parallelize local runs). The hosted container gets a fraction of its
# 4 labels (0.4 CPU measured in apps/server/web's budgets, twice), and bun's
# --parallel at ANY width - even 1, which is not serial - starved
# time-budgeted suites into per-test timeouts across three CI attempts on
# PR #277. scripts/par-test.sh omits the flag on this variable; the shard
# matrix is CI's speed, not worker threads.
env:
SUBSHELL_TEST_SERIAL: "1"

permissions:
contents: read
# The container jobs pull ghcr.io/subshell-ai/desktop-builder, which inherits
Expand Down
2 changes: 1 addition & 1 deletion apps/client/desktop/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"lint": "biome check --write --unsafe src ui",
"lint:check": "biome check src ui",
"lint:staged": "biome check --no-errors-on-unmatched --write --unsafe --staged src ui",
"test": "bun test --path-ignore-patterns \"**/ui/**\" src && cd ui && bun test",
"test": "bash \"$(git rev-parse --show-toplevel)/scripts/par-test.sh\" 4 bun test --path-ignore-patterns \"**/ui/**\" src && cd ui && bash \"$(git rev-parse --show-toplevel)/scripts/par-test.sh\" 4 bun test",
"verify-types": "tsc --project tsconfig.json --noEmit && tsc --project ui/tsconfig.json --noEmit",
"compile:release": "bun run src/scripts/release.ts"
},
Expand Down
7 changes: 6 additions & 1 deletion apps/client/desktop/ui/src/__tests__/tauri-config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,12 @@ describe("the two test runs", () => {
});

it("runs the web half with ui/ as the cwd, which is how it finds its preload", () => {
expect(pkg.scripts.test).toContain("cd ui && bun test");
// Issue #261: each half runs through scripts/par-test.sh (the --parallel
// flag has to be omittable for CI, which package.json cannot express);
// the second half still executes from ui/ as its cwd.
expect(pkg.scripts.test).toContain(
'cd ui && bash "$(git rev-parse --show-toplevel)/scripts/par-test.sh" 4 bun test',
);
const bunfig = readFileSync(join(import.meta.dir, "../../bunfig.toml"), "utf8");
expect(bunfig).toContain('preload = ["./src/test-setup.ts"]');
});
Expand Down
2 changes: 1 addition & 1 deletion apps/client/mobile/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"lint": "biome check --no-errors-on-unmatched --write --unsafe src app scripts",
"lint:check": "biome check --no-errors-on-unmatched src app scripts",
"lint:staged": "biome check --no-errors-on-unmatched --write --unsafe --staged src app scripts",
"test": "bun test --pass-with-no-tests",
"test": "bash \"$(git rev-parse --show-toplevel)/scripts/par-test.sh\" 4 bun test --pass-with-no-tests",
"verify-types": "tsc --noEmit",
"harness:m1": "bun scripts/m1-transport-harness.ts"
},
Expand Down
2 changes: 1 addition & 1 deletion apps/docs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"lint": "biome check --no-errors-on-unmatched --write --unsafe app lib components src content",
"lint:check": "biome check --no-errors-on-unmatched app lib components src content",
"lint:staged": "biome check --no-errors-on-unmatched --write --unsafe --staged app lib components src content",
"test": "bun test",
"test": "bash \"$(git rev-parse --show-toplevel)/scripts/par-test.sh\" 4 bun test",
"verify-types": "tsc --noEmit"
},
"dependencies": {
Expand Down
2 changes: 1 addition & 1 deletion apps/node/agent/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"lint": "biome check --write --unsafe src && biome format src --write && biome lint src --fix",
"lint:check": "biome check --no-errors-on-unmatched src",
"lint:staged": "biome check --no-errors-on-unmatched --write --unsafe --staged src",
"test": "bun test",
"test": "bash \"$(git rev-parse --show-toplevel)/scripts/par-test.sh\" 12 bun test",
"verify-types": "tsc --project tsconfig.json --noEmit",
"compile": "bun build --compile --bytecode --minify --sourcemap ./src/main.ts --outfile ./dist/subshell",
"compile:release": "bun run src/scripts/release.ts"
Expand Down
10 changes: 8 additions & 2 deletions apps/node/agent/src/__tests__/daemon.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2366,9 +2366,15 @@ describe("the update transaction, settled by the daemon (spec 2026-09-15 §5.1/
// is what proves this plane is talking TO this binary.
await signAndSend(h, { type: "set_allowed_dirs", dirs: [] });
const deadline = Date.now() + 2000;
while (existsSync(previous) && Date.now() < deadline) await sleep(10);
// Wait for BOTH drops, not one then an immediate peek at the other: the
// settle unlinks are not atomic against each other, and on a loaded
// runner (CI's 4-core box under `--parallel=12`) the gap between them
// outlived the single-file wait - the shape of the 2026-09-30 flake,
// `.previous` gone while the marker still read present.
const marker = join(h.config.dataDir, "update-pending.json");
while ((existsSync(previous) || existsSync(marker)) && Date.now() < deadline) await sleep(10);
expect(existsSync(previous)).toBe(false);
expect(existsSync(join(h.config.dataDir, "update-pending.json"))).toBe(false);
expect(existsSync(marker)).toBe(false);
expect(readFileSync(binary, "utf8")).toBe("NEW");
});

Expand Down
2 changes: 1 addition & 1 deletion apps/node/web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"lint": "biome check --no-errors-on-unmatched --write --unsafe src",
"lint:check": "biome check --no-errors-on-unmatched src",
"lint:staged": "biome check --no-errors-on-unmatched --write --unsafe --staged src",
"test": "bun test --pass-with-no-tests",
"test": "bash \"$(git rev-parse --show-toplevel)/scripts/par-test.sh\" 4 bun test --pass-with-no-tests",
"verify-types": "tsc --noEmit"
},
"dependencies": {
Expand Down
35 changes: 20 additions & 15 deletions apps/server/api/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -558,23 +558,28 @@ one path string per process, so the app's Kysely connection and better-auth's
handle share one DB, and concurrent `bun test` invocations cannot contend)
and a throwaway log directory. `NODE_ARTIFACTS_DIR` derives from that temp
data dir under the test env, which is how the download-route tests write
fixtures straight in. All test files within one invocation share
that DB, so suites must not assume it starts empty. Tests never touch
`data/`.

**A run reaps its own tmux servers**: the `test` script prefixes
`TMUX_TMPDIR=$(mktemp -d /tmp/subshell-test-tmux-XXXXXX)` and the
preload's `afterAll` `kill-server`s exactly what that directory lists,
so a concurrent run's panes are outside it. A bare hand-typed `bun test`
sets no such variable and gets no net, which is correct, since it also
gets the shared default socket dir, where killing anything would reach panes
the run never started. The net covers THIS package only;
fixtures straight in. A bare `bun test` runs files serially in one process,
so they all share that DB; the `test` script adds `--parallel=12` (issue
#261), where every file gets a fresh module registry and so its own DB
file. Suites must therefore migrate their own DB (`@/__tests__/helpers/
test-database.js`) and may assume neither an empty one nor a sibling file's
boot. Tests never touch `data/`.

**A run reaps its own tmux servers**: `test` runs `scripts/test-run.sh`,
which owns the run's `TMUX_TMPDIR=$(mktemp -d /tmp/subshell-test-tmux-XXXXXX)`
namespace and, after `bun test` exits, `kill-server`s every socket it lists
and removes the directory. It lives in the script, not the preload, because
issue #261 measured that a preload `afterAll` fires after EVERY file under
`--parallel` - a directory-scoped sweep there would kill sibling workers'
live panes. A bare hand-typed `bun test` gets no net, which is correct,
since it also gets the shared default socket dir, where killing anything
would reach panes the run never started. The net covers THIS package only;
`packages/pane-runtime` and `apps/node/agent` stand on each suite's own
register-before-spawn `afterAll`. The `--timeout 30000` in the script is
measured: this package's DB + better-auth setup against the shared temp
DB is what blows bun's 5 s default under CI load, not the tests. Why
each is shaped this way (the leaked-harness incident, where the variable
may and may not be set, the CI run numbers):
measured: this package's DB + better-auth setup against the temp DB is
what blows bun's 5 s default under CI load, not the tests. Why each is
shaped this way (the leaked-harness incident, the parallel per-file firing
that moved the sweep out of the preload, the CI run numbers):
`apps/server/api/docs/testing-notes.md`.

**A new column on a shared table must also join the HAND-CURATED migration
Expand Down
65 changes: 40 additions & 25 deletions apps/server/api/docs/testing-notes.md
Original file line number Diff line number Diff line change
@@ -1,31 +1,40 @@
# Testing internals: why the tmux reaper, the 30 s timeout and turbo dependsOn are shaped this way. Moved verbatim from AGENTS.md ("Testing"); AGENTS.md keeps the rules and routes here.
# Testing internals: why the tmux reaper, the per-file database, the 30 s timeout and turbo dependsOn are shaped this way. Moved verbatim from AGENTS.md ("Testing"); AGENTS.md keeps the rules and routes here.

**A run reaps its own tmux servers, and that net lives in the `test` script,
not the preload.** The script prefixes `TMUX_TMPDIR=$(mktemp -d
/tmp/subshell-test-tmux-XXXXXX)`; tmux resolves `-L <name>` under it, so the
preload's `afterAll` can `kill-server` exactly what this run started by
listing a directory, and a concurrent run's panes are outside it. What leaks
without it is not a socket file but a live harness: measured 2026-09-14, 17
real `claude` panes at ~220 MB each were alive on a developer's machine, the
oldest a day old, one per full `bun test` since, from
`subshells-local-launch-off.test.ts`, which asserted `not 403 / not 404` on a
create and got a 409 on CI (no claude) and a genuine 200 on a dev box. That
suite now pins `CLAUDE_PATH` at a path that does not exist, which is the real
fix; this is the net under it, for the leak classes a per-file `afterAll`
cannot catch (a launch that throws before its socket is registered, a file
with no reaper, a timeout that ends a file before its hooks).
**A run reaps its own tmux servers, and that net lives in the script that
wraps the `test` command (`scripts/test-run.sh`), not the preload.** The
script mktemps the run's `TMUX_TMPDIR=/tmp/subshell-test-tmux-XXXXXX`
namespace, runs `bun test` with it, and after `bun test` exits
`kill-server`s every socket under it and removes the directory; tmux
resolves `-L <name>` under the variable, so "what this run started" is a
directory listing, and a concurrent run's panes are outside it. Until
issue #261 the sweep itself sat in the preload's `afterAll`. That moved
because of a second measured fact (bun 1.4.0): a preload `afterAll` fires
once per run serially, but after EVERY file under `--parallel`. A
directory-scoped sweep there would kill the panes of sibling workers still
mid-suite. The script outlives every worker, so it owns the kill. What
leaks without the net is not a socket file but a live harness: measured
2026-09-14, 17 real `claude` panes at ~220 MB each were alive on a
developer's machine, the oldest a day old, one per full `bun test` since,
from `subshells-local-launch-off.test.ts`, which asserted `not 403 / not
404` on a create and got a 409 on CI (no claude) and a genuine 200 on a dev
box. That suite now pins `CLAUDE_PATH` at a path that does not exist, which
is the real fix; the net is under it, for the leak classes a per-file
`afterAll` cannot catch (a launch that throws before its socket is
registered, a file with no reaper, a timeout that ends a file before its
hooks).

Two measured facts fix WHERE the variable is set. A child does not see a
`process.env` written after startup (bun 1.4.2): Bun hands a spawned process
the environment this one was STARTED with, so setting it in the preload
reaches `tmuxSocketPath()` in-process and not the tmux client; the socket
would land in `/tmp` while `cleanSocket` unlinked a path in the temp dir,
which is worse than no net. And `/tmp` rather than `$TMPDIR`: on macOS the
latter is a ~49-byte `/var/folders/...` path against a 104-byte socket-path
cap, so the derived socket lands within a few bytes of tmux's bare "File name
too long". A bare hand-typed `bun test` sets no such variable and gets no
net, correctly, since it also gets the shared default socket dir, where
killing anything would reach panes this run never started.
which is worse than no net. The script sets it before `bun test` starts, so
every worker and every child inherits it. And `/tmp` rather than `$TMPDIR`:
on macOS the latter is a ~49-byte `/var/folders/...` path against a 104-byte
socket-path cap, so the derived socket lands within a few bytes of tmux's
bare "File name too long". A bare hand-typed `bun test` sets no such
variable and gets no net, correctly, since it also gets the shared default
socket dir, where killing anything would reach panes this run never started.

**The net covers this package only.** `packages/pane-runtime` and
`apps/node/agent` also spawn real tmux and run a bare `bun test`, so nothing
Expand All @@ -38,8 +47,9 @@ extending the variable to those packages is a change to how their sockets are
named, not a line of cleanup.

**The `--timeout 30000` in the `test` script is measured, not caution.**
This package's suites set up against that shared DB through migrations and
better-auth table creation, and bun's 5000 ms per-test/hook default blew
This package's suites set up against that temp DB through migrations and
better-auth table creation (each file's own DB under `--parallel`), and
bun's 5000 ms per-test/hook default blew
three times in three CI runs, each time in a different file, which is the
signature of load, not of a bug: the auth-registration `beforeAll` at
8830 ms (run 34581907693), the heaviest `default-profiles` case (suite since
Expand Down Expand Up @@ -67,7 +77,12 @@ correct and this package now inherits it.

## The testing history this replaced (moved from AGENTS.md "Testing")

All test files within one invocation share that DB, so suites must not
assume it starts empty. Tests never touch `data/`. (It used to be the URI string `file::memory:?cache=shared`, but
Serial `bun test` (one process, shared module cache) gives every file the
same DB, so a suite may never assume it starts empty; under the `test`
script's `--parallel=12` the opposite holds, every file gets a fresh module
registry and its own DB file, so a suite may never assume another file's
migrations happened either - DB-touching suites call
`@/__tests__/helpers/test-database.js` explicitly. Tests never touch
`data/`. (The path used to be the URI string `file::memory:?cache=shared`, but
Bun treats URI strings as file names; every suite was sharing one literal
CWD file.)
2 changes: 1 addition & 1 deletion apps/server/api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"lint:check": "biome check --no-errors-on-unmatched src",
"lint:staged": "biome check --no-errors-on-unmatched --write --unsafe --staged src",
"prod": "bun run ./dist/index.js",
"test": "TMUX_TMPDIR=$(mktemp -d /tmp/subshell-test-tmux-XXXXXX) bun test --timeout 30000 src",
"test": "bash scripts/test-run.sh",
"verify-types": "tsc --noEmit"
},
"dependencies": {
Expand Down
39 changes: 39 additions & 0 deletions apps/server/api/scripts/test-run.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#!/usr/bin/env bash
# The test runner @internal/server's `test` script wraps.
#
# Owns the run's throwaway tmux namespace: TMUX_TMPDIR is set for the whole
# `bun test` (children inherit the STARTUP env - a preload cannot re-home it
# later, measured on bun 1.4.2), so tmux resolves `-L <name>` under it and
# "which servers did this run start" is a directory listing. This file lived
# inside `src/test-preload.ts`'s afterAll until issue #261: under
# `bun test --parallel` a preload afterAll fires after EVERY file (measured
# on 1.4.0), and a sweep scoped by directory would kill the panes of sibling
# workers still mid-suite. The script runs after every worker has exited,
# serial or parallel.
#
# The DB-file sweep stays in the preload: it is scoped by this process's PID
# prefix, so a per-file firing is exactly its right moment there.
#
# --parallel (issue #261): worker processes for the run's files. Default 12,
# measured on this repo's suite at 48 cores; SUBSHELL_TEST_PARALLEL overrides
# the width and SUBSHELL_TEST_SERIAL=1 omits the flag (CI's regime - its
# container starves parallel workers; see scripts/par-test.sh). bunfig's
# `parallel` key is silently ignored by bun (1.4.0 and 1.4.2), so the flag
# lives here and in every package's `test` script, and a bare hand-typed
# `bun test` stays serial (the preload above still applies to it). Trailing
# args pass through to `bun test` (CI appends --shard for its matrix legs).
set -u
par="--parallel=12"
[ -n "${SUBSHELL_TEST_PARALLEL:-}" ] && par="--parallel=$SUBSHELL_TEST_PARALLEL"
[ -n "${SUBSHELL_TEST_SERIAL:-}" ] && par=""
dir="$(mktemp -d /tmp/subshell-test-tmux-XXXXXX)"
status=0
TMUX_TMPDIR="$dir" bun test $par --timeout 30000 src "$@" || status=$?
if command -v tmux >/dev/null 2>&1; then
for sock in "$dir"/tmux-*/*; do
[ -e "$sock" ] || continue
tmux -L "${sock##*/}" kill-server 2>/dev/null
done
fi
rm -rf "$dir"
exit "$status"
20 changes: 20 additions & 0 deletions apps/server/api/src/__tests__/helpers/test-database.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { runAuthMigrations } from "@/db/auth-migrations.js";
import { runMigrations } from "@/db/migrate.js";

/**
* Ensures the test database carries both the app's tables and better-auth's,
* via the same runners the server calls at boot. Both are idempotent (each
* tracks applied state inside the database itself), so this is a full build
* on a pristine file and a fast no-op afterwards.
*
* Every suite that touches `db` - directly or through a service - must call
* this in `beforeAll`. Until issue #261 the rule was implicit: serial `bun
* test` shares one module cache across files, so one early file's migrations
* served every later file in the process. `--parallel` isolates each file
* (fresh `db` singleton, fresh database file), and the implicit-boot suites
* surfaced it as `no such table` - 14 suites, measured 2026-09-29.
*/
export async function ensureMigratedTestDb(): Promise<void> {
await runMigrations();
await runAuthMigrations();
}
19 changes: 19 additions & 0 deletions apps/server/api/src/__tests__/helpers/until.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* Poll `cond` until it is true, throwing after `budgetMs`.
*
* Command dispatch through the node link is JWS-signed before it is sent, so
* "assert the frame landed" cannot be a fixed `setTimeout` peek: under CI load
* (4-core runner, `--parallel`) the chain is slow-but-fine and a 5 ms nap
* reads it as nothing-sent - the shape of the 2026-09-30 flake in
* `held-connections.test.ts`. The budget keeps a real regression red rather
* than merely late. (Two ws integration suites carried private copies of
* this; this is the third, extracted at the point the duplication stopped
* paying rent.)
*/
export async function until(cond: () => boolean, what = "condition", budgetMs = 4000): Promise<void> {
for (let waited = 0; ; waited += 5) {
if (cond()) return;
if (waited > budgetMs) throw new Error(`timed out waiting for ${what}`);
await new Promise((r) => setTimeout(r, 5));
}
}
Loading
Loading