Skip to content

chore: take upstream pi's toolchain split for install, checks, and tests - #2079

Merged
flora131 merged 18 commits into
mainfrom
chore/pi-toolchain-parity
Jul 30, 2026
Merged

chore: take upstream pi's toolchain split for install, checks, and tests#2079
flora131 merged 18 commits into
mainfrom
chore/pi-toolchain-parity

Conversation

@flora131

@flora131 flora131 commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Takes upstream earendil-works/pi's toolchain split task for task, not just its test runner. npm installs, builds, checks, and runs the suites; vitest replaces bun test for the three root suites; Bun keeps exactly the two jobs pi also gives it — compiling release binaries and running scripts/*.ts.

The gate: does npm/Node preserve raw-TypeScript extension loading?

Yes for the shipped loader and for tests; no for bare node. The boundary, not the yes/no, is the design constraint:

.js.ts specifier rewriting for raw-TypeScript packages survives under Node only through a transpiling loader. jiti provides it in production; Vite provides it under vitest. Node's own resolver and type-stripping do not. Therefore no entrypoint invoked by bare node may depend on it.

Verified on a real npm ci tree: workspace layout is identical to Bun's hoisted linker (all seven node_modules/@bastani/* are symlinks into packages/*), jiti + Node loads packages/workflows/src/durable/factory.ts out of a 316-file graph, and the Bun runtime loads the same tree unchanged. Changing the installer does not change the runtime — which is exactly why pi can install with npm and still compile binaries with Bun. loader-virtual-modules.ts is untouched. scripts/*.ts therefore stay on bun run; the one script covered by node --test is .mjs.

Scope migrated

