rfc(pnpr): integrity-addressed patch revisions - #19
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:
📝 WalkthroughWalkthroughThe PR adds two RFCs for immutable, registry-scoped digest tarballs and revision metadata. They define provider ingestion, projected revision history, pnpm lockfile and fetch behavior, revision selection, compatibility checks, refresh operations, and audit rules. ChangesIntegrity-addressed revisions
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant pnprRegistry
participant RegistryPackument
participant pnpmResolver
participant Lockfile
Provider->>pnprRegistry: submit signed artifact revision
pnprRegistry->>RegistryPackument: publish verified revision metadata
pnpmResolver->>RegistryPackument: resolve name@version and revision
pnpmResolver->>Lockfile: record integrity and revision
pnpmResolver->>pnprRegistry: fetch registry-scoped digest URL
pnprRegistry-->>pnpmResolver: return immutable tarball bytes
pnpmResolver->>Lockfile: verify SRI and install selected revision
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
PR Summary by QodoRFC: Integrity-addressed patch revisions and pnpm fetching
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@pnpr/text/0000-integrity-addressed-patch-revisions.md`:
- Around line 181-204: The “Provider input remains integrity-pinned”
specification must define anti-rollback enforcement for signed patch manifests.
Add a persisted, signed monotonic checkpoint for each provider identity and
require pnpr to reject manifests selecting revisions older than the checkpoint;
document an explicit, authorized rollback mechanism and its verification
requirements.
- Around line 491-505: Update the provider refresh sequence to make `(registry,
name@version, digest)` revision allocation unique and idempotent: claim or reuse
an existing digest mapping within a transaction/unique constraint before
assigning an ordinal, and append revision metadata plus select the mapping
atomically. Ensure concurrent refreshes and retries reuse the same stable
ordinal rather than allocating duplicates.
- Around line 282-292: Update the public CDN caching and policy sections
describing successful immutable responses, including the related sections on
revocation and cache behavior, to define authorization-aware edge enforcement
and purge/invalidation when a revision is blocked. If immediate revocation on
warm immutable hits cannot be guaranteed, weaken the policy wording to state the
actual propagation behavior instead of promising an immediate 403.
- Around line 157-171: Clarify the immutability and access-policy rules in the
RFC sections covering tarball URLs, historical artifact screening, and legacy
lockfiles: define immutability as successful responses returning unchanged
bytes, specify whether vulnerability screening applies to canonical URLs as well
as integrity URLs, and document the response legacy lockfiles receive when
access is denied.
In `@text/0000-integrity-addressed-registry-tarballs.md`:
- Around line 207-209: Restrict the registry-base portability guarantee and its
tests to hostname aliases or deployment moves of the same logical registry.
Update the digest URL resolution and hydration behavior described in the
registry portability section, including the identity gap noted around the
lockfile discussion, so changing to an unrelated configured registry is not
treated as equivalent; alternatively persist and validate registry identity
before hydration.
- Around line 312-316: Define the lockfile version gate for revision-aware
integrity and the `?vN` representation, including when pnpm emits it and how
hydrated lockfile parsing rejects it in older clients. Update the affected
proposal sections consistently and add a compatibility test verifying older
clients fail fast on the gated lockfile rather than reconstructing the canonical
`dist.tarball` URL.
🪄 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: 39f74957-4168-4ccf-809c-ebc7cea26467
📒 Files selected for processing (2)
pnpr/text/0000-integrity-addressed-patch-revisions.mdtext/0000-integrity-addressed-registry-tarballs.md
📜 Review details
🧰 Additional context used
🪛 LanguageTool
pnpr/text/0000-integrity-addressed-patch-revisions.md
[grammar] ~465-~465: Ensure spelling is correct
Context: ...is longer by one sha512 digest. Patched packuments contain a small revision list. Both cos...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~481-~481: This phrase is redundant. Consider writing “same”.
Context: ...ked registry package, pnpm resolves the same exact name@version. If dist.integrity cha...
(SAME_EXACT)
[grammar] ~506-~506: Ensure spelling is correct
Context: ...validate projected full and abbreviated packuments. No tarball cache object is invalidated o...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~518-~518: To elevate your writing, try using an alternative expression here.
Context: ... returns a policy-specific 403. This matters for a vulnerable original. A reproducib...
(MATTERS_RELEVANT)
text/0000-integrity-addressed-registry-tarballs.md
[grammar] ~145-~145: Use a hyphen to join words.
Context: ...egrity`; 4. the digest is complete, well formed, and unambiguous; 5. the selected...
(QB_NEW_EN_HYPHEN)
[style] ~349-~349: This phrase is redundant. Consider writing “same”.
Context: ... pnpm resolves current metadata for the same exact name@version. If dist.integrity cha...
(SAME_EXACT)
🔇 Additional comments (2)
pnpr/text/0000-integrity-addressed-patch-revisions.md (1)
1-156: LGTM!Also applies to: 172-180, 205-281, 293-408, 428-449, 470-490, 523-690, 691-725
text/0000-integrity-addressed-registry-tarballs.md (1)
1-206: LGTM!Also applies to: 210-266, 278-311, 318-475, 477-499
Replace the empty-option/`?vN` markers with explicit `?r0`/`?rN` ordinals: `r` abbreviates *registry revision*, matches `dist.revisions` numbering, and follows Alpine/Gentoo `-rN` precedent, while an always-non-empty option removes the raw-SRI-parsing fragility. Specify workspace revision selection through ordinary overrides whose targets carry the ordinal as semver build metadata (`<version>+rN`): `+r0` pins the original, positive ordinals adopt or freeze replacements, unknown ordinals fail hard, and registry policy 403s still win. Require each `dist.revisions` entry to carry a `manifest` object with the abbreviated-packument field subset, derived from the verified tarball and never provider-declared, so a pinned non-selected revision resolves with its own dependency metadata. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pnpr/text/0000-integrity-addressed-patch-revisions.md (1)
328-332: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine revision-reference equality separately from artifact equality.
Line 328 says the
?rNoption is excluded from equality, while selection still depends on the digest plus ordinal anddist.revisions[]advertises each ordinal as a distinct artifact. Document that canonical digest equality ignores options, but resolving or checking?rNmust match the canonicaldist.revisions[].revisionfor that integrity, and reject or canonicalize forged mismatched ordinals.🤖 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 `@pnpr/text/0000-integrity-addressed-patch-revisions.md` around lines 328 - 332, Clarify the revision semantics in the document: canonical artifact digest equality must ignore the ?rN option, while resolving or validating a revision reference must match the canonical dist.revisions[].revision for the corresponding integrity digest. Specify that forged or inconsistent ordinals are rejected or canonicalized, and keep option-only changes from creating new stored artifacts.
🤖 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 `@pnpr/text/0000-integrity-addressed-patch-revisions.md`:
- Line 718: Resolve the ordinal contract before retaining the conformance
requirement: explicitly require stable monotonic rN ordinals across provider
changes, or define the migration behavior that applies when providers change.
Update the surrounding unresolved questions and the conformance statement for
the first replacement so they consistently reflect the chosen rule.
- Around line 509-514: Update the revision-override handling for +rN selections
to persist the addressed registry together with the package identity and pinned
ordinal. When resolving or applying an override, require the stored registry and
name@version to match the current package before using the revision; ensure pnpm
update --patches recognizes these workspace overrides as pinned and does not
refresh them.
In `@text/0000-integrity-addressed-registry-tarballs.md`:
- Around line 149-155: Update the integrity-validation wording to compare the
decoded URL digest with the decoded payload from dist.integrity after removing
its canonical rN option, while retaining the sha512- algorithm prefix handling.
Clarify that literal comparison against the full integrity string is invalid
because the URL contains only the digest.
---
Outside diff comments:
In `@pnpr/text/0000-integrity-addressed-patch-revisions.md`:
- Around line 328-332: Clarify the revision semantics in the document: canonical
artifact digest equality must ignore the ?rN option, while resolving or
validating a revision reference must match the canonical
dist.revisions[].revision for the corresponding integrity digest. Specify that
forged or inconsistent ordinals are rejected or canonicalized, and keep
option-only changes from creating new stored artifacts.
🪄 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: d0504695-6831-4e86-b826-b5927fe1d04b
📒 Files selected for processing (2)
pnpr/text/0000-integrity-addressed-patch-revisions.mdtext/0000-integrity-addressed-registry-tarballs.md
📜 Review details
🧰 Additional context used
🪛 LanguageTool
pnpr/text/0000-integrity-addressed-patch-revisions.md
[style] ~648-~648: Consider an alternative for the overused word “exactly”.
Context: ...ements. Rejected: - an empty option is exactly the state a generic SRI library is most...
(EXACTLY_PRECISELY)
text/0000-integrity-addressed-registry-tarballs.md
[style] ~596-~596: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...n policy for pnpm update --patches? - What should the global posture setting that ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (2)
pnpr/text/0000-integrity-addressed-patch-revisions.md (1)
19-21: LGTM!Also applies to: 24-27, 31-33, 94-96, 118-123, 146-147, 164-166, 266-267, 308-327, 334-337, 357-388, 409-424, 458-462, 643-659, 705-706, 716-717, 719-724, 766-767
text/0000-integrity-addressed-registry-tarballs.md (1)
367-372: 🗄️ Data Integrity & IntegrationSpecify verification of revision manifests before using them for resolution.
dist.revisions[N].manifestbecomes authoritative for dependencies, peers, engines, and install scripts, but the fetch steps only verify tarball bytes against the digest. Explicitly require the companion manifest-verification contract—including its binding to the selected name, version, revision, and digest—and fail before subtree resolution when that metadata is stale or mismatched.Also applies to: 546-548
Exact-version override selectors already match intersecting declared ranges in current pnpm (pnpm/pnpm#13470, retained for npm parity), so no resolver semantics change is needed. <version>+rN becomes a plain revision-addressed spec — usable as an override target or a directly declared dependency — resolved by picking the version normally, then selecting the revision from dist.revisions. State the version-pinning trade-off and the npm degradation (pin without revision selection) honestly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Provider manifests carry a signed monotonic sequence; pnpr rejects replayed lower-sequence documents, so selection rollback requires a fresh manifest or an explicitly authorized operator action. - Revision allocation is transactional: (registry, name, version, digest) maps to exactly one ordinal, idempotent under retries. - Immutability means stable bytes, not availability: policy refusal requires edge purge or non-shared caching, applies identically to canonical and digest routes, and the digest route authorizes against the principal's package-level access. - Ordinals are documented as registry-scoped; portability guarantees integrity, not identical ordinal meaning across registries. - Override rewriting applies once (no chains or cycles), and conflicting revision demands for one name@version fail explicitly. - deprecated is registry-managed mutable metadata, excluded from the artifact-derived revision manifest subset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Allocation serializes on (registry, name, version) with separate uniqueness constraints on the ordinal and the digest mapping, so concurrent refreshes with different digests cannot share an ordinal. - The registry owns the ordinal history: provider renames, replacements, or removals never renumber existing revisions (previously an open question, now normative since rN pins are a public API). - The provider sequence checkpoint advances atomically with durable manifest acceptance, keeping a crashed sequence retryable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ordinal moves out of the integrity value: dist.revision on the registry side and resolution.revision in the lockfile, present only for replacements (N >= 1). Originals are never marked — the canonical URL is pinned to revision zero by invariant, so plain integrity-only entries stay correct on every registry, a lockfile with no adopted patches is byte-identical to today's format, and only revision entries need the lockfile-version gate. dist.integrity returns to a standard SRI value that strict consumers parse. Registry feature detection (the pnpr handshake) is specified as advisory only — diagnostics and an optional originals-by-digest optimization — never load-bearing for the fetch convention, which the lockfile field alone determines. Digest-route tarballs without dist.revision are normalized to integrity-only entries so hostnames never enter the lockfile. Document that npm/yarn patch delivery rides dist.tarball (unchanged by canonical immutability), that convention-reconstructing clients fail loudly rather than silently, and add a client-matrix verification question. The SRI-option and mark-everything designs move to rejected alternatives; +rN spec syntax is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pnpr/text/0000-integrity-addressed-patch-revisions.md (1)
203-218: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftConstrain provider artifact fetches before download.
entries.*.tarballis an arbitrary server-side URL, but the RFC does not restrict its origin, redirects, DNS rebinding, or private-network targets. A signed or compromised provider manifest could therefore turn refresh into SSRF. Require HTTPS provider-origin allowlists, private-IP/egress checks, redirect validation, and ensure provider credentials are never forwarded to artifact hosts.🤖 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 `@pnpr/text/0000-integrity-addressed-patch-revisions.md` around lines 203 - 218, The artifact download flow described for manifest entries.*.tarball must enforce SSRF protections before fetching: require HTTPS URLs from an approved provider-origin allowlist, reject private or otherwise disallowed resolved addresses and egress targets, validate every redirect against the same rules, and never forward provider credentials to artifact hosts. Apply these checks before download and preserve complete integrity verification afterward.
🤖 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 `@pnpr/text/0000-integrity-addressed-patch-revisions.md`:
- Around line 438-443: Update the abbreviated historical metadata specification
to retain each history entry’s integer revision ordinal alongside its integrity,
digest URL, and manifest fields. Ensure +rN resolution can map N to the correct
historical record, and add a focused test covering revision ordinals in
abbreviated packuments.
- Around line 438-443: Define a transactional carry-forward/merge rule for
revision swaps so projected version entries preserve registry-managed mutable
fields, including deprecated, when refreshed. Update the revision
refresh/projection process described in the metadata rules, keeping immutable
historical provenance separate while merging existing mutable metadata into the
replacement entry.
- Around line 220-230: Update the per-provider sequence checkpoint logic to
persist both the highest accepted sequence and its canonical manifest digest.
Treat an exact replay of the same sequence and digest as an idempotent no-op,
but reject any manifest using the same sequence with a different digest as
equivocation; retain advancement only for higher sequences and keep checkpoint
updates atomic with durable manifest acceptance.
- Around line 334-346: Define and document a single validation policy for
dist.revision across registry and client validation, covering absent,
non-integer, out-of-range, and mismatched values. Ensure full and abbreviated
packuments apply the same policy, and explicitly reconcile malformed-value
rejection with structured revision metadata being authoritative; update the
relevant test matrix and rules accordingly.
In `@text/0000-integrity-addressed-registry-tarballs.md`:
- Around line 376-381: Make revision refresh graph-transactional: when updating
the resolved entry’s revision and manifest, re-resolve the entire affected
dependency graph using the refreshed revision metadata, including dependency and
peer edges, engines, binaries, and install-script state. Commit all graph,
snapshot, and peer-resolution changes atomically, and roll back the refresh if
re-resolution produces conflicts. Add coverage for a revision whose dependency
metadata changes, including the corresponding lines around the revision refresh
logic.
- Around line 207-225: Update the revisioned digest tarball fetch requirements
at this section and the corresponding sections around lines 612 and 642 to
reject every 3xx response as a hard failure, not only redirects crossing
origins. Preserve the single authenticated GET behavior and prohibit following
same-origin or cross-origin redirects.
---
Outside diff comments:
In `@pnpr/text/0000-integrity-addressed-patch-revisions.md`:
- Around line 203-218: The artifact download flow described for manifest
entries.*.tarball must enforce SSRF protections before fetching: require HTTPS
URLs from an approved provider-origin allowlist, reject private or otherwise
disallowed resolved addresses and egress targets, validate every redirect
against the same rules, and never forward provider credentials to artifact
hosts. Apply these checks before download and preserve complete integrity
verification afterward.
🪄 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: 92882297-ba60-41e2-9dfc-8fc8fe0a3e60
📒 Files selected for processing (2)
pnpr/text/0000-integrity-addressed-patch-revisions.mdtext/0000-integrity-addressed-registry-tarballs.md
📜 Review details
🧰 Additional context used
🪛 LanguageTool
text/0000-integrity-addressed-registry-tarballs.md
[grammar] ~137-~137: Use a hyphen to join words.
Context: ...egrity; 4. the digest is complete, well formed, and unambiguous; 5. dist.revisi...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (6)
pnpr/text/0000-integrity-addressed-patch-revisions.md (2)
1-14: LGTM!Also applies to: 26-58, 62-138, 141-192, 232-263, 265-283, 290-333, 347-410, 424-437, 445-505, 507-552, 554-604, 606-735, 737-758, 775-796, 798-835
15-22: 🗄️ Data Integrity & IntegrationManual verification needed for revision selection rules.
The excerpt raises a plausible concern about inconsistent keying between
dist.tarballencoding, multiple SRI hash expressions, missingdist.revision, and absent revision handling, but the draft context is needed before ruling this as actionable.text/0000-integrity-addressed-registry-tarballs.md (4)
130-138: Compare the URL digest with the SRI payload, not the full SRI token.
dist.integritycontainssha512-<base64>, while the URL contains only digest bytes. Define validation as decoding the SRI payload after removing the algorithm prefix and comparing those bytes with the decoded base64url path digest.
315-319: Specify the concrete lockfile gate.The RFC requires revision fields to be gated but still leaves the lockfile version and emission/rejection behavior unresolved. Define the exact version, writer behavior, hydrated-lockfile rejection, and mixed-version compatibility matrix.
Also applies to: 606-608, 676-678
151-161: 🗄️ Data Integrity & Integration | ⚡ Quick winDefine the ordinal’s numeric bounds and canonical encoding.
“Positive integer” is insufficient for a JSON/JavaScript protocol: without a safe upper bound or canonical decimal-string representation, large ordinals can round or compare inconsistently and break revision selection. Add overflow, leading-zero, and maximum-value tests.
Also applies to: 636-637
357-368: 🎯 Functional CorrectnessState that
+rNoverrides need new override behavior.The linked pnpm issue is not publicly available as a stable contract here, so this should not present pnpm’s existing override behavior as sufficient if the feature allows a version-pinning override to force a selected version regardless of the declared range.
|
pnpm might start supporting vendors like echo, that upload multiple releases that resolve to the same npm version (but differ in tarball and integrity). a feature to consider is the ability to refresh a lockfile to get the newest release while keeping the same version. Since a query of the version would only return the release the registry exposes there is no decision logic to be made by pnpr/pnpm, it should probably only query each version in the lockfile directly and replace the entry if it's different. pnpm has a feature that does something similar (npm install --fix-lockfile --lockfile-only), but what could be added is restriction to only update the hash if it belongs to a pre configured vendor etc |
| Versions such as `2.7.4-echo.2` are prereleases under semver. Ordinary ranges | ||
| containing `2.7.4` do not necessarily select them, peer ranges may reject them, | ||
| and the package no longer reports the version the application was tested | ||
| against. Build metadata is unsuitable because semver ignores it for precedence |
There was a problem hiding this comment.
There's a possibility of using build metadata. npm clients will ignore it, thereby seeing it as just the regular version, but the registry itself can be organized to serve the newest revision whenever its published. This (mostly) will not break any old lockfile as long as the urls of different versions are different (you can encode them with pkg-version+vendor.N.tgz for example). If the old urls stay on the registry users can always get the latest version when they ask for it, and they will not break if they're lockfile is out of date.
There was a problem hiding this comment.
Right, we do leverage the build metadata in this rfc for explicitly specifying which revision to install via overrides. For the tarball URL I have suggested to use the integrity checksum of the tarball. Alternatively we could consider adding the revision number to the url as you suggest.
There was a problem hiding this comment.
Added this to the alternatives as Encode the revision number in the tarball
URL, since it deserved a written comparison rather than a passing mention.
The scheme shares the append-only intent, and the readability/proxy-log
argument is real. What decided it for the fetch convention is that such a URL
only names the bytes — it doesn't commit to them. Immutability becomes a
policy the registry has to honor rather than something the client can verify:
nothing in the request detects a wrong body until integrity verification of a
completed download. With the digest route the cache key is the content itself,
pnpm constructs the URL from registry plus the integrity it already stores,
and "never fall back to another URL" holds by construction.
A registry can still expose a per-revision readable route as an alias for
humans and non-pnpm tooling — that doesn't conflict with anything here. The
convention pnpm relies on stays the digest URL.
sure, such feature would make sense. The current architecture would make it slow though. We'd have to read the whole package document for every package found in the lockfile. I wonder if there is a better way. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The SRI-option rejection asserted that strict consumers may reject options. The real failure is worse: pnpm's Rust ssri has no option grammar, so it absorbs the option into the digest, round-trips it through Display, and fails far from the parse. Integrity-string equality is also byte equality across pnpm, so an ordinal inside the value would make a revision-only difference read as a content change. Record encoding the ordinal in the lockfile package key as an alternative, since the registry-qualified keys of pnpm/pnpm#13528 invite it. It is rejected on churn: a dep path repeats once per dependent, so one revision bump would rewrite roughly fifty lines for a package as common as semver, and patchedDependencies already demonstrates this with patch_hash. The key may carry extra data; it may not carry data designed to change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What if we use a Get package/version request that only fetches the manifest of that version? Probably impossible to do better but will more efficient than fetching entire package documents |
|
pnpr has server side resolution, so I think it can do it fast. The npm registry has an audit endpoint which detects vulnerabilities. Maybe something similar could be added for this in the future. I think it wouldn't conflict with the design described in this PR. I think for the initial install having this info in the package document is good. |
|
Really excited about this - it's slipstream patching: the patched build |
| additionally carry all provenance fields. Abbreviated metadata should keep | ||
| current `dist` plus the integrity, digest URL, and `manifest` of historical | ||
| revisions, so revision pinning works from the metadata pnpm actually | ||
| fetches. An unpatched version does not need a `dist.revisions` array or a |
There was a problem hiding this comment.
The example has revision on each entry; the requirement sentence doesn't
ask for it, and +rN resolves against the abbreviated packument.
| additionally carry all provenance fields. Abbreviated metadata should keep | |
| current `dist` plus the integrity, digest URL, and `manifest` of historical | |
| revisions, so revision pinning works from the metadata pnpm actually | |
| fetches. An unpatched version does not need a `dist.revisions` array or a | |
| additionally carry all provenance fields. Abbreviated metadata should keep | |
| current `dist` plus the `revision` ordinal, integrity, digest URL, and | |
| `manifest` of each historical revision, so revision pinning works from the | |
| metadata pnpm actually fetches. An unpatched version does not need a | |
| `dist.revisions` array or a |
There was a problem hiding this comment.
Applied. The requirement now reads:
Abbreviated metadata should keep current
distplus therevisionnumber,
integrity, digest URL, andmanifestof each historical revision —+rN
resolution maps a number to its history record using the metadata pnpm
actually fetches, so the number must survive abbreviation.
Also added a test bullet for the number being present in the abbreviated form.
| those package-level rules; "the registry references this digest" alone is | ||
| not sufficient. pnpr may deduplicate physical storage globally, but it must | ||
| not expose an object merely because another organization or registry stored | ||
| the same digest. Knowledge of a digest is not a bearer credential. |
There was a problem hiding this comment.
What's a proxy in the middle supposed to do?
Today anything sitting between the client and the registry - a corporate
proxy, a mirror, a registry firewall - can tell which package a request is
for just by reading the URL: lodash/-/lodash-4.17.21.tgz. That's what those
layers use to enforce policy, cache, and keep audit logs.
/-/tarballs/sha512/<digest> tells them nothing, on purpose (dedup, provider
neutrality - fair). But the doc doesn't say what that layer should do
instead. Two things worth spelling out:
- "effective registry" when a proxy is in front. Revision numbers are
scoped per registry and credentials are scoped same-origin, and both get
ambiguous when the origin the client sees is the proxy, not the registry. - How to map a digest back to a package. Presumably
dist.revisionsis
the lookup - and the answer can be plural, since dedup means one digest can
back severalname@versions.
Without this, every proxy invents its own heuristic.
There was a problem hiding this comment.
Good catch — the RFC took the package name out of the URL without saying what
the layers that relied on it should do instead. Added a section,
Intermediaries and the digest route, spelling out both points you raised:
- Effective registry. When a proxy fronts pnpr, the proxy's base URL is
the registry from the client's point of view: revision numbers are scoped to
that base and credentials are scoped same-origin to it. A proxy projecting
several upstream registries has to present them as distinct bases (distinct
path prefixes, say), because one base implies one numbering and one
credential scope. - Digest → package.
dist.revisionsis the mapping, and the answer may be
plural, since dedup lets one digest back severalname@versions. A pnpr
deployment should expose that reverse lookup to authorized policy and audit
tooling.
| against the base version. An integrity-aware pnpm audit extension can identify | ||
| the installed revision and subtract only fixes declared for those exact bytes. | ||
| A later provider revision does not retroactively add fixes to an older locked | ||
| revision. |
There was a problem hiding this comment.
How does audit tell "r2 replaces r1" from "r2 sits alongside r1"?
Both are legal here (different fixes sets), and revision numbers only tell
you the order - not whether an old one is dead. The withdrawal state the doc
defers is cheapest to add now - a per-entry withdrawn/supersededBy fits
append-only fine.
There was a problem hiding this comment.
Agreed, and cheaper to add now than to retrofit. Revision entries may carry
optional withdrawn and supersededBy fields: supersededBy records which
revision replaced a retired one, and two live revisions with disjoint fixes
sets simply coexist with neither set.
This is consistent with append-only — the fields describe the record's
standing, never its artifact, so annotating one touches no artifact-derived
field, byte, or number, and nothing is removed or renumbered. The RFC now says
that explicitly, since "append-only" and "mutable status field" read as a
contradiction otherwise. The audit section notes that the number alone never
distinguishes the two cases; there's also a test for annotating status without
renumbering.
| resolution and validate the pair (origin, algorithm, digest equality, | ||
| positive integer ordinal). | ||
| 2. Record `revision: N` in the resolution object and gate the field with an | ||
| appropriate lockfile version; lockfiles without the field keep the current |
There was a problem hiding this comment.
When resolution.revision ships, please land it in @pnpm/lockfile-types +
docs in the same release. Lots of tools parse pnpm-lock.yaml; a typed field
on day one beats everyone reverse-engineering it.
There was a problem hiding this comment.
Agreed — added to the implementation steps: ship the field's type in
@pnpm/lockfile-types and document it in the same release that first emits it,
for exactly the reason you give.
👍 - this can come later without conflicting with the design here. A shape Audit is the strongest reason to build it: advisories are keyed on |
|
|
||
| Rules: | ||
|
|
||
| - `N` is a positive base-10 integer; |
There was a problem hiding this comment.
It might be worth specifying that 0 < N <= MAX_SAFE_INTEGER. I'd assume this is the case but, because registries might not be implemented in JavaScript, it'd be worth defining this precisely.
There was a problem hiding this comment.
Good point — "positive integer" isn't a spec for a protocol that
non-JavaScript registries implement. The revision field now requires a
canonical base-10 integer, no leading zeros, sign, or exponent, bounded by
0 < N ≤ 2^53 − 1. Registries must not allocate outside the range, and
clients reject values outside it. The malformed-value test now covers leading
zeros and above-range values alongside zero, negative, and non-integer.
| naming in resolution metadata. It couples lockfiles to a provider and | ||
| duplicates structured registry metadata, and a provider rename would look | ||
| like an artifact change. Neutral integers explain artifact changes without | ||
| making provider identity part of package resolution. |
There was a problem hiding this comment.
Neutral integers make artifact change comprehensible but they carry less meaning at rest.
For projects that end up with multiple registries for a single project (e.g. different registries for different @scopes), it would be useful to pack more information into the revision than a single ordinal can carry. The integrity field doesn't help here, and the registry identify is not present in the lock-file entry.
Additionally, a registry might source revisions from multiple providers, and then interleave those revisions into a single sequence of ordinals. In this scenario, the ordinal still conveys "this revision is presumably newer/better than the previous revision you were using" but otherwise requires sleuthing if someone wants to understand the provenance of a new revision being adopted.
It might be worth considering something ike -[a-zA-Z0-9_]+ as an optional suffix to the ordinal: meaningless to and thrown away by the package manager, but present in the lock-file for human readers.
There was a problem hiding this comment.
Both scenarios you describe are real: interleaved providers behind one
numbering, and multiple registries in one project where the lockfile entry
doesn't record which registry it came from.
I've recorded the suffix idea as an open question rather than adopting it,
because it pulls against the Put provider identity in the revision record
rejection — once the suffix exists, provider naming is back in resolution
metadata for anyone reading a lockfile, and a provider rename shows up as a
diff on entries whose bytes never changed. The counter-argument is that the
package manager throws it away, so it can't affect resolution. That trade
seems worth deciding deliberately rather than in passing.
Worth separating from the registry-identity half of your comment: the lockfile
genuinely can't say which registry an entry resolved from today, and a suffix
would only paper over that for human readers. It's tracked as its own open
question — representing named-registry identity so two registries can serve
the same name@version without colliding. Fixing that properly would give
provenance to tooling as well as to readers.
Co-authored-by: John-David Dalton <jdalton@users.noreply.github.com>
Co-authored-by: John-David Dalton <jdalton@users.noreply.github.com>
Co-authored-by: John-David Dalton <jdalton@users.noreply.github.com>
Co-authored-by: John-David Dalton <jdalton@users.noreply.github.com>
Co-authored-by: John-David Dalton <jdalton@users.noreply.github.com>
Co-authored-by: John-David Dalton <jdalton@users.noreply.github.com>
Review feedback from #19: - sequence checkpoint stores the manifest digest: exact replay is a no-op, same sequence with different content is provider equivocation - one validation rule for dist.revision, identical for full and abbreviated packuments - abbreviated packuments retain each history entry's revision number - new section on intermediaries: effective registry behind a proxy, digest-to-package mapping via dist.revisions - per-entry withdrawn/supersededBy status in revision history - every 3xx fails the digest fetch, same-origin included - revision numbers bounded to 2^53 - 1 in canonical decimal form - ship resolution.revision in @pnpm/lockfile-types with docs - new unresolved questions: batch revision endpoint, human-readable revision annotation suffix Terminology: a registry revision is now defined as the history entry; its integer is its revision number, matching the dist.revision field. The word "ordinal" no longer appears. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 participants
Summary
This design supersedes #14 (closed in its favor). It contains two companion
RFCs:
pnpr/text/0000-integrity-addressed-patch-revisions.mdprojects providerbuilds over the original
name@version. Every artifact is served from animmutable, registry-scoped URL keyed by its complete sha512 digest; the
canonical
name/-/name-version.tgzURL stays pinned to the original bytesforever.
dist.revisionsretains an append-only history of every acceptedartifact — integrity, provider, revision number, VEX/provenance,
withdrawn/supersededBystatus, and each artifact's ownresolution-relevant
manifestfields.text/0000-integrity-addressed-registry-tarballs.mdteaches pnpm thelockfile representation and the
<version>+rNrevision-addressed spec.The revision field
The revision number is a plain integer field, present only for replacements:
Originals are never marked. The canonical URL is pinned to revision zero
by invariant, so integrity-only entries stay correct on every registry —
revision-aware or not — and a lockfile with no adopted patches needs no new
format. Only
revision: Nentries are gated by lockfile version and fetchedfrom the digest route (one direct CDN request; falling back to the canonical
URL is impossible by construction, since original bytes cannot match a
replacement's integrity). Registry feature detection (the pnpr handshake) is
advisory only — better diagnostics, optional originals-by-digest optimization
— never load-bearing: the lockfile field alone determines the fetch
convention.
Explicit revision selection:
<version>+rNThe revision number carried as semver build metadata makes a
revision-addressed spec,
usable as an override target or a directly declared dependency:
{ "pnpm": { "overrides": { "ejs@2.7.4": "2.7.4+r0" } } } // keep the originalNo new override semantics: exact-version selectors already match intersecting
declared ranges (pnpm/pnpm#13470, retained for npm parity). pnpm resolves the
version half normally, then selects revision
Nfromdist.revisions—using that revision's own
manifestfields, since revisions may legallydiffer in dependencies. Unknown revision numbers fail hard; rewriting applies once;
conflicting revision demands for one
name@versionare an explicit error.Registry policy wins: refused bytes fail the install, they are never
substituted.
Important properties
dist.tarballonfresh resolutions; pnpm additionally gets revision management (host-free
lockfile entries,
+rNpinning,pnpm update --patches). Clients thatreconstruct conventional URLs fail loudly, never silently wrong.
canonical immutability makes that honest instead of breaking them.
sequence, checkpointedtogether with the accepted manifest's digest: an exact redelivery is a
no-op, the same sequence with different content is rejected as provider
equivocation, and replayed older documents are rejected. The checkpoint
advances atomically with durable acceptance, so rollback requires a fresh
manifest or an explicitly authorized operator action.
(registry, name, version)with separateuniqueness constraints on revision number and digest; the registry owns the
numbering — provider changes never renumber.
against the principal's package-level access; knowledge of a digest is not
authorization. Full sha512 values are required.
identically to canonical and digest routes and requires edge purge or
non-shared caching.
configuration guarantees integrity, never identical revision-number
meaning.
deprecatedand other registry-managed mutable metadata stay outside theimmutable revision records, whose artifact-derived fields never change; the
only registry-managed annotations on a history entry are its
withdrawn/supersededBystatus fields.Compatibility
revisionentries are byte-identical to the currentformat — no gate, readable by older pnpm.
whole unpatched graph installable; only replacement entries fail, as
genuine unavailability.
dist.integrityis a standard SRI value; npm-compatible clients ignore theunknown
dist.revisionfield and followdist.tarballnormally.lockfile-identity problem.
Summary by CodeRabbit
pnpm update --patches.