fix: harden allowBuilds artifact approvals#12294
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR migrates allowBuild to accept a depPath plus an AllowBuildContext (trustPackageIdentity), adds dep-path identity keys and normalization, computes trust from resolution/resolvedVia, propagates metadata through resolvers/graphs, and updates call sites, fetchers, installers, GVS, Rust policy, and tests to use the new contract. ChangesBuild Policy and Package Identity Trust
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0fb2c4e to
8939ad5
Compare
Contributor
Micro-Benchmark ResultsLinux |
Contributor
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 10.31357480088,
"stddev": 0.1682821483406344,
"median": 10.23536635238,
"user": 3.255627439999999,
"system": 3.3214429599999997,
"min": 10.20779217388,
"max": 10.65390506088,
"times": [
10.598718656880001,
10.313594632880001,
10.65390506088,
10.20937575588,
10.21593648088,
10.22898596188,
10.24174674288,
10.20779217388,
10.21273596388,
10.252956578880001
]
},
{
"command": "pacquet@main",
"mean": 10.276961247280001,
"stddev": 0.1255636240863856,
"median": 10.21254091488,
"user": 3.2719158399999997,
"system": 3.31710066,
"min": 10.20487420788,
"max": 10.60613774188,
"times": [
10.21062738788,
10.27138096088,
10.21036235288,
10.21039086588,
10.21286456388,
10.21221726588,
10.20487420788,
10.27183542188,
10.60613774188,
10.35892170388
]
},
{
"command": "pnpr@HEAD",
"mean": 5.43597003628,
"stddev": 0.13574802248289128,
"median": 5.40986779138,
"user": 2.49019294,
"system": 2.9913655599999993,
"min": 5.29913213688,
"max": 5.75816215588,
"times": [
5.37770098288,
5.44203459988,
5.33875208488,
5.37454833588,
5.48784779488,
5.31154139388,
5.44209782688,
5.52788305088,
5.29913213688,
5.75816215588
]
},
{
"command": "pnpr@main",
"mean": 5.4499742827799995,
"stddev": 0.15773626957662712,
"median": 5.377866234880001,
"user": 2.4857402399999993,
"system": 3.0042431599999997,
"min": 5.30191117988,
"max": 5.82012109488,
"times": [
5.30191117988,
5.37123741788,
5.330141465880001,
5.47835161688,
5.38205911188,
5.3736733578800004,
5.82012109488,
5.33878913988,
5.54379234288,
5.55966609988
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6480816608200001,
"stddev": 0.007916934885130303,
"median": 0.6471735929200001,
"user": 0.37598235999999996,
"system": 1.32931402,
"min": 0.6368856799200001,
"max": 0.6627796439200001,
"times": [
0.6457016579200001,
0.6627796439200001,
0.6368856799200001,
0.6427420459200001,
0.6493084489200001,
0.6462606699200001,
0.65579180692,
0.6480865159200001,
0.6388820859200001,
0.6543780529200001
]
},
{
"command": "pacquet@main",
"mean": 0.6650810359200001,
"stddev": 0.010045972844961996,
"median": 0.6672847979200001,
"user": 0.37191856000000006,
"system": 1.34038902,
"min": 0.64695436392,
"max": 0.6786790409200001,
"times": [
0.6667850089200001,
0.6680522659200001,
0.6621381959200001,
0.6585109789200001,
0.6762344939200001,
0.64695436392,
0.6531027089200001,
0.6677845869200001,
0.6786790409200001,
0.6725687149200001
]
},
{
"command": "pnpr@HEAD",
"mean": 0.7792431392200001,
"stddev": 0.06522956560344192,
"median": 0.7540621029200001,
"user": 0.38839816000000005,
"system": 1.3575941200000001,
"min": 0.7411615079200001,
"max": 0.9575985179200001,
"times": [
0.7411615079200001,
0.7754480689200001,
0.7804139149200001,
0.7461503069200001,
0.7466692999200001,
0.9575985179200001,
0.7604410019200001,
0.7476832039200001,
0.74315460192,
0.7937109679200001
]
},
{
"command": "pnpr@main",
"mean": 0.78508450682,
"stddev": 0.06940547257029529,
"median": 0.7609526254200001,
"user": 0.39146316,
"system": 1.34215762,
"min": 0.7446227699200001,
"max": 0.9734397349200001,
"times": [
0.7993152159200001,
0.8023462519200001,
0.7580201059200001,
0.7457537299200001,
0.7512441919200001,
0.9734397349200001,
0.7657426379200001,
0.7638851449200001,
0.7464752849200001,
0.7446227699200001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 9.22393501734,
"stddev": 0.0458108553288942,
"median": 9.20318118374,
"user": 3.5934363399999993,
"system": 3.3175235,
"min": 9.18927345474,
"max": 9.33371170174,
"times": [
9.19148250074,
9.18927345474,
9.23828126574,
9.261857517740001,
9.19582126874,
9.20954988374,
9.190244056740001,
9.33371170174,
9.19681248374,
9.23231603974
]
},
{
"command": "pacquet@main",
"mean": 9.219522806839999,
"stddev": 0.05091091824698436,
"median": 9.22538705524,
"user": 3.6060664399999993,
"system": 3.3045235000000006,
"min": 9.14918589374,
"max": 9.27540290974,
"times": [
9.212282997740001,
9.14918589374,
9.15504852174,
9.27540290974,
9.20636569674,
9.272507924740001,
9.27251065674,
9.25452840474,
9.23849111274,
9.15890394974
]
},
{
"command": "pnpr@HEAD",
"mean": 5.08883403524,
"stddev": 0.05131198736158377,
"median": 5.09166804024,
"user": 2.3107602399999996,
"system": 2.8620053000000008,
"min": 5.01651607574,
"max": 5.17120557374,
"times": [
5.06088809774,
5.14445034274,
5.11388571174,
5.17120557374,
5.02512035974,
5.01651607574,
5.04750117474,
5.0969554747400005,
5.0863806057400005,
5.12543693574
]
},
{
"command": "pnpr@main",
"mean": 5.09916515864,
"stddev": 0.030277911297965053,
"median": 5.104242188740001,
"user": 2.30650094,
"system": 2.8935749999999993,
"min": 5.03580012774,
"max": 5.13772838474,
"times": [
5.11615517074,
5.09865910374,
5.07945044574,
5.13578906474,
5.03580012774,
5.08821899074,
5.11020585074,
5.10982527374,
5.13772838474,
5.07981917374
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.40951243594,
"stddev": 0.014820437435387027,
"median": 1.40648435614,
"user": 1.5562699999999998,
"system": 1.78996954,
"min": 1.3933117376400002,
"max": 1.4409042166400001,
"times": [
1.40668715664,
1.42817358864,
1.40037268164,
1.4062815556400001,
1.40884831064,
1.41320557964,
1.4025103326400001,
1.3948291996400002,
1.4409042166400001,
1.3933117376400002
]
},
{
"command": "pacquet@main",
"mean": 1.4032797258400003,
"stddev": 0.013577131089162479,
"median": 1.3986116061400002,
"user": 1.5534367999999998,
"system": 1.7843861399999998,
"min": 1.38636109764,
"max": 1.4239260816400001,
"times": [
1.3983274166400002,
1.42213601464,
1.38883714864,
1.39616212964,
1.38636109764,
1.39889579564,
1.40661467964,
1.3942690346400002,
1.4172678596400001,
1.4239260816400001
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6784664314400002,
"stddev": 0.022611249853267915,
"median": 0.67186877864,
"user": 0.33375799999999994,
"system": 1.3039025400000002,
"min": 0.6573152656400001,
"max": 0.7371397246400001,
"times": [
0.7371397246400001,
0.6794470126400001,
0.6873469146400001,
0.6818857616400001,
0.6689483646400001,
0.6720590736400001,
0.6693080546400001,
0.6573152656400001,
0.6595356586400001,
0.67167848364
]
},
{
"command": "pnpr@main",
"mean": 0.6871697412400001,
"stddev": 0.07721976369280653,
"median": 0.6662378306400001,
"user": 0.3356513,
"system": 1.2686546399999998,
"min": 0.6478949586400001,
"max": 0.9049919946400001,
"times": [
0.67079162564,
0.6478949586400001,
0.6522299576400001,
0.9049919946400001,
0.6517495126400001,
0.6593550256400001,
0.6781958806400001,
0.6700964066400001,
0.67401279564,
0.6623792546400001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.9945924132599995,
"stddev": 0.024689653253851945,
"median": 4.99144822156,
"user": 1.75535426,
"system": 1.9159239999999997,
"min": 4.96193104256,
"max": 5.05022347956,
"times": [
4.99214712456,
4.98067214656,
4.99464706856,
5.01145079056,
4.98343591056,
4.99074931856,
5.00857036756,
4.97209688356,
4.96193104256,
5.05022347956
]
},
{
"command": "pacquet@main",
"mean": 4.97048391076,
"stddev": 0.029841372023626283,
"median": 4.97447128956,
"user": 1.76122646,
"system": 1.9030434999999997,
"min": 4.91788343656,
"max": 5.01109332756,
"times": [
4.98366090356,
4.97273139556,
4.97076533056,
4.91788343656,
4.97621118356,
4.99057130556,
4.99435327156,
5.01109332756,
4.92155726156,
4.96601169156
]
},
{
"command": "pnpr@HEAD",
"mean": 0.69538089906,
"stddev": 0.020419719982585044,
"median": 0.6889438855600001,
"user": 0.34533756,
"system": 1.3071393,
"min": 0.66849981956,
"max": 0.72543791256,
"times": [
0.7220474475599999,
0.67909352556,
0.71242725056,
0.67905665056,
0.68398389756,
0.72543791256,
0.66849981956,
0.69390387356,
0.6795107875599999,
0.70984782556
]
},
{
"command": "pnpr@main",
"mean": 0.70161629576,
"stddev": 0.0711325176406548,
"median": 0.67733528706,
"user": 0.34346156,
"system": 1.2755782,
"min": 0.65555372956,
"max": 0.89908418456,
"times": [
0.70406431356,
0.68298184356,
0.89908418456,
0.66896825556,
0.67232886956,
0.67640508656,
0.65555372956,
0.67826548756,
0.67109153856,
0.70741964856
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/12294 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 9,223.94 ms(+17.19%)Baseline: 7,870.61 ms | 9,444.73 ms (97.66%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 4,994.59 ms(-0.55%)Baseline: 5,022.02 ms | 6,026.43 ms (82.88%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,409.51 ms(-0.58%)Baseline: 1,417.71 ms | 1,701.25 ms (82.85%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 10,313.57 ms(+6.71%)Baseline: 9,665.28 ms | 11,598.33 ms (88.92%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 648.08 ms(-1.47%)Baseline: 657.75 ms | 789.30 ms (82.11%) |
8939ad5 to
17d1bfc
Compare
Code Review by Qodo
Context used 1. runUnignoredDependencyBuilds rebuilds in-place
|
PR Summary by QodoHarden allowBuilds approvals for git/tarball artifacts using trusted identity + depPath keys WalkthroughsDescription• Require trusted package identity before name-based allowBuilds can permit lifecycle scripts. • Allow explicit approvals for git/tarball artifacts via peer-suffix-free lockfile depPath keys. • Propagate the policy through install/build/fetch flows (and pacquet), with added coverage. Diagramgraph TD
cfg["Workspace allowBuilds config"] --> policy["building.policy: allowBuild + context"] --> install["Installer/build flows"]
policy --> hash["Graph hash/builder"]
fetch["Git/Tarball fetchers"] --> prep["preparePackage gate"] --> policy
pacq["pacquet: mirrored policy"] --> policy
High-Level AssessmentThe following are alternative approaches to this PR: 1. Split artifact approvals into a dedicated map (e.g. allowBuildArtifacts)
2. Persist explicit approvals in lockfile/modules manifest only
Recommendation: Keep the PR’s approach: it preserves existing name-based workflow for trusted registry/workspace packages while closing the artifact-spoofing gap by requiring depPath-specific keys for git/tarball sources. The added context plumbing is unavoidable for correctness, and mirroring in pacquet reduces policy divergence. File ChangesEnhancement (6)
Bug fix (20)
Tests (13)
Documentation (1)
Other (13)
|
17d1bfc to
ba4cc44
Compare
|
Code review by qodo was updated up to the latest commit ba4cc44 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
building/commands/src/policy/approveBuilds.ts (1)
232-237: ⚡ Quick winExtract
allowBuildKeyFromIgnoredBuildinto a shared utility.This same key-normalization logic is duplicated in
building/commands/src/policy/getAutomaticallyIgnoredBuilds.tsandinstalling/commands/src/handleIgnoredBuilds.ts. Centralizing it avoids drift between approval matching and ignored-build serialization behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@building/commands/src/policy/approveBuilds.ts` around lines 232 - 237, Extract the duplicated normalization logic currently implemented as allowBuildKeyFromIgnoredBuild into a single shared utility function (e.g., export function normalizeIgnoredBuildKey) and replace the inline implementation in approveBuilds.ts, getAutomaticallyIgnoredBuilds.ts, and handleIgnoredBuilds.ts with calls to that new exported helper; preserve the current behavior (call removeSuffix, parse, return normalizedDepPath when parsed.nonSemverVersion != null or parsed.name == null, otherwise return parsed.name), export the helper from a common module used by the three callers, and update imports in the three files accordingly so approval matching and ignored-build serialization use the exact same function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@building/after-install/src/index.ts`:
- Around line 376-379: The fallback to find a linked global virtual store
directory ignores custom globalVirtualStoreDir because findLinkedGvsDir
hardcodes the links path; update findLinkedGvsDir to accept the current
globalVirtualStoreDir (add a parameter, e.g. globalVirtualStoreDir) and use it
to construct the links lookup instead of a hard-coded path, then update calls
from gvsDirByDepPath.set (where preferredGvsDir, pkgMeta.depPath, pkgMeta.name
and ctx.projects are used) to pass the globalVirtualStoreDir; ensure the
fallback uses the returned linked path only when defined and otherwise keeps
preferredGvsDir, and apply the same change for the other occurrence mentioned.
In `@fetching/git-fetcher/src/index.ts`:
- Around line 112-119: createGitPkgResolutionId is incorrectly prefixing
https:// for SCP-style SSH git URLs (e.g. git@host:user/repo.git) because it
only checks for '://'; update createGitPkgResolutionId to detect SCP-style SSH
(e.g. strings starting like git@host: or matching /^[^@]+@[^:]+:/) and normalize
them to an ssh form (e.g. produce a git+ssh://... shape or reuse the same
normalization logic as hosted.sshurl()/hosted.ssh()) instead of adding https://;
also update the matching resolver-id generation path that builds IDs from
parsedSpec.fetchSpec to use the same detection/normalization and add a unit test
asserting that an input of git@host:user/repo.git yields a git+ssh:// (or
equivalent ssh-shaped) id with the correct #<commit> and &path when present.
In `@pacquet/crates/package-manager/src/build_modules.rs`:
- Around line 642-651: The current computation of trust_package_identity uses
is_none_or(package_identity_is_trusted) which treats missing metadata as
trusted; change it to default to untrusted by returning false when metadata is
absent. Replace the is_none_or-based expression with an explicit Option check
(e.g., use Option::map_or(false, |meta| package_identity_is_trusted(meta)) or
equivalent) on packages.and_then(|packages| packages.get(&metadata_key)) so
allow_build_policy.check_with_context receives false for unknown package
identity.
In `@pacquet/crates/package-manager/src/virtual_store_layout.rs`:
- Around line 208-217: The current trust logic uses
packages.and_then(...).is_none_or(package_identity_is_trusted) which treats
missing metadata as trusted; change it so missing metadata is untrusted by
default by evaluating trust_package_identity using a map_or(false,
package_identity_is_trusted) or is_some_and(package_identity_is_trusted) on
packages.and_then(...). Update the expression that sets trust_package_identity
(referencing packages, metadata_key, and package_identity_is_trusted) so
policy.check_with_context receives false for missing entries instead of true.
In `@pnpm-workspace.yaml`:
- Line 440: The dependency pin for shell-quote (shell-quote: 1.8.4) lacks a
comment explaining why it is pinned; add a short inline comment next to the
shell-quote: 1.8.4 entry in pnpm-workspace.yaml that references the security
advisory (GHSA-w7jw-789q-3m8p / CVE-2026-9277) and notes that 1.8.4 patches the
critical vulnerability present in <=1.8.3 so future maintainers understand the
pin.
---
Nitpick comments:
In `@building/commands/src/policy/approveBuilds.ts`:
- Around line 232-237: Extract the duplicated normalization logic currently
implemented as allowBuildKeyFromIgnoredBuild into a single shared utility
function (e.g., export function normalizeIgnoredBuildKey) and replace the inline
implementation in approveBuilds.ts, getAutomaticallyIgnoredBuilds.ts, and
handleIgnoredBuilds.ts with calls to that new exported helper; preserve the
current behavior (call removeSuffix, parse, return normalizedDepPath when
parsed.nonSemverVersion != null or parsed.name == null, otherwise return
parsed.name), export the helper from a common module used by the three callers,
and update imports in the three files accordingly so approval matching and
ignored-build serialization use the exact same function.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a0fcd8f4-682f-4484-bc08-7810888967f0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.changeset/tough-allow-builds-identities.mdbuilding/after-install/src/index.tsbuilding/commands/src/policy/approveBuilds.tsbuilding/commands/src/policy/getAutomaticallyIgnoredBuilds.tsbuilding/commands/test/build/index.tsbuilding/during-install/package.jsonbuilding/during-install/src/index.tsbuilding/during-install/tsconfig.jsonbuilding/policy/src/index.tsbuilding/policy/test/index.tscore/types/src/config.tsdeps/graph-builder/package.jsondeps/graph-builder/src/lockfileToDepGraph.tsdeps/graph-builder/tsconfig.jsondeps/graph-hasher/src/index.tsdeps/graph-hasher/test/allowBuildIdentity.test.tsexec/prepare-package/src/index.tsexec/prepare-package/test/index.tsfetching/git-fetcher/src/index.tsfetching/tarball-fetcher/src/gitHostedTarballFetcher.tsinstalling/commands/src/handleIgnoredBuilds.tsinstalling/deps-installer/src/install/index.tsinstalling/deps-installer/src/install/link.tsinstalling/deps-installer/test/install/lifecycleScripts.tsinstalling/deps-resolver/src/index.tsinstalling/deps-resolver/src/resolveDependencies.tsinstalling/deps-restorer/src/index.tsinstalling/deps-restorer/src/linkHoistedModules.tspacquet/crates/git-fetcher/src/fetcher.rspacquet/crates/git-fetcher/src/fetcher/tests.rspacquet/crates/git-fetcher/src/prepare_package.rspacquet/crates/git-fetcher/src/prepare_package/tests.rspacquet/crates/git-fetcher/src/tarball_fetcher.rspacquet/crates/git-fetcher/src/tarball_fetcher/tests.rspacquet/crates/package-manager/src/build_modules.rspacquet/crates/package-manager/src/build_modules/tests.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/virtual_store_layout.rspnpm-workspace.yaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: ubuntu-latest / Node.js 24 / Test
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use Standard Style with trailing commas, prefer functions over classes, declare functions after they are used (relying on hoisting), limit functions to no more than two or three arguments, and use a single options object for functions needing more parameters
Follow import order: standard libraries first, then external dependencies (sorted alphabetically), then relative imports
Do not write comments that restate what the code already says; rename variables, split helpers, or move checks to more obvious places instead
Do not repeat documentation at call sites that already lives on the callee; update the JSDoc once and let every call site benefit
Use JSDoc for the function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the function body
Do not record past implementation shape, refactor history, or removed code in comments; use git log and git blame for that information instead
Write comments only when the reason for code is non-obvious, a hidden invariant exists, a workaround for a known bug is needed, or an exception to surrounding pattern is deliberate
Files:
core/types/src/config.tsinstalling/deps-installer/src/install/link.tsdeps/graph-hasher/test/allowBuildIdentity.test.tsbuilding/commands/src/policy/getAutomaticallyIgnoredBuilds.tsinstalling/deps-resolver/src/index.tsexec/prepare-package/src/index.tsfetching/tarball-fetcher/src/gitHostedTarballFetcher.tsinstalling/deps-restorer/src/linkHoistedModules.tsinstalling/commands/src/handleIgnoredBuilds.tsinstalling/deps-restorer/src/index.tsdeps/graph-builder/src/lockfileToDepGraph.tsfetching/git-fetcher/src/index.tsbuilding/policy/test/index.tsinstalling/deps-resolver/src/resolveDependencies.tsbuilding/during-install/src/index.tsexec/prepare-package/test/index.tsbuilding/commands/src/policy/approveBuilds.tsbuilding/after-install/src/index.tsdeps/graph-hasher/src/index.tsbuilding/policy/src/index.tsinstalling/deps-installer/src/install/index.tsinstalling/deps-installer/test/install/lifecycleScripts.tsbuilding/commands/test/build/index.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
instanceof Errorfor checking if a caught error is an Error object in Jest tests; useutil.types.isNativeError()instead to work across realms
Files:
deps/graph-hasher/test/allowBuildIdentity.test.ts
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/git-fetcher/src/tarball_fetcher.rspacquet/crates/git-fetcher/src/tarball_fetcher/tests.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/git-fetcher/src/fetcher.rspacquet/crates/git-fetcher/src/prepare_package.rspacquet/crates/git-fetcher/src/prepare_package/tests.rspacquet/crates/package-manager/src/build_modules/tests.rspacquet/crates/git-fetcher/src/fetcher/tests.rspacquet/crates/package-manager/src/build_modules.rs
🧠 Learnings (9)
📚 Learning: 2026-05-05T23:03:04.286Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11479
File: __utils__/scripts/package.json:6-9
Timestamp: 2026-05-05T23:03:04.286Z
Learning: The pattern cross-env NODE_OPTIONS="$NODE_OPTIONS ..." in package.json scripts is an established convention in the pnpm/pnpm repository and is used across many packages (e.g., fs/hard-link-dir, worker, __utils__/scripts). Do not flag this as a cross-platform issue in individual files; if a change is needed, apply it as a repo-wide change in a separate PR. Scope this guidance to all package.json files in the repo; use the minimatch pattern '**/package.json' to identify relevant files and review changes at the repository level rather than per-file.
Applied to files:
building/during-install/package.jsondeps/graph-builder/package.json
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
core/types/src/config.tsinstalling/deps-installer/src/install/link.tsdeps/graph-hasher/test/allowBuildIdentity.test.tsbuilding/commands/src/policy/getAutomaticallyIgnoredBuilds.tsinstalling/deps-resolver/src/index.tsexec/prepare-package/src/index.tsfetching/tarball-fetcher/src/gitHostedTarballFetcher.tsinstalling/deps-restorer/src/linkHoistedModules.tsinstalling/commands/src/handleIgnoredBuilds.tsinstalling/deps-restorer/src/index.tsdeps/graph-builder/src/lockfileToDepGraph.tsfetching/git-fetcher/src/index.tsbuilding/policy/test/index.tsinstalling/deps-resolver/src/resolveDependencies.tsbuilding/during-install/src/index.tsexec/prepare-package/test/index.tsbuilding/commands/src/policy/approveBuilds.tsbuilding/after-install/src/index.tsdeps/graph-hasher/src/index.tsbuilding/policy/src/index.tsinstalling/deps-installer/src/install/index.tsinstalling/deps-installer/test/install/lifecycleScripts.tsbuilding/commands/test/build/index.ts
📚 Learning: 2026-06-05T13:47:26.046Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/src/install/index.ts:2337-2343
Timestamp: 2026-06-05T13:47:26.046Z
Learning: In the pnpm/pnpm codebase, `PnpmError` automatically prefixes `err.code` with `ERR_PNPM_` when you pass a code that does not already start with `ERR_PNPM_` (it normalizes `this.code` via `code.startsWith('ERR_PNPM_') ? code : `ERR_PNPM_${code}``). Therefore, during code review you should NOT flag `new PnpmError(...)` call sites for passing a bare error code (e.g., `new PnpmError('FROZEN_STORE_INCOMPATIBLE_WITH_PNPR', ...)`); the resulting `err.code` will still be `ERR_PNPM_FROZEN_STORE_INCOMPATIBLE_WITH_PNPR`.
Applied to files:
core/types/src/config.tsinstalling/deps-installer/src/install/link.tsdeps/graph-hasher/test/allowBuildIdentity.test.tsbuilding/commands/src/policy/getAutomaticallyIgnoredBuilds.tsinstalling/deps-resolver/src/index.tsexec/prepare-package/src/index.tsfetching/tarball-fetcher/src/gitHostedTarballFetcher.tsinstalling/deps-restorer/src/linkHoistedModules.tsinstalling/commands/src/handleIgnoredBuilds.tsinstalling/deps-restorer/src/index.tsdeps/graph-builder/src/lockfileToDepGraph.tsfetching/git-fetcher/src/index.tsbuilding/policy/test/index.tsinstalling/deps-resolver/src/resolveDependencies.tsbuilding/during-install/src/index.tsexec/prepare-package/test/index.tsbuilding/commands/src/policy/approveBuilds.tsbuilding/after-install/src/index.tsdeps/graph-hasher/src/index.tsbuilding/policy/src/index.tsinstalling/deps-installer/src/install/index.tsinstalling/deps-installer/test/install/lifecycleScripts.tsbuilding/commands/test/build/index.ts
📚 Learning: 2026-05-26T21:01:06.666Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11966
File: .changeset/require-tarball-integrity.md:6-6
Timestamp: 2026-05-26T21:01:06.666Z
Learning: In pnpm lockfile-related release notes/docs (especially changeset markdown), preserve URL hostnames exactly as they appear in pnpm-lock.yaml tarball resolution entries—keep hosts like `codeload.github.com`, `bitbucket.org`, and `gitlab.com` in lowercase. Do not “correct” them to title-case/preserve brand capitalization (e.g., LanguageTool rules like `GITHUB` capitalization) because these are literal URL fragments, not platform brand names.
Applied to files:
.changeset/tough-allow-builds-identities.md
📚 Learning: 2026-06-05T13:47:05.929Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/test/install/frozenStore.ts:2-17
Timestamp: 2026-06-05T13:47:05.929Z
Learning: In the pnpm/pnpm repository, the shared Jest preset keeps `injectGlobals` at its default (`true`), so `test` and `expect` are available as Jest globals. Therefore, reviewers should not flag (or treat as TypeScript/compilation errors) missing `import { test, expect } from 'jest/globals'` when a test file uses `test`/`expect` without importing them. Importing from `jest/globals` may still be used for consistency with sibling files, but it is not required for execution in this repo unless a Jest preset is explicitly configured with `injectGlobals: false`.
Applied to files:
deps/graph-hasher/test/allowBuildIdentity.test.tsbuilding/policy/test/index.tsexec/prepare-package/test/index.tsinstalling/deps-installer/test/install/lifecycleScripts.tsbuilding/commands/test/build/index.ts
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/git-fetcher/src/tarball_fetcher.rspacquet/crates/git-fetcher/src/tarball_fetcher/tests.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/git-fetcher/src/fetcher.rspacquet/crates/git-fetcher/src/prepare_package.rspacquet/crates/git-fetcher/src/prepare_package/tests.rspacquet/crates/package-manager/src/build_modules/tests.rspacquet/crates/git-fetcher/src/fetcher/tests.rspacquet/crates/package-manager/src/build_modules.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/git-fetcher/src/tarball_fetcher.rspacquet/crates/git-fetcher/src/tarball_fetcher/tests.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/git-fetcher/src/fetcher.rspacquet/crates/git-fetcher/src/prepare_package.rspacquet/crates/git-fetcher/src/prepare_package/tests.rspacquet/crates/package-manager/src/build_modules/tests.rspacquet/crates/git-fetcher/src/fetcher/tests.rspacquet/crates/package-manager/src/build_modules.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/git-fetcher/src/tarball_fetcher.rspacquet/crates/git-fetcher/src/tarball_fetcher/tests.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/git-fetcher/src/fetcher.rspacquet/crates/git-fetcher/src/prepare_package.rspacquet/crates/git-fetcher/src/prepare_package/tests.rspacquet/crates/package-manager/src/build_modules/tests.rspacquet/crates/git-fetcher/src/fetcher/tests.rspacquet/crates/package-manager/src/build_modules.rs
📚 Learning: 2026-06-06T18:58:37.156Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12243
File: pacquet/crates/package-manager/src/install_package_by_snapshot.rs:319-322
Timestamp: 2026-06-06T18:58:37.156Z
Learning: When reviewing Rust code, do not assume `matches!(expr, Pattern(_))` will move out of `expr` if `Pattern(_)` contains no by-value bindings. `matches!` desugars to a `match` that auto-borrows the scrutinee for discrimination, so even if `expr` is a non-`Copy` value behind a shared reference (e.g., `&T`), the macro should not move-out of the borrowed data purely due to `matches!`. Treat `matches!(&expr, Pattern(_))` as a readability/clarity improvement, not a correctness requirement. Only flag potential move-out-of-borrow risks when the pattern includes by-value bindings that would require moving the matched value.
Applied to files:
pacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/git-fetcher/src/tarball_fetcher.rspacquet/crates/git-fetcher/src/tarball_fetcher/tests.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/git-fetcher/src/fetcher.rspacquet/crates/git-fetcher/src/prepare_package.rspacquet/crates/git-fetcher/src/prepare_package/tests.rspacquet/crates/package-manager/src/build_modules/tests.rspacquet/crates/git-fetcher/src/fetcher/tests.rspacquet/crates/package-manager/src/build_modules.rs
🔇 Additional comments (40)
.changeset/tough-allow-builds-identities.md (1)
1-17: LGTM!building/commands/src/policy/getAutomaticallyIgnoredBuilds.ts (1)
3-3: LGTM!Also applies to: 21-21, 38-43
installing/commands/src/handleIgnoredBuilds.ts (1)
2-2: LGTM!Also applies to: 50-57
building/commands/test/build/index.ts (1)
50-50: LGTM!Also applies to: 60-60, 333-333, 341-341, 387-387, 402-402
exec/prepare-package/src/index.ts (1)
22-50: LGTM!exec/prepare-package/test/index.ts (2)
11-32: LGTM!
34-52: LGTM!fetching/git-fetcher/src/index.ts (1)
50-51: LGTM!fetching/tarball-fetcher/src/gitHostedTarballFetcher.ts (2)
83-88: LGTM!
129-135: LGTM!installing/deps-installer/test/install/lifecycleScripts.ts (4)
3-3: LGTM!Also applies to: 16-16, 20-20
314-333: LGTM!
335-378: LGTM!
380-408: LGTM!pacquet/crates/git-fetcher/src/fetcher.rs (1)
1-154: LGTM!pacquet/crates/git-fetcher/src/prepare_package.rs (1)
1-180: LGTM!pacquet/crates/git-fetcher/src/prepare_package/tests.rs (1)
1-252: LGTM!pacquet/crates/git-fetcher/src/tarball_fetcher/tests.rs (1)
1-14: LGTM!pacquet/crates/package-manager/src/build_modules/tests.rs (1)
1-241: LGTM!core/types/src/config.ts (1)
3-8: LGTM!deps/graph-hasher/src/index.ts (1)
2-2: LGTM!Also applies to: 163-170, 336-376
deps/graph-hasher/test/allowBuildIdentity.test.ts (1)
1-53: LGTM!installing/deps-resolver/src/index.ts (1)
560-565: LGTM!building/during-install/src/index.ts (1)
7-7: LGTM!Also applies to: 95-95
installing/deps-installer/src/install/index.ts (1)
6-6: LGTM!Also applies to: 61-61, 469-469, 487-491, 1096-1127
building/policy/src/index.ts (1)
63-65: Verify missing-context calls cannot bypass the trust gate.Line 64 only blocks package-spec approvals when
context.trustPackageIdentity === false; ifcontextis omitted, the same rule still returnstrue. Please verify all productionallowBuildcall sites now passcreateAllowBuildContext(...).building/policy/test/index.ts (1)
2-3: LGTM!Also applies to: 35-154
building/during-install/package.json (1)
37-37: LGTM!building/during-install/tsconfig.json (1)
59-62: LGTM!deps/graph-builder/package.json (1)
33-33: LGTM!deps/graph-builder/src/lockfileToDepGraph.ts (1)
4-4: LGTM!Also applies to: 245-248
installing/deps-resolver/src/resolveDependencies.ts (2)
260-260: LGTM!
1837-1837: LGTM!deps/graph-builder/tsconfig.json (1)
12-14: LGTM!installing/deps-installer/src/install/link.ts (2)
4-4: LGTM!
490-490: LGTM!installing/deps-restorer/src/index.ts (2)
6-6: LGTM!
935-935: LGTM!installing/deps-restorer/src/linkHoistedModules.ts (2)
4-4: LGTM!
139-139: LGTM!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12294 +/- ##
==========================================
+ Coverage 87.53% 87.70% +0.17%
==========================================
Files 280 288 +8
Lines 33441 35177 +1736
==========================================
+ Hits 29271 30851 +1580
- Misses 4170 4326 +156 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ba4cc44 to
24fa6f8
Compare
…provals - Extract allowBuildKeyFromIgnoredBuild into @pnpm/building.policy (was triplicated) and reuse createAllowBuildContext in deps/graph-hasher. - Reuse createGitHostedPkgId from @pnpm/resolving.git-resolver in the git fetcher instead of a local re-implementation. - preparePackage always treats the fetched manifest as an untrusted package identity; drop its trustPackageIdentity and depPath options (mirrored in pacquet's PreparePackageOptions). - Detect revoked build approvals for git/tarball artifacts in mutateModules; non-semver depPaths were skipped entirely. - pacquet: normalize allowBuilds depPath keys with remove_suffix instead of a hand-rolled paren-truncating fallback. - Drain the global log stream in the lifecycleScripts tests so reporter assertions only see their own installs' events instead of the buffered backlog of earlier reporterless installs.
|
Code review by qodo was updated up to the latest commit b4366f4 |
The dlx git test still approved the shx build by package name, which the allowBuilds hardening in this PR no longer accepts for git-hosted artifacts. Every other test in the PR was already migrated to depPath-keyed approvals; this one was missed.
|
Code review by qodo was updated up to the latest commit 7227a1d |
Selectors and lockfile keys are now both normalized with getPkgIdWithPatchHash, the same canonical form allowBuilds keys use. The previous exact-or-removeSuffix comparison dropped the patch hash, so the pkgIdWithPatchHash-shaped specs that runUnignoredDependencyBuilds passes for patched artifacts matched neither branch.
|
Code review by qodo was updated up to the latest commit 1dbfad8 |
The AllowBuild function now takes the package's DepPath instead of
caller-supplied name and version, which were duplicates of (or, in
preparePackage's case, manifest-claimed stand-ins for) information the
depPath already carries. The policy parses name and version out of the
depPath itself, so package-name rules can no longer be fed an identity
that disagrees with the artifact being gated.
- @pnpm/deps.path: add removePeersSuffix, the string-level primitive
for normalizing user-written allowBuilds keys; getPkgIdWithPatchHash
delegates to it.
- AllowBuildContext shrinks to { trustPackageIdentity }.
- preparePackage requires pkgResolutionId and synthesizes the depPath
from it (the one deliberate brand cast, since that identity is not a
lockfile key).
- pacquet: AllowBuildPolicy::check/check_with_context take the dep
path; the git-fetcher closure shape becomes (dep_path, trust).
Contributor
|
| Branch | pr/12294 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 5,088.83 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 695.38 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 678.47 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 5,435.97 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 779.24 ms |
|
Code review by qodo was updated up to the latest commit 1f896a9 |
matches() now compares pre-normalized pkgIdWithPatchHash values: the user-written depPath spec is stripped of its peer suffix when the selector is constructed, and the lockfile key when the candidate is derived, so the matcher itself is plain equality. The suffixed key still flows out of findPackages — its return value keys the lockfile entries and virtual-store lookups.
|
Code review by qodo was updated up to the latest commit af4e687 |
The fallback in _rebuild (findLinkedGvsDir) exists because the GVS projection hash includes the allowBuilds verdict: an install that deferred a build creates the projection under the not-built hash, and a later rebuild running under the granting policy recomputes the built hash, which points at a directory the install never created. The new test proves the flow: it fails when the fallback is replaced with the plain hash join. Also type allowBuildKeyFromIgnoredBuild's parameter as DepPath — ignoredBuilds is Set<DepPath>, so the brand flows in without casts and the peer suffix is stripped with getPkgIdWithPatchHash.
|
Code review by qodo was updated up to the latest commit 3478af8 |
…ords recordLockfileVerified (and pacquet's record_lockfile_verified) write the verification cache record after a fresh resolve, but did so without the resolutionShapeCheck flag, so the next offline no-op install missed the cache and re-contacted the registry. Both recorders now share the gate's withResolutionShapeCacheIdentity helper; the freshly-resolved lockfile satisfies the shape invariant by construction. Also migrate the dlx cache e2e to approve the git-hosted shx by its depPath, like the other dlx tests in this PR.
|
Code review by qodo was updated up to the latest commit 41f0b16 |
The relocation design lives in #12302.
|
Code review by qodo was updated up to the latest commit eca97f8 |
…solvers Two holes in the structural lockfile-shape pass: - gitHosted was trusted as a strict `=== true`, so a tampered lockfile could set a non-boolean (e.g. 'true') or an explicit `false` on a git-host URL to make a git-hosted tarball look registry-shaped under a name@semver key. The check now treats any non-boolean flag as git-hosted and falls back to isGitHostedTarballUrl, so the verdict never depends on the flag alone. pacquet gates on the URL too (is_git_hosted_tarball_url made public). - custom resolver protocols (type: 'custom:*') are a legitimate non-registry source under a semver key and were wrongly rejected with ERR_PNPM_RESOLUTION_SHAPE_MISMATCH. They are exempt: an unrecognized custom type can only be materialized by a project-configured custom fetcher and otherwise throws at fetch time, so the exemption cannot launder an artifact into a build. pacquet has no custom-resolution variant, so only the TS gate needs the carve-out.
|
Code review by qodo was updated up to the latest commit f415118 |
- isRegistryShapedResolution / is_registry_shaped_resolution now require a present tarball URL to be http(s): the npm verifier's tarball-URL binding skips non-http(s) schemes, so a file: (or other) tarball under a name@semver key would otherwise be trusted with no safety net. Absent / empty tarball stays registry-shaped (URL reconstructed from name+version). - normalizeGitRepoForPkgResolutionId short-circuits when the repo already carries a URL scheme, so a fully-qualified ssh://user@host:port/path is no longer mangled into ssh://ssh://... by the scp-shorthand rewrite. pacquet's create_git_hosted_pkg_id has no scp regex, so it isn't affected. Also add a regression test for the artifact-approval revocation path: a tarball dep approved by depPath, then reinstalled with the approval removed, must surface as an ignored build (covers the snapshot-name/version revocation walk that reaches non-semver artifact depPaths).
|
Code review by qodo was updated up to the latest commit f38b7bd |
isGitHostedTarballUrl / is_git_hosted_tarball_url matched lowercase host prefixes only, so a tampered lockfile could uppercase the host (https://CODELOAD.GITHUB.COM/...) and set gitHosted: false to slip a git-hosted tarball past the resolution-shape gate as a registry-trusted identity under a name@semver key. Both helpers now lowercase the URL before the scheme/host/tar.gz checks (detection only; the URL is never rewritten). Also dedupe the lockfile.fs copy of the helper, which already depended on @pnpm/lockfile.utils, so there's a single hardened implementation.
|
Code review by qodo was updated up to the latest commit 13ef6c8 |
zkochan
added a commit
that referenced
this pull request
Jun 10, 2026
Port of #12294 (commit bf1b731) to release/10. Package-name entries in onlyBuiltDependencies (and allowBuilds) no longer approve lifecycle scripts for artifacts whose identity a name cannot pin: git, git-hosted tarball, direct tarball, and local directory dependencies. To approve such an artifact explicitly, use its peer-suffix-free lockfile depPath as the key — the GIT_DEP_PREPARE_NOT_ALLOWED hint, pnpm ignored-builds, and pnpm approve-builds print exactly that key. - AllowBuild policy functions identify packages by DepPath instead of caller-supplied name/version. Identity trust is derived from the depPath shape: a registry-style depPath (name@semver) is a trusted identity. - The trust is sound because lockfile entries are structurally checked wherever they are materialized into fetchable resolutions (pkgSnapshotToResolution) and before rebuild runs scripts: a registry-style key backed by a git, directory, or git-hosted tarball resolution is rejected with ERR_PNPM_RESOLUTION_SHAPE_MISMATCH. - preparePackage requires a pkgResolutionId and gates on the synthesized name@<resolution id> depPath; scp-style git URLs are normalized to ssh:// form in resolution ids and the git fetcher reuses createGitHostedPkgId from the resolver. - isGitHostedTarballUrl matches case-insensitively and is shared from @pnpm/lockfile.utils; new removePeersSuffix() in @pnpm/dependency-path and allowBuildKeyFromIgnoredBuild() in @pnpm/builder.policy. - pnpm rebuild and approve-builds accept depPath specs for selecting and approving artifact builds; installs rebuild ignored builds approved by depPath keys. - shell-quote is overridden to 1.8.4 (GHSA-w7jw-789q-3m8p / CVE-2026-9277). Differences from main: v10 has no lockfile resolution verifier, so the structural pass lives in pkgSnapshotToResolution and the rebuild loop; the AllowBuild policy keeps v10's boolean return; the revoked-approval detection and global-virtual-store hash changes have no v10 counterpart.
Member
Author
|
🚢 v11.5.3 |
zkochan
added a commit
that referenced
this pull request
Jun 10, 2026
* fix(security): harden build-approval artifact identities on v10 Port of #12294 (commit bf1b731) to release/10. Package-name entries in onlyBuiltDependencies (and allowBuilds) no longer approve lifecycle scripts for artifacts whose identity a name cannot pin: git, git-hosted tarball, direct tarball, and local directory dependencies. To approve such an artifact explicitly, use its peer-suffix-free lockfile depPath as the key — the GIT_DEP_PREPARE_NOT_ALLOWED hint, pnpm ignored-builds, and pnpm approve-builds print exactly that key. - AllowBuild policy functions identify packages by DepPath instead of caller-supplied name/version. Identity trust is derived from the depPath shape: a registry-style depPath (name@semver) is a trusted identity. - The trust is sound because lockfile entries are structurally checked wherever they are materialized into fetchable resolutions (pkgSnapshotToResolution) and before rebuild runs scripts: a registry-style key backed by a git, directory, or git-hosted tarball resolution is rejected with ERR_PNPM_RESOLUTION_SHAPE_MISMATCH. - preparePackage requires a pkgResolutionId and gates on the synthesized name@<resolution id> depPath; scp-style git URLs are normalized to ssh:// form in resolution ids and the git fetcher reuses createGitHostedPkgId from the resolver. - isGitHostedTarballUrl matches case-insensitively and is shared from @pnpm/lockfile.utils; new removePeersSuffix() in @pnpm/dependency-path and allowBuildKeyFromIgnoredBuild() in @pnpm/builder.policy. - pnpm rebuild and approve-builds accept depPath specs for selecting and approving artifact builds; installs rebuild ignored builds approved by depPath keys. - shell-quote is overridden to 1.8.4 (GHSA-w7jw-789q-3m8p / CVE-2026-9277). Differences from main: v10 has no lockfile resolution verifier, so the structural pass lives in pkgSnapshotToResolution and the rebuild loop; the AllowBuild policy keeps v10's boolean return; the revoked-approval detection and global-virtual-store hash changes have no v10 counterpart. * test: serve the direct-tarball fixture from an off-registry origin The registry mock binds only localhost, so a hard-coded 127.0.0.1 URL is refused in CI, and a tarball URL on the registry's own origin resolves as a registry package, which defeats the direct-tarball identity the test needs. An in-test HTTP server redirecting to the mock provides a separate origin whose binding the test controls. * test: approve the git-protocol licenses fixture by its depPath A git-hosted artifact has an untrusted package identity, so the ajv-keywords build has to be approved by its depPath. Pin the fixture to the commit the depPath names.
KSXGitHub
pushed a commit
that referenced
this pull request
Jun 10, 2026
Integrate the 9 commits main gained (#12271, #12294, #12301, #12303, #12305, #12312, #12315, #12316, and the release/version bumps). Conflict resolution: all four conflicts (record_lockfile_verified, build_modules, hoisted_dep_graph, install) were between this branch's lint edits and main's feature changes — took main's authoritative versions; lint compliance is re-derived by re-running clippy in the follow-up commit.
zkochan
added a commit
to vsumner/pnpm
that referenced
this pull request
Jun 11, 2026
… backstop The AllowBuild signature changed to (depPath, context?) on main (pnpm#12294); the frozen-store blocking predicate still passed (name, version).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Package-name
allowBuildsentries no longer approve lifecycle scripts for artifacts whose identity a name cannot pin: git, git-hosted tarball, direct tarball, and local directory dependencies. To approve such an artifact explicitly, use its peer-suffix-free lockfile depPath as theallowBuildskey — error hints,pnpm ignored-builds, andpnpm approve-buildsprint exactly that key.AllowBuildpolicy functions identify packages byDepPathinstead of caller-supplied name/version. The policy parses name and version out of the depPath itself, so name-keyed rules can never be fed an identity that disagrees with the gated artifact.AllowBuildContextcarries only an explicittrustPackageIdentityoverride, used to evaluate a previously recorded policy under its original semantics when detecting revoked approvals.name@semver) is a trusted identity. This is sound because lockfile verification runs a new unconditional, offline structural pass that rejects lockfiles where such a key is backed by a git, directory, or git-hosted tarball resolution (ERR_PNPM_RESOLUTION_SHAPE_MISMATCH), while the npm resolution verifier already binds explicit tarball URLs of semver-keyed entries to the registry's owndist.tarballunconditionally. The pass runs inside the existing candidate walk and participates in the verification cache key (resolutionShapeCheck) on both the gate's and the fresh-resolve record paths, so the stat-only cache fast path stays sound and records written before the rule existed are re-verified.preparePackagealways treats the fetched manifest as an untrusted identity: it requires apkgResolutionIdand gates on the synthesizedname@<resolution id>depPath. scp-style git URLs are normalized tossh://form in resolution ids, and the git fetcher reusescreateGitHostedPkgIdfrom the resolver instead of re-deriving ids.pnpm rebuildlocates a projection created before the approval was granted by following the project's node_modules link, since the projection hash includes the allowBuilds verdict (relocating the projection instead is tracked in rebuild: relocate global virtual store projections instead of building in place #12302).removePeersSuffix()in@pnpm/deps.path(string-level peer-suffix stripping for user-written keys) andallowBuildKeyFromIgnoredBuild()in@pnpm/building.policy(the key under which an ignored build is approved).AllowBuildPolicy::check(dep_path)derives trust from the dep path, the git-fetcher allow-build closures take only the dep path,pacquet-lockfile-verificationgains the same structural pass, error code, and cache identity, and dep-path keys normalize viaremove_suffix.shell-quoteis overridden to 1.8.4 (GHSA-w7jw-789q-3m8p / CVE-2026-9277).Validation
Written by agents (Codex, GPT-5 and Claude Code, claude-fable-5).
Summary by CodeRabbit
New Features
Bug Fixes