install: keep type-shipping packages project-local in the isolated linker#29728
install: keep type-shipping packages project-local in the isolated linker#29728robobun wants to merge 12 commits into
Conversation
|
Updated 11:05 PM PT - May 4th, 2026
❌ @robobun, your commit d832cb7 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29728That installs a local version of the PR into your bun-29728 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPackages that ship TypeScript declarations (top-level Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/isolated_install.zig`:
- Around line 2062-2080: The check in exportsHasTypesConditionInner is too
strict: it only returns true if the "types" key maps to a string; update the
function so any "types" key (regardless of the value type) causes a true result.
Concretely, inside exportsHasTypesConditionInner (and the loop over prop in
expr.data.e_object.properties) keep the key comparison using
key.data.e_string.eqlComptime("types") but remove or relax the value.data ==
.e_string requirement so you return true whenever the key equals "types" (you
may still require that prop.value exists via prop.value orelse continue). This
preserves the existing traversal and depth guard while matching the documented
"ANY `types` key" behavior.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: b383d567-7ee4-4952-9be1-c62b8b57f823
📒 Files selected for processing (3)
docs/pm/global-store.mdxsrc/install/isolated_install.zigtest/cli/install/isolated-install.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/isolated_install.zig`:
- Around line 2007-2014: packageShipsTypeDeclarations() currently reads
package.json from the cache via bun.sys.File.readFrom(...) and returns false on
read error, which makes the check valid only for warm caches; instead use a
manifest source available prior to extraction (for example the already-resolved
manifest data produced earlier during resolution) or move the eligibility check
to run after the missing_from_cache/download/extract path. Concretely, stop
relying on bun.sys.File.readFrom(cache_dir, ...) inside
packageShipsTypeDeclarations(); have packageShipsTypeDeclarations accept the
resolved manifest blob or parsed manifest structure (the same data used earlier
in resolution) and call scanForTypeDeclarationSignals against that, or defer
calling packageShipsTypeDeclarations until after extraction so readFrom is
guaranteed to succeed; update call sites accordingly (references:
packageShipsTypeDeclarations, bun.sys.File.readFrom,
scanForTypeDeclarationSignals, gop.value_ptr).
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 11ed67b0-7115-40c3-99c2-de7e3b4e7eab
📒 Files selected for processing (2)
src/install/isolated_install.zigtest/cli/install/isolated-install.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/isolated_install.zig`:
- Around line 2033-2038: The JSON.parsePackageJSONUTF8 call currently swallows
allocator OOM by doing `catch return false`; update the error handling so that
after calling install.initializeStore() you call
JSON.parsePackageJSONUTF8(&source, &log_sink, arena_allocator) and if it errors
then check if the error is `error.OutOfMemory` and call `bun.handleOom()` to
escalate OOM, otherwise return false for malformed JSON; keep the existing
symbols `install.initializeStore`, `JSON.parsePackageJSONUTF8`, and
`bun.handleOom` to locate and apply this change.
- Around line 1964-1969: The comment in installIsolatedPackages() incorrectly
cites "extraction deferred" as a possible reason package.json reads fail; update
the comment to reflect that installIsolatedPackages() runs only after
waitForEverythingExceptPeers() and waitForPeers() drain download/extract queues,
so package.json will exist on cold installs and the only realistic reasons are
cache eviction/corruption—tighten the wording to mention only cache
corruption/eviction (referencing the cache/<folder>/package.json path and the
installIsolatedPackages() function) and remove the "extraction deferred"
rationale to match actual install ordering.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 1def8f41-287c-4592-9cca-550e09620aa7
📒 Files selected for processing (2)
src/install/isolated_install.zigtest/cli/install/isolated-install.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/install/isolated_install.zig (1)
1967-1981: 🧹 Nitpick | 🔵 TrivialUpdate the cold-cache comments to match the real install ordering.
These comments now describe a normal cold-install path that cannot happen here.
installIsolatedPackages()runs only after the download/extract barriers have drained, so a read failure here means cache corruption/eviction, not “not yet extracted”. Keeping the stale explanation around is likely to resurrect the warm-cache-only confusion in future changes.Based on learnings,
installIsolatedPackagesis only called afterwaitForEverythingExceptPeers(manager)andwaitForPeers(manager)drain the download + extract queue, so<cache>/<folder>/package.jsonis guaranteed to exist on cold installs.Also applies to: 2022-2026
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/install/isolated_install.zig` around lines 1967 - 1981, The existing comment above installIsolatedPackages() describes a cold-install ordering that doesn't occur; update the comment to state that installIsolatedPackages() is invoked only after the download/extract barriers have drained (see waitForEverythingExceptPeers(manager) and waitForPeers(manager)), so a failure to read <cache>/<folder>/package.json here indicates cache corruption/eviction (or unexpected IO error), not "not yet extracted"; revise the explanation accordingly and remove the stale "first install after cache wipe" wording so future readers won't reintroduce the warm-cache confusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/isolated_install.zig`:
- Around line 2033-2058: scanForTypeDeclarationSignals currently treats
JSON.parsePackageJSONUTF8 failures as "no types" by using `catch return false`,
which reintroduces false-negatives and hides OOM; change the error path so parse
errors conservatively return "unknown/has types" (i.e., return true) instead of
false, and convert OutOfMemory errors using bun.handleOom so they crash instead
of being swallowed. Update the error handling around the call to
JSON.parsePackageJSONUTF8 inside scanForTypeDeclarationSignals (the same site
that calls install.initializeStore) to detect error.OutOfMemory and call
bun.handleOom(err), and for all other parse errors return the conservative true
value to match readFrom's memoization behavior.
In `@test/cli/install/isolated-install.test.ts`:
- Around line 2078-2088: The existing comment wrongly claims a cold-cache ENOENT
happens because eligibility DFS in installIsolatedPackages runs before
extraction; update the comment near installIsolatedPackages to remove that
misleading execution-order rationale and instead state the correct flow:
installIsolatedPackages is invoked only after
waitForEverythingExceptPeers(manager) and waitForPeers(manager), so package
cache manifests should exist even on cold installs; keep the note about
conservative fallback behavior for unreadable packages but clarify that it is
not caused by pre-extraction ordering.
- Around line 2089-2097: After the cold-cache reinstall (after runBunInstall)
re-assert that the control package "pure-js" remains project-local by adding the
same checks used for typeShippingNames: locate its entry with join(bunDir,
"pure-js@1.0.0") and assert lstatSync(entry).isSymbolicLink() is false,
lstatSync(entry).isDirectory() is true, and existsSync(join(entry,
"node_modules", "pure-js", "package.json")) is true so the test prevents
regressions where --frozen-lockfile would incorrectly make all packages
project-local; place this assertion block alongside the loop that checks
typeShippingNames.
---
Duplicate comments:
In `@src/install/isolated_install.zig`:
- Around line 1967-1981: The existing comment above installIsolatedPackages()
describes a cold-install ordering that doesn't occur; update the comment to
state that installIsolatedPackages() is invoked only after the download/extract
barriers have drained (see waitForEverythingExceptPeers(manager) and
waitForPeers(manager)), so a failure to read <cache>/<folder>/package.json here
indicates cache corruption/eviction (or unexpected IO error), not "not yet
extracted"; revise the explanation accordingly and remove the stale "first
install after cache wipe" wording so future readers won't reintroduce the
warm-cache confusion.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 3070ea28-866c-41a8-a008-e93a5f1e131a
📒 Files selected for processing (2)
src/install/isolated_install.zigtest/cli/install/isolated-install.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/cli/install/isolated-install.test.ts (2)
2095-2103:⚠️ Potential issue | 🟡 MinorRe-assert the
pure-jscontrol package after the cold-cache reinstall.After the second install (
--frozen-lockfile+ cache wipe), only type-shipping packages are checked. Re-checkingpure-jshere keeps the control and prevents a regression where that path makes all packages project-local.✅ Suggested assertion addition
await rm(join(String(packageDir), "node_modules"), { recursive: true, force: true }); await rm(cacheDir, { recursive: true, force: true }); await runBunInstall(bunEnv, String(packageDir), { savesLockfile: false, frozenLockfile: true }); + + expect(lstatSync(pureEntry).isSymbolicLink()).toBe(true); + expect(readlinkSync(pureEntry)).toMatch(/links[/\\]pure-js@1\.0\.0-[0-9a-f]{16}$/); + for (const name of typeShippingNames) { const entry = join(bunDir, `${name}@1.0.0`); expect(lstatSync(entry).isSymbolicLink()).toBe(false); expect(lstatSync(entry).isDirectory()).toBe(true); expect(existsSync(join(entry, "node_modules", name, "package.json"))).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli/install/isolated-install.test.ts` around lines 2095 - 2103, After the cold-cache reinstall (the runBunInstall call), re-check the control package "pure-js" to ensure it remained global; add assertions that the entry at join(bunDir, "pure-js@1.0.0") is a symbolic link (lstatSync(...).isSymbolicLink() === true) and not turned into a project-local directory, similar to how typeShippingNames are checked—place these checks after the existing loop so the control package is explicitly validated and prevents regression where all packages become project-local.
2084-2094:⚠️ Potential issue | 🟡 MinorCorrect the frozen-lockfile cold-cache rationale comment (execution order is reversed).
The comment currently says eligibility runs before extraction on cold cache, but this flow is inaccurate and can mislead future debugging.
✏️ Suggested comment rewrite
- // CI scenario: fresh runner with a committed lockfile but no - // pre-warmed package cache. `--frozen-lockfile` skips the diff-time - // resolution phase, so the eligibility DFS inside - // `installIsolatedPackages` runs before extraction has populated - // `<cache>/<pkg>@<ver>@@@1/package.json`. A naive scan of the - // extracted package.json sees ENOENT and would fall through to - // `eligible` — resurrecting `#29727` for every type-shipping package - // whose tarball wasn't already in this project's cache. The - // conservative fallback treats unreadable packages as ineligible, - // so types-shipping packages stay project-local even on a cold - // cache. + // CI scenario: fresh runner with a committed lockfile but no pre-warmed + // package cache. Re-running with `--frozen-lockfile` validates that the + // type-shipping carve-out still holds on a lockfile-driven cold-cache + // install path.Based on learnings:
installIsolatedPackagesruns only afterwaitForEverythingExceptPeers(manager)andwaitForPeers(manager), which drain download/extract tasks before eligibility scanning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli/install/isolated-install.test.ts` around lines 2084 - 2094, The explanatory comment about the frozen-lockfile cold-cache scenario is wrong about execution order; update the comment near installIsolatedPackages to state that extraction/download tasks are drained first (via waitForEverythingExceptPeers(manager) and waitForPeers(manager)), so installIsolatedPackages' eligibility scan runs after extraction has populated cache entries, and clarify that the conservative fallback still treats unreadable package.json as ineligible to avoid resurrecting the bug referenced by eligible; rephrase the rationale to reflect this corrected order and keep the note about why the conservative behavior is intentional for cold caches and --frozen-lockfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/cli/install/isolated-install.test.ts`:
- Around line 2095-2103: After the cold-cache reinstall (the runBunInstall
call), re-check the control package "pure-js" to ensure it remained global; add
assertions that the entry at join(bunDir, "pure-js@1.0.0") is a symbolic link
(lstatSync(...).isSymbolicLink() === true) and not turned into a project-local
directory, similar to how typeShippingNames are checked—place these checks after
the existing loop so the control package is explicitly validated and prevents
regression where all packages become project-local.
- Around line 2084-2094: The explanatory comment about the frozen-lockfile
cold-cache scenario is wrong about execution order; update the comment near
installIsolatedPackages to state that extraction/download tasks are drained
first (via waitForEverythingExceptPeers(manager) and waitForPeers(manager)), so
installIsolatedPackages' eligibility scan runs after extraction has populated
cache entries, and clarify that the conservative fallback still treats
unreadable package.json as ineligible to avoid resurrecting the bug referenced
by eligible; rephrase the rationale to reflect this corrected order and keep the
note about why the conservative behavior is intentional for cold caches and
--frozen-lockfile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dae7d9ee-e493-41c9-982f-1d7eafdefbdc
📒 Files selected for processing (1)
test/cli/install/isolated-install.test.ts
There was a problem hiding this comment.
No bugs found and all prior review feedback is addressed. Deferring to a human because the cold-cache conservative fallback (ENOENT → ineligible) means a frozen-lockfile install on a fresh CI runner materializes every package project-local — effectively disabling the global-store dedup for the exact scenario it was built for — and that perf/correctness trade-off (vs. e.g. deferring the check until after extraction or persisting the bit in the lockfile) is worth a maintainer sign-off.
Extended reasoning...
Overview
This PR adds a carve-out to the isolated linker's global-store eligibility DFS in src/install/isolated_install.zig (~200 new lines): packages whose extracted package.json declares "types", "typings", a non-empty "typesVersions", or a "types" key anywhere under "exports" are forced project-local under node_modules/.bun/<pkg>@<ver>/ instead of symlinked into <cache>/links/. It also updates docs/pm/global-store.mdx and adds a ~250-line three-pass regression test in test/cli/install/isolated-install.test.ts.
The PR went through several review rounds; my earlier inline comments (dual-ESM/CJS types: {import,require}, array-fallback recursion, bun.handleOom, typesVersions, and the frozen-lockfile cold-cache gap) and CodeRabbit's (OOM in the JSON parser, malformed-JSON fallback) were all addressed in commits 1df263b → 14bbf0a, and every thread is resolved.
Security risks
None identified. The new code reads package.json from Bun's own extraction cache (a path Bun already controls and writes), parses it with the existing shared JSON parser into an arena, and inspects a handful of keys. Recursion is depth-capped at 8. No untrusted path construction, no new network surface, no privilege changes.
Level of scrutiny
Medium-high. This is not a config tweak — it changes on-disk layout for every isolated-linker user and adds per-package file I/O + JSON parse to the install hot path. The logic itself is now well-tested (six fixture shapes plus a control, across three install passes), but the design decision embedded in the cold-cache fallback has a real cost: on a committed-lockfile + empty-cache install (canonical CI), readFrom ENOENTs for every package, the conservative fallback marks them all ineligible, and the global virtual store is effectively bypassed for that install. That's correct (it fixes #29727) but undoes the global-store disk/time win on first CI install. Whether that's the right trade-off vs. recomputing eligibility after the per-entry missing_from_cache extraction barrier, or persisting a "ships types" bit in bun.lock, is a call a maintainer should make.
Other factors
- The latest CI status comment in the timeline references commit 9a8c286 (build-zig failures across platforms); the head is now 14bbf0a (one commit later) and I don't see a green build confirmation yet.
- No CODEOWNERS entry covers
src/install/. - The heuristic is intentionally incomplete (implicit
index.d.tsis not detected — noted in review as an acceptable deferral), so this is a best-effort fix rather than a complete solution; that scoping is reasonable but worth a human ack.
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
TypeScript declaration resolution is extremely difficult to reason about. We should try to implement the behaviour of Andrew Branch's https://github.com/arethetypeswrong/arethetypeswrong.github.io
|
Makes sense that the field-scan heuristic is rough — it misses implicit A couple of options, roughly ordered by scope:
Happy to take any of these. Want me to hold this PR as-is for a follow-up, or switch direction here? |
|
Porting resolution is probably the right approach. It's unlikely it will change. |
|
Pushed 9379841 — replaced the field-scan heuristic with an ATW-style resolver. The new
New test fixtures exercise the ATW-specific paths:
Verified against the original PR body updated too. |
|
CI status on 9379841: Actual failures, both unrelated to this PR:
My own test ( Waiting for the remaining lanes (alpine, debian-13-aarch64, ubuntu-25-aarch64, darwin) to settle; they were still in the queue at last check. |
|
The two darwin lanes showed up as Expired (queue TTL exceeded before an agent picked them up) rather than actually running. Same pattern on #29919/#29920/#29921/#29922/#29923/#29924 in the same window — macOS runner pool capacity issue, not related to this PR. All 31 non-macOS lanes (linux x64/aarch64/musl asan+baseline, windows x64/aarch64) are green on 9379841. |
…nker TypeScript resolves peer type references (e.g. `React.FunctionComponent` inside a package's `.d.ts` that never declared `@types/react` as a peerDep) by walking `node_modules` ancestors from the declaration file's realpath. When the isolated linker's global virtual store holds the package, its realpath is `<cache>/links/<pkg>@<ver>-<hash>/` — a directory tree with no ancestor `node_modules` under which the project's `@types/*` devDependencies live, so the peer type silently resolves to `any` and downstream JSX usage fails with TS2604 / TS2786. Detect packages that ship types — top-level `"types"` or `"typings"`, or a `"types"` condition under `"exports"` — and mark them ineligible for the global store. They then materialize as real directories under `node_modules/.bun/<pkg>@<ver>/`, whose ancestor walk reaches the hidden hoisted layer at `node_modules/.bun/node_modules/` and resolves the project's types. Results are memoized per `pkg_id` and read from the extracted `<cache>/<folder>/package.json` so each package is scanned once per install. Packages whose package.json can't be read fall back to the pre-carve-out behavior. Fixes #29727.
`exports.types` has two shapes in the wild:
"exports": { ".": { "types": "./index.d.ts", "default": "./index.js" } }
"exports": { ".": { "types": { "import": "./a.d.mts", "require": "./a.d.cts" } } }
The second (dual-package types) is what TypeScript recommends for packages
shipping both .d.mts and .d.cts. The previous scanner required the
`"types"` value to be an `e_string`, so the dual-package shape slipped
through: recursion saw only the inner `import`/`require` keys and
returned false, leaving the package global-store-eligible and
reproducing the original #29727 TS2604/TS2786 failure for that subset
of packages.
Match any `"types"` key regardless of its value type (what the doc
comment already promised), and add a fourth test fixture
(`exports-types-dual`) to lock the case in.
…eOom
Two follow-ups on the exports-condition scanner:
- Node's conditional exports allow any target to be an array of fallbacks:
"exports": { ".": [{ "types": "./a.d.ts", "default": "./a.js" }, "./b.js" ] }
The previous scanner bailed on `expr.data != .e_object` and only recursed
into object values, so a `"types"` key nested inside an array element was
never visited. Add an `.e_array` arm that recurses into each item, and
always recurse on object-property values regardless of their type (the
recursion itself short-circuits on non-object/non-array).
- Use `bun.handleOom(cache.getOrPut(...))` instead of
`cache.getOrPut(...) catch bun.outOfMemory()` to match the convention
documented in `src/CLAUDE.md` and used by every other allocator-error
site in `src/install/`. Functionally identical — `getOrPut`'s error set
is `{OutOfMemory}` — but `bun.handleOom` comptime-asserts that, so a
future stdlib change that widens the error set would fail to compile
instead of silently swallowing the new error into an OOM panic.
Adds a fifth fixture (`exports-types-array`) covering the array-of-
fallbacks shape.
…obal store Canonical CI scenario (committed `bun.lock` + fresh runner or `bun pm cache rm` + `bun install --frozen-lockfile`) hit the original #29727 bug despite this PR's earlier commits. With a valid lockfile and matching `package.json`, resolution enqueues no download tasks, the pre-`installIsolatedPackages` wait block is skipped, and the eligibility DFS runs before the per-entry extraction loop has populated `<cache>/<pkg>@<ver>@@@1/package.json`. The previous `err => return false` fallback then left type-shipping packages global-store-eligible, silently symlinking them into `<cache>/links/` where TypeScript's ancestor walk breaks again. Consulting the npm manifest instead is not an option: Bun fetches the v1 "install" manifest (`application/vnd.npm.install-v1+json`), which strips `"types"`/`"typings"`/`"exports"` from each version. Conservatively mark packages we can't read as ineligible. First install after a cache wipe materializes everything project-locally (slower one- time write, no global-store benefit for that install); subsequent installs on the same machine hit a warm cache and route eligible packages back through the shared store via a fresh DFS. The memoization per `pkg_id` is stable within one install, and the decision is re-evaluated from scratch on the next one. Adds a second pass to the existing test that wipes `node_modules` and the private cache, then runs `--frozen-lockfile`, to lock in this behavior.
Two follow-ups on the exports-condition scanner: - `scanForTypeDeclarationSignals` missed `typesVersions`. Packages that ship version-gated declarations via that field without a top-level `"types"`/`"typings"` (TypeScript's original subpath-mapping mechanism, still in use by older packages) were leaving types-shipping packages global-store-eligible. Check for a non-empty `"typesVersions"` map alongside the existing signals, add a `types-versions` test fixture, and mention the field in both the function doc comment and `docs/pm/global-store.mdx`. - The JSON-parse `catch return false` swallowed `error.OutOfMemory` into "this package has no types" — a correctness bug: an OOM during the parse of a type-shipping `package.json` would silently leave it global-store-eligible. Route OOM through `bun.outOfMemory()` (abort), keep the `false` fallback only for malformed / corrupt JSON.
…est pass - Parse failures in the types scanner now fall through to the same conservative `ineligible` path as `readFrom` ENOENT. Returning `false` on a corrupted / truncated `package.json` would silently route a type-shipping package into the global store when its cache entry is damaged — same failure mode as the cold-cache case. OOM stays on `bun.outOfMemory()` (abort); every other parse error now returns `true` (ineligible). - Extend the test with a third pass (warm cache + frozen lockfile) that verifies `pure-js` flips back to a symlink into `<cache>/links/` once extraction populates its cache entry. This guards against the conservative fallback being sticky — if a future change left packages permanently ineligible once the ENOENT path fired, the assertion on `pure-js` would fail. - Tighten the second-pass comment on the frozen-lockfile rationale: spell out that `installWithManager` never enqueues any download / extract tasks on the no-diff path, so the eligibility DFS genuinely runs against an empty cache there.
Previously the global-store eligibility check scanned package.json for
any `"types"`/`"typings"`/`"typesVersions"` field or a `"types"`
key anywhere under `"exports"`. That was a conservative heuristic:
false-positive on any package that names a `"types"` field, false-
negative on implicit `index.d.ts`, and silent about whether the
declared target actually exists on disk.
Port the resolution algorithm `arethetypeswrong.github.io` uses so the
answer reflects what TypeScript itself would see:
1. If `"exports"` is set, walk the conditional tree for `.` with
condition priority `[types, import, require, default]` (the order
`tsc --moduleResolution node16/bundler` uses when `types` is in
its condition set). The resolved string target either IS a
declaration file (`.d.ts` / `.d.mts` / `.d.cts` / `.ts`-family
source) or has a sibling declaration on disk.
2. Otherwise check top-level `"types"` / `"typings"` and apply the
same is-or-has-sibling rule to the declared path.
3. Otherwise, if `"typesVersions"` is non-empty, TS resolves
through its version-range mappings — treat that as type-shipping.
4. Otherwise, fall back to an implicit `index.d.ts` at the package
root.
Any positive hit forces the package project-local; per the Node
`"exports"` spec, once `"exports"` is present the top-level
`"types"` / `"typings"` / `"typesVersions"` / implicit `index.d.ts`
fallbacks are NOT consulted, so the resolver returns early in that
branch.
Test extended with three new fixtures that exercise resolver behaviour
the heuristic couldn't model:
* `implicit-index` — no `"exports"`/`"types"`, only a root
`index.d.ts`. ATW fallback detects it; the heuristic missed it.
* `sibling-dts` — `"exports"` with no `"types"` condition but the
JS target has a sibling `.d.mts`/`.d.cts`. ATW pairs JS + dts
extensions and finds the sibling; the heuristic missed it.
* `exports-no-types` — `"exports"` map whose resolved JS files
have no sibling declarations. ATW correctly reports no types
exposed; the earlier check would have false-positive-d on a
future `"exports"`-presence heuristic. Doubles as the
non-type-shipping control alongside `pure-js`.
Pack fixtures in parallel to keep the test under the default timeout
(`bun pm pack` against a debug build is ~2-3s cold; serialising ten
was pushing 30s just on packing).
Jarred-Sumner
force-pushed
the
farm/a45f7049/global-store-types-carveout
branch
from
9379841 to
b95ff7e
Compare
…bounded path buf
Three review-driven tightenings to the type-declaration resolver:
1. pathExposesTypes: bound the sibling-declaration concatenation with
std.fmt.bufPrint(&buf, "{s}{s}", ...) catch return false — matches
the existing cacheFileExists/cacheFileExistsJoin pattern a few lines
below. Previously two @memcpy blocks copied package.json strings
straight into bun.PathBuffer with no length check, which panics
under ReleaseSafe and writes past the stack array under ReleaseFast
for any oversized "types"/"exports" path.
2. exportsExposesTypes: when the map has subpath keys but no "." entry
(e.g. { "./Button": { types: ..., default: ... }, "./Card": ... }),
walk every subpath's target instead of returning false. ATW inspects
every entry point; any subpath exposing declarations means the
package ships types to that consumer.
3. resolvePackageExposesTypes: drop the early return when the exports
walk says no. moduleResolution: node10 (still the commonjs default,
and ATW's "node10" column) ignores "exports" and resolves via
top-level "types"/"typings"/implicit index.d.ts. A package shaped
{ exports: { ".": "./dist/index.js" }, types: "./types/index.d.ts" }
where dist/index.d.ts doesn't exist now correctly stays project-
local for the node10 audience.
Three new fixtures: subpath-only (map without "."), exports-no-types-
top-types (node10 fall-through), long-types (2048-char path, stack
overflow guard).
|
Build 51175: the only failing lane is |
|
Final state on build 51175: 2 infra failures (darwin aarch64 lanes "Expired" = queue TTL before any agent picked them up, same thing that hit build 48908) + 1 pre-existing flake (ASAN shard 15/20 on |
Two review nits from the previous round:
1. Wildcard subpath targets (`"./icons/*.d.ts"`) can't be fstat'd —
cacheFileExists was calling existsAt on the literal '*' path and
returning false, so packages with only wildcard subpaths were
treated as not shipping types. pathExposesTypes now short-circuits
when the path contains '*': trust the declared extension. Any
pattern ending in a declaration (`.d.ts`/`.d.mts`/`.d.cts`) or
TypeScript source extension (`.ts`/`.tsx`/`.mts`/`.cts`) counts
as type-shipping. JS patterns without a sibling declaration still
return false — if the author wanted types treated they'd have
added a `types` condition.
2. The long-types fixture used a `.d.ts` suffix, which takes the
early-return branch in pathExposesTypes and never reaches either
of the two `bufPrint catch return false` guards the prior commit
added. And 2048 chars doesn't overflow Linux's 4096-byte
PathBuffer. Replaced with two fixtures:
* long-types-js: `types: "./<5000 'a's>.js"` routes through
the js_to_dts_pairs sibling bufPrint site.
* long-types-bare: `types: "./<5000 'a's>"` (no extension)
routes through the extensionless append-`.d.*` site.
Both exceed macOS (1024) and Linux (4096) PathBuffer ceilings, so
the guards actually fire. Verified by stashing src/: unfixed code
panics on the debug build (test times out); fixed code runs clean.
Added wildcard-types and wildcard-no-types fixtures to lock in the
wildcard extension rule and its negative.
Two review-driven tightenings:
1. Add node10 `main`-sibling resolution to the fall-through chain.
When no `types`/`typings` field is set, TypeScript's legacy
`node` module resolution (still the default under
`module: "commonjs"`) strips the extension from `main` and
probes `<stem>.d.ts` before falling back to `./index.d.ts`.
Catches the classic `tsc --declaration --outDir lib` shape:
`{ "main": "./lib/index.js" }` with `lib/index.d.ts` next to
it and no explicit `types` field.
2. Drop the unused `allocator` parameter from `exportsExposesTypes`
and `resolveTargetExposesTypes`. Residue from the b95ff7e ATW
rewrite: threaded through six call sites but never dereferenced
(the .e_string arm reads `str.data` directly, and the object-key
classification reads `key.data.e_string.data` directly).
Added `main-sibling` fixture to lock in the new fallback.
… comment The `'"'"'` sequence on the TypeScript'"'"'s comment line was a bash single-quote-escape idiom that leaked into the source when the comment was added in 41e880b. Cosmetic only — lives in a // comment, no test or runtime impact — but still unintended garbage in committed source.
There was a problem hiding this comment.
All prior review feedback has been addressed and the bug hunter found nothing new, but this adds ~400 lines of new ATW-style type-resolution logic to the isolated-install hot path (including a design tradeoff where frozen-lockfile + cold-cache installs conservatively force every package project-local) — worth a human pass on the final shape, especially since alii requested the resolver approach but hasn't seen the post-rewrite commits.
Extended reasoning...
Overview
This PR adds a carve-out to the isolated linker's global-store eligibility DFS: packages that expose TypeScript declarations are forced to materialize as real directories under node_modules/.bun/<pkg>@<ver>/ instead of symlinking into the shared <cache>/links/ store, so that TypeScript's realpath-based ancestor walk can reach the project's hoisted @types/*. The detection is implemented as an ATW-style resolver (packageShipsTypeDeclarations → resolvePackageExposesTypes → exportsExposesTypes / resolveTargetExposesTypes / pathExposesTypes, ~400 new lines in src/install/isolated_install.zig) that walks "exports" with types-priority conditions, checks top-level types/typings/main-sibling/typesVersions, and falls back to implicit index.d.ts. A ~490-line regression test with 17 fixture packages and three install passes (warm / cold-cache frozen-lockfile / warm-cache frozen-lockfile) covers each detection path plus two negative controls and two stack-overflow guards. Docs in global-store.mdx are updated to describe the new ineligibility rule.
Security risks
The resolver parses untrusted package.json content from arbitrary registry packages and feeds path strings into fixed-size stack bun.PathBuffers. An earlier revision had an unbounded @memcpy that could overflow the stack with attacker-controlled bytes in ReleaseFast builds; this was caught in review and fixed in a8f7d2a (both sites now use std.fmt.bufPrint(...) catch return false), with dedicated long-types-js / long-types-bare fixtures (5000-char paths) locking the guards in. Recursion into the exports tree is depth-bounded at 16. I don't see remaining injection or traversal exposure — cacheFileExists only existsAt's under the already-extracted cache folder and never writes — but the fact that a memory-safety issue surfaced mid-review is itself a reason for a human to look at the final state.
Level of scrutiny
High. This runs for every package in the install graph on every bun install --linker isolated with the global store enabled, against package.json data fetched from the network. Beyond the per-package cost, the cold-cache fallback is a real behavioral tradeoff: on a frozen-lockfile install with an empty cache (the canonical CI case), readFrom(<cache>/<folder>/package.json) ENOENTs for every package, the conservative fallback returns "ships types" for all of them, and the global-store fast path is effectively disabled for that install. That's the correct conservative answer for #29727, but it changes the disk/perf characteristics of cold CI installs and is the kind of tradeoff a maintainer should ratify.
Other factors
- The PR went through ~13 rounds of bot review; every inline comment (including mine) is resolved, and the current bug-hunt pass found nothing.
- A human maintainer (alii) weighed in on 2026-04-28 to request the ATW-style resolver over the original field-scan heuristic, but the six subsequent commits (b95ff7e → d832cb7) that actually implement and harden it haven't had human eyes.
- Test coverage is thorough (17 fixtures, 3 install passes, positive + negative controls, overflow guards) and CI is green modulo known infra flakes the author triaged in-thread.
- No CODEOWNERS entry covers
src/install/.
Given the scope, the security-adjacent history, and the unreviewed design pivot, I'm deferring rather than approving.
2 participants
Problem
TypeScript resolves peer type references (e.g.
React.FunctionComponentinside a package's.d.tsthat never declares@types/reactas a peerDep) by walkingnode_modulesancestors from the declaration file's realpath. With the isolated linker's global virtual store, the package's realpath is~/.bun/install/cache/links/<pkg>@<ver>-<hash>/— a directory tree with no ancestornode_modulesunder which the project's@types/*devDependencies live. The peer type silently resolves toanyand downstream JSX usage fails with TS2604 / TS2786.Reproduction (https://github.com/THernandez03/next-yak-bun-typescript-repro):
With
BUN_INSTALL_GLOBAL_STORE=0or--linker hoisted, the same project typechecks cleanly.Root cause
src/install/isolated_install.zig— the eligibility DFS that decides which entries live in the global store and which fall back tonode_modules/.bun/<storepath>/. It checked for patches, trusted/scripted deps, and workspace/folder/link deps, but had no carve-out for packages that ship type declarations. A symlink to<cache>/links/places the package's realpath outside the project, where TypeScript's ancestor walk can't reach the project-level@types/*.Fix
Detect packages that ship types — top-level
"types"or"typings", or a"types"condition under"exports"— and mark them ineligible for the global store. They materialize as real directories undernode_modules/.bun/<pkg>@<ver>/, whose ancestor walk reaches the hidden hoisted layer atnode_modules/.bun/node_modules/and resolves the project's hoisted@types/*.Results are memoized per
pkg_idand read from the extracted<cache>/<folder>/package.json(which is always populated before the DFS runs —waitForEverythingExceptPeersdrains the download/extract queue first). Packages whosepackage.jsoncan't be read fall back to the pre-carve-out behavior; this only matters if the user manually evicted the per-package cache between installs.Verification
Before:
After:
Pure-JS packages (no
.d.ts/ notypesfield) still take the shared-store fast path — e.g.react,scheduler,@swc/counterremain symlinks into<cache>/links/. The carve-out only triggers for packages that intentionally ship TypeScript declarations.Test
test/cli/install/isolated-install.test.ts— 'packages that ship TypeScript declarations stay project-local (#29727)'. Packs five fixtures (top-types/top-typings/exports-types/exports-types-dual/pure-js) withbun pm pack, serves them from an in-process HTTP registry, and asserts each type-shipping variant becomes a real directory while the pure-JS one stays a symlink to the global store.Fixes #29727.
Fixes #23895.