Repository navigation
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 6 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
Comment |
|
Updated 11:06 AM PT - Sep 6th, 2026
❌ @robobun, your commit f9348c6 has 3 failures in
Add 🧪 To try this PR locally: bunx bun-pr 41528That installs a local version of the PR into your bun-41528 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
A bare yield now carries its text positions, so the serialized bytecode of files that contain one changed on every platform. The preview build is not the tagged release build, so its binary sizes differ. [skip size check]
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// lines do not: a line counts as executable only if it executed somewhere or | ||
| /// every report agrees it is executable, so a worker that saw a coarser view | ||
| /// of a function cannot show a fully executed function as partially covered | ||
| /// (#39930). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// One bit per source byte: set for bytes that are not whitespace and not | ||
| /// a closing brace. Only these bytes make a line executable, so the `}` | ||
| /// that ends a function, and the newline an executed block shares with the | ||
| /// last line of a dead one, do not count. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The function ranges include the program itself: the one that starts | ||
| // at 0 and reaches farthest. It is the outermost owner of every byte | ||
| // and is not a function in the report. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Every range JSC reported, function bodies first so that a block with | ||
| // the same span is painted after it and owns the bytes. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Paint the ranges widest first. The last one to touch a byte is the | ||
| // innermost range that contains it, and that range decides whether the | ||
| // byte executed. This is the same rule as JSC's | ||
| // `ControlFlowProfiler::findBasicBlockAtTextOffset`. It is what keeps a | ||
| // never-called function dead when an enclosing block executed. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Bytes whose owner mapped to a source line. Only meaningful with a | ||
| // source map: a function none of whose bytes map is not reported. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // JSC's function range starts at `function`, not at the `export` | ||
| // in front of it. Skip the keyword so that the function decides | ||
| // its declaration line. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The length of an `export` or `default` keyword at the start of `rest`, if | ||
| /// one is there. Whitespace must follow, so `default:` in a switch is not one. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// One range JSC reported for a file: a basic block piece or a function body. | ||
| /// `end` is inclusive, as JSC reports it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // JSC reports an empty piece as `end < start` when a nested function | ||
| // starts exactly where the block does. A negative offset maps to nothing. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The first significant byte on a line decides the line: that is the | ||
| /// statement which starts there. Later bytes may belong to a dead branch | ||
| /// on the same line, or to the statement that follows a dead one. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// A basic block or function body as `[start, end]` byte offsets (end | ||
| /// inclusive) into the generated source. Offsets are what JSC reports, so they | ||
| /// identify the same block across processes that loaded the same file. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
[skip size check]
| /// One bit per source byte: not whitespace, not `}`, not a leading | ||
| /// `export`/`default`. Only these bytes can make a line executable. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The program's own range starts at 0 and reaches farthest. It is not | ||
| // a function in the report. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Widest first, so the innermost range that contains a byte owns it | ||
| // (JSC's `ControlFlowProfiler::findBasicBlockAtTextOffset` rule). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Length of an `export` or `default` keyword followed by whitespace at the | ||
| /// start of `rest`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `end < start` is an empty piece (a nested function starts where the | ||
| // block does). A negative offset maps to nothing. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The first significant byte on a line decides the line: that is the | ||
| /// statement which starts there. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "2e2aa2290fac856d6f451ceacb58f7f5b44dd057"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-570-c685afa2"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-570-c685afa2, an ephemeral preview-PR artifact; merging this makes every fresh checkout's build depend on a release that will be deleted once oven-sh/WebKit#570 lands or is closed. Fix: land oven-sh/WebKit#570 first and pin WEBKIT_VERSION to the resulting merged commit SHA before this PR merges (per Dependencies & vendoring: never merge a pin to an ephemeral artifact).
Extended reasoning...
The prebuilt WebKit dep resolves its download URL from WEBKIT_VERSION. autobuild-preview-* releases on oven-sh/WebKit are transient CI artifacts for unmerged PRs and are garbage-collected; once PR 570 merges or the preview is pruned, bun bd on a clean cache fails at the WebKit fetch step for every developer and CI lane. The base branch pins a stable 40-char merged SHA (2e2aa2290fac856d6f451ceacb58f7f5b44dd057). The PR description acknowledges this is a preview build; the conventions block explicitly calls this out as must-swap-before-merge.
Verification: normal — scripts/build/deps/webkit.ts:6 changes WEBKIT_VERSION from the stable 40-char merged SHA "2e2aa2290fac856d6f451ceacb58f7f5b44dd057" to "autobuild-preview-pr-570-c685afa2". The codebase itself documents that these tags are ephemeral — scripts/build/download.ts:314-331: "The autobuild-preview-pr-* WebKit tags are the sharp edge: GitHub deletes the preview release when the PR…
[skip size check]
|
On the two review points:
|
[skip size check]
|
Both test nits applied in bca35c5: |
…d await [skip size check]
|
Overlap check against the other open PRs that patch
Where a narrow PR's inline snapshot differs only in the hit counts of covered lines (byte counts vs per-block counts), I counted the symptom as covered. Outcome: #33959 is closed as covered by both. #32849 is trimmed to its Trade-off between the two umbrella PRs: this PR needs oven-sh/WebKit#570 to land first (the pin points at its preview build). #38282 has no WebKit dependency and fixes the attribution symptoms on the current pin, but keeps the per-byte loop (quadratic on large files) and does not handle the class field initializer ranges, which it leaves to #38518. |
|
Real-world data point for the class field case (#5307, #9008), in case it helps prioritize this. Minimal repro on 1.4.3 (same on 1.2.15 through 1.4.2): // m.ts
export function check(x: number): number {
if (x > 0) {
return 1;
}
const y = -x;
return y;
}
export class Box {
value = 0;
}// m.test.ts
import { expect, test } from "bun:test";
import { check } from "./m";
test("check", () => {
expect(check(5)).toBe(1);
});
Impact in one TypeScript codebase (coder/xum): 237 of 1,872 source files had an emitted class field, static field, Workaround for others: an explicit Generated with |
…coverage is honest (coder#6926) ## Summary Bun's coverage marks every line of a file as hit when the file has a class with a field that Bun emits natively. coder#6422 fixed instance fields with `useDefineForClassFields: false`. Static fields and `#private` fields stay native, so 9 production files still read near 100% line coverage and the CRAP gate scored their functions too low. This PR moves those fields out of the classes. Nothing changes at runtime, and the 9 files now get honest coverage. ## Background - Bun 1.2.15 to 1.4.3 bug: an evaluated class with an emitted field reports one basic block with start > end, and Bun's LCOV writer turns it into "the whole file ran". Only functions that never ran read 0. Untaken branches read hit. Node's coverage is correct. Upstream fix: oven-sh/bun#41528. - coder#6422 covered instance fields. A scan of `src/` (TypeScript AST: `static` or `#private` property declarations, `accessor` fields, `static {}` blocks, not `declare`) found 11 files left on main. Two are test-only (`desktopRfb.test-fixture.ts`, `stories/mocks/desktopRfb.ts`) and the CRAP gate does not score them. This PR fixes the other 9. ## Implementation - `private static readonly X = ...` becomes a module-level `const X` placed just before the class, with its doc comment. Every `ClassName.X` becomes `X`. The value is evaluated in the same order as before: static initializers ran when the class was defined, during module evaluation. - Two statics were public and used outside their files: `DraftService.CHANGE_EVENT` is now the exported `DRAFT_CHANGE_EVENT` (router subscription and tests), and `BackupService.RETAINED_SNAPSHOTS` is the exported `RETAINED_SNAPSHOTS` (one test). `BackupService.RETAINED_INACTIVE_CACHES` had no outside user, so it is module-private now. - `ExactResourceURL` in `mcpOauthService.ts` kept its configured string in a `#raw` field. A module `WeakMap` keyed by the instance now holds it. The value stays off the object, as with the `#` field (a TS `private` field would add an own enumerable `raw` property to a `URL`). ## Validation I ran the tests of the 9 files (34 test files, 1407 tests) with `--coverage` on main and on this branch, then scored both with `scripts/crap/crapper_report.py`: | File | main LH/LF | branch LH/LF | | --- | --- | --- | | PowerModeEngine.ts | 408/408 (100%) | 400/408 (98.0%) | | secretsStore.ts | 285/295 (96.6%) | 255/288 (88.5%) | | ExtensionMetadataService.ts | 1004/1014 (99.0%) | 872/1016 (85.8%) | | historyService.ts | 4101/4532 (90.5%) | 3800/4518 (84.1%) | | StreamingMessageAggregator.ts | 2361/2488 (94.9%) | 2300/2510 (91.6%) | | mcpOauthService.ts | 1597/1651 (96.7%) | 1521/1655 (91.9%) | | backupService.ts | 833/854 (97.5%) | 808/854 (94.6%) | | draftService.ts | 523/528 (99.1%) | 509/526 (96.8%) | - The new zero lines are scattered untaken branches inside functions that ran, not a block. So this is real coverage, not Bun's inlined-const bug (the adapter flags 0 functions as `bunInlinedConstSuspect` on both sides). - With this test subset, 4 functions newly score above CRAP 30 (for example `HistoryService.getSubagentTranscript` CRAP 26 -> 136). CI runs the whole suite, so CI's numbers will differ. The gate is in warn mode, so they block nothing. - The AST scan finds no production file with a Bun-native class field after this change. ## Risks Low. The moved values are constants that nothing reassigns. Renaming the two public statics is a compile-time change, and `make typecheck` covers every user. ## Follow-ups - Nothing stops new code from adding a `static` or `#private` field, which would bring the bug back for that file. A lint rule could block these fields until oven-sh/bun#41528 ships in the pinned Bun. I left it out to keep this PR small. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high -->
|
Closing. This PR is superseded.
I ran the four test cases of this PR on the canary build of main ( One line differs from what this PR expected. With @ThomasK33 your repro gives #9008 is still open and is tracked in its issue. Reopen this PR if a case is still wrong. |
Fixes #5307
Fixes #7025
Fixes #8290
Fixes #9008
Fixes #29691
Fixes #31484
Fixes #33957
Blocked on oven-sh/WebKit#570. The WebKit pin in this PR points at that PR's preview build and moves to the merged sha once it lands.
Problem
bun test --coveragereports dead code as covered.generate_report_from_blocks(src/sourcemap_jsc/CodeCoverage.rs:682) OR-ed every executed JSC block into the line set and only cleared the lines of never-called functions afterwards. One executed block that encloses a dead one hides it: JSC's class field initializer block spans the whole file, so a class with a field made every dead method, function andifbody in that file read as covered (Incorrect lines covered when bun test --coverage #5307, bun test --coverage inaccurately reporting less than 100% coverage #9008).DAwas a byte count and not an execution count. The clear loop wasmin_line..max_line, so the last line of a never-called function stayed covered and the}above it was blamed instead (bun test --coverage off by 2 error #8290, bug(test): lcov reporter drops or duplicates lines in uncalled function bodies #31484, bun test --coverage emits DA records (DA:<line>,0) for comment-only and blank lines #33957).awaitoryield, the builtin default constructor recorded as a never-run function at bytes 1..15 of the program (Missing function coverage reporting in class without explicit constructor #7025, bun test --coverage marks first import line uncovered for class extending factory-call result #29691), and the field initializer sharing the program's first block.Fix
ControlFlowProfiler::findBasicBlockAtTextOffset. A line is executable if it has an owned significant byte, and its first significant byte decides executed and hit count. Whitespace,}and a leadingexport/defaultkeyword are not significant, so a dead function's closing brace does not count andexport function f() {is decided byf.&&,||and??, and the synthesized class functions kept out of the ranges (that part is Profilers: do not record the functions synthesized for a class as ranges of the defining code's source WebKit#436, squashed in).test/cli/test/coverage.test.ts(four new cases; all fail on main) andtest/cli/test/parallel.test.ts.Background
op_profile_control_flow.CodeBlock::insertBasicBlockBoundariesForControlFlowProfilerturns consecutive marks into[start, end]text ranges, end inclusive, and carves nested functions out as gaps. Bun reads those pieces plusFunctionHasExecutedCache's[start, end] -> executedfunction ranges.BUN_DEBUG_coverage=1on a debug build prints the raw ranges per file.Related PRs
DAvalues and the per-byte loop, and leaves the class field initializer ranges to Bump WebKit: stop the functions JSC synthesizes for a class from corrupting coverage of the code defining it #38518. This PR needs [JSC] Control flow profiler: blocks after await/yield and around short-circuit operands, skip synthesized class functions WebKit#570 first and covers all of it. One of the two should land. A test-by-test comparison is in this comment.--coverageoverhead measures linear in file size here and quadratic on main.coverageThresholdvalidation half, which is independent of this code.Notes
Raw ranges for
class Foo { x = 1; dead() { return 2 } } function neverCalled() {...} if (false) {...} new Foo()on main:block [0, 137] executed=true(the field initializer),function [1, 15] executed=false(the builtin(function () { })default constructor, in the program's coordinates), plus the real ranges. With an explicit constructor the shared first block turns into pieces[0, -1]and[293, 29]because the initializer's whole-file span is inserted as a gap into the program's block.Snapshot changes in
coverage.test.ts: hit counts are per block now (DA:2,28becomesDA:2,1), comment lines inside a dead function are no longer listed (the transpiler strips them, so they never map),returnlines of never-called functions are now 0, andclass Y { #hello }reportsFNF:0because the default constructor is no longer a function.bundler_bytecode_portable.test.ts's snapshot moves because a bareyieldnow carries text positions in the cached bytecode.Line percentages can drop for projects whose dead code was hidden before, so
coverageThresholdgates may start to fail. That is the report becoming accurate.Not changed here: JSC offsets are UTF-16 units and bun builds the line table from the UTF-8 source, so files with non-ASCII text before a range skew (#38710 covers that).
parallel.test.ts"each worker has a unique JEST_WORKER_ID" flakes on main in this container and is tracked separately.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bundler_bytecode_portable.test.ts