Conversation
|
Warning Review limit reached
Next review available in: 8 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: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — type-declaration-only fixes, each verified against runtime.
What was reviewed:
- Confirmed
jest.nowis wired atjest.rs:435andJSMock__jsSetSystemTimereturnscallframe->thisValue();toHaveBeenCalledOnce/toReturn/lastReturnedWith/nthReturnedWithexist injest.classes.ts. resize()/grow()→voidmatches the ES spec and TS's ownlib.es2024;FormDataEntryValueiterators now matchget()/forEach()andlib.dom.- Line-offset bump in
bun-types.test.ts(307→311) matches the 4 lines added to theglobals.tsfixture.
Extended reasoning...
Overview
This PR touches only packages/bun-types/{bun,globals,test}.d.ts plus four fixture files under test/integration/bun-types/fixture/ and a single expected-diagnostic line offset in bun-types.test.ts. No runtime code is modified. Each change corrects a type declaration to match already-shipped runtime behavior: YAML.stringify returning undefined for undefined/symbol/function (mirroring JSON.stringify), ArrayBuffer.resize/SharedArrayBuffer.grow returning void per ES spec, FormData iterators yielding FormDataEntryValue, and the addition of jest.now(), chainable jest.setSystemTime(), and four jest-extended matcher aliases.
Security risks
None. Pure .d.ts edits — no compiled code, no auth/crypto/permission surfaces, no data handling.
Level of scrutiny
Low. Per CLAUDE.md, .d.ts edits don't require a native build, and the bun-types.test.ts integration suite type-checks the fixtures against the packed tarball with skipLibCheck: false. I spot-checked every runtime claim in the PR table against source: JSMock__jsNow returns jsNumber(globalObject->jsDateNow()) and is registered on the jest object at jest.rs:435; JSMock__jsSetSystemTime returns callframe->thisValue(); the four matcher names appear in jest.classes.ts:308,342,358,366; Bun.FormDataEntryValue is File | string so the satisfies clauses in the fixture are exact.
Other factors
Each fix has a corresponding fixture assertion (expectType(...).is<...>() or satisfies) that would fail without the .d.ts change, satisfying the repo rule that tests must fail on the unfixed build. The 307→311 offset bump in the lib.dom diagnostic list correctly accounts for the 4-line for...of block added to the FormData fixture. No CODEOWNERS on packages/bun-types/, no outstanding reviewer comments, and no prior review from me on this PR.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
packages/bun-types/package.json:35-37— This change topackages/bun-types/package.json(adding"version": "1.4.0"and stripping the trailing newline) is an accidentally-committed test artifact — it's byte-for-byte what an interrupted run ofbun-types.test.ts/scripts/build.tsleaves behind, and is unrelated to the.d.tsfixes in this PR. Please revert this file to its pre-PR state (noversionfield, trailing newline restored).Extended reasoning...
What the bug is
The PR modifies
packages/bun-types/package.jsonto add a hardcoded"version": "1.4.0"field at the end of the object and strips the file's trailing newline. Prior to this PR (at base commit468dac3c), this file had noversionfield and did end with a trailing newline — verified viagit show 468dac3c:packages/bun-types/package.json.How it manifests / the code path that produces it
This diff is not an intentional edit — it is byte-for-byte the residue of the bun-types build/test flow:
packages/bun-types/scripts/build.ts:7does:The spread placesawait Bun.write(join(import.meta.dir, "../package.json"), JSON.stringify({ ...pkg, version: BUN_VERSION }, null, 2));
versionlast in the object, andJSON.stringifyemits no trailing newline — exactly matching this diff.test/integration/bun-types/bun-types.test.ts(inbeforeAll) runscp package.json package.json.backup, then writes the sameJSON.stringify({ ...pkg, version: BUN_VERSION }, null, 2), then runsbun run build/bun pm pack, then restores viamv package.json.backup package.json.- If either the test or a manual
bun run buildinpackages/bun-typesis interrupted between the write and the restore, the working tree is left with precisely this diff.
Why existing code doesn't prevent it
The backup/restore in
bun-types.test.tsexists specifically so this artifact is never committed — but it only works if the process reaches themv package.json.backup package.jsonstep. An interrupted local run (Ctrl-C, crash, timeout) leaves the modifiedpackage.jsonin place, and it was then staged alongside the real.d.tschanges.Step-by-step proof
- Base state (
468dac3c):od -con the file ends with... ] \n } \nandjson.load(...)reports'version' in d→False. - PR state: the diff shows
+ "version": "1.4.0"appended afterkeywords, and\ No newline at end of file. - Fingerprint match:
JSON.stringify({...pkg, version: "1.4.0"}, null, 2)on the base file produces the PR's exact bytes — spread order puts injected keys last, andJSON.stringifynever appends a trailing newline. - PR intent: the PR description and title mention only
.d.tsfixes (YAML.stringify, ArrayBuffer.resize, FormData iterators, jest matchers).package.jsonis never mentioned. There is no reason a types-only PR would hardcode a package version.
Impact
Functionally, publishing is unaffected —
scripts/build.tsoverwritesversionat pack time via{ ...pkg, version: BUN_VERSION }. But committing this residue:- Checks a stale, meaningless
1.4.0into the source of a published package's manifest. - Defeats the purpose of the test's backup/restore dance (which exists precisely to keep this file version-free in git).
- Introduces unrelated churn in a PR that is otherwise pure
.d.tsedits; per the repo's own review rules on scope, unrelated accidental changes get reverted before merge.
Fix
Revert
packages/bun-types/package.jsonto its pre-PR state:git checkout 468dac3c -- packages/bun-types/package.json
(i.e., drop the
"version"field and restore the trailing newline.)
|
Already reverted in f676896; the current diff no longer touches |
|
CI: the
Ready for review. |
There was a problem hiding this comment.
LGTM — pure .d.ts corrections aligning types with runtime behavior, each covered by a fixture assertion.
What was reviewed:
- Verified each runtime claim:
JSMock__jsSetSystemTimereturnscallframe->thisValue(),JSMock__jsNowreturnsjsNumber, andtoHaveBeenCalledOnce/toReturn/lastReturnedWith/nthReturnedWithall exist injest.classes.ts. - Checked
FormDataiterator changes for lib.dom merge conflicts —FormDataIterator<T>(viaIteratorObject) is structurally assignable toIterableIterator<T>, so the newsatisfiesfixtures hold under both configs. - Confirmed the
globals.ts:307→311diagnostic offset matches the net +4 fixture lines, andpackages/bun-types/package.jsonis no longer touched.
Extended reasoning...
Overview
This PR batches seven small .d.ts corrections in packages/bun-types/{bun,globals,test}.d.ts: YAML.stringify → string | undefined, ArrayBuffer#resize/SharedArrayBuffer#grow → void, FormData iterators yield FormDataEntryValue, jest.now() added, jest.setSystemTime() → typeof jest, and four missing jest matcher declarations. Each change is exercised by a fixture in test/integration/bun-types/fixture/, and one hard-coded diagnostic line number in bun-types.test.ts is bumped to account for the +4 lines added to globals.ts.
Security risks
None. Type declarations only; no compiled code, no runtime logic, no I/O, no auth/crypto surface.
Level of scrutiny
Low. These are documentation-grade edits to ambient declarations. The correctness bar is "matches runtime", and I spot-checked every claim against source: JSMockFunction.cpp:1437,1460 confirm jest.now() and setSystemTime() return values; jest.classes.ts:308,342,358,366 confirm the four matcher aliases exist; resize/grow returning void is per ES spec and matches lib.es2024; Bun.FormDataEntryValue = File | string already backs get()/forEach() so extending it to the iterators is consistent with lib.dom.
Other factors
- The lib.dom interface-merge case was the only non-obvious risk: adding
[Symbol.iterator]()and changing iterator return types could conflict withlib.dom.iterable'sFormDataIterator<T>. SinceFormDataIteratorextendsIteratorObject(a superset ofIterableIterator), the fixture'ssatisfies IterableIterator<string | File>holds regardless of which merged overload resolves, and no new diagnostic appears in the lib.dom test case. - Fixture chain
jest.setSystemTime(...).useRealTimers()type-checks becausetypeof jestincludesuseRealTimers. - The accidental
package.jsonversion bump was already reverted in f676896; the current diff is clean. - No prior human review comments to address.
grow() resizes the buffer in place and returns undefined (https://tc39.es/ecma262/#sec-sharedarraybuffer.prototype.grow), but bun-types declared it as returning the SharedArrayBuffer. Because bun-types merges its own SharedArrayBuffer interface into the lib declaration, its signature is the one a grow(n) call resolves to, so chaining off the result type-checked even though it is undefined at runtime. Companion to the same correction for ArrayBuffer.prototype.resize in #32484; the two merge independently.
f676896 to
f50af7b
Compare
|
Updated 10:59 PM PT - Aug 12th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 36505That installs a local version of the PR into your bun-36505 --bun |
robobun
changed the title
|
Status: this PR has been cut down from the seven-fix batch to the single
|
…ckages/bun-types; raise the file's timeout (#37990) ### Problem - `bun test test/integration/bun-types/bun-types.test.ts` (the invocation CLAUDE.md documents, and the one `.github/workflows/bun-types.yml` uses) runs the file under bun's 5s default timeout. On a slow or busy machine `beforeAll` is cut off with `a beforeEach/afterEach hook timed out for this test.` and nothing runs. Buildkite never sees this because `scripts/runner.node.mjs` passes `--timeout=150000` for integration files. - The hook does not fit in 5s: `bun install --no-cache` inside `packages/bun-types` is a full workspace install that re-fetches manifests (0.7s to 6.2s in this container depending on the registry), then `bun run build`, `bun pm pack` and a `bun add` from the registry. The type-checking cases are in the same situation: a plain run here had the tsgo case at 5.1s and several LanguageService cases at 5 to 7s (those only pass today because the check is synchronous, so the timer cannot fire before the test resolves). - `beforeAll` built the package in place (`bun-types.test.ts:41-61` on main): it rewrote the tracked `packages/bun-types/package.json`, and `bun run build` writes `CLAUDE.md` and `docs/` next to it. The restore was the `mv package.json.backup package.json` at the end of a shell chain, so a timed out hook (bun abandons the hook's promise and moves on to `afterAll`) or a failing `bun pm pack` left the checkout with a modified `package.json` plus `package.json.backup`. That residue has already been committed by accident once (#36505, caught in review and reverted there). ### Fix - `packages/bun-types/scripts/build.ts` takes an optional output directory for the files it generates (`package.json` with the version filled in, `CLAUDE.md`, `docs/`). With no argument it still writes into the package itself, which is what `release.yml` runs and publishes, so the release flow is unchanged. - The test copies `packages/bun-types` (minus `node_modules`) into its temp dir, runs `bun run build <copy>` and `bun pm pack` inside the copy, and installs the tarball from there. Nothing under `packages/` is written anymore, so there is no restore step that an interrupted hook can skip. `bun pm pack` only needs a lockfile for `workspace:`/`catalog:` specifiers and `build.ts` imports nothing, so the `bun install --no-cache` step is dropped rather than moved. - The setup commands are now separate `&&` chains: a newline-separated Bun Shell script keeps going after a failing command and only throws if the last one fails, so a `build`/`pack` failure used to surface as a cascade of four errors instead of the first one. - The file calls `setDefaultTimeout(2 minutes)` before registering anything (the runner captures the default when each hook/test is declared). Every entry in the file installs from the registry or type-checks for several seconds, so a per-test override would just repeat the value 15 times. 2 minutes is 15 to 25x the slowest entry measured here (the whole hook is 1 to 5s on a release build, about 4s under the debug build) and stays under the 150s the CI runner gives integration files, so a hang in CI is still attributed to a specific test rather than to the file. - A new case, `building and packing bun-types leaves packages/bun-types untouched`, compares `package.json`'s text and the presence of `CLAUDE.md`/`docs/` in the checkout before the module's setup and after it. With the old `build.ts` it fails in setup (`bun pm pack` on the versionless copy reports `package.json must have name and version fields`) and the run leaves `package.json` modified plus `CLAUDE.md` and `docs/` behind, which is the bug; with the new one the tree stays clean. - Verified: `bun test test/integration/bun-types/bun-types.test.ts` (release, no `--timeout`): 15 pass, `git status` clean afterwards. `bun bd test test/integration/bun-types/bun-types.test.ts`: 3 pass, 12 skip (the LanguageService cases are `skipIf(isDebug)` already), tree clean. `bun run build` with no argument in `packages/bun-types` still builds in place. The tarball packed from the copy has the same 367 files as one packed in place (`files` in package.json selects them either way). ### Background - `beforeAll` / test timeouts in `bun:test`: each hook and test gets a deadline when it starts, resolved at declaration time as explicit argument, then `setDefaultTimeout()`, then `--timeout` (default 5000ms). A `setDefaultTimeout()` call therefore has to come before the declarations it should apply to, and it overrides the CLI value. When a `beforeAll` times out its promise is abandoned (nothing is cancelled), the tests in its scope are skipped and `afterAll` still runs. - `packages/bun-types/scripts/build.ts` is the `bun run build` script the release workflow runs before `npm publish` from inside `packages/bun-types`: it stamps the Bun version into `package.json`, copies `src/cli/init/rule.md` to `CLAUDE.md` and copies `docs/**/*.md(x)` in. `package.json` is tracked; `CLAUDE.md` and `docs/` are gitignored in that directory. - This integration test packs the package with `bun pm pack`, installs the tarball into a fixture project together with a stub `@types/bun`, and type-checks the fixture with TypeScript's compiler API, so it needs the real build output (it is how the missing `CLAUDE.md` in #33940 was caught), just not inside the checkout.
1 participant
Problem
packages/bun-types/globals.d.tsdeclaresSharedArrayBuffer.prototype.grow()as returning theSharedArrayBuffer(globals.d.ts:1083). Per the spec it returnsundefined(sec-sharedarraybuffer.prototype.grow, last step), andbun -e 'const b = new SharedArrayBuffer(1, { maxByteLength: 8 }); console.log(b.grow(4))'printsundefined.interface SharedArrayBufferinto the lib one, and the merged-in signature is the one agrow(n)call resolves to, so code that chains off the result (sab.grow(n).byteLength) type-checks and then throws at runtime.Fix
grow(size: number): SharedArrayBufferbecomesgrow(size: number): void, matching the spec, the runtime, and TypeScript's ownlib.es2024.sharedmemory.d.ts.ArrayBuffer.prototype.resize(); that PR stays as is, and the two merge independently in either order (checked locally).bun test test/integration/bun-types/bun-types.test.ts(types-only change, so it runs with the released bun): with only the fixture line, every lib configuration fails witharray-buffer.ts(14,31): error TS2344: Type 'void' does not satisfy the constraint 'SharedArrayBuffer'.; with the declaration change, 14 pass.Background
lib.dom/es2024enabled.History: this PR used to be a batch of seven type fixes
The earlier revision of this PR bundled
YAML.stringify,ArrayBuffer.resize, theFormDataiterator types,jest.now(),jest.setSystemTime()chaining,toHaveBeenCalledOnce()and the return-matcher aliases. Each of those already has a focused PR from its original author, and all seven still merge cleanly on current main with the types test passing, so this PR was cut down to the one line that none of them covers. The focused PRs are the ones to merge:Bun.YAML.stringify()returnsstring | undefinedArrayBuffer.prototype.resize()returnsvoidFormDatavalues()/entries()/[Symbol.iterator]()yieldFormDataEntryValue(#27194)toHaveBeenCalledOnce()declaration (#32332)toReturn()/lastReturnedWith()/nthReturnedWith()declarations (#32334)jest.now()declarationjest.setSystemTime()returnstypeof jestNote for whoever merges the four
bun:testones (#32333, #32335, #32455, #33923): they all append totest/integration/bun-types/fixture/mocks.ts, so after the first one lands the other three need a trivial rebase (keep both sides). #32455 and #33923 additionally touch adjacent lines of thejestnamespace intest.d.ts. The combined result of all seven plus this PR was checked locally and passes the types test.