Repository navigation
🤖 ci: collect coverage from isolated unit test files - #6307
Merged
Merged
Conversation
Each isolated unit file and the tooling run now write their own LCOV report (coverage/isolated/<n>/, coverage/tooling/). Files isolated because Bun coverage crashes them are marked no-coverage: in the one list. The last crash retry runs without coverage. CI uploads every report and Quality / CRAP reads them all. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
Member
Author
|
@codex review |
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. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/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: 4f1bc15042
ℹ️ 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".
--- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
Member
Author
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d390a94b35
ℹ️ 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".
--- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
Member
Author
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 13, 2026
## Summary Upgrade the pinned Bun from 1.3.12 to 1.4.3. With 6 small fixes (3 in tests, 1 in dev dependencies, 2 in production code), the full PR CI run is green on Bun 1.4.3, and CI time does not change measurably. The main gain: the Bun 1.3.12 coverage bug that hides lines after a `using` declaration is gone, so the CRAP gate scores those functions correctly again. `resumeStream` (`workspaceService.ts`) goes from 5% to 99% line coverage and `executeBash` from 9% to 96.5% on the same tree. ## Background Bun 1.3.12 loses line coverage when a method is followed directly by an async or generator method that no test calls. With a `using` declaration in the method, every line after the `using` reads as never run. The CRAP gate scores functions from Bun coverage, so these methods read about 0%. coder#4958 (`AbortSignal.timeout` never fires after its last listener is removed, Bun 1.3.13 and 1.3.14) blocked every bump past 1.3.12. Its repro passes on Bun 1.4.2 and 1.4.3 on this host (`abort event aborted= true`). ## Implementation Pins changed to 1.4.3: `.tool-versions`, `package.json`, `packages/chat-components/package.json`, `vscode/package.json` (`packageManager`), `.github/actions/setup-xum/action.yml`, `.github/workflows/{auto-cleanup-fixup,auto-cleanup,pr,update-models}.yml`, `Dockerfile`, `benchmarks/terminal_bench/mux_setup.sh.j2`, `docs/guides/github-actions.mdx`, `docs/AGENTS.md` (host requirement) and the regenerated `builtInSkillContent.generated.ts`. Three test comments that name the pinned runtime now name 1.4.3. `flake.nix` takes Bun from nixpkgs, so it has no pin. Not changed: `tests/bugbash/sandbox/Dockerfile` (`oven/bun:1.3.12-slim`) and its published image in `image.json`, and `.github/workflows/perf-profiles.yml` (pins Bun 1.3.5, not 1.3.12. I did not check why). ### Fixes for what 1.4.3 breaks 1. `bun install` on 1.4.3 hoisted `playwright-core@1.63.0` (from `@e2e-dev/web`) to the root. `@coder/pixel-storybook` then loaded 1.63, whose Chromium CI does not install (**Test / Storybook** failed), and `tests/bugbash/sandbox/build.sh` found no `@e2e-dev/web/playwright-core` entry (**Test / Bug-bash sandbox** failed). Fix: a root devDependency `"playwright-core": "1.57.0"` (the version `playwright@1.57.0` pins), which restores the 1.3.12 tree. The new `bun.lock` also drops two optional peer entries that nothing installs or imports (`@opentelemetry/api`, `esbuild-register`). 2. `tests/bugbash/sandbox/runner.test.ts`: `process.env.TMPDIR = undefined` now stores the string `"undefined"` (Node does the same), and `os.tmpdir()` returned it. The test now deletes the variable when it was unset. 3. `tests/bugbash/sandbox/inContainer.test.ts` and `src/node/services/browser/agentBrowserCommandClient.test.ts`: a fake server that calls `socket.end()` without reading never sees the peer's FIN, so `server.close()` waits forever. Node does the same; Bun 1.3.12 closed it anyway. The fake servers now call `socket.resume()` first. 4. `src/node/orpc/replayOwnership.test.ts`: on Bun 1.4.2 and 1.4.3, `AsyncLocalStorage.disable()` does not hide the store from a continuation that resumes after it. Node and Bun 1.3.12 do. The product runs on Node (`#!/usr/bin/env node`, the Docker `ENTRYPOINT`, Electron), so the test now asserts the release only when a probe shows that the runtime implements it. The rest of the test runs everywhere. 5. `computePriorHistoryFingerprint` and its async twin: their sort comparator was inconsistent for a non-finite persisted `historySequence` (`NaN || tiebreak`). On 1.4.3 the reference sorted such rows differently once the DFG JIT optimized it (`BUN_JSC_useDFGJIT=0` makes the test pass), so the twin test failed. Fix: a shared total-order comparator (`comparePriorHistoryOrder`) that puts non-finite sequences after finite ones, then orders by id. The order of finite sequences does not change. It changes the fingerprint only for histories with a malformed sequence, and a fingerprint mismatch falls back to a full replay. 6. `LocalBaseRuntime` (child `stdout`/`stderr` and `readFile`) now uses `nodeReadableToWeb` (`src/node/utils/nodeReadableToWeb.ts`) instead of `Readable.toWeb`. On Bun 1.4.3, `Readable.toWeb` can get a `data` event from a child's pipe after the web stream was cancelled. Its listener then calls `enqueue` on the closed controller and throws `TypeError: Invalid state: Controller is already closed` outside any caller. The bash tool hits this when it cancels its readers on output overflow. In tests it fails the test. In the CLI (terminal-bench runs it on Bun, `mux-run.sh`) the uncaught exception ends the run. The adapter follows Node's adapter (pull resumes the source, byte-sized queue at the source's highWaterMark, chunks copied out of the pool, cancel destroys the source, a premature close is an AbortError), and it drops `data` that arrives after cancel or end. A repro harness that spawns a fast-exiting writer and cancels at 1 MB threw in 15 of 300 runs on 1.4.3 with `Readable.toWeb`, 0 of 600 with the adapter, and 0 of 300 on 1.3.12. ## Validation ### CI | Run | Head | Result | | --- | --- | --- | | [38372263624](https://github.com/coder/xum/actions/runs/38372263624) (+1 rerun of failed jobs) | `4efc81977a` (pins only) | Storybook, Bug-bash sandbox (fix 1), Unit 1/6 (fixes 2, 3), Unit 3/6 (fix 4 and a flake, below). Same failures on the rerun. | | [38378155990](https://github.com/coder/xum/actions/runs/38378155990) | `d19fec9773` | Unit 4/6 only: `agentBrowserCommandClient` (fix 3, second file). | | [38382892697](https://github.com/coder/xum/actions/runs/38382892697) | `f7e1fb5fea` | All 30 jobs green. | | [38396451754](https://github.com/coder/xum/actions/runs/38396451754) | `6f4b6d727a` (fix 6) | All 30 jobs green (attempt 1, no rerun). The run for `562063cf36` was cancelled by the newer push. | | [38419481375](https://github.com/coder/xum/actions/runs/38419481375) | `0928afbff0` (rebased onto `b01fe77f6b`) | All 30 jobs green (attempt 1). | Flake (fix 6): `bash tool > should reject overlong line at boundary (IPC mode)` failed once in run 1 with `Controller is already closed`. Failure counts in `src/node/services/tools/bash.test.ts`, each test alone 40 times and the whole file 15 times: | Test | 1.4.3 before | 1.4.3 after | 1.3.12 after | | --- | --- | --- | --- | | reject overlong line at boundary (IPC mode) | 4/40 | 0/40 | 0/40 | | truncate overflow output (`overflow_policy: 'truncate'`) | 1/40 | 1/40 | 0/40 | | whole file | 1/15 | 0/15 | 0/15 | All 6 "before" failures were `Controller is already closed`. The one "after" failure is a second, older race in the truncate test: the tool cancels at 1 MB while `perl` still writes 1.5 MB, and when the pipe closes first, `perl` exits 1 and the tool reports `success: false`. It happens with and without fix 6 (1.4.3: 1 of 150 runs before, 1 of 300 after, 1.3.12: 0 of 150), so it is not caused by the fix (follow-up below). PR CI runs (the first three predate the rebase onto current main): ### Local (`make static-check`, `make test`) `make static-check` passes on 1.4.3 and on 1.3.12. `make test` (one `bun test src` process, 25,321 tests) has 12 failures on 1.4.3 and 14 on 1.3.12. Both share `taskGitPatchEngine` (host `init.templateDir`) and one `BackupRepoCache` timeout. The other failures are 5 s timeouts that differ between runs. Every 1.4.3-only failure passes when its file runs alone on both versions, except the 3 that fixes 3–5 address. <details> <summary>Coverage (CRAP gate)</summary> Main moved during the measurement and coder#6307 added coverage from isolated unit files, so the CI comparison uses only each shard's shared `lcov.info` and counts only functions in files that are the same in both trees. | Measure | Before (1.3.12) | After (1.4.3) | | --- | --- | --- | | Functions above CRAP 30, all 9 lcov files, repo-wide | 876 | 848 | | Same, only files unchanged between the trees (13,518 functions) | 818 | 809 | | Same, unit lcov only | 912 | 902 | | Above CRAP 30 in `workspaceService.ts` (9 files) | 14 | 14 | | `resumeStream` (`workspaceService.ts`, cc 83): coverage, CRAP (9 files) | 33.3%, 2124 | 99.5%, 83 | | `executeBash` (cc 38): coverage, CRAP (9 files) | 41.1%, 333 | 97.2%, 38 | | `resumeStream`, unit only | 5.1% | 99.0% | | `executeBash`, unit only | 9.2% | 96.5% | Before: unit lcov from run [38372261688](https://github.com/coder/xum/actions/runs/38372261688) (a browser-only PR, a Checkbox component and CSS, merged into `32364fb956`, the same base), integration lcov from run [38374718068](https://github.com/coder/xum/actions/runs/38374718068) (the closest green run that ran the backend integration tests, merged into a newer main). Main push runs upload no `crap-lcov-*` artifacts. After: run 38382892697, scored on its merge tree (`f83f8e1182` + this branch). Integration coverage comes from Jest, so Bun does not change it. Same-tree A/B (the 9 `workspaceService*.test.ts` files that call these methods, run with coverage under each Bun on one tree): `resumeStream` 5.1% to 99.0%, `executeBash` 9.2% to 96.5%, `fork` 2% to 77%, `getFileCompletions` 20% to 98%. No function lost 10 points or more. </details> <details> <summary>CI time</summary> | Measure | 7 green PR runs on 1.3.12 (range) | This PR, run 3 | | --- | --- | --- | | Total wall time (first job start to last job end) | 8.9–12.2 min | 10.2 min | | Slowest `make test-unit-ci` step | 439–589 s | 440 s | | Sum of the 6 unit test steps | 1754–1972 s | 1809 s | | Slowest integration test step (backend runs only) | 253–304 s | 264 s | Runs 1 and 2 were slower to set up (unit setup 54 s vs 30 s) because the `node_modules` cache key includes the Bun version. This PR runs 2 jobs more than the baselines (Nix Build, Lint Actions), because it changes `bun.lock` and the workflows. </details> <details> <summary>Bun 1.3.12 workarounds: status on 1.4.3</summary> | Workaround | Bug on 1.4.3 | Action | | --- | --- | --- | | coder#4958 `AbortSignal.timeout` | Fixed (repro passes on 1.4.2 and 1.4.3) | Close coder#4958 when this lands. | | `tests/bunEachTableGcWorkaround.ts`, `tests/bunEachTableGc.test.ts` (coder#6020) | Fixed: the fixture passes 10 of 10 runs, and the workaround only runs on 1.3.12 | Left. Remove after the bump. | | `scripts/test-unit-ci.sh` isolation of `bunEachTableGc.test.ts` | Same as above | Left. | | `scripts/perf/desktopColdStart.ts:8` (runs under Node) | Fixed: Playwright `_electron.launch` from Bun hangs on 1.3.12 and works on 1.4.3 | Left. | | `scripts/crap/crapper_report.py:163` (a loaded but never-run function lists every line at 0) | Still present (same DA lines on both) | Keep. | | `scripts/crap/crapper_report.py:201` (an uncalled arrow counts a later line as hit) | Still present | Keep. | | `tests/bugbash/sandbox/proxy.ts:280` (`node:http` misses a client disconnect) | Not tested here (out of scope) | Follow-up. | </details> ## Risks Low for the product. The product runs on Node, and the production changes are the comparator (fix 5) and the stream adapter (fix 6), which follows Node's `Readable.toWeb` behavior. The remaining Bun-only risk is in tests: `AsyncLocalStorage.disable()` semantics that differ from Node, and the truncate test's pipe race. ## Follow-ups Not done in this PR: 1. Delete the inert test.each GC workaround: `tests/bunEachTableGcWorkaround.ts`, its `bunfig.toml` preload, `tests/bunEachTableGc*.ts`, and the `jest.config.js` and `scripts/test-unit-ci.sh` entries. Then close coder#4958. 2. Move the bug-bash sandbox image to `oven/bun:1.4.3` (`tests/bugbash/sandbox/Dockerfile`, rebuild and publish `image.json`), and make the "a client that goes away" test in `proxy.test.ts` strict again (the `proxy.ts:280` removal trigger). 3. Give the `AsyncLocalStorage.disable()` gate in `replayOwnership.test.ts` a removal trigger. 4. Bump `playwright-core` together with `playwright` and `@playwright/test`: the root pin from fix 1 must move with them. 5. Make the truncate test independent of the pipe race (for example, a script whose exit code does not depend on the pipe). --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$16.55`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=16.55 -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CI now collects coverage from the isolated Bun unit test files and the tooling run, so Quality / CRAP scores the sources those files test. Before this change, only the shared-process run of each unit shard wrote
coverage/lcov.info. For example, CI coverage ofsrc/node/services/coderOauthService.tswas about 5% although its 101-test file covers about 97% of it.Background
scripts/test-unit-ci.shruns each file ofisolated_unit_testsin its ownbun testprocess, without--coverage. The tooling run on shard 1 also had no--coverage. The CI upload, Codecov, andscripts/crap/ci.tsread onlycoverage/lcov.info.Implementation
scripts/test-unit-ci.sh: each isolated file writescoverage/isolated/<n>/lcov.info, and the tooling run writescoverage/tooling/lcov.info(--coverage-dir). The shared process still writescoverage/lcov.info.no-coverage:prefix in the one existing list marks the files that are isolated because Bun coverage crashes or breaks them. The script strips the prefix and records the choice per index, so no second list can drift. Marked: the QuickJS group (WorkflowRunner*,WorkflowService.evaluate,quickjsRuntime,code_execution*,sandboxHostService,hookService,router.test.ts),CommandPalette.prompt.test.tsxandmcpIconDecodeClient.test.ts.sandboxHostServiceandhookServicejoined the list as QuickJS-heavy suites (🤖 feat: add shared agent-foundation layer (event spine, journal kit, sandbox host, capability grants) #3865, 🤖 feat: make provider requests a pure function of the session log + sandboxed plugin hooks #3872).router.test.tshas sat under the QuickJS coverage comment since the list began (🤖 feat: heartbeat trigger and when-busy scheduling modes #3689), with no other recorded reason, so I kept it without coverage.UNIT_COVERAGE_DIR(defaultcoverage):scripts/test-unit-ci.test.tsruns the lane again from inside the tooling run, and points that nested lane at a temp folder so it cannot delete the outer lane's reports..github/workflows/pr.yml: the unit job uploadscoverage/**/lcov.infoin the samecrap-lcov-unit-<shard>artifact, and sends the same list to Codecov.scripts/crap/ci.ts:lcovFiles()reads everylcov.infounder each artifact folder, at any depth. The artifact count check is unchanged. An artifact counts as a shard report only with its top-levellcov.info, so a shard whose shared process passed only on its no-coverage retry shows as partial.crapper_report.pyalready merges the reports per function.Validation
A fake
bunonPATHproved the retry paths: two coverage crashes, then a pass without coverage (exit 0, warning). A coverage-run exit 1 fails at once. Ano-coverage:file runs without coverage flags.--list-alloutput is identical toorigin/main, socheck-test-routing.shsees the same files.CI coverage, scored by
scripts/crap/crapper_report.py(function coverage weighted by function size) over everycrap-lcov-unit-*artifact. Before: PR run 38365653767 on main. After: this PR's run 38370497621.src/node/services/coderOauthService.tssrc/browser/components/ChatPane/WorkspaceFooterBar.tsxsrc/browser/features/RightSidebar/Memory/MemoryTab.tsxThe isolated report alone hits 1,488 of 1,535 lines of
coderOauthService.ts(96.9%) on run 38371558887. A raw line union across reports reads lower (64%), because Bun lists every line of an unrun function in the shards that only load the file. The CRAP scorer already ignores those reports per function.Slowest unit shard, job seconds: 538, 683, 504, 549 and 571 on five recent main-based PR runs. 533, 523 and 512 on this PR's three runs. Shard 1, which now also covers the tooling run and
coderOauthService.test.ts: 393 to 504 before, 393 to 472 after.Run 38371558887: Codecov found 3 reports on shard 1 (
coverage/isolated/12/lcov.info,coverage/lcov.info,coverage/tooling/lcov.info). No shard printed abun crashedline.Follow-ups
::warning::in the Test / Unit job name the case. A manifest of expected reports would close it. I deferred it: it is a new on-disk artifact for a case that needs two signal crashes in a row on one file. Trigger: the first time that warning shows up in CI.router.test.tsstays without coverage only because of where it sits in the list. A later PR can try it with coverage.Risks
Low, CI only. Coverage slows the isolated files and the tooling run. A coverage crash in a newly covered file costs one or two retries, then runs without coverage, so the required unit job does not get less reliable.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high