Before After
Install bun install --frozen-lockfile, bun.lock npm ci --ignore-scripts, package-lock.json
Build / check bun run build, bun run typecheck npm run build, npm run check
Root suites bun test --timeout 30000 test/{unit,integration,ci} vitest --run --project {unit,integration,ci}
coding-agent suite bun run --cwd packages/coding-agent --bun test npm run test --workspace=@bastani/atomic (Node)
Script tests none node --test scripts/*.test.mjs
Binaries bun build --compile unchanged, Bun pinned 1.3.14

Also migrated in the same PR, so nothing is left contradictory: every .github/workflows/test.yml job and publish.yml (which would otherwise install from a deleted bun.lock), prek.toml hooks renamed off the bun- prefix, engines (node >=22.19.0, bun >=1.3.14), packageManager: bun@1.3.14 removed so Corepack cannot drive npm through Bun, and AGENTS.md / CONTRIBUTING.md / DEV_SETUP.md / docs/ci.md / packages/coding-agent/docs/development.md.

The package-manager decision, and what happens to the security control

No security control is dropped. bunfig's [install] block ports 1:1 to a committed .npmrc:

bunfig .npmrc Note
minimumReleaseAge = 259200 min-release-age=3 seconds → days; verified present in npm 11.17.0
minimumReleaseAgeExcludes min-release-age-exclude[] same three pi packages, same semantics
exact = true save-exact=true
linker = "hoisted" hoisted is npm's only layout; the resulting tree is identical, symlinks included
saveTextLockfile = true moot; package-lock.json is already reviewable JSON

Because npm ci installs the lockfile verbatim, the age gate only binds when the lockfile is updated. Two reinforcements: the .npmrc is committed, so it binds every contributor's npm install rather than only CI, and .github/dependabot.yml gains cooldown: 3 on all three ecosystems so automated bumps cannot outrun it.

This is also a defect fix. package-lock.json was already tracked and already the input to the published shrinkwrap, and npm ci failed on main (prek@0.4.5 vs 0.4.10, vitest@4.1.9 vs 4.1.10, tsx, @napi-rs/cli). The repository maintained two lockfiles and verified one, while the unverified one fed a published artifact. Regenerating it changes 62 already-implied devDependency versions — the notable ones are prek 0.4.5→0.4.10, vitest 4.1.9→4.1.10, tsx 4.23.0→4.23.1, @napi-rs/cli 3.7.2→3.7.3, vite 8.1.0→8.1.5 — called out here rather than left to ride inside a large lockfile diff.

Distinct test counts

Diffed on test name, not file + name: Bun attributes barrel-re-exported tests to the importing file, so file attribution legitimately changes. scripts/compare-test-inventory.mjs is committed and gates on an empty symmetric difference plus a floor.

Suite Before After Symmetric difference
test/unit 4417 (579 files) 4417 (579 files) empty (8 reviewed renames)
test/integration 289 289 empty
test/ci 32 33 empty; +1 new contract
packages/coding-agent 2928 2928

The brief's 4426/581 does not reproduce; the merge-base measures 4417/579, and that frozen number is the contract. Both skipped tests are still skipped, asserted by --expect-skipped.

Vitest reports 5398 records for those 4417 distinct unit names: Bun's single process caches a shared module so a test defined there registers once, while vitest isolates files so it registers per importer. That is more execution, not more coverage — hence the name-level diff.

The eight reviewed renames (all in the two rewritten guard suites)
Old New
duration parsing attributes each sample to its file and ignores non-test lines …and skips tests that never ran
GitHub Actions log groups do not become part of the test file path reported absolute file paths are normalised to repository-relative posix paths
an explicit budget still resolves from output wrapped in Actions log groups …from the file path the report names
a run that prints no per-test durations is reported blind, not clean a run that reports no per-test durations…
the spawned suite never inherits Bun's per-test record suppression every reported test is scored, including ones that printed no duration under Bun
a passing no-retry file header does not suppress an unrelated flake retry a passing no-retry file does not…
the wrapper scores real Bun output under the agent-quiet and Actions reporters the wrapper scores a real vitest run through the JSON reporter it requests
the wrapper scores real Bun output behind the bun run <script> form CI uses …behind the npm run <script> form CI uses

Each old name described Bun-stdout scraping that no longer exists; each new one asserts the equivalent property of the JSON reporter. No assertion was deleted.

Wall clock

Measured on the same machine, /usr/bin/time -p, natives built.

Suite Before (Bun) After (vitest/Node)
test/unit 118.21 s 67.65 s
test/integration 10.79 s 8.42 s
test/ci 1.80 s 1.98 s
Root suites total 130.80 s 78.05 s
packages/coding-agent 33.91 s 32.57 s

Each suite was run twice back to back to expose order dependence; counts and results were identical.

Per-API Bun replacements

A lexer distinguished real code from Bun API names appearing inside child-process source strings. That materially narrowed the work: all four Bun.serve calls are inside bun --eval scripts and need no replacement at all, and so are 7 Bun.sleep, 4 Bun.file, 3 Bun.write, 3 Bun.spawnSync and 3 Bun.spawn occurrences.

API Real calls Replacement Why a helper rather than a direct port
Bun.sleep 169 sleepnode:timers/promises
import.meta.dir 60 moduleDir(import.meta.url) undefined under Node
Bun.file(p).text/json/exists 36 readText / readJson / fileExists Bun.file is lazy, helpers eager; readJson<T> returns unknown, not any
process.execPath (spawning Bun) 29 bunExecutable() under vitest it is Node, so a .ts child silently ran under the wrong runtime
Bun.spawnSync 15 spawnSyncCollect Node returns status; a direct port makes every assert.equal(r.exitCode, 0) compare against undefined and pass on a failed child
Bun.spawn 14 spawnProcess no .exited promise, no web-stream stdio, no stdin.flush(), and ENOENT reported asynchronously so a caller's catch loses the code
Bun.write 10 writeFileEnsuringDir Bun.write creates parent directories; fs.writeFile throws ENOENT
Bun.Glob 2 tinyglobby already installed
Bun.Archive 2 tar-stream (new devDependency) restores symmetry with the JSZip half of the same file
Bun.YAML.parse 1 yaml, promoted to an explicit devDependency
import { $ } from "bun" 1 file spawnSyncCollect the only bun module import; the commands, bun pm pack included, are unchanged

Two Bun surfaces were left alone on purpose. test/unit/fixtures/blocking-tool-extension.ts is only ever passed as a path to a Bun-spawned CLI, and the seven packages/ files behind isBunBinary/isBundledBuild are shipped runtime code.

One of those shipped files needed handling rather than editing: packages/web-access/subprocess.ts calls Bun.spawn/Bun.sleep unguarded, and its five unit tests import it in-process. Re-executing that file under Bun would have collapsed five distinct test names into one wrapper assertion — a real coverage loss to work around a runtime detail — so installBunGlobal() supplies the two primitives instead. All five names and all five assertions survive; what is no longer covered is Bun's own spawn implementation, and the product file is untouched.

Duration guard: rewritten, not retired

The brief expected vitest output to trip the guard's blind state. It would not — and the truth is worse. resolveDefaultTimeoutMs accepted only a bun/bunx binary whose subcommand was test, so under vitest it returned undefined, enabled became false, and blind was unreachable: the guard silently became a no-op. That was already live, because agent-suite wrapped bun run --cwd packages/coding-agent --bun test, which resolves to vitest. That job's gate has been scoring nothing for as long as it has existed.

Rewriting it strictly improves it:

Bun stdout parser vitest JSON reporter
Unit tests scored 4288 of 4417 (97.1 %) 5396 of 5398 (the 2 skipped carry no duration)
File attribution wrong for ~67 barrel-re-export files correct
Budget source regex over bun test --timeout resolved testTimeout from the config the command selects
blind unreachable meaningful for the first time

Deleted: the four stdout regexes, ranTestCount, perTestReportingEnv/QUIET_REPORTER_ENV, the ::group:: handling, and the bun/bunx budget allowlist. Kept unchanged: WARN_RATIO 0.4, FAIL_RATIO 0.7, the sort, renderDurationTable, .ci-diagnostics/<suite>-durations.md written on green runs too, and the declaredTimeouts source scan with its scope > name qualification. scripts/run-flaky-test-suite.ts keeps retry-once, --no-retry-file, per-attempt logs, the debug dump, the annotations, and all-attempt scoring; only its input contract changed. It requests --reporter=default --reporter=json so the step log stays diagnosable while the gate gets a machine-readable report, and it adds those flags itself so the CI command stays byte-identical to what a developer runs.

End-to-end on this branch: Samples: 33 of 33 test(s) run, budget 30000 ms resolved through npm run test:ci-contracts → config. Against the full unit suite: 0 failures, 0 warnings, worst test at 33.7 % of budget.

Both guard suites were rewritten alongside, including two tests that drive a real vitest run through the real wrapper — one invoking vitest directly, one through npm run <script>.

Contracts re-pointed

  • ci-workflow-contracts: the --timeout 30000 assertion becomes "the three projects resolve to one identical testTimeout in [30000, 120000]", by importing the config. The intent — one platform-neutral budget, declared once, identical locally and in CI — is preserved verbatim, and it now also rejects a --timeout flag reappearing in any script.
  • test-workflow-topology: updated for the new child commands, keeping the --parallel|--shard|--concurrent|--max-concurrency prohibition, which matters more now that vitest parallelises by default and sharding is the tempting way to hide a load-sensitive test. Adds a new contract that every work job installs with npm ci --ignore-scripts and pins both runtimes it uses.
  • The two required contexts are untouched. The test gate job's id, name:, matrix values and if: always() are unchanged; it runs no install, build, or test.

Tests fixed for load sensitivity

  1. subagents-attempt-watchdog.test.ts — three sites set a 600/700 ms wall-clock cap and asserted the literal. The fragility was never the stalled attempt (that child stalls forever, so detection is deterministic at any cap) but the healthy fallback attempt, which must spawn a real CLI and print inside the same window. Replaced with a named STALLED_ATTEMPT_CAP_MS = 2_500, with each assertion derived from the constant. Strictly more robust, not weaker: ~4× headroom on the healthy path, detection unchanged. No skip, no fileParallelism: false, no serialization.

  2. subagents-async-event-journal.test.ts — surfaced a genuine crash vector. runPiStreaming creates its transcript WriteStream with no error listener, so any failure to open or write it becomes an uncaught exception that kills the host process, asynchronously and long after the call returned. Under load the open landed after the test's scratch directory was removed and failed the whole run. Fixed at the source, in packages/subagents/src/runs/background/subagent-runner-streaming.ts: the transcript is diagnostic, so it now degrades with a logged warning exactly as the child-event journal beside it already does. This is the only shipped file changed for behaviour.

  3. slash-dispatch-resume.ts had a duplicate import { InMemoryDurableBackend }. Bun tolerated the repeated binding; every standards-conforming parser rejects it. Latent bug, removed.

  4. web-access-subprocess.test.ts and subagents-pi-spawn.test.ts needed the Bun runtime named explicitly rather than inherited from process.execPath — the shipped resolver only treats a .ts CLI as runnable when the exec path is Bun, which is precisely what those tests assert.

No test was skipped, disabled, weakened, or deleted, and the assertion style stays node:assert/strict throughout.

Deliberately declined

pi's CI is one ubuntu-latest job with no matrix and no timeout-minutes. Copying it would delete Windows coverage, orphan the two required check contexts, and break the per-job budgets test-workflow-topology.test.ts asserts. Parity is a toolchain goal, not a CI-topology goal, and AGENTS.md now says so.

One deviation from the plan, on evidence: oven-sh/setup-bun stays in static-checks. The plan had it removed, but that job runs npm run docs:check, which is bun run scripts/validate-docs-links.ts. Every work job now sets up both runtimes, and the new topology contract asserts it.

Left for follow-up

  • Codemod from "bun:test"from "vitest" across 629 files and delete the shim alias. Deliberately separate: bundling a 629-file mechanical rewrite with a package-manager swap, a CI rewrite and a guard rewrite would make behavioural regressions indistinguishable from churn. scripts/compare-test-inventory.mjs is committed so the same proof can be re-run when the shim goes.
  • Adopt Biome for lint and format, as pi does, and fold it into npm run check. It would reformat effectively every file and drown this diff. Until then lint is an alias for check rather than pretending to be a linter.
  • The 11 packages/coding-agent tests that fail without built native bindings fail identically under Bun and Node — pre-existing, unrelated, and green once packages/natives is built (as CI does).

Validation

bun run typecheck                              clean
bun run lint                                   clean
vitest --project unit          x2   579 files  5396 passed | 2 skipped   67.65 s / 66.35 s
vitest --project integration   x2    34 files   469 passed | 1 skipped    8.42 s
vitest --project ci            x2     4 files    33 passed                1.98 s
npm run test --workspace=@bastani/atomic  x2   2898 passed | 30 skipped  32.57 s
npm run test:scripts                            3 passed
npm ci --ignore-scripts (clean checkout)        2.9 s, workspace symlinks intact
run-flaky-test-suite.ts -- npm run test:ci-contracts   33/33 scored, budget 30000 ms

Repair pass — independent verification findings

Reviewed against a full independent reproduction. Every finding is fixed in substance; nothing was argued away.

Blocking: the bun:sqlite loss was eleven tests, not one

src/core/tools/resource-selectors.ts loads bun:sqlite through createRequire and throws "SQLite selectors require Atomic's Bun runtime" without it. Moving packages/coding-agent off bun --bun test therefore did not fail its SQLite suites — it emptied them:

File Shape Effect under Node
resource-selector-tools.test.ts (loadBunSqlite() ? it : it.skip) 1 test skipped (the 29→30 skip the reviewer found)
resource-selector-tools.test.ts if (sqlite) { … } around 2 expects test green, 2 assertions dead
read-sqlite-search-meta-parity.test.ts const mod = sqlite(); if (!mod) return; 5 tests green, 0 assertions
resource-write-parity-edges.test.ts same 2 tests green, 0 assertions
resource-selector-hardening.test.ts same 2 tests green, 0 assertions

The last three files were invisible to the skip count and to the name diff, because an early return is not a skip. Running all four under Node with the guards made hard fails exactly 11 tests, which is the size of the hole.

Fix. Those four files now form a Bun-hosted vitest project:

npm run test --workspace=@bastani/atomic       # vitest --run --project agent      (Node)
npm run test:bun --workspace=@bastani/atomic   # bun --bun vitest --project agent-bun

agent excludes exactly the files agent-bun collects, so the new agent-suite CI step is coverage, not a repeat. The guards are hard requires again through test/helpers/bun-sqlite.ts, which throws and names the right command. test/ci/ci-workflow-contracts.test.ts makes the split structural rather than a convention: every test file naming bun:sqlite must be in BUN_HOSTED_TESTS, collected by agent-bun, excluded from agent, sharing one testTimeout, carrying no if (!sqlite) return and no ? it : it.skip, and CI must run the step.

scripts/test-duration-guard.ts now steps over a leading bun/bunx so the new step is scored like every other suite — resolved at 30000 ms, 47 of 47 samples, no blindness. Only the runtime's own leading flags are dropped: an earlier draft filtered every - argument, which swallowed --project and made the guard fall back to whichever projects happened to agree. That bug has its own test.

Blocking (process): the migration's own guard, pointed at all four suites

scripts/compare-test-inventory.mjs was committed unreferenced and was never run against the suite that regressed. It now:

  • takes a repeatable --candidate and unions the parts, because a suite split across runtimes must be compared whole — pointing it at one half of packages/coding-agent is precisely how the loss stayed hidden (that half alone reports 47 missing names);
  • diffs the skip set as well as the name set, so a test that ran before and skips now fails the comparison instead of keeping its name and its green tick;
  • auto-detects a Bun stdout log or a vitest JSON report as the baseline.

Its rules are covered by scripts/compare-test-inventory.test.mjs, which CI executes in static-checks via npm run test:scripts. The comparison itself stays a migration-time gate — it needs a baseline captured from the runner being replaced, and after this merges there is none — and AGENTS.md records the four invocations and that reason.

Re-run against a baseline captured at HEAD~1 under Bun:

Suite Baseline Candidate Missing Newly skipped Skipped
test/unit 4417 4423 0 0 2 → 2
test/integration 289 289 0 0 1 → 1
test/ci 32 34 0 0 0 → 0
packages/coding-agent 2893 2893 (agentagent-bun) 0 0 29 → 29

29, not the 30 the Node-only run produced. The eight unit renames are the allowlist already documented above; test/ci carries one more (every Bun test suite entry point declares one shared per-test timeoutevery test suite entry point resolves to one shared per-test timeout). The six new unit names are the tests added by this repair.

Everything else

  • Named timeout constants. test/unit/flaky-test-suite-runner.test.ts uses REAL_VITEST_SUITE_TIMEOUT_MS at both structural call sites. The PR required this in AGENTS.md and then broke its own rule with two bare 120_000 literals.
  • Aliased declarations have a fixture again. declarationPattern still reads const runTest = built ? test : test.skip; the fixture covering it was dropped in the rewrite. Restored, in both argument shapes, plus an it alias.
  • Both uncovered guard paths. run-flaky-test-suite.ts now treats an unreadable report as blind, not only a missing one — a corrupt report measures exactly as much as no report, and would otherwise have thrown inside the scorer. Two fixture modes cover what nothing did: a suite that writes no report at all, and a corrupt report whose deterministic failure is found by the log scan in findFailedDeterministicFile.
  • setDefaultTimeout does what it says. It clamps with Math.min against the declared budget instead of an unconditional vi.setConfig. TEST_TIMEOUT_MS moved to a leaf module so the shim does not pull vitest/config into 629 files' workers for one number; the clamp is unit-tested.
  • bumpNpmLock is exercised. test/unit/bump-version-script.test.ts's fixture root has a package-lock.json, so the branch is entered: workspace entries and first-party ranges stamped, third-party pins and link entries untouched, 3 workspace entries reported. The no-lockfile case is kept as an explicit test rather than the accidental default.
  • WARN_RATIO is unchanged, and AGENTS.md now says why it must stay so: vitest's transform cost put the slowest unit test just under the 40 % warn line, and a warning there is the gate working.
  • The Bun.spawn fidelity gap is stated for reviewers, in AGENTS.md as well as the helper: installBunGlobal() keeps all five web-access-subprocess names and assertions and everything subprocess.ts itself does, but Bun's own spawn implementation — what the shipped binary runs on — is no longer covered by those tests.
  • Docs follow: docs/ci.md, DEV_SETUP.md, packages/coding-agent/docs/development.md. A stale paragraph describing the removed 500-line file-length gate is gone.

Validation after the repair

npm run check                                        clean (tsc + shrinkwrap)
npm run test:unit                        580 files   5402 passed | 2 skipped
npm run test:integration                  34 files    469 passed | 1 skipped
npm run test:ci-contracts                  4 files     34 passed
npm run test --workspace=@bastani/atomic  370 files   2852 passed | 29 skipped
npm run test:bun --workspace=@bastani/atomic         47 passed | 0 skipped
npm run test:scripts                                 13 passed
npm ci --ignore-scripts --dry-run                    clean
run-flaky-test-suite.ts -- npm run test:bun ...      47/47 scored, budget 30000 ms
compare-test-inventory.mjs, all four suites          0 missing, 0 newly skipped

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Adopt earendil-works/pi's task-for-task toolchain rather than only its test
runner. npm installs, builds, checks, and runs the suites; vitest replaces
`bun test` for the three root suites; Bun keeps exactly the two jobs pi also
gives it, compiling release binaries and running `scripts/*.ts`.

Install moves to `npm ci --ignore-scripts` against a regenerated
`package-lock.json`, and `bun.lock` is deleted. The repository tracked both
lockfiles and only verified one: `npm ci` failed on `main` because the tracked
lock had drifted from package.json, while that same unverified lock is the input
to the shrinkwrap published inside @bastani/atomic. One verified source of truth
closes that gap. bunfig.toml's supply-chain gate ports 1:1 to a committed
.npmrc (`min-release-age=3`, `min-release-age-exclude`, `save-exact`), with a
matching dependabot `cooldown` so automated bumps cannot outrun it.

The three root suites move to vitest projects sharing a pi-shaped
`vitest.base.ts` that sets only `resolve.alias`. A `bun:test` alias lets 629
test files migrate unedited; the 95 files using `Bun.*`, `import.meta.dir`, or a
Bun-spawning `process.execPath` move onto `test/helpers/runtime.ts`, whose
helpers exist mainly to close the differences that fail silently (`Bun.write`
creating parent directories, `spawnSync` returning `status` rather than
`exitCode`, `Bun.spawn` refusing a missing binary synchronously).

The duration guard is rewritten for vitest's JSON reporter rather than retired.
Under Bun's stdout it scored 4288 of 4417 unit tests and mis-attributed
barrel-re-exported files; it now scores 5396 of 5398 records with correct
attribution, and its `blind` state finally means the harness broke instead of
being unreachable. The flaky runner keeps every behaviour and changes only its
input contract.

Distinct test names are unchanged: 4417 unit, 289 integration, 32 -> 33 ci,
with an itemised eight-rename allowlist proved by scripts/compare-test-inventory.mjs.

Assistant-model: Claude Opus 5
@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown

Too many files changed for review. (1799 files found, 100 file limit)

@mintlify

mintlify Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
bastani 🟢 Ready View Preview Jul 30, 2026, 6:22 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

Comment thread test/unit/assert-builtin-archive-set.test.ts Fixed
…guard at them

Repairs the toolchain migration against independent verification.

The blocking loss was larger than reported. Moving packages/coding-agent from
`bun --bun test` to Node did not skip one test; it silently emptied eleven.
`src/core/tools/resource-selectors.ts` loads `bun:sqlite` and throws without it,
and its tests guarded that with `if (!sqlite) return` or `? it : it.skip` — so
under Node one declaration skipped and ten more kept their names, kept passing,
and ran no assertions. Running the four affected files under Node with hard
requires fails exactly 11 tests, which is the size of the hole.

Those four files now form a Bun-hosted vitest project (`agent-bun`), run by a
new `npm run test:bun --workspace=@bastani/atomic` and a new agent-suite CI step.
The Node project (`agent`) excludes exactly the files the Bun one collects, so
the second step is coverage rather than a repeat. The guards are hard requires
again via test/helpers/bun-sqlite.ts, which throws and names the command to use.
test/ci/ci-workflow-contracts.test.ts makes it structural: every test file naming
`bun:sqlite` must be in BUN_HOSTED_TESTS, collected by `agent-bun`, excluded from
`agent`, sharing one testTimeout, with no early-return guard or `? it : it.skip`
left in it, and the CI step must exist.

scripts/test-duration-guard.ts steps over a leading `bun`/`bunx` so the new step
is scored like every other suite. Only the runtime's own leading flags are
dropped — filtering every `-` argument swallowed `--project` and made the guard
average whichever projects happened to agree.

scripts/compare-test-inventory.mjs was committed unreferenced and was never
pointed at the suite that regressed. It now takes a repeatable `--candidate`
(a suite split across runtimes is compared as the union of its parts; either
half alone reads as a loss), auto-detects a bun log or a vitest report as the
baseline, and diffs the *skip* set as well as the name set — a test that ran
before and skips now keeps its name and keeps the suite green. Its rules are
covered by scripts/compare-test-inventory.test.mjs, which CI runs in
static-checks via `npm run test:scripts`. AGENTS.md records the four invocations
and why the comparison itself stays a migration-time gate.

Evidence, all four suites, baseline captured at HEAD~1 under Bun:

  unit         4417 -> 4423 distinct names, 0 missing, 0 newly skipped,
               2 skipped both sides, 8 reviewed renames
  integration   289 ->  289, 0 missing, 0 new, 0 newly skipped, 1 skipped both
  ci             32 ->   34, 0 missing, 0 newly skipped, 1 reviewed rename
  coding-agent 2893 -> 2893 (union of agent + agent-bun), 0 missing, 0 new,
               0 newly skipped, 29 skipped both sides — not the 30 Node produced

Also fixed:

- test/unit/flaky-test-suite-runner.test.ts names REAL_VITEST_SUITE_TIMEOUT_MS
  at both structural call sites, as AGENTS.md requires and the PR did not do.
- The aliased-declaration branch of `declarationPattern` has a fixture again
  (`const runTest = built ? test : test.skip`), in both argument shapes.
- run-flaky-test-suite.ts treats an unreadable report as blind, not just a
  missing one, and two fixture modes cover the paths that had none: a suite that
  writes no report at all, and a corrupt report whose deterministic failure is
  found by the log scan in `findFailedDeterministicFile`.
- bun-test-shim's `setDefaultTimeout` clamps with Math.min instead of claiming
  to. TEST_TIMEOUT_MS moved to a leaf module so the shim does not pull
  `vitest/config` into 629 test files' workers; the clamp is unit-tested.
- test/unit/bump-version-script.test.ts's fixture root has a package-lock.json,
  so `bumpNpmLock` is entered: workspace entries and first-party ranges stamped,
  third-party pins and link entries untouched, plus the no-lockfile case.

WARN_RATIO is unchanged and must stay so; AGENTS.md now says why, alongside the
Bun.spawn fidelity gap in test/unit/web-access-subprocess.test.ts, which
`installBunGlobal()` closes for the module's own logic but not for Bun's spawn.
docs/ci.md, DEV_SETUP.md and packages/coding-agent/docs/development.md follow,
and a stale paragraph describing the removed file-length gate is gone.

Assistant-model: Claude Opus 5
The SQLite selectors loaded `bun:sqlite`, which exists only under Bun. Moving
the suites to Node did not fail them, it emptied them: one test became it.skip
and eleven kept their names, kept passing, and ran no assertions behind
`if (!sqlite) return`. The previous commit quarantined those four files onto a
Bun-hosted vitest project. This replaces the quarantine with a loader that
works on both runtimes.

`sqliteDatabase()` now tries `node:sqlite` first and falls back to
`bun:sqlite`:

- node:sqlite is unflagged from Node v22.13.0 and is the module upstream pi
  uses (packages/storage/sqlite-node); it lets the selectors and their tests
  run under Node.
- Bun 1.3.14 does not ship it. oven-sh/bun#32498 is merged but unreleased, and
  the shipped binary is Bun-compiled, so the fallback is what keeps that binary
  working. When Bun releases node:sqlite both runtimes take the first branch
  and the fallback can be deleted.
- better-sqlite3 was evaluated and rejected: it segfaults Bun 1.3.14 on
  construction, which is worse than a catchable missing-module error.

Two deltas are absorbed so callers see no change: node:sqlite spells the option
readOnly and rejects it passed explicitly as undefined, and it refuses to bind
the booleans bun:sqlite stores as integer 1/0 (normalizeSqliteWriteValue
permits booleans).

Removed with the quarantine: the agent-bun project and BUN_HOSTED_TESTS, the
test:bun script, the separate CI step, and the hard-require test helper. Test
fixtures now go through test/helpers/sqlite.ts, which mirrors the same
preference order behind the bun:sqlite-shaped API the suites were written
against.

The CI contract is rewritten rather than dropped: it now asserts the loader
order, a single vitest project, that no SQLite test is excluded from
collection, and that no soft guard returns. Docs updated in AGENTS.md,
docs/ci.md and development.md.

Verified on both runtimes: the four SQLite files pass 47/47 under `vitest`
(Node) and 47/47 under `bun --bun vitest`. Full coding-agent suite 2899 passed
/ 29 skipped, restoring the pre-migration skip count of 29 that the quarantine
pass had left at 30. Root suites: unit 5402 passed / 2 skipped, integration 469
passed / 1 skipped, ci-contracts 34 passed, script tests pass, typecheck clean.

Assistant-model: Claude Opus 5
flora131 added 5 commits July 30, 2026 13:16
scripts/compare-test-inventory.mjs proved this migration shed no test names,
and it required a baseline captured from the runner being replaced. Once the
migration lands there is no such runner, so the script cannot run again, and
AGENTS.md already documented it as a migration-time gate rather than a CI step.

Removed with its unit test and its AGENTS.md section. The coverage evidence it
produced is recorded in the PR body.

Assistant-model: Claude Opus 5
Completes the pi practice the migration deferred: test files import from
"vitest", not from "bun:test" through an alias.

Codemodded 631 files. The shim was not a pure re-export, so the mechanical
rename carried four adaptations with it:

- `.serial` -> `.sequential` (241 declarations); vitest spells Bun's in-file
  ordering modifier differently.
- `mock(...)` -> `vi.fn(...)`, `mock.restore()` -> `vi.restoreAllMocks()`.
- `spyOn(...)` -> `vi.spyOn(...)`; vitest has no top-level export.
- `setDefaultTimeout(30_000)` dropped: it equalled TEST_TIMEOUT_MS, so the
  shim's clamp made it a no-op already.

Five files genuinely need Bun's module registry and keep `bun:test`, because
they re-exec their bodies under `bun test` in a child process where the
specifier resolves natively:

- overlay-adapter-autowrap and overlay-adapter-hidden-render now take `mock`
  from a dynamic `await import("bun:test")` inside the child-only function, so
  the parent no longer needs the alias to load them.
- mcp-oauth-lifecycle-reset, the session-manager preload fixture and the 2791
  fswatch regression keep a static import, because theirs sits inside a
  generated child script or a Bun preload rather than in the parent module.

Deleted test/helpers/bun-test-shim.ts, its unit test, and the `bun:test` alias
in vitest.base.ts. Docs updated in AGENTS.md, DEV_SETUP.md and the vitest
configs; the AGENTS.md example now imports from "vitest".

Also aligned the root `test` script with pi's shape: it was running only the
unit project, and now runs `test:scripts`, every vitest project, and the
workspace suites, matching `npm run test:scripts && npm run test --workspaces`.

Verified: unit 579 files / 5400 passed / 2 skipped (exactly the two tests of
the deleted shim's own suite fewer, nothing else moved), integration 469 passed
/ 1 skipped, ci-contracts 34 passed, all projects together 5903 passed / 3
skipped, coding-agent 2899 passed / 29 skipped, script tests 3 passed,
typecheck and check clean. The four SQLite selector files still pass 47/47
under both Node and `bun --bun vitest`, and all four child-process files pass.

Assistant-model: Claude Opus 5
AGENTS.md and DEV_SETUP.md are instructions, not a backlog.

Assistant-model: Claude Opus 5
Adds biome.json modelled on upstream pi: tab indent width 3, line width 120,
recommended lints with the same handful of overrides pi disables. Scope follows
pi as well -- package sources and tests, root suites, scripts -- excluding
generated files, fixtures and vendored skills.

This commit is the formatter pass only, so the lint fixes that follow are
reviewable apart from 2550 whitespace changes. tsc --noEmit is clean across the
1748 reformatted files.

Assistant-model: Claude Opus 5
Completes the Biome adoption. The rule set is upstream pi's exactly: the
recommended preset plus the same six overrides (noNonNullAssertion,
useConst, useNodejsImportProtocol, noExplicitAny, noControlCharactersInRegex,
noEmptyInterface). Nothing else is disabled.

`biome check` is now the first step of `npm run check`, so the prek hook and the
CI static-checks job enforce it without further wiring. `npm run format` applies
the formatter.

Roughly 2100 findings are resolved. Most were mechanical, but one class was not:
biome's noConfusingVoidType autofix rewrites `T | void` to `T | undefined`, and
this repository uses `T | void` deliberately at its SDK boundary -- the host
ExtensionAPI's `on()` returns void while the internal event bus returns an
unsubscribe function, so the union is what accepted both. Rewriting it broke 47
type contracts. Rather than disable the rule and lose parity, the affected
surfaces now type those returns as `unknown` and narrow with a typeof guard at
the call site, which satisfies both the rule and the compiler:

- WorkflowEventSurface and PiResultIntercomExtensionAPI `on`
- the ExtensionAPI event-handler return in public-types
- run-tool-execution-tracker's drain result

Test doubles that returned Promise<void> now return undefined explicitly so they
satisfy the signatures they implement. No test was skipped, weakened or deleted,
and no `as any`, ts-ignore or biome-ignore suppression was added anywhere.

biome.json is migrated to the 2.5.5 schema, where `recommended` is spelled
`preset`.

Verified: biome check clean across 2279 files, tsc --noEmit clean, unit 5400
passed / 2 skipped, integration 469 passed / 1 skipped, ci-contracts 34 passed,
script tests 3 passed, coding-agent 2899 passed / 29 skipped, and the SQLite
selector files still 47/47 under both Node and `bun --bun vitest`. Every count
is identical to before the lint pass.

Assistant-model: Claude Opus 5
Comment thread packages/coding-agent/examples/extensions/subagent/index.ts Fixed
Comment thread packages/coding-agent/examples/extensions/subagent/index.ts Fixed
Comment thread packages/coding-agent/examples/extensions/subagent/index.ts Fixed
Comment thread packages/coding-agent/src/core/tools/resource-selectors.ts Fixed
Comment thread packages/coding-agent/src/core/tools/resource-selectors.ts Fixed
Comment thread test/unit/assert-builtin-archive-set.test.ts Fixed
flora131 added 4 commits July 30, 2026 14:23
All three only appear in CI, which is why they survived local validation.

1. Windows suites and agent-suite: ENOENT uv_spawn 'npm'.
   test/helpers/runtime.ts resolved the executable to decide whether to throw
   ENOENT, then spawned the bare name anyway. Bun resolved and ran Windows
   `.cmd` shims itself; Node does not, and since 20.12 (CVE-2024-27980) it
   refuses to exec a `.cmd` or `.bat` without a shell. Spawn the resolved path,
   and use a shell only for a `.cmd`/`.bat` shim.

2. release-archive: "Required runtime dependency not found: css-select".
   scripts/build-binaries.sh installs every platform's native binding with
   `npm install --no-save --force`. On a versionless base those resolve to the
   0.0.0 placeholder, which is not published, so npm fails with ETARGET -- but
   not before mutating node_modules and pruning real runtime dependencies. Skip
   the fetch entirely at the placeholder version, and restore the tree with
   `npm ci` if it fails for any other reason.

3. static-checks: "must be run in a directory where a docs.json file exists".
   That message is misleading; mintlify actually refuses to start on Node 25+,
   and static-checks installs no Node toolchain, so npx picked up the runner's
   Node 26. Run mintlify through `bunx --bun` as it was before, which hosts it
   regardless of the runner's Node.

Verified locally: npm run check, test:unit 5400 passed / 2 skipped,
test:ci-contracts 34 passed, script tests 3 passed, and an end-to-end
run-flaky-test-suite.ts invocation that spawns npm through the repaired helper.

Assistant-model: Claude Opus 5
Two more npm-vs-bun differences the first pass did not reach.

1. release-archive: "Required runtime dependency not found: css-select".
   Skipping the 0.0.0 binding fetch was necessary but not sufficient -- the
   dependency was never in the root node_modules to begin with. `bun install`
   runs with `linker = "hoisted"`, so every transitive dependency lands at the
   root; npm nests on a version conflict, at arbitrary depth. css-select
   resolves to node_modules/linkedom/node_modules/css-select, and that copy's
   own boolbase sits beside it rather than beneath it.

   copy-runtime-dependencies.ts now resolves each package by walking up through
   every ancestor node_modules exactly as require.resolve would, instead of
   assuming a single hoisted root. It copies 270 packages where it previously
   aborted at 151, and ./scripts/build-binaries.sh now produces a complete
   darwin-arm64 archive locally.

2. Windows suites: ENOENT uv_spawn on the resolved npm path.
   The previous fix resolved the executable but preferred the extensionless
   match. On Windows npm ships as both `npm` -- a POSIX shell script Windows
   cannot exec -- and `npm.cmd` beside it, so the resolver returned the one that
   cannot run. Try PATHEXT candidates before the bare name.

Verified: npm run check, test:unit 5401 passed / 1 skipped, test:ci-contracts
34 passed, a full copy-runtime-dependencies run with no missing packages, and a
complete ./scripts/build-binaries.sh --platform darwin-arm64 archive build.

Assistant-model: Claude Opus 5
The .npmrc this migration wrote claimed to replace bunfig's supply-chain gate
one-for-one, but it did not hold: `min-release-age-exclude` only exists in npm
11.17.0 and later. CI runs the npm bundled with its Node, which is older, so npm
reported "Unknown project config" and silently ignored the key. The exemption
was doing nothing there, and npm warns it will stop working entirely in the next
major.

Upstream pi's entire .npmrc is two lines:

    save-exact=true
    min-release-age=2

Ours is now byte-identical to that. The exclusion list is gone, which does
change behaviour: @earendil-works/pi-* releases were exempt from the age gate
under bunfig because they are consumed same-day, and they no longer are. A
same-day pi bump now needs an explicit `--min-release-age=0` on that one
install. pi itself carries no exemption, so this is the parity cost.

Node in CI moves 24 -> 22 to match pi's workflows. Both repositories already
declare `engines.node >= 22.19.0`, and Bun was already pinned to 1.3.14
everywhere, which is pi's pin too.

The dependabot `cooldown` is realigned 3 -> 2 so it still matches the .npmrc
gate; the two exist to cover each other and drifting apart would leave a hole.
Docs corrected in AGENTS.md and docs/ci.md.

Verified: npm ci --dry-run emits no unknown-config warning, npm run check
passes, test:ci-contracts 34 passed, and both workflows validate.

Assistant-model: Claude Opus 5
…sbind

Addresses every open CodeQL and code-quality finding on this PR. All four are
real; none is a false positive.

Security (CodeQL):

- resource-selectors.ts, polynomial regex on uncontrolled data. The scheme
  scanner `/[a-z][a-z0-9+.-]*:\/\/.../gi` is unanchored under /g, and its two
  character classes overlap, so every interior position of a long
  `[a-z0-9+.-]` run is retried. A `(?<![a-z0-9+.-])` lookbehind pins each match
  to a real scheme boundary. Measured on a 40,000-character adversarial string:
  793 ms before, 0 ms after. Match results are identical on realistic input.

- resource-selectors.ts, the same class in the skill:// parser.
  `([^/]+)\/?(.*)$` is ambiguous between the optional separator and the tail;
  `([^/]+)(?:\/(.*))?$` is not. Verified equivalent across the empty tail,
  trailing slash, nested path and non-matching cases.

- assert-builtin-archive-set.test.ts, zip slip. A tar entry name is
  attacker-controlled and may contain `..`, so `join(root, header.name)` can
  write outside the extraction root. Entries are now resolved and rejected
  unless they stay under it.

Code quality:

- examples/extensions/subagent/index.ts called `new Text(text, 0, 0)` with no
  `Text` in scope, so it bound to the DOM global, which takes one argument.
  CodeQL reported it three times as superfluous trailing arguments. The intended
  class is pi-tui's `Text`; it is now imported. This was a genuine bug in the
  example, not a lint artifact.

Verified: npm run check clean, test:unit 5401 passed / 1 skipped,
test:ci-contracts 34 passed, the archive suite and the three SQLite selector
suites pass, and a regex-equivalence probe confirms both patterns match exactly
as before.

Assistant-model: Claude Opus 5
Comment thread test/unit/assert-builtin-archive-set.test.ts Fixed
flora131 added 2 commits July 30, 2026 15:17
Two tests in subagents-workflow-session-persistence.test.ts failed in CI with
"No API key found for the selected model" while passing locally, because they
were reading the developer's real ~/.atomic/agent credentials.

The cause is a runtime assumption the toolchain migration removed. The tests
hand the executor a `.ts` stub CLI via piArgv1, and `resolvePiCliScript` accepts
a `.ts` entry only when the runtime is Bun. Under `bun test` that held, and on
main these pass in 169ms with no network. Under vitest's Node worker the stub is
rejected, the executor falls through to the real installed `atomic`, and the
test makes a live provider call -- 4.4s and a 401 once a dummy key was supplied.
A unit test reaching the network is worse than one that fails, so raising the
timeout or injecting a key would both have been wrong.

The background test in this same file already had the answer: run the child in a
runtime that can execute the entry. Both foreground tests now drive the executor
through a Bun child the same way, which keeps the stub in play. Coverage is
unchanged -- all three still assert the persisted session header's workflow
classification and that no session leaks into the listing.

Verified with HOME pointed at an empty directory and OPENAI_API_KEY and
ANTHROPIC_API_KEY unset, reproducing the CI environment: 3 passed, no network.
Full suite 5401 passed / 1 skipped, npm run check clean.

Assistant-model: Claude Opus 5
…slip guard

Windows suites: ten tests in flaky-test-suite-runner.test.ts failed while the
two real-vitest cases passed. The fixture writes an extensionless `vitest` file
carrying a `#!/usr/bin/env node` shebang and invokes it as `./vitest`. Windows
has no shebang support, so Node cannot exec it; Bun.spawn used to paper over
this before the runner moved to node:child_process. The fixture now runs that
fake suite through Bun explicitly. The duration guard already steps over a
leading Bun runtime before matching `basename` against `vitest`, so the budget
still resolves and the gate stays on -- which the budget assertions in the same
file continue to prove.

CodeQL zip slip: the previous guard compared the resolved target against the
resolved root with startsWith, which is correct but is not the shape CodeQL
recognises as a barrier, so the alert stayed open. Replaced with the canonical
form from CodeQL's own remediation guidance: compute the path relative to the
root and reject when it is empty, escapes with `..`, or is absolute.

Verified: flaky-test-suite-runner 12 passed, the archive suite 2 passed, full
unit suite 5401 passed / 1 skipped, npm run check clean.

Assistant-model: Claude Opus 5
Comment thread test/unit/assert-builtin-archive-set.test.ts Fixed
@blacksmith-sh

This comment has been minimized.

…ip guard

js/zipslip stayed open through two attempts. The check was functionally correct
each time; the problem was that CodeQL did not recognise it as a barrier, so the
tainted flow was still reported at the `resolve(root, header.name)` call.

The first attempt guarded with `target !== containedRoot && !target.startsWith(
containedRoot + sep)`. The extra disjunct is what broke recognition. The second
attempt switched to a `relative()` check, which CodeQL does not model for this
query either.

This restores the exact shape the js/zipslip remediation documents: one
condition, `startsWith` against the resolved root plus a separator. The
`!== root` disjunct is unnecessary anyway, since a tar entry never names the
extraction root itself.

Behaviour verified directly rather than assumed:

  builtin/ok.txt      -> allow
  nested/../ok.txt    -> allow   (contains .. but stays inside)
  ../escape.txt       -> block
  ../../etc/passwd    -> block
  /abs/evil           -> block

Archive suite 2 passed, unit suite 5401 passed / 1 skipped, npm run check clean.

Assistant-model: Claude Opus 5
Comment thread test/unit/assert-builtin-archive-set.test.ts Fixed
flora131 and others added 3 commits July 30, 2026 15:58
Windows resolves executables through PATHEXT, whose entries are
conventionally uppercase, so the fixture's resolved Bun runtime arrives
as `...\bun.EXE`. The guard's case-sensitive binary regexes then failed
to step over the runtime prefix, no budget resolved, and the headroom
gate silently disabled — failing the five guard-dependent flake-runner
fixture tests on the Windows suites job. Windows filename matching is
case-insensitive; the npm/vitest/bun binary checks now match the same
way, with a regression assertion covering the uppercase-extension shape.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ity PR

Three fixes for the github-advanced-security annotations:

- js/polynomial-redos (resource-selectors.ts x2): the archive and SQLite
  selector regexes carried an ambiguous greedy `(.+\.ext)` prefix that
  backtracks polynomially on adversarial input. Both parsers now use a
  linear right-to-left scan for the extension split and reuse the
  unchanged, unambiguous suffix grammar. Verified behaviorally identical
  to the old regexes across ~214k generated inputs, including drive
  letters, multi-colon members, and case variants.

- js/zipslip (assert-builtin-archive-set.test.ts): the startsWith
  barrier was sound but CodeQL barrier recognition does not follow the
  guarded target variable into a separate .then callback, which kept the
  alert open across two earlier revisions. The guard and the filesystem
  writes now live in the same function.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Code fixes for seven pre-existing alerts, preserving behavior for all
valid inputs:

- js/insecure-randomness (live-browser-dom.js, and the same class in
  live-browser-session.js): live-edit session ids fall back to
  crypto.getRandomValues instead of Math.random when crypto.randomUUID
  is unavailable (plain-http LAN preview).
- js/resource-exhaustion (live-server.mjs): the client-supplied poll
  timeout is clamped to the 10-minute default ceiling; NaN falls back
  to the default instead of firing immediately.
- js/regex-injection (live-accept.mjs): the digit-validated --variant
  value is additionally regex-escaped before splicing into a RegExp.
- js/double-escaping (live-manual-edit-evidence.mjs): decodeBasicHtml
  now decodes &amp; last, so crafted input such as &amp;lt; decodes to
  the literal &lt; instead of being double-unescaped to <.
- js/incomplete-sanitization (auth-storage-01.suite.ts x2): the test's
  shell-path escaping handles backslash and quote in one pass; output
  is unchanged because the preceding slash conversion removes every
  backslash.

The impeccable script changes extend the vendored tree's existing
CodeQL-fix divergences from upstream. The two
js/shell-command-injection-from-environment alerts on pi's spawnProcess
wrapper were dismissed as false positives instead: command and args are
an argument vector with no shell-string concatenation, and cross-spawn
exists to escape arguments safely for Windows .cmd shims.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@flora131
flora131 merged commit 0fd3746 into main Jul 30, 2026
13 checks passed
@flora131
flora131 deleted the chore/pi-toolchain-parity branch July 30, 2026 23:24
flora131 added a commit that referenced this pull request Jul 31, 2026
Rebasing onto main crossed the pi toolchain-parity change (#2079), which moved
the root suites from bun test to vitest under Node and adopted pi's biome rule
set. This adapts everything the branch added:

- Migrate branch-added tests off bun:test/Bun.* to the vitest API and
  test/helpers/runtime.js (sleep, spawnProcess, spawnSyncCollect, moduleDir,
  bunExecutable), including test.serial -> test.sequential.
- default-main-driver fixture: spawn through the helpers so the fixture host
  still runs under Bun while the suite runs under Node.
- admission-backlog-engine fixture: a Node host hands the child a non-blocking
  stdout pipe, so the synchronous admission burst raised EAGAIN and killed the
  child mid-write; writeAllSync retries until the host drains.
- bootstrap publication-failure test: scope the observed temp root, because
  vitest runs test files in parallel and other files create bootstrap
  directories in the shared tmpdir during the snapshot window.
- Align branch-added sources with the biome config on main, and rewrite the
  two noAssignInExpressions sites (queued-writer pump loop, engine-health
  attempt id) without behavior change.
flora131 added a commit that referenced this pull request Jul 31, 2026
Rebasing onto main crossed the pi toolchain-parity change (#2079), which moved
the root suites from bun test to vitest under Node and adopted pi's biome rule
set. This adapts everything the branch added:

- Migrate branch-added tests off bun:test/Bun.* to the vitest API and
  test/helpers/runtime.js (sleep, spawnProcess, spawnSyncCollect, moduleDir,
  bunExecutable), including test.serial -> test.sequential.
- default-main-driver fixture: spawn through the helpers so the fixture host
  still runs under Bun while the suite runs under Node.
- admission-backlog-engine fixture: a Node host hands the child a non-blocking
  stdout pipe, so the synchronous admission burst raised EAGAIN and killed the
  child mid-write; writeAllSync retries until the host drains.
- bootstrap publication-failure test: scope the observed temp root, because
  vitest runs test files in parallel and other files create bootstrap
  directories in the shared tmpdir during the snapshot window.
- Align branch-added sources with the biome config on main, and rewrite the
  two noAssignInExpressions sites (queued-writer pump loop, engine-health
  attempt id) without behavior change.
flora131 added a commit that referenced this pull request Jul 31, 2026
…ape from killing the engine (#2076)

* fix(coding-agent): recover interactive engine failures without losing input

Escape used to race the engine's cooperative abort against a 250 ms deadline
and kill plus restart the engine child whenever it lost, reporting
"Engine terminated; … result unknown; inspect side effects before retrying".
Escape now only requests the engine's own cancellation and waits for it, and
that message no longer exists in any code path.

Terminating a wedged engine is an explicit Ctrl+C, armed only when the engine
is provably not answering: watchdog-confirmed unresponsive, a cooperative abort
unanswered past the same one-second threshold, or a replacement still waiting
for readiness past it. That last case has no heartbeat and no watchdog
coverage, so it was previously unrecoverable. Ctrl+C reaches the host even
while an engine-owned custom UI or overlay owns input, and host-native
selectors, dialogs, and forms keep Ctrl+C-as-cancel while the engine is
healthy.

Engine death is now a host-local lifecycle event. The host closes every remote
component from the dead generation, settles its ui.custom() promises, releases
widget keys, resets terminal modes, remounts and refocuses the editor, and
unwinds the blocking inline custom-UI depth, without waiting for a replacement
engine_ready. One automatic replacement attempt follows, with calm status text.

The engine child is launched with an environment that never contains the four
engine control values; they travel in an owner-only bootstrap file read once
and unlinked. Deleting them from process.env afterwards cannot work under Bun,
where a child spawned without an explicit env inherits the runtime's
launch-time environment.

A submission the engine never accepted returns to the editor, ahead of anything
typed while the send was pending, on every submit route including /atomic,
deferred commands, compaction-time extension commands, streaming steer, bash,
/compact, and Alt+Enter.

Assistant-model: Claude Opus 5

* fix(coding-agent): make Ctrl+C escape remote UI and keep engine recovery repeatable

Ctrl+C is now always handled by the host while an engine-owned ctx.ui.custom()
component or overlay holds input, even with a healthy engine: such a component
forwards every key to the child, so one that never resolves trapped the escape
key too. Ownership is answered exactly by the remote component controller
rather than inferred from overlay presence or inline depth, so native
selectors, dialogs, input forms, session pickers, and unrelated native overlays
keep Ctrl+C as their own cancel. That path reports "Restarting interactive
engine." instead of falsely calling a healthy engine unresponsive.

Escape's wait is now genuinely unbounded: abort joined LONG_LIVED_COMMANDS, so a
stopped or blocked child no longer turns Escape into a red timeout after 30
seconds while the engine is still working.

A replacement that fails on its own now latches Ctrl+C armed, so recovery stays
available without Atomic ever retrying on its own.

RPC select, confirm, input, and editor dialogs are owned by the generation that
opened them: engine death cancels exactly those mounts and suppresses their
replies, so a dead generation can no longer leave a dialog on screen or answer
through the replacement child. Dialog hides are instance-scoped so stale
cleanup cannot dismiss a newer dialog.

Remote mounts unwind newest-first, each overlay is hidden through its own
handle, and the editor is refocused only when no surviving modal owns input.

Draft restoration reads expanded editor text, so a pending large paste survives
instead of being reduced to a dead marker.

Bootstrap cleanup is ownership-scoped: the child unlinks exactly the file named
on the command line, recursive removal requires the handle the host received
when it created the directory, and a failed publication removes its own
temporary credential file and directory before rethrowing.

Assistant-model: Claude Opus 5

* test(coding-agent): pin nested remote teardown order and overlay close targeting

The newest-first unwind and the exact-overlay-handle close were implemented but
only guarded by end-state assertions in the live harness, which passes either
way because pi-tui skips unmounted focus targets.

The fake host bridge now records the order in which each mount's host close
callback runs, so an inline proxy with an overlay stacked above it asserts the
overlay closes first. A new suite drives the real showExtensionCustom to assert
that an overlay is hidden through its own handle rather than the generic
top-overlay call, and that an inline close never takes focus from a surviving
overlay.

Both fail when the corresponding fix is reverted.

Assistant-model: Claude Opus 5

* fix(coding-agent): route safety keys by physical identity and keep recovery armed

Escape and Ctrl+C were classified through the configurable app.clear action, so
binding app.clear to Escape sent Escape into the engine stop/restart branch and
left Ctrl+C with no host route. Both are now matched by physical key identity
through pi-tui's parser, with key-release events filtered, and the editor
applies the same fixed Escape guard before its configurable handlers.

A focused remote proxy now receives the first Ctrl+C, so extension UIs that bind
it keep working; the host takes the next press against the same component. That
preserves the workflows prompt-card Skip and stage-chat Close while keeping the
escape hatch.

Every cooperative-cancellation command is exempt from the generic request
deadline, not just abort, so cancelling a running bash no longer produces a red
timeout while the engine is still working.

A child dying while a replacement is starting latches Ctrl+C instead of being
ignored, so the host is never left with no engine and nothing armed.

Line widgets are generation-owned, so a dead generation's lines are released
while a newer generation's content survives stale cleanup.

The editor snapshots its expanded buffer for the dispatch that submits it, so a
restored draft is what was typed rather than the trimmed callback argument, and
a restored draft no longer also raises a red transport error.

Heartbeats moved off the consumptive generic engine-message channel, and the
queued writer rejects its in-flight frame so a callback-level EPIPE cannot hang
a prompt forever.

Assistant-model: Claude Opus 5

* fix(coding-agent): give Ctrl+C a first-press escape from a trapped remote UI

A remote ctx.ui.custom() component owns every key while it holds input, so a
component that never resolves swallowed Ctrl+C. The previous escape needed two
presses and then replaced the whole engine, discarding everything else that
generation was doing.

Ownership is now declared per mount through a new handlesCtrlC option. An
unresponsive engine is still terminated on the first press, since a wedged child
cannot run a local handler either. A component that declared the option keeps
its own Skip, Close, or cancel binding, and is closed if it still owns input on
the next press. An undeclared component is closed on the first press through the
ordinary close path, so its promise resolves with undefined and the engine keeps
running. The bundled workflow surfaces declare it.

Engine death is retained rather than transient, so a child that exits between
startup returning and the host attaching is still recovered instead of leaving a
live TUI bound to a dead engine.

Buffered custom-UI frames and extension UI requests are tagged with the
generation that produced them and dropped when it dies, so a stale mount frame
cannot remount UI that death teardown just closed, or collide with the
replacement child's identical component ids.

Each submission carries its own raw draft end to end instead of sharing one
slot, so two entries that differ only in whitespace can no longer restore each
other's text, and submissions still queued behind a failed send come back with
it in the order they were entered.

Assistant-model: Claude Opus 5

* fix(coding-agent): classify transport failures instead of matching error text

A submission that the engine never accepted is still the user's text, but the
host decided that by matching five message fragments. A dying engine produces
write EPIPE, Cannot call write after a stream was destroyed, or write after end,
none of which matched, so those submissions were reported as red errors and the
typed text was thrown away. Node also documents error.message as free to change
in any release.

The transport boundary already knows the frame never landed, so it says so. A
non-enumerable marker is added to the existing error, leaving its identity,
instanceof, code, errno, and syscall untouched, because RpcClient rejections are
public. A frozen or non-Error value is wrapped with the original as cause.

Marking happens in the queued writer, the child exit, error, and stdin handlers,
an explicit stop, a malformed transport, a missing writer, a not-started client,
and the request write catch. Request timeouts after a successful write, RPC
error responses, provider failures, and anything after agent_start stay
unclassified and still surface.

Assistant-model: Claude Opus 5

* fix(coding-agent): keep the raw draft on idle Alt+Enter and migrate MCP panels

Alt+Enter is an app action, so the editor returns before its own pre-trim
snapshot runs and the submit handler's only draft is the value the Alt+Enter
path hands over. That value was the already-trimmed text, so an idle follow-up
the engine never accepted came back without the whitespace the user typed. The
raw expanded buffer is passed instead; the handler still trims it for delivery.

The /mcp, /mcp setup, and MCP OAuth panels bind Ctrl+C for their own cancel and
cleanup, so they now declare handlesCtrlC. Without it the host would close them
on the first press and skip their handlers.

The changelog gains a Breaking Changes bullet for the migration existing
ctx.ui.custom components need: an extension that consumes Ctrl+C keeps that
binding only by declaring handlesCtrlC. The runtime default is unchanged, since
forwarding an undeclared first press is exactly the trap this work removed.

Assistant-model: Claude Opus 5

* fix(coding-agent): decide submission ownership by admission, and fence disposal

Ownership of a failed submission was inferred from the first byte of output, so
a command that changes the working tree and prints nothing looked exactly like
one the child never received. Killing the engine during `!touch marker && sleep
400` put that line back in the editor, inviting a second run.

The child now announces ownership of every correlated request and flushes the
announcement before its handler can touch the shell, an extension, the queue, or
compaction. The host restores a draft only for a transport failure that arrived
without an announcement, and reports an accepted failure as an ordinary failure.
The exit rejection waits for the dead child's stdout to finish parsing so a
queued announcement is never missed, bounded so a descendant holding stdout
cannot strand the caller. The protocol version becomes 2, because a child that
cannot announce must not bind to a host that assumes it can.

Disposal now fences engine recovery. A replacement sits between its own stop and
its spawn with no child attached, so a disposal-time stop found nothing to do
and returned, and the attempt then started an engine after teardown finished.
An explicit stop voids the restart permit, a superseded restart fails as
cancelled instead of quietly succeeding, and health shutdown joins the attempt
before disposal returns.

Assistant-model: Claude Opus 5

* fix(coding-agent): let a dead generation settle ownership before it is replaced

An engine child announces that it owns a request before it starts the work, but
that announcement can still be unparsed in the pipe when the child dies. Because
automatic recovery starts in the same turn as the death event, its stop detached
the reader, retired the generation, and re-failed those requests with `Agent
process stopped` — so work that had already run was reported as never sent and
offered back to the user for a second run.

Death still publishes immediately: the TUI must never wait on a pipe. The
generation now gets a bounded settling window on top of it. Its stdout keeps
being read, only its ownership frames are honoured, its requests are classified
exactly once with the error of whatever ended it, and the replacement starts
only afterwards. An explicit stop claims that error before terminating, so a
deliberate stop keeps its own wording while an exit keeps its own.

Every terminal cause now shares one path: exit, spawn error, stdin error,
malformed transport, and explicit stop. The JSONL reader also reports completion
for a stream that ends with nothing buffered, which previously left that wait to
expire on its timeout.

Assistant-model: Claude Opus 5

* fix(coding-agent): decide draft restore by admission alone and fence the watchdog latch

Three review findings from the recovery branch, plus the leak that surfaced
while reproducing them.

isEngineSendFailure no longer falls back to matching error text. Every
rejection RpcClient raises is already built through rpcTransportError, and an
accepted request is re-marked per request, so admission is decisive on its own.
The legacy marker list could classify any provider, extension, or command error
quoting a phrase like "Agent process stopped" as unsent, restoring a draft the
engine had already run and hiding the real failure. The tests that leaned on
that fallback now inject typed transport errors, which is what production
raises, and a new case pins that identical wording resolves differently based
only on admission.

recover() clears the unresponsive latch when a replacement attempt starts. The
watchdog verdict describes the generation being replaced; a fresh child emits no
heartbeat before engine_ready, so the latch kept needsExplicitTermination() true
through the whole pre-ready window and the first Ctrl+C would stop a replacement
that never misbehaved. The fence stays time-bounded: an overdue replacement arms
Ctrl+C again on its own account.

The abort-lifetime test now resumes and stops its child in a finally block. It
SIGSTOPs the engine, and a failed assertion before the resume leaked a frozen
child holding this process's stdio pipes - the same leak class that hung the
Windows job. One such orphan was still running from an earlier local run.

Assistant-model: Claude Opus 5

* test: adapt the interactive-engine branch to the toolchain now on main

Rebasing onto main crossed the pi toolchain-parity change (#2079), which moved
the root suites from bun test to vitest under Node and adopted pi's biome rule
set. This adapts everything the branch added:

- Migrate branch-added tests off bun:test/Bun.* to the vitest API and
  test/helpers/runtime.js (sleep, spawnProcess, spawnSyncCollect, moduleDir,
  bunExecutable), including test.serial -> test.sequential.
- default-main-driver fixture: spawn through the helpers so the fixture host
  still runs under Bun while the suite runs under Node.
- admission-backlog-engine fixture: a Node host hands the child a non-blocking
  stdout pipe, so the synchronous admission burst raised EAGAIN and killed the
  child mid-write; writeAllSync retries until the host drains.
- bootstrap publication-failure test: scope the observed temp root, because
  vitest runs test files in parallel and other files create bootstrap
  directories in the shared tmpdir during the snapshot window.
- Align branch-added sources with the biome config on main, and rewrite the
  two noAssignInExpressions sites (queued-writer pump loop, engine-health
  attempt id) without behavior change.

* test: update package and integration suites to the branch's submission contracts

The branch changed several InteractiveMode contracts without updating the
coding-agent package suite and one integration helper; CI has been red on
exactly these files since before the rebase. Align them with the intended
behavior:

- Queued prompts and input callbacks now carry InteractiveSubmission
  ({ text, draft }) instead of bare strings, so a failed send can restore the
  exact editor buffer. Updated assertions and stub types in the startup-input,
  first-run-onboarding, status-autocomplete, and paused-queued-messages suites.
- runUserPromptTurn subscribes to the session to detect turn start; session
  stubs in the deferred-startup, startup-latency, loader-continuity, and
  resource-gate suites now provide subscribe().
- restoreFailedSubmissionDraft reads mode.pendingUserInputs; the resource-gate
  fake mode now seeds it.
- IsolatedInteractiveRuntime and the engine dialog host observe generation
  death; client/runtime stubs in rpc-bash-streaming and
  startup-resource-ordering now provide onGenerationEnded().
- The showExtensionCustom focus fence consults ui.hasOverlay() before
  restoring editor focus; the shared integration overlay host helper now
  declares it.

Also fix three load-sensitive tests the full parallel suite exposed while
landing this change, per the repository's fix-it-where-it-lives policy:

- interactive-engine-generation-lifecycle: tolerate ESRCH when the engine
  child loses the startup race and is already gone before the explicit
  SIGKILL; an early death is still the generation death under test.
- subagents-async-event-journal: temp-root cleanup races the journal drain
  these tests deliberately provoke, so recursive removal can hit ENOTEMPTY
  when a final async append lands mid-delete; use rm's bounded retry.
- subagents-foreground-intercom-detach: gate the fake children on a release
  file written only after the detach commit is processed, instead of fixed
  output delays a loaded event loop can outlive; otherwise the commit finds
  the attempt closed, nothing detaches, and the detached-exit wait hangs
  until the suite timeout.

* test(coding-agent): realpath the graph-manifest fixture root for macOS temp symlinks

os.tmpdir() on macOS is a /var -> /private/var symlink and jiti resolves
transitive imports to realpaths, so the recorded manifest keyed chain files
under /private/var while the test compared literal /var fixture paths and
reported them missing. Linux and Windows runners have no such symlink, which
is why CI stayed green while every macOS checkout failed these two tests.

* docs: make DEV_SETUP accurate for the hybrid toolchain and the natives build

npm ci --ignore-scripts skips lifecycle scripts by design and the workspace
natives package has no install hook, so a fresh clone never compiles
packages/natives/native/*.node; the CLI then silently degrades (pty:true falls
back to pipes, native grep/find and tree-sitter block ops fall back to JS) and
five coding-agent test files fail. Document the explicit
'npm run build --workspace=@bastani/atomic-natives' step, the required Node and
Rust toolchains, the node-run dist path the published bin actually uses, and
the current CI shape. Verified end to end on a fresh checkout: install, natives
build, 'bun packages/coding-agent/src/cli.ts', and build + 'node dist/cli.js'.

* test: assert bootstrap owner-only mode bits on POSIX platforms only

Windows has no POSIX permission bits: writeFileSync's mode option maps only to
the read-only attribute and stat reports 0o666 for any writable file, so the
0o600 assertion can never hold there. The record's protection on Windows is the
per-user temp directory ACL. Keep the secret-free path and directory assertions
on every platform.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants