install: ignore patchedDependencies from folder dependency's package.json#29594
install: ignore patchedDependencies from folder dependency's package.json#29594robobun wants to merge 4 commits into
patchedDependencies from folder dependency's package.json#29594Conversation
|
Updated 11:38 AM PT - May 4th, 2026
❌ @robobun, your commit f5dc6ea has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29594That installs a local version of the PR into your bun-29594 --bun |
WalkthroughGuarded processing of the JSON Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal fix that gates patchedDependencies parsing on the existing features.patched_dependencies flag, matching the established trusted_dependencies pattern.
Extended reasoning...
Overview
This PR wraps two existing patchedDependencies parse blocks in src/install/lockfile/Package.zig with if (comptime features.patched_dependencies). The Features.patched_dependencies flag was already defined in src/install/install.zig (set to true only for Features.main) but was never actually checked, so folder/link/workspace dependencies' patchedDependencies were incorrectly merged into the root lockfile. A regression test is added to bun-install-patch.test.ts.
Security risks
None. This change narrows what is read from non-root package.json files — it stops processing patchedDependencies from folder dependencies rather than adding new parsing surface. No auth, crypto, network, or filesystem-write logic is touched.
Level of scrutiny
Low. The diff is purely additive guarding (indentation + a comptime conditional) with no logic changes inside the blocks. It exactly mirrors the existing if (comptime features.trusted_dependencies) gate at line 1558 of the same file, and I verified that Features.main is the only preset with patched_dependencies = true (install.zig:156-190), so root-package behavior is unchanged.
Other factors
The PR includes a focused regression test that reproduces issue #13531 and asserts both that install succeeds and that the lockfile doesn't leak the folder dependency's patchedDependencies. No CODEOWNERS apply to these paths, no outstanding reviewer comments, and the bug-hunting system found no issues.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
LGTM — straightforward fix gating patchedDependencies parsing on features.patched_dependencies, matching the existing trustedDependencies pattern.
Extended reasoning...
Overview
This PR wraps the two patchedDependencies parse blocks in Package.parseWithJSON (src/install/lockfile/Package.zig) with if (comptime features.patched_dependencies), so that only the root package.json contributes patch entries to the lockfile. Two regression tests are added to test/cli/install/bun-install-patch.test.ts covering both the folder-dependency case (#13531) and the workspace-member case (#27894).
Security risks
None. This is a comptime feature gate on package.json parsing; it strictly reduces the set of inputs that affect the lockfile (non-root patchedDependencies are now ignored). No auth, crypto, filesystem-write, or network changes.
Level of scrutiny
Low. The Features.patched_dependencies flag already existed in src/install/install.zig (default false, only true for Features.main) but was never checked — this PR simply wires it up. The change is mechanically identical to the adjacent features.trusted_dependencies gate at Package.zig:1558, so it follows an established pattern with no ambiguity.
Other factors
- The two CI failures (
require-cache.test.tson Windows,load-file-loader-a-lot.test.tson Linux ASAN) are in unrelated subsystems (cli/runandjs/bun/resolve) and are not caused by this change. - The duplicate-PR bot flagged #27895 as the same fix; this PR's test coverage is a superset (covers both folder and workspace variants).
- No CODEOWNERS entries match the modified paths.
- Bug-hunting system found no issues.
![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 `@test/cli/install/bun-install-patch.test.ts`:
- Around line 613-623: Remove the flaky stdout assertion by deleting or
commenting out the line that checks stdout for "pkg-a@" (the
expect(stdout).toContain("pkg-a@") in the test where stdout, stderr, and
exitCode are collected from proc); keep the existing assertions on stderr,
exitCode, and the lockfile read via Bun.file(join(filedir, "pkgB",
"bun.lock")).text() (i.e., the expect(stderr).not.toContain(...),
expect(exitCode).toBe(0), and
expect(lockfile).not.toContain("patchedDependencies")) so the regression is
still validated without asserting variable bun install output.
🪄 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: b28d06ce-2c18-4887-8275-b5288dfd3378
📒 Files selected for processing (1)
test/cli/install/bun-install-patch.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 `@test/cli/install/bun-install-patch.test.ts`:
- Around line 618-623: The exitCode assertion is performed before
filesystem/behavioral checks; move the exit-code assertion to the end of the
test blocks so behavioral checks run first: keep the lockfile/read and
expect(lockfile).toContain/expect(lockfile).not.toContain and any stdout/stderr
expectations before asserting exitCode (the expect({ stdout, exitCode
}).toMatchObject or expect(exitCode).toBe(0)), and update both occurrences where
exitCode is asserted early (the block checking pkgB's bun.lock and the later
block around the other lockfile checks) so exitCode is asserted last.
🪄 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: bad24571-5433-4a80-8263-3507cc177bbf
📒 Files selected for processing (1)
test/cli/install/bun-install-patch.test.ts
There was a problem hiding this comment.
LGTM — minimal comptime gate matching the existing trusted_dependencies pattern, with regression tests for both folder and workspace cases.
Extended reasoning...
Overview
This PR wraps the two patchedDependencies parse blocks in Package.parseWithJSON (src/install/lockfile/Package.zig:1321 and :1653) with if (comptime features.patched_dependencies). The inner logic is unchanged — only an outer comptime guard was added. Two regression tests were added to test/cli/install/bun-install-patch.test.ts covering the folder-dependency and workspace-member scenarios from #13531.
Security risks
None. This is a comptime feature gate that narrows when a JSON property is read; it does not introduce new parsing, file access, or external input handling.
Level of scrutiny
Low. The change is mechanical and follows the exact pattern already used one block away for trustedDependencies (if (comptime features.trusted_dependencies) at Package.zig:1558). I verified in src/install/install.zig that Features.patched_dependencies defaults to false and is set to true only in Features.main — folder, workspace, link, npm, tarball, and npm_manifest all leave it false, which is the intended semantics (only the install root's patchedDependencies is honored, consistent with bun patch --commit and the lockfile's single top-level patchedDependencies key).
Other factors
- No bugs found by the bug-hunting system.
- CodeRabbit's minor flakiness note about asserting on
bun installstdout was addressed in 4c9f2d6 (positive assertion moved to the lockfile), and the thread is resolved. - No CODEOWNERS cover
src/install/or this test file. - The PR supersedes #27895 with the same one-line conceptual fix plus broader test coverage.
Features.patched_dependencies was defined but never checked in parseWithJSON, so patchedDependencies entries from folder dependencies (and any other non-root package.json) were merged into the root lockfile. Because patch paths are resolved relative to the project root, install would then fail with "Couldn't find patch file" when the patch lived in the folder dependency's directory. Gate both patchedDependencies parse blocks on features.patched_dependencies, which is only true for Features.main (the root package.json). Fixes #13531
bun install stdout can vary between runs; use the lockfile for the positive assertion instead.
Jarred-Sumner
force-pushed
the
farm/0ab403ae/ignore-patched-deps-in-folder-deps
branch
from
4c9f2d6 to
795f5c7
Compare
There was a problem hiding this comment.
LGTM — minimal comptime guard matching the existing trustedDependencies pattern, with regression tests for both folder and workspace cases.
Extended reasoning...
Overview
This PR touches two files: src/install/lockfile/Package.zig wraps the two existing patchedDependencies parse blocks in if (comptime features.patched_dependencies) { ... }, and test/cli/install/bun-install-patch.test.ts adds two regression tests (folder dependency and workspace member). The Zig change is purely a comptime guard around unchanged inner logic — the diff is indentation plus one wrapping conditional per block.
Correctness
Features.patched_dependencies already existed in src/install/install.zig (defaulting to false, set to true only in Features.main) but was never checked in parseWithJSON. This PR adds the missing check, exactly mirroring how trustedDependencies is gated on comptime features.trusted_dependencies a few lines below (Package.zig:1558). I confirmed Features.folder, Features.workspace, Features.link, Features.npm, and Features.tarball all leave patched_dependencies at its false default, so after this change only the root manifest contributes patch entries — consistent with bun patch --commit writing to root and the lockfile serializing patchedDependencies as a single top-level key.
Security risks
None. The change narrows what is read from non-root package.json files; it does not introduce new file reads, path resolution, or external input handling.
Level of scrutiny
Low-to-moderate. The production change is a two-line mechanical guard following an established in-file pattern, evaluated at compile time (so dead-code-eliminated for non-main feature sets). The behavioral change is a bug fix: previously the merged patch paths were resolved relative to the wrong directory and failed with Couldn't find patch file, so there is no working behavior being removed.
Other factors
All CodeRabbit review threads are resolved (stdout assertion moved to lockfile; assertion ordering justified to avoid ENOENT masking the real failure). CI failures (webview-chrome, v8-heap-snapshot, spawn-stdin-readable-stream, load-file-loader-a-lot) are on unrelated test files and platforms. No CODEOWNERS coverage for these paths. The bug-hunting system found no issues.
There was a problem hiding this comment.
LGTM — minimal comptime gate matching the existing trusted_dependencies pattern, with regression coverage for both folder and workspace cases.
Extended reasoning...
Overview
Two changes: (1) wraps both patchedDependencies parse blocks in Package.parseWithJSON (src/install/lockfile/Package.zig) with if (comptime features.patched_dependencies), and (2) adds two regression tests in test/cli/install/bun-install-patch.test.ts covering folder dependencies and workspace members. The Zig diff is pure indentation inside the new guards — no logic inside the blocks changed.
Correctness
Features.patched_dependencies defaults to false and is only set to true in Features.main (src/install/install.zig:142,162); folder, workspace, link, npm, and tarball all leave it false. The new guard therefore restricts patchedDependencies parsing to the install root, exactly mirroring the adjacent if (comptime features.trusted_dependencies) block at Package.zig:1558. This is the documented intent (bun patch --commit writes to root, lockfile serializes a single top-level key) and fixes #13531 without changing root-install behavior.
Security risks
None. The change strictly narrows which manifests can inject entries into lockfile.patched_dependencies — a non-root package can no longer cause the installer to look for patch files at arbitrary root-relative paths. No auth, crypto, or untrusted-input surface is added.
Level of scrutiny
Low. The production change is a two-line comptime guard around unchanged code, following an established in-file pattern. No CODEOWNERS apply. All CodeRabbit threads are resolved (stdout assertion moved to lockfile; exitCode-before-lockfile-read ordering justified and accepted). The one inline finding is explicitly pre-existing, non-blocking, and notes this PR reduces its trigger surface.
Other factors
Tests follow the file's existing conventions (tempDirWithFiles, Bun.spawn + Promise.all, lockfile content assertions) and use the local verdaccio-served is-even fixture already exercised throughout the suite.
|
CI status summary across builds 47275 / 51104 / 51258:
The actual change here is a two-line |
1 participant
What does this PR do?
Fixes #13531
When a package depends on another package via a local path (folder dependency, e.g.
"pkg-a": "../pkgA") or as a workspace member, and that package'spackage.jsoncontainspatchedDependencies,bun installwould fail with:because the patch entries from the dependency were merged into the root lockfile's
patched_dependenciesmap, and patch file paths are resolved relative to the installing project's root (not the dependency's directory).Root cause
Features.patched_dependencieswas defined insrc/install/install.zig(onlytrueforFeatures.main) but was never checked inPackage.parseWithJSON. BothpatchedDependenciesparse blocks ran unconditionally for everypackage.json, including folder/link/workspace dependencies.Fix
Gate both
patchedDependenciesparse blocks oncomptime features.patched_dependencies, matching howtrustedDependenciesis already gated onfeatures.trusted_dependencies.This means
patchedDependenciesis only read from the rootpackage.json, consistent with:bun patch --commitwriting to the rootpackage.jsonpatchedDependenciesas a single top-level keypnpm.patchedDependenciesto the rootHow did you verify your code works?
Added two regression tests in
test/cli/install/bun-install-patch.test.ts:pkgBdepends on"../pkgA"wherepkgAhaspatchedDependenciespatchedDependenciesBoth fail on main with
Couldn't find patch fileand pass with this fix. Also verified that installing inpkgAdirectly still applies its patch correctly.Supersedes #27895 (same fix, originally targeting #27894 which was closed as a duplicate of #13531).