Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (37)
📝 WalkthroughWalkthroughChangesThe pull request replaces Bun test manifests with filesystem-discovered test roots. It adds executor coverage validation, updates Bun runners and CI wiring, removes obsolete manifest references, and adjusts tests for current runtime behavior. Bun test discovery and execution
Test maintenance updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
WalkthroughThis PR changes 41 file(s).
Changes
Magnitude🎯 5 (XXL) RelatedNo related items found. Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![github-actions[bot]](https://avatars.github.com/in/15368?s=60&v=4){kind=small}
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
|
Not reproducible against the code as written — For That is one element of For The regex is The suite asserts exactly this for every non-credentialed root in The underlying concern — that the executor table could claim coverage a package no longer provides — is real and is covered separately in this same file by the bespoke-runner wiring assertions added below, which read each workspace's real |
|
Not reproducible — The docblock matches the implementation. This guard predates the PR; the only change here was broadening the file matcher to also accept |
|
Correct, and fixed. The pattern was documented as a "filename pattern" while Matching against the absolute path is the behavior worth keeping — it lets an override scope itself to a directory rather than to a bare name, which a monorepo needs. So the contract is now documented rather than changed.
|
|
Declining both of the This repository's stated architecture preference is fail fast over defense in depth. More specifically, the previous review round on this PR classified the opposite behavior as a blocker: the discovery walker used to swallow On the specific cases raised:
The clean-message path is reserved for the findings the guard is designed to report (uncovered files, doubly-executed files), which |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/core/run-bun-tests.ts (1)
62-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the shared discovery helper.
discoverTestFilesandfindTestFilesare now near-identical inpackages/core/run-bun-tests.ts,packages/auth/run-bun-tests.ts, andpackages/agents/run-bun-tests.ts. OnlyTEST_ROOTSand the pruned-directory handling differ. The three copies must stay in agreement becausescripts/check-test-file-coverage.tstreats each as an authoritative executor claim. A shared helper parameterized by roots and pruned directories would remove the drift risk.This is a cross-package extraction, so it can be deferred to a follow-up.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/run-bun-tests.ts` around lines 62 - 77, Defer this cross-package refactor as a follow-up; leave discoverTestFiles and findTestFiles unchanged for now.scripts/tests/bun-test-roots.bun.test.ts (1)
285-300: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider guarding the symlink fixture on Windows.
symlinkSynccommonly throwsEPERMon unprivileged Windows. Sibling suites underscripts/testsguard such fixtures withdescribe.skipIf(process.platform === 'win32'). CI runs this suite on macOS only, so this affects local Windows development rather than CI.Based on learnings: for test files under
scripts/teststhat use real filesystem symlinks, guard these tests withdescribe.skipIf(process.platform === 'win32').🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/bun-test-roots.bun.test.ts` around lines 285 - 300, Guard the symlink-cycle test containing symlinkSync in a describe.skipIf(process.platform === 'win32') block, following the existing pattern in scripts/tests. Keep the test behavior unchanged on non-Windows platforms.Source: Learnings
scripts/bun-test-roots.ts (1)
546-555: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider failing loudly on an unknown
rootFilter.
resolveBunTestFilesreturns an empty array whenrootFiltermatches no entry inBUN_TEST_ROOTS. A typo in--workspacethen produces the generic "No native Bun test files found" message inscripts/run_bun_tests.tsline 645, which does not distinguish a misspelled root from a root that legitimately resolved zero files. Every other failure mode in this module fails loudly with a specific message. An explicit unknown-root error would keep that property.Note:
scripts/tests/bun-test-roots.bun.test.tsline 512-514 currently codifies the empty-array behavior, so this change requires a matching test update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bun-test-roots.ts` around lines 546 - 555, The resolveBunTestFiles function should reject an explicitly provided rootFilter when it matches no entry in BUN_TEST_ROOTS, raising a specific unknown-root error instead of returning an empty array; preserve empty results for valid roots that resolve no files, and update the corresponding bun-test-roots test to expect the new failure.scripts/tests/run_bun_tests.subprocess.test.ts (1)
166-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep subprocess coverage for
--preloadpropagation.Both subprocess runner cases use
test-setup, whoseBUN_TEST_ROOTSentry declares no preload. Keep one case that targets a root with declared preloads, or add an equivalent subprocess assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/run_bun_tests.subprocess.test.ts` at line 166, Update the subprocess runner tests around the `test-setup` cases to retain coverage for `--preload` propagation: ensure at least one subprocess case targets a test root whose `BUN_TEST_ROOTS` configuration declares preloads, or add an equivalent assertion while preserving the existing runner coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/agents/run-bun-tests.ts`:
- Around line 363-364: The test runners use inconsistent path anchors between
discovery and child execution. In packages/agents/run-bun-tests.ts:363-364,
packages/auth/run-bun-tests.ts:153-154, and
packages/core/run-bun-tests.ts:164-165, preserve absolute paths from
import.meta.dir when calling runTestFile or configure the spawned child cwd to
root; additionally resolve each PRELOAD and the agents junit.xml output against
root so all execution and output paths share the discovery anchor.
In `@scripts/check-test-file-coverage.ts`:
- Around line 153-170: Update collectExecutorClaims to canonicalize each
discovered executor file with realpathSync before adding it to files or
recording it in counts, ensuring symlink aliases share one coverage identity.
Add a symlink-based fixture or test covering a real file discovered through an
alias and verify it produces one canonical claim.
In `@scripts/tests/bun-test-roots.bun.test.ts`:
- Around line 583-586: Update the ordinary-file assertion in the bun test root
checks to first verify that the find result for run_bun_tests.test.ts is
defined, then assert its timeout is undefined. Keep the existing timeout
expectation while preventing a missing file from passing vacuously.
In `@test-setup/augment-bun-vi.ts`:
- Around line 132-136: Update the comment describing the bounded microtask-drain
loop to replace “arbitrarily deep” with “up to MICROTASK_DRAIN_ROUNDS rounds,”
and explicitly state that callbacks requiring additional rounds may remain
pending.
---
Nitpick comments:
In `@packages/core/run-bun-tests.ts`:
- Around line 62-77: Defer this cross-package refactor as a follow-up; leave
discoverTestFiles and findTestFiles unchanged for now.
In `@scripts/bun-test-roots.ts`:
- Around line 546-555: The resolveBunTestFiles function should reject an
explicitly provided rootFilter when it matches no entry in BUN_TEST_ROOTS,
raising a specific unknown-root error instead of returning an empty array;
preserve empty results for valid roots that resolve no files, and update the
corresponding bun-test-roots test to expect the new failure.
In `@scripts/tests/bun-test-roots.bun.test.ts`:
- Around line 285-300: Guard the symlink-cycle test containing symlinkSync in a
describe.skipIf(process.platform === 'win32') block, following the existing
pattern in scripts/tests. Keep the test behavior unchanged on non-Windows
platforms.
In `@scripts/tests/run_bun_tests.subprocess.test.ts`:
- Line 166: Update the subprocess runner tests around the `test-setup` cases to
retain coverage for `--preload` propagation: ensure at least one subprocess case
targets a test root whose `BUN_TEST_ROOTS` configuration declares preloads, or
add an equivalent assertion while preserving the existing runner coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c05f8ad-0678-450e-86f5-d4b311cb8c41
⛔ Files ignored due to path filters (3)
dev-docs/bun.mdis excluded by!dev-docs/**dev-docs/test-runner-inventory.mdis excluded by!dev-docs/**project-plans/issue2979/plan.mdis excluded by!project-plans/**
📒 Files selected for processing (36)
.github/workflows/ci.ymlpackage.jsonpackages/agents/run-bun-tests.tspackages/auth/run-bun-tests.tspackages/core/run-bun-tests.tspackages/lsp/package.jsonpackages/providers/src/openai/OpenAIRequestPreparation.issue2853.test.tspackages/providers/src/runtime/promptEnvelopeProjections.test.tspackages/tools/src/tools/check-async-tasks-shell-formatter.test.tsscripts/bun-test-manifest-data-mcp.tsscripts/bun-test-manifest-data-providers.tsscripts/bun-test-manifest-data-storage.tsscripts/bun-test-manifest-data-tools.tsscripts/bun-test-manifest-validation.tsscripts/bun-test-manifest.tsscripts/bun-test-roots.tsscripts/check-affected-test-shards.tsscripts/check-test-file-coverage.tsscripts/run_bun_tests.tsscripts/test.tsscripts/tests/bun-test-manifest.bun.test.tsscripts/tests/bun-test-root-ownership.bun.test.tsscripts/tests/bun-test-roots.bun.test.tsscripts/tests/ci-docs-only-skip.bun.test.tsscripts/tests/issue-2994-lint-scoped.bun.test.tsscripts/tests/issue-planner-confinement.bun.test.tsscripts/tests/issue-planner-enrichment.bun.test.tsscripts/tests/ocr-concurrency-canary-2673.test.tsscripts/tests/ocr-review-workflow.bun.test.tsscripts/tests/pr-review-walkthrough-sanitize.bun.test.tsscripts/tests/run_bun_tests.subprocess.test.tsscripts/tests/test-file-coverage.bun.test.tsscripts/tests/test-shard-orchestrator.test.tstest-setup/augment-bun-vi.test.tstest-setup/augment-bun-vi.tstsconfig.scripts.json
💤 Files with no reviewable changes (8)
- scripts/bun-test-manifest-data-providers.ts
- scripts/bun-test-manifest-validation.ts
- scripts/bun-test-manifest-data-storage.ts
- scripts/bun-test-manifest-data-mcp.ts
- scripts/tests/bun-test-manifest.bun.test.ts
- scripts/tests/ci-docs-only-skip.bun.test.ts
- scripts/bun-test-manifest.ts
- scripts/bun-test-manifest-data-tools.ts
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
|
Fixed in e110141. The inconsistency was real: discovery moved to Rather than passing absolute paths to the child, each of the three runners now anchors everything at one constant, which removes the class of bug rather than one instance of it:
Verified: |
|
Fixed in e110141, and it mattered more than the symlink case alone — the guard's whole claim is exactly-once execution, so comparing lexical paths could both invent an uncovered file and hide a genuine duplicate. Canonicalization had to be applied on both sides or the two sets would stop matching:
Both go through the injected Added the symlink fixture you asked for — two executors scanning the same directory under different names — which asserts one real file yields exactly one duplicate entry claimed by both: The temp-fixture helper now returns canonical paths too: on macOS the temp root is itself reached through a symlink ( |
|
Both fixed in e110141. Vacuous assertion — added Bounded drain docblock — corrected. The loop stops after
|
The shared Bun test runner chose which files to execute from a hand-maintained list in scripts/bun-test-manifest.ts. Any list that must be edited by hand drifts, and this one had: 43 test files existed on disk that no CI job ran, so they looked like coverage while asserting nothing. Two of them had rotted into failures nobody could see. Selection is now structural. scripts/bun-test-roots.ts declares, per root, only how a file should run — cwd, scanned directories, filename pattern, preloads, tsconfig, timeout, retries, globalSetup, credentials and per-file timeout overrides. Which files run is answered by walking the filesystem, so a new test file executes by virtue of existing. There is no files/include/exclude member anywhere, and the walker now propagates read, stat and realpath failures as BunTestRootStatError rather than silently returning a short list: a dropped test is always loud. Dot-prefixed directories are still pruned, but a dot-prefixed file that matches the pattern is not, and files are deduplicated by real path so a symlink alias cannot execute one file twice. scripts/check-test-file-coverage.ts is what keeps this honest. It walks the repository for test files and fails when one is executed by no executor or by more than one, deriving the covered set from each executor's own discovery code rather than restating it. It runs on every non-docs PR from bun_test_orchestrator_smoke, so it cannot be skipped by affected-shard selection on exactly the PR that would introduce a gap. That subsumes the bun_native_test_parity job, which is deleted. Consequences of running everything: - providers gains 41 files, tools 1, storage 1. Three failed and are fixed: a stale vi.mock specifier, an assertion ordered after a Bun matcher that unfreezes its subject, and an ISO literal that did not match its own input timestamp. - cli and core lose their shared roots and agents keeps only test-bun; their bespoke runners already discover those files, and agents/src was running twice per CI run. - core and auth broaden to *.spec, adding 13 files they had been skipping, and their JUnit classnames strip the suffix accordingly. - lsp moves onto the shared runner. Its bare `bun test` could only be modelled by the guard, never derived, which was a hole in the guarantee. core, agents and auth gain import.meta.main guards and export the discovery function main() itself calls, so the guard reads what CI runs.
Running every test file surfaced seven packages/providers auth suites the deleted manifest had commented out with "Bun fake-timer incompatibility on Linux CI ... re-add when Bun runtime is fixed". Every one of their cases timed out at exactly the per-test timeout on Linux while passing on macOS. Reproduced in oven/bun:1.3.14 on linux/arm64. The compat shim's flushPendingTasks ended its microtask drain by awaiting a setImmediate. Under Bun's fake timers, once a timer has fired and the clock is then advanced with no pending timers, setImmediate (and setTimeout(_, 0)) is gated by the fake-timer scheduler and never becomes due, so the await never returns and advanceTimersByTimeAsync hangs until teardown. macOS happens to keep firing it, which is why the failure looked platform specific. Probes in that state show queueMicrotask, process.nextTick and Promise.resolve all return in under a millisecond, because microtasks drain inside the current macrotask before the scheduler regains control. The settling boundary is now a microtask. That also restores Vitest parity the macrotask had broken: for a timer whose awaited continuation schedules a nested timer, Vitest fires it at 25ms and the shim's own test asserted 35ms — the extra macrotask turn had deferred the continuation past the next timer. Confirmed by running the same scenario under Vitest. Also repoints two orchestrator tests at the surviving scripts roots; they still asserted the scripts-tests-slow root that folded into a per-file timeout override.
) The field was described as a filename pattern while resolveTimeoutForFile tests it against the resolved absolute path, so a basename-anchored regex would silently never match and the file would quietly keep the root timeout. Path matching is the behavior worth keeping — it lets an override scope itself to a directory — so the contract is documented rather than changed, in the interface, the root field, and dev-docs/bun.md.
…#2979) Two soundness gaps found in review of the discovery work. The core, agents and auth runners discovered test files under import.meta.dir and then mapped them back to paths relative to it, while the spawned child, its preload and the JUnit report still resolved against process.cwd(). The two anchors agree only when the runner is invoked from its own package directory, which the workspace scripts happen to satisfy, so this was latent rather than broken. Each runner now anchors everything at a single WORKSPACE_ROOT constant: discovery, the child's working directory, the preload and the report path. The coverage guard compared lexical paths. An executor that reaches a test through a symlink alias would then be credited with a file the repository walk recorded under a different path, reporting a false uncovered file on one side and hiding a genuine duplicate on the other — which would undermine the exactly-once guarantee the guard exists to prove. Both the repository walk and the executor claims now canonicalize, so one real file has one coverage identity, and a fixture with two executors scanning the same directory through different names asserts it. The temp-fixture helper canonicalizes too, since on macOS the temp root is itself reached through /var -> /private/var. Also asserts the timeout-override control file was actually found before checking that it has no override, so the negative case cannot pass vacuously, and corrects the shim's drain docblock: the loop is bounded at MICROTASK_DRAIN_ROUNDS, so it does not drain arbitrarily deep chains.
AuthDialog's Close case wrote a keystroke, slept a fixed 50ms and then asserted onSelect had fired. On a loaded CI runner that is not enough for the render and keypress to propagate, and the cli shard failed on it twice in a row while passing locally and on the two preceding CI runs of this same branch. The three sibling cases in this file were already converted to waitFor for exactly this reason and carry the comment explaining it; this one was missed. It now polls the same way, so the assertion is unchanged and only the waiting strategy differs.
Deleting the shared cli root means packages/cli/run-bun-tests.ts is the only executor for the workspace, and it discovers every file rather than consulting a per-file configuration. A suite that drives the real TodoStore against disk therefore cannot opt into storage isolation the way the deleted manifest entry did for src/ui/contexts/__tests__/todoProvider.observation.bun.tsx (#3052). isolateStorageRoots() has to run before any test module imports the Storage singleton, so it belongs in the workspace preload rather than in a test. Every other workspace that touches storage already preloads it this way. Without it that suite would read and write the developer's real storage root.
Rebase reconciliation onto latest
|
#3115) (#3119) The nightly release job creates its GitHub Release with `gh release create --target <sha>`, which asks the Releases API to materialize the tag. Since GitHub's Nov-2023 "enforcing workflow scope when creating a release" change that endpoint requires `workflows: write` in addition to `contents: write` whenever the target commit's `.github/workflows/` tree has drifted from the default branch tip -- and `workflows: write` cannot be granted to `GITHUB_TOKEN` through a `permissions:` block. The refusal arrives as an opaque `HTTP 403: Resource not accessible by integration`. Because a release run takes about an hour, any PR touching `.github/` that merges to main during the run creates exactly that drift. Run 31112396891 lost its tag and Release this way after PR #3103 landed thirteen minutes in, leaving npm and ghcr published but the repository untagged. Create the tag through `POST /repos/{owner}/{repo}/git/refs` instead, then create the Release from the now pre-existing tag with no `--target`. That endpoint needs only `contents: write` and does not pass through receive-pack, so the workflow-file guard never applies. A plain `git push` of the tag is not an alternative: it goes through receive-pack and is rejected the same way (actions/checkout#1421). Dropping `--target` on its own would be worse than the bug, since the Releases API would then default the tag to the default branch tip and silently tag a commit other than the one that was built and published. Tag handling is idempotent for resumed runs: an existing tag is peeled if annotated and reused when it resolves to the release commit, and is a hard error when it does not. The existence probe uses `git/matching-refs`, which returns 200 with an empty array when nothing matches, so any non-zero `gh` exit is a genuine failure that aborts the step -- the previous shape would have masked it. Matching-refs is a prefix search, so the result is filtered for exact ref equality; this repository has real prefix collisions, with `v0.11.0` matching every `v0.11.0-nightly.*` tag.
1 participant
TLDR
The shared Bun test runner decided which test files to execute from a hand-maintained list in
scripts/bun-test-manifest.ts. That list had drifted: 43 test files existed on disk that no CI job ever ran. They looked like coverage and asserted nothing, and two of them had rotted into real failures nobody could see.This deletes the manifest and makes selection structural.
scripts/bun-test-roots.tsdeclares only how a root runs (cwd, scanned directories, filename pattern, preloads, tsconfig, timeout, retries, globalSetup, credentials, per-file timeout overrides). Which files run is answered by walking the filesystem, so a new test file executes by virtue of existing. There is nofiles,include, orexcludemember anywhere in the selection path — the issue's hard requirement.scripts/check-test-file-coverage.tsis the new guard that keeps this honest: it walks the repository for test files and fails when one is executed by no executor or by more than one. It runs on every non-docs PR, which is what lets thebun_native_test_parityjob be deleted.Turning discovery on immediately paid for itself: it exposed a Linux-only Bun fake-timer hang that the deleted allowlist had been commenting out for seven
packages/providersauth suites, and that fix took the providers shard from 16m09s to 6m23s on CI.Reviewer attention: the
agentsroot narrowing totest-bun, the removal of thecli/coreroots, and the migration oflsponto the shared runner all reduce what the shared runner claims. The coverage guard is the mechanism that proves nothing fell through those gaps — please satisfy yourself it actually does.Dive Deeper
Why a canary job was not the answer
bun_native_test_parityonly ran--dry-run: it proved every listed file existed. It could never prove the list was complete, which is precisely how 43 files went unexecuted. The replacement guard asserts completeness instead, and derives the covered set from each executor's own discovery code rather than restating it, so it cannot drift out of sync with what CI runs.Fail fast, not silently
The walker previously would have swallowed
readDirectory/stat/realpathfailures and returned a short list — an unreadable subdirectory could drop tests while another file kept the root non-empty, so the "no files found" check never fired. All three now propagate asBunTestRootStatError. Related fixes in the same vein:.git/,.github/,.integration-tests/), but a dot-prefixed file matching the pattern is no longer silently dropped.The guard has to run on the PR that would break it
Affected-shard selection maps a package-test-only change to that package's shard, so a PR adding
packages/providers/src/new.test.tsselects[providers]and skipsscripts. A guard living only in the scripts shard would therefore be skipped on exactly the PR that could introduce an uncovered file.scripts/check-test-file-coverage.tsis now directly executable (npm run lint:test-file-coverage) and wired as a step in the always-runbun_test_orchestrator_smokejob. The behavioralbun:testsuite is kept as well.Consequences of actually running everything
cliandcoreshared roots deletedrun-bun-tests.tsrunners already discover every file with no allowlist. Redundant.agentsshared root narrowed totest-bunsrc/**entries were being executed twice per CI run — once by the shared root, once by the bespoke runner.coreandauthbroadened to*.spec*.test, so 13 spec files ran nowhere. All 13 pass. JUnit classnames strip the suffix accordingly.lspmoved onto the shared runnerbun testcould only be modelled by the guard, never derived — a hole in the guarantee. All 13 lsp files pass under one-process-per-file.scripts-tests-slowfolded awaytimeoutOverridesentry. An override changes the budget only, never membership (300s per-test / 600s process, matching the deleted root).The three previously-invisible failures
providers/src/openai/OpenAIRequestPreparation.issue2853.test.tsvi.mock('../../prompt-config/subagent-delegation.js')resolves to nothing; production imports@vybestack/llxprt-code-core/prompt-config/subagent-delegation.js, which is what every executed sibling test mocks.providers/src/runtime/promptEnvelopeProjections.test.tstoMatchObjectmutates its received object when resolving asymmetric matchers, unfreezing it; theObject.isFrozenassertion ran after it. Product freezing is correct and still asserted, now before the matcher.tools/src/tools/check-async-tasks-shell-formatter.test.ts2023-11-14T22:14:10.000Zbutnew Date(1_700_000_005_000).toISOString()is2023-11-14T22:13:25.000Z— the expectation, not the formatter, was wrong.Bespoke runner changes
packages/{core,agents,auth}/run-bun-tests.tsgainimport.meta.mainguards and export the samediscoverTestFilesfunctionmain()itself calls, so the guard reads exactly what CI executes rather than a copy.coreandauthalso nowawait main()— an unawaited asyncmain()turns a discovery failure into an unhandled rejection instead of a clean non-zero exit.scripts/tests/bun-test-root-ownership.bun.test.tswas extended to read the realpackage.jsonfiles and prove each bespoke executor's workspacetestscript still invokes its runner, so the executor table cannot silently claim coverage a package no longer provides.Reviews performed
DeepThinker (3 blockers, 4 should-fix) and Open Code Review (7 findings) were run against this change; every finding is resolved in the branch. The blockers were the swallowed filesystem errors, the dot-file pruning, and the guard not running under affected-shard selection.
The blocker the allowlist was hiding
The seven suites the manifest had commented out carried the note "Bun fake-timer incompatibility on Linux CI ... re-add when Bun runtime is fixed." Every case in them timed out at exactly the per-test timeout on Linux while passing on macOS. Reproduced in
oven/bun:1.3.14onlinux/arm64:test-setup/augment-bun-vi.ts'sflushPendingTasksended its microtask drain by awaiting asetImmediate. Under Bun's fake timers, once a timer has fired and the clock is then advanced with no pending timers,setImmediate(andsetTimeout(_, 0)) is gated by the fake-timer scheduler and never becomes due — so the await never returns andadvanceTimersByTimeAsynchangs until teardown. Probes in that state showqueueMicrotask,process.nextTickandPromise.resolveall returning in under a millisecond, because microtasks drain inside the current macrotask before the scheduler regains control. macOS happens to keep firing the macrotask, which is the whole reason the failure looked platform-specific.The settling boundary is now a microtask. That also restored Vitest parity the macrotask had broken: for a timer whose awaited continuation schedules a nested timer, Vitest fires it at 25ms and the shim's own test asserted 35ms — the extra macrotask turn had deferred the continuation past the next timer. Confirmed by running the identical scenario under Vitest, which produces 25.
Two more failures this surfaced, both pre-existing
scripts/tests/ocr-concurrency-canary-2673.test.tsasserted the client observes a 200 after the upstream destroys the socket mid-body. The proxy forwards the 200 and then tears the client connection down withresponse.destroy(), which sends an RST; on Linux an RST discards data still unread in the client's receive buffer, so the status line may never arrive. This failed thescriptsshard onmain(run 31071511528) before this branch existed. Reproduced 1-in-12 in alinux/amd64Node 24 container. The three telemetry assertions — one request, no upstream errors, one forwarded 200 — are unchanged and remain the proof of the behavior the test is named for; only the client-side assertion now accepts either outcome the RST race can produce, and names the observed one so an unexpected third outcome reports its status and body.packages/agentsfails a different file on each run and passes in isolation. Reproduced onmainwith this branch stashed (four files failed there). Not addressed here.Review findings folded in
Two soundness gaps were found reviewing the discovery work itself and are fixed in this PR:
core/agents/authrunners discovered underimport.meta.dirwhile the spawned child, its preload and the JUnit report still resolved againstprocess.cwd(). The anchors agreed only when the runner was invoked from its own package directory. Each now anchors everything at oneWORKSPACE_ROOTconstant.Not in scope
Migrating remaining workspaces (#2845/#2846/#2847), removing Vitest (#2970), rewriting the
vitestspecifier (#2969), and re-recording CI critical-path timings in #2702 (a post-merge measurement, not a code change).Reviewer Test Plan
grep -rn "files:\|include:\|exclude:" scripts/bun-test-roots.tsreturns nothing in the root table. The new suite asserts this mechanically.touch packages/providers/src/zz-scratch.test.ts && bun scripts/run_bun_tests.ts --root providers --dry-run | grep zz-scratch— it appears. Delete it afterwards.mkdir -p research/orphan && touch research/orphan/x.test.ts && bun scripts/check-test-file-coverage.ts— should fail and name the file. Delete it afterwards.directories: ['src', 'test-bun']back to a root whose bespoke runner also scanssrcand re-run the guard — it should report the file with both claiming executors.chmod 000a subdirectory under a scanned root and runbun scripts/run_bun_tests.ts --root <that root> --dry-run— it must raiseBunTestRootStatError, not quietly return fewer files. Restore permissions afterwards.bun test scripts/tests/bun-test-roots.bun.test.ts scripts/tests/test-file-coverage.bun.test.ts scripts/tests/bun-test-root-ownership.bun.test.ts, thencd packages/lsp && npm test,cd packages/core && npm test,cd packages/auth && npm test.bun scripts/run_bun_tests.ts --root providers(544 files, was 503).Testing Matrix
Verified locally on macOS:
npm run format,npm run typecheck,npm run lint,npm run build,npm run test, and the CLI smoke (bun scripts/start.ts --profile-load stepfun-37). Linux is covered by CI. The change is filesystem-walk based, so the Windows path-separator handling is worth a reviewer's eye.Residual
npm run testfailures on this machine are pre-existing local flakes:packages/agentsandpackages/corefail a different file on each run and pass in isolation (reproduced onmainwith the branch stashed, which failed four files), and the threepackages/cliintegration failures are in a workspace this PR does not touch (git diff HEAD -- packages/cliis empty).Linked issues / bugs
Fixes #2979
Contributes to #2578 (finish the Bun migration) and #2702 (CI execution optimization). Same silent-exclusion class as #2923. Precedes #2970.
Summary by CodeRabbit
New Features
.spec.tsand.spec.tsxfiles.Bug Fixes
CI