Repository navigation
Conversation
|
Updated 5:52 PM PT - Oct 8th, 2026
❌ @autofix-ci[bot], your commit 376d6f6 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 44836That installs a local version of the PR into your bun-44836 --bun |
|
Status: reproduced with the script of #44162 and a real |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// `bun test --changed`: a bare specifier that names a package installed | ||
| /// under `node_modules` is external, and the package is not read. A link | ||
| /// whose real path is outside `node_modules` (a workspace package) is not | ||
| /// an installed package. |
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
| /// `out` for a package path that stays as it is written. The path is | ||
| /// empty: the caller of `load_node_modules` names it (`name_external`). |
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
| /// An external match that `load_node_modules` left without a path is the | ||
| /// specifier as the caller holds 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
| /// `installed_packages_are_external`: the `<dir>/node_modules/<package_name>` | ||
| /// entry, read from the listing of the directory that holds it, so the | ||
| /// package is not opened. `None`: there is no such entry. `Some(false)`: a | ||
| /// link whose real path is outside `node_modules`. |
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
| // No `node_modules/<name>` entry: the probe found a file that | ||
| // lives in `node_modules` itself. |
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
| //! 2. Run the bundler over every discovered test file. The scan resolves an | ||
| //! import as the test run does, and does not enter a package that is | ||
| //! installed under node_modules. This produces the full parse graph | ||
| //! (transitive imports) without linking or emitting code. |
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 scan must resolve an import to the file that the run loads, so it | ||
| // takes the run's target and the run's resolver settings. It does not take | ||
| // `preserve_symlinks`: git names a file by its real path. |
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 run reads `process.env` when the code runs. A scan that inlines it | ||
| // drops a `require()` in a branch that is dead now and live in a test. |
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 change to an installed package is not a local edit, so the scan does | ||
| // not enter one. Every other bare specifier names a local 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
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/resolver/resolver.rs— Users who set NODE_PATH (common for globally installed CLIs such as typescript or eslint) will seebun test --changedparse the global package tree on every scan when a test imports a package not installed locally. Withpackages=Externalgone, the walk reaches the NODE_PATH loop at src/resolver/resolver.rs:2919-2941 and returns a real file; the installed-package check only covers thenode_moduleswalk above it. Fix: treat a NODE_PATH hit as external wheninstalled_packages_are_externalis set (or when its resolved path contains thenode_modulesneedle), matching the rule applied at line 2691. [also at: src/resolver/resolver.rs:2594 - Projects whose tsconfigpathsorbaseUrlmap a bare specifier intonode_moduleswill havebun test --changedparse that installed package and everything it imports on every scan.]Why this was flagged
NODE_PATH=/usr/lib/node_modules is set in the environment (nvm, Homebrew and many CI images do this). A test file imports
typescriptwhich is not in the project'snode_modules. The walk at src/resolver/resolver.rs:2675-2916 finds nothing, then the NODE_PATH loop at lines 2925-2941 joins/usr/lib/node_modules/typescriptandload_as_file_or_directorysucceeds with a file path andis_externalfalse. BundleV2 enqueues and parseslib/typescript.js(about 9 MB) plus whatever it requires, on everybun test --changedrun and on each--watchrerun; the base branch never got here becauseis_external_pattern(resolver.rs:957, 1261) returned external first. The scan also adds those files tomodule_graph_files(ChangedFilesFilter.rs:57), so--changed --watchregisters watchers on the global tree. The author states NODE_PATH packages are scanned like local files; nothing in a global prefix is a local edit git can report, so the cost buys no selection. Remedy: returnexternal_matchfor NODE_PATH hits when the flag is set.Verification: NODE_PATH is set and a test imports a bare specifier absent from enclosing
node_modulesbut under a NODE_PATH directory. Theinstalled_packages_are_externalgate covers only thenode_moduleswalk; the NODE_PATH loop at src/resolver/resolver.rs:2919-2950 returnsMatchStatus::Success. On the basepackages = Externalmade them external, so NODE_PATH packages were never parsed.
|
|
||
| git(String(dir), "checkout", "-q", "--", "."); | ||
| appendFileSync(join(String(dir), "node_modules", "dep", "index.js"), "// touched\n"); | ||
| appendFileSync(join(String(dir), store, "index.js"), "// touched\n"); | ||
| appendFileSync(join(String(dir), "node_modules", "single.js"), "// touched\n"); | ||
| { | ||
| const { stderr, exitCode } = await runTestChanged(String(dir)); |
There was a problem hiding this comment.
🔴 On Windows CI the new "an installed package is not followed" case fails: git reports 4 changed files, not the asserted "3 changed files". The fixture commits node_modules, and node_modules/linked is a junction created by linkDirectory (test/cli/test/test-changed.test.ts:69-72) before initRepo. Git for Windows treats a junction as a plain directory, so git add -A also tracks node_modules/linked/index.js, and the append to the store file shows up under both paths. Fix: keep the junction out of the index on Windows (add node_modules/linked to .gitignore, or create the link after the initial commit) while still committing node_modules/dep and node_modules/single.js so the "changed file inside node_modules" assertion stays meaningful.
Why this was flagged
The test at test/cli/test/test-changed.test.ts:627-685 writes node_modules/.bun/linked@ 1.0.0/node_modules/linked/{package.json,index.js}, calls linkDirectory(store, node_modules/linked) at line 662, then initRepo at line 663 which runs git add -A and commits. There is no .gitignore in this fixture, so node_modules is tracked on purpose. On POSIX git stores node_modules/linked as a symlink blob; on Windows symlinkSync(..., "junction") creates a junction, which Git for Windows lstat()s as a directory, so git recurses into it and tracks node_modules/linked/index.js and node_modules/linked/package.json as regular files. Line 674 appends to the store's index.js; git then reports it as modified under both node_modules/.bun/linked@ 1.0.0/node_modules/linked/index.js and node_modules/linked/index.js, plus node_modules/dep/index.js and node_modules/single.js, so bun prints "4 changed files, but no test files are affected" and expect(stderr).toContain("3 changed files, but no test files are affected") at line 681 fails. The describe block has no skipIf(isWindows). The base branch has no such test, so this is a new red Windows CI job, not a runtime defect.
Verification: linkDirectory (test/cli/test/test-changed.test.ts:69-72) does symlinkSync(target, link, "junction"). The case creates the link at line 659 and only afterwards calls initRepo at line 660. The output is "4 changed files, but no test files are affected", so expect(stderr).toContain("3 changed files, but no test files are affected") at line 684 fails on Windows.
| if status.is_success() && out.is_external && out.path_pair.primary.text.is_empty() { | ||
| let Ok(package_path) = self | ||
| .fs_ref() | ||
| .dirname_store | ||
| .append_slice(&esm_resolution.path) | ||
| else { | ||
| return MatchStatus::NotFound; | ||
| }; |
There was a problem hiding this comment.
🟡 nit (optional): an allocation failure here is reported as a module-not-found instead of an OOM, hiding the real cause. When load_node_modules returns an external match for a "#alias" that maps to an installed package, the copy of esm_resolution.path into dirname_store at resolver.rs:5170 maps Err to MatchStatus::NotFound. Fix: route the AllocError through bun_core::handle_oom (or unwrap_or_oom) like the sibling copy at resolver.rs:2157, so a failed allocation is a controlled crash rather than a silent NotFound.
Why this was flagged
Trigger: bun test --changed scans a test whose package.json imports map "#dep" to an installed package name, so load_package_imports at src/resolver/resolver.rs:5161 calls load_node_modules with installed_packages_are_external set and gets an external match with an empty path. At resolver.rs:5170 the code appends esm_resolution.path to dirname_store; the let-else at 5174-5176 turns an allocation error into MatchStatus::NotFound. The caller then treats the alias as unresolvable (the scan has ignore_module_resolution_errors, so the edge is silently dropped) instead of surfacing the OOM. The repository rule is to route allocation failure through bun_core::handle_oom; the neighbouring copy at resolver.rs:2157 uses .expect("unreachable") and the base branch has no such NotFound-on-OOM path. Only reachable under memory exhaustion, hence nit.
Verification: nit. In a bun test --changed scan, an imports alias maps to an installed package and malloc returns null. src/resolver/resolver.rs:5169-5176 maps the AllocError from dirname_store.append_slice to MatchStatus::NotFound; the scan sets ignore_module_resolution_errors = true (ChangedFilesFilter.rs:164), so a genuine OOM is masked as an unresolved import rather than a controlled crash.
| fn is_node_modules_entry_installed(&mut self, dir: &[u8], package_name: &[u8]) -> Option<bool> { | ||
| // `@scope/name` is the entry `name` of `node_modules/@scope`. | ||
| let (scope, base) = match strings::last_index_of_char(package_name, b'/') { | ||
| Some(slash) => (&package_name[..slash], &package_name[slash + 1..]), | ||
| None => (&b""[..], package_name), | ||
| }; | ||
| let parts: [&[u8]; 3] = [dir, b"node_modules", scope]; | ||
| let listing_path = self.fs_ref().abs_buf_checked( | ||
| &parts[..if scope.is_empty() { 2 } else { 3 }], | ||
| bufs!(node_modules_check), | ||
| )?; | ||
| let listing = self.dir_info_cached(listing_path).ok().flatten()?; | ||
| let lookup = listing.get_entry(self.generation, base)?; | ||
| // SAFETY: `rfs_ptr` points at the process-global RealFS; the lazy-stat | ||
| // rewrite inside `symlink()` is serialized on `Entry.mutex`. | ||
| let real_path = unsafe { lookup.entry().symlink(self.rfs_ptr(), self.store_fd) }; | ||
| Some(real_path.is_empty() || strings::contains(real_path, bun_paths::NODE_MODULES_NEEDLE)) |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing gap the fix leaves open: a monorepo that links the whole scope directory (node_modules/@ org -> ../packages) still gets no test selected when a package behind it changes. is_node_modules_entry_installed applies the "link whose real path is outside node_modules" rule only to the final <name> entry (resolver.rs:1044-1048); the listing of node_modules/@ org is read through the link, so lib there is a plain directory, symlink() is empty and it is reported installed. Fix: when scope is non-empty, apply the same real-path test to the @ scope entry of the <dir>/node_modules listing (or to the listing directory itself) before deciding, so a scope link that leaves node_modules is followed like a package link.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Setup: a repo with packages/lib/index.ts and a link node_modules/@ org -> ../packages (no per-package links). a.test.ts imports @ org/lib; the user edits packages/lib/index.ts and runs bun test --changed. The scan resolver has installed_packages_are_external = true (ChangedFilesFilter.rs:175). In load_node_modules the first check (resolver.rs:2691-2698) calls is_node_modules_entry_installed(dir, "@ org/lib"). At resolver.rs:1034-1043 it splits into scope @ org / base lib, builds <dir>/node_modules/@ org and reads that directory's listing with dir_info_cached, which follows the link and lists packages. The lib entry there is a real directory, so symlink() at resolver.rs:1047 returns an empty slice and line 1048 yields Some(true); external_match is returned and the package is never entered. The test is not selected and --changed prints that no test files are affected. The base branch behaves the same (everything bare was external), so this is a leftover of the partial fix rather than a regression; the PR's stated rule is only evaluated on the leaf entry, never on the scope directory the leaf was found in.
Verification: pre-existing (base marks every bare specifier external). Trigger: a scope directory linked as a whole (node_modules/@ org -> ../packages) with a test importing @ org/lib and an edit to packages/lib/index.ts, run under bun test --changed. entry().symlink() (src/resolver/resolver.rs:1047) stays empty and line 1048 returns Some(true), so the scan never follows @ org/lib.
| if skips_installed_packages | ||
| && self.is_node_modules_entry_installed(dir_info.abs_path, package_name) | ||
| == Some(true) | ||
| { | ||
| if let Some(d) = self.debug_logs.as_mut() { | ||
| d.decrease_indent(); | ||
| } | ||
| return Self::external_match(out); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Monorepo users of bun test --changed get a test silently skipped when a nearer node_modules/<name> entry exists that the run itself cannot load from. The first check at src/resolver/resolver.rs:2691-2698 returns external_match as soon as is_node_modules_entry_installed finds any real directory named <name>, before load_as_file_or_directory has shown it resolves. The run (and the base walk) fails there and continues to the parent node_modules, where a workspace link is followed. Fix: only short-circuit when the entry resolves (run the probe, then classify), or treat an entry that fails to load as absent and keep walking; the same rule applies to the .is_none() check at line 2900. A stale or empty nested directory left by a previous install is enough to trigger it.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A workspace package packages/app has a leftover packages/app/node_modules/lib directory (no package.json, no index) from a previous registry install, while the current install links node_modules/lib -> ../packages/lib. A test in packages/app imports lib. At runtime the walk tries packages/app/node_modules/lib, cannot load it, moves to the parent and loads packages/lib through the link. In the scan, src/resolver/resolver.rs:2691-2698 calls is_node_modules_entry_installed first; the entry is a real directory so symlink() (src/resolver/fs.rs:269) returns empty, line 1048 returns Some(true), and external_match is returned without ever probing the directory. The base branch never had this check and load_as_file_or_directory at line 2891 failed and continued to the parent. Result: --changed: N changed files, but no test files are affected, exit 0, for an edit to packages/lib.
Verification: On the base the test was already skipped. Triggered when a nearer node_modules/<name> is a real directory the run cannot load from while the parent is a workspace link. src/resolver/resolver.rs:2691-2698 returns Self::external_match(out) on Some(true) before any load attempt. The run fails load_as_file_or_directory (2890) and continues to the parent, so the test in packages/app is silently skipped.
|
Note for the next PR of this series: #46608 changes how
Deleted files, preload files and snapshot files are not in #46608. |
Fixes #44162
Problem
bun test --changedskips a test that imports a changed file through a bare specifier (#imports, workspace package, own name, tsconfigbaseUrl). It prints--changed: 1 changed file, but no test files are affectedand exits 0.packages = External(ChangedFilesFilter.rs:149), so the resolver (resolver.rs:942) makes each bare specifier external before it resolves it.Fix
installed_packages_are_external, stops thenode_moduleswalk inload_node_modulesat an entry that is a directory, or a link that stays undernode_modules. The package is not opened.bun:testaliases and env behaviour, so it follows the file that the run loads.test/cli/test/test-changed.test.ts(10 new cases fail on the base),bundler_edgecase.test.ts -t External,resolve.test.ts,packagejson.test.ts.Background
bun test --changedscans the import graph of each test file and runs the tests that reach a file git reports as changed.node_modulespaths after it resolves (it reads every installed package), and a check before the lookup (it copied the lookup order).bun build --packages=externalis unchanged.Downsides
Notes
Measurements (Linux x64, release builds of #44835 at e7c347c and of this PR unless stated)
node_modulesdirectory (openat, 2 getdents64, close). Isolated pays newfstatat, openat, fstat, readlink and close per link.bun install --backend=symlink, a#alias to an installed package, abaseUrlproject). Local files newly parsed on the bun test --changed skips tests that import through package.json "imports" (#subpath) or a workspace package #44162 reproduction: +1 (packages/lib/src/lib.ts).load_node_modulesruns 304 of its own instructions against 287,resolve_without_remapping58 against 47.resolve_and_auto_installruns 226 in both builds, and a relative import does not reach the changed functions (gdbnexticount from entry to return, warm third call, 3 runs each). valgrind is not installed here.bun build --packages=external: 0 bytes differ across 1 output file, for a fixture with#internal, abaseUrlpath, the package's own name, a linked workspace package and an npm package. The default bundle of the same fixture is identical too.size, 88,978,875 to 88,980,411).file:row.How the walk decides
load_node_moduleslooks in its usual order: tsconfigpaths,baseUrl, package.jsonimports, the package's own name, then eachnode_modulesdirectory upward. Only the last step has the new check.node_modulesdirectory. A workspace link and abun linklink point outside, so the lookup continues and the scan enters the package.node_modulesis a file ofnode_modulesitself (node_modules/single.js). It is external too.#alias to an installed package is the one case that copies the target.Resolver::for_workercopies the switch, so the browser transpiler that the scan creates for an.htmlimport has it.Two fixes that come with the scan settings
browserin package.json and thebrowserexport condition. It now takes the run's target beforeTranspiler::init.process.env.NODE_ENV, so arequire()in a branch that is dead at scan time had no edge. It now takes the run's env behaviour, which does not inline.The self-review
load_node_modules. The review reproduced two wrong answers of that copy: a change tonode_modules/single.jsselected a test, and a tsconfigpathsalias through a linked directory was not selected. The check now sits inside the walk. Both cases are test rows.opts.packages, which the bundler projects again for each derived transpiler. The browser transpiler of an.htmlimport lost it and the scan parsed an installed package. The switch is now a resolver field. This case is a test row.--preserve-symlinksfrom the run. With it, an import through a linked source directory stopped selecting under that flag, and it selects on main. git names a file by its real path. A test pins it.Not covered
file:folder dependencies.bun installhardlinks them intonode_modules, so the file that the run loads is undernode_modules.NODE_PATHis resolved and scanned like a local file.Other open PRs on these lines
packages=externalgate inresolve_and_auto_install. This PR no longer touches that gate.--preserve-symlinksin the resolver. test(test-changed): copy one committed repo per case and pin the selected files #41030 and test: add --changed-first to run affected test files first #36865 change the same test file and the same filter function.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/test/test-changed.test.ts