feat(pnpr): add registry/resolver feature toggles#12563
Merged
Conversation
pnpr now exposes two independently toggleable surfaces via per-surface
config blocks, both enabled by default:
registry:
enabled: true # packuments, tarballs, publish, search, users
resolver:
enabled: true # /-/pnpr, /v1/resolve, /v1/verify-lockfile
Disabling the registry runs a stateless resolver tier in front of an
existing registry; disabling the resolver runs a plain registry with no
server-side resolution. At least one surface must stay enabled
(validated at config load and again after CLI overrides). The CLI flags
--disable-registry and --disable-resolver override the config.
The resolver paths overlap the registry's catch-all param routes, so
when the resolver is disabled they are registered to an explicit 404
stub. Without it a capability probe (GET /-/pnpr) would fall through to
the registry and be proxied upstream, surfacing a 502 where clients
expect the "no resolver" 404.
OSV stays a top-level setting rather than nesting under either surface,
because it is a cross-cutting policy. Registry-side OSV screening is not
yet implemented and is tracked in #12561.
These types are part of `Config`'s public surface (its `registry` and `resolver` fields), so they must be re-exported from the crate root like the other config types. Without it, the public `Config` field docs link to private items and `cargo doc -D warnings` fails.
|
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:
📝 WalkthroughWalkthroughAdds ChangesRegistry/Resolver Feature Flags
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI arguments
participant Config as Config::resolve_with_overrides
participant Startup as serve/serve_listener
participant Auth as load_startup_auth
participant OSV as load_active_osv_index
participant Router as router_with_auth_and_osv
participant ResolverPaths as resolver routes
participant RegistryPaths as registry routes
CLI->>Config: parse YAML + apply --disable flags
Config->>Config: compute effective enablement
Config->>Config: validate at least one enabled
Config->>Startup: return validated Config
Startup->>Auth: if registry.enabled: recover journal & load auth
Auth->>Startup: auth backend (or in-memory)
Startup->>OSV: if resolver.enabled: load OSV index
OSV->>Startup: OSV (or empty)
Startup->>Router: pass enabled flags + backends
Router->>ResolverPaths: enabled? mount handlers : mount 404 stub
Router->>RegistryPaths: enabled? mount routes : skip
Router->>Startup: return wired router
Startup->>ResolverPaths: incoming request
ResolverPaths-->>Startup: response (handler or 404)
Startup->>RegistryPaths: incoming request
RegistryPaths-->>Startup: response (handler or 404)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ 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 Qodopnpr: add toggleable registry/resolver surfaces (config + CLI) Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
Context used 1.
|
| // documents. Not part of the standard npm registry API — | ||
| // `pnpm publish --batch` opts into it explicitly. | ||
| .route("/-/pnpm/v1/publish", put(serve_batch_publish)) | ||
| .route("/{name}", get(get_packument_unscoped).put(put_one_segment)) |
There was a problem hiding this comment.
1. get_packument_unscoped lacks osv screening 📎 Requirement gap ⛨ Security
The registry packument and tarball routes are mounted, but when an OSV DB is configured the registry surface does not consult state.inner.osv_index to filter/mark vulnerable versions in packuments or to refuse/block serving tarballs for OSV-known-vulnerable versions. This allows registry clients to resolve to and fetch vulnerable package versions, violating the required cross-surface OSV enforcement.
Agent Prompt
## Issue description
Registry packument and tarball endpoints are served without OSV screening. When an OSV database is configured, packument responses must omit or clearly mark OSV-vulnerable versions so clients cannot resolve to them, and tarball endpoints must refuse/block serving tarballs for OSV-known-vulnerable package versions to ensure cross-surface OSV enforcement.
## Issue Context
The router mounts registry routes (including packuments and tarballs), but OSV is only wired into resolver flows; the registry handler paths never consult `state.inner.osv_index`. As a result, `serve_packument` does not filter/mark vulnerable versions and `serve_tarball` serves from cache or proxies upstream without an OSV check.
## Fix Focus Areas
- pnpr/crates/pnpr/src/server.rs[235-258]
- pnpr/crates/pnpr/src/server.rs[641-660]
- pnpr/crates/pnpr/src/server.rs[695-748]
- pnpr/crates/pnpr/src/server.rs[2089-2109]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
This is the pre-existing registry-side OSV screening gap: the registry serving path has never consulted osv_index, and this PR doesn't change that behavior — it only gates which surfaces are mounted. The expected behavior (omit/mark OSV-vulnerable versions in served packuments and refuse vulnerable tarballs, for both proxy and cache) is tracked in #12561. Deferring to that issue rather than widening this PR's scope.
Written by an agent (Claude Code, claude-opus-4-8).
Comment on lines
+235
to
262
| // The npm-registry surface: every packument/tarball read, publish, | ||
| // unpublish, dist-tag, search, and the user/login endpoint. When the | ||
| // feature is disabled, none of these routes are mounted — not merely | ||
| // hidden — so a resolver-only tier exposes no registry surface at all. | ||
| if registry_enabled { | ||
| router = router | ||
| // Batch publish: one request carrying many packages' publish | ||
| // documents. Not part of the standard npm registry API — | ||
| // `pnpm publish --batch` opts into it explicitly. | ||
| .route("/-/pnpm/v1/publish", put(serve_batch_publish)) | ||
| .route("/{name}", get(get_packument_unscoped).put(put_one_segment)) | ||
| .route("/{first}/{second}", get(get_two_segments).put(put_two_segments)) | ||
| .route( | ||
| "/{first}/{second}/{third}", | ||
| get(get_three_segments).put(put_three_segments).delete(delete_three_segments), | ||
| ) | ||
| .route("/{scope}/{name}/-/{filename}", get(get_tarball_scoped)) | ||
| .route("/{a}/{b}/{c}/{d}", get(get_four_segments).delete(delete_four_segments)) | ||
| .route( | ||
| "/{a}/{b}/{c}/{d}/{e}", | ||
| get(get_five_segments).put(put_five_segments).delete(delete_five_segments), | ||
| ) | ||
| // Scoped tarball delete: `DELETE /@scope/name/-/<basename-version>.tgz/-rev/<rev>` | ||
| .route("/{a}/{b}/{c}/{d}/{e}/{f}", delete(delete_six_segments)); | ||
| } | ||
| router | ||
| .layer(DefaultBodyLimit::max(MAX_PUBLISH_BODY_BYTES)) | ||
| // gzip metadata responses for clients that send `Accept-Encoding: |
There was a problem hiding this comment.
2. Resolver-only 100mib request dos 🐞 Bug ⛨ Security
In resolver-only mode (registry.enabled=false), the server still applies a global 100MiB DefaultBodyLimit, while /v1/resolve and /v1/verify-lockfile buffer the full request body into memory (axum::body::Bytes) and JSON-deserialize it. This enables straightforward resource-exhaustion (memory/CPU) by sending near-100MiB bodies to resolver endpoints in the new resolver-only tier configuration.
Agent Prompt
## Issue description
Resolver-only deployments still accept request bodies up to `MAX_PUBLISH_BODY_BYTES` (100MiB) because `DefaultBodyLimit::max(MAX_PUBLISH_BODY_BYTES)` is applied at the router level. Resolver endpoints (`/v1/resolve`, `/v1/verify-lockfile`) fully buffer request bodies (`Bytes`) and JSON-deserialize them, so a 100MiB limit is an avoidable DoS vector when the registry surface is disabled.
## Issue Context
- The large body limit is needed for publish APIs, but not for the resolver endpoints.
- With the new feature toggles, operators can run a resolver-only tier; that tier should not inherit publish-sized body limits.
## Fix Focus Areas
- pnpr/crates/pnpr/src/server.rs[206-262]
### Suggested implementation direction
- Remove the single router-level `DefaultBodyLimit::max(MAX_PUBLISH_BODY_BYTES)`.
- Build separate sub-routers per surface and apply different body limits:
- Registry router: keep `MAX_PUBLISH_BODY_BYTES`.
- Resolver router: introduce a smaller `MAX_RESOLVER_BODY_BYTES` (pick a value based on expected max lockfile/request size) and apply it only to resolver routes.
- Merge the sub-routers into the top-level router, keeping other global layers (compression/trace) as-is.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
The 100 MiB DefaultBodyLimit and the resolver endpoints' full-body buffering both pre-date this PR — /v1/resolve and /v1/verify-lockfile already carried the same global limit, so this isn't a regression introduced here. A per-surface limit is reasonable hardening, but the resolver legitimately accepts large bodies (a full lockfile plus every workspace member's manifest), so the cap needs a considered value rather than an arbitrary one. Tracking as a follow-up rather than bundling it into this change.
Written by an agent (Claude Code, claude-opus-4-8).
|
Code review by qodo was updated up to the latest commit d9e63ca |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12563 +/- ##
==========================================
+ Coverage 88.02% 88.07% +0.05%
==========================================
Files 321 321
Lines 44941 45078 +137
==========================================
+ Hits 39559 39704 +145
+ Misses 5382 5374 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…re blocks Addresses PR review: - A bare `registry:` / `resolver:` block parses as YAML null, which could not deserialize into the non-Option `FeatureFile` and would fail config load despite the documented "empty block means enabled". The fields are now `Option<FeatureFile>`, defaulting to enabled. - Resolver-only mode (registry disabled) no longer recovers the publish journal or loads on-disk auth backends, so the resolver tier stays stateless and runs on read-only/ephemeral hosts. - Registry-only mode (resolver disabled) no longer loads the OSV index, since only resolver routes consult it today. The gate widens once registry-side OSV screening lands (#12561).
|
Code review by qodo was updated up to the latest commit 2623445 |
|
Code review by qodo was updated up to the latest commit 2623445 |
- Reject unknown keys in `registry:` / `resolver:` blocks (`deny_unknown_fields`), so a typo like `enable:` is a loud config error instead of silently leaving the surface enabled. - When the resolver is disabled, stub its paths for every method (`any`), not just POST/GET, so a `GET /v1/resolve` probe can't fall through to the registry's catch-all routes and be proxied. - Skip building uplink `Upstream` clients in resolver-only mode; only registry routes consult them.
|
Code review by qodo was updated up to the latest commit 82b3093 |
|
Code review by qodo was updated up to the latest commit 82b3093 |
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": 3.78411104856,
"stddev": 0.18601871543814447,
"median": 3.79318289036,
"user": 3.51676402,
"system": 3.39010192,
"min": 3.5333442483599997,
"max": 4.0819406233599995,
"times": [
4.01022764236,
3.5787937473599998,
3.61843603336,
3.5333442483599997,
3.89091484036,
3.86448311936,
3.67660445036,
3.86217243636,
3.7241933443599997,
4.0819406233599995
]
},
{
"command": "pacquet@main",
"mean": 3.7147523041599997,
"stddev": 0.13818553839612083,
"median": 3.68345239686,
"user": 3.52963382,
"system": 3.37100082,
"min": 3.58793159136,
"max": 3.96541519536,
"times": [
3.8231786243599997,
3.69364091436,
3.58793159136,
3.5909769353599996,
3.96541519536,
3.91150050236,
3.68897782336,
3.59528583536,
3.61268864936,
3.6779269703599997
]
},
{
"command": "pnpr@HEAD",
"mean": 2.0757421134599996,
"stddev": 0.12343229536315078,
"median": 2.0585418748599995,
"user": 2.63369912,
"system": 2.9959905199999994,
"min": 1.94335740636,
"max": 2.36520852436,
"times": [
2.0304418003599998,
1.9931122083600001,
1.94335740636,
2.0952892243599996,
2.01735195236,
2.0866419493599997,
1.95619653736,
2.36520852436,
2.1055838153599997,
2.1642377163599997
]
},
{
"command": "pnpr@main",
"mean": 2.1509354277600004,
"stddev": 0.14013866973616249,
"median": 2.1002297573599997,
"user": 2.6342170200000004,
"system": 2.98809942,
"min": 1.9975044183600001,
"max": 2.34825187936,
"times": [
2.14230686636,
1.9975044183600001,
2.05782086736,
1.9993814933600003,
2.34825187936,
2.0581526483599997,
2.0557250683599997,
2.19461730436,
2.34228299736,
2.31331073436
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6418182510599999,
"stddev": 0.010051146175356455,
"median": 0.6397127194600001,
"user": 0.39837648000000003,
"system": 1.32626884,
"min": 0.62591367546,
"max": 0.65780609946,
"times": [
0.65780609946,
0.63723401146,
0.6545257304600001,
0.6335717444600001,
0.63727874646,
0.64214669246,
0.64622112046,
0.6343244234600001,
0.62591367546,
0.64916026646
]
},
{
"command": "pacquet@main",
"mean": 0.6813926193600001,
"stddev": 0.1055712760508469,
"median": 0.6503207359600001,
"user": 0.38886968000000005,
"system": 1.3441092399999999,
"min": 0.63260028246,
"max": 0.98051342846,
"times": [
0.63260028246,
0.64338392546,
0.6505220324600001,
0.64502082646,
0.6501194394600001,
0.6639722464600001,
0.6544927084600001,
0.63411984746,
0.65918145646,
0.98051342846
]
},
{
"command": "pnpr@HEAD",
"mean": 0.72265320836,
"stddev": 0.07157459888585466,
"median": 0.69496520046,
"user": 0.4187642800000001,
"system": 1.3539389400000001,
"min": 0.66919496046,
"max": 0.91329855546,
"times": [
0.74612214046,
0.70897758946,
0.68939440746,
0.69627255146,
0.6936578494600001,
0.67958866046,
0.68716547046,
0.66919496046,
0.91329855546,
0.74285989846
]
},
{
"command": "pnpr@main",
"mean": 0.70968261726,
"stddev": 0.05425771264359275,
"median": 0.69669788546,
"user": 0.40417228000000005,
"system": 1.3504490399999998,
"min": 0.67207354046,
"max": 0.85927088246,
"times": [
0.67207354046,
0.7015908844600001,
0.70875925746,
0.69484764946,
0.6854424344600001,
0.6756587874600001,
0.6866608954600001,
0.85927088246,
0.71397371946,
0.69854812146
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.2297486697,
"stddev": 0.03997425407120895,
"median": 4.231480659900001,
"user": 3.7449470999999996,
"system": 3.3720886799999996,
"min": 4.152729931400001,
"max": 4.294089464400001,
"times": [
4.2697239304000005,
4.214309832400001,
4.1949131444,
4.214585432400001,
4.261622586400001,
4.2318188314,
4.231142488400001,
4.294089464400001,
4.152729931400001,
4.2325510554
]
},
{
"command": "pacquet@main",
"mean": 4.2363411035,
"stddev": 0.05021069584467642,
"median": 4.251960626400001,
"user": 3.7301562999999995,
"system": 3.3939906799999995,
"min": 4.154595121400001,
"max": 4.3051655654,
"times": [
4.1704024094,
4.2021089584,
4.3051655654,
4.2798165494000004,
4.2526540254,
4.205590426400001,
4.2512672274000005,
4.2666004064,
4.154595121400001,
4.2752103454000006
]
},
{
"command": "pnpr@HEAD",
"mean": 2.1966845655,
"stddev": 0.1245549287215409,
"median": 2.1737197254,
"user": 2.5043341999999997,
"system": 2.9233930799999994,
"min": 2.0497554044,
"max": 2.3748175203999997,
"times": [
2.1280890273999997,
2.3721547354,
2.2223962324,
2.0822586824,
2.0497554044,
2.2193504234,
2.0827657104,
2.1111213034,
2.3241366154,
2.3748175203999997
]
},
{
"command": "pnpr@main",
"mean": 2.2060389271,
"stddev": 0.11327467623630479,
"median": 2.1701280983999998,
"user": 2.5149003,
"system": 2.91744538,
"min": 2.0691793034,
"max": 2.4163967304,
"times": [
2.2221225103999998,
2.1281737634,
2.1200077374,
2.1639002614,
2.0691793034,
2.3912019954,
2.2096849873999997,
2.1763559353999997,
2.4163967304,
2.1633660463999997
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.3547591423000003,
"stddev": 0.018940665656176678,
"median": 1.3556966279,
"user": 1.3569460999999998,
"system": 1.7373884,
"min": 1.3306710114,
"max": 1.3854921444000001,
"times": [
1.3567457564,
1.3570331554000001,
1.3546474994000002,
1.3655718104,
1.3854921444000001,
1.3339189164,
1.3795341474,
1.3306710114,
1.3520792014,
1.3318977804
]
},
{
"command": "pacquet@main",
"mean": 1.3825238044000003,
"stddev": 0.02129703646379773,
"median": 1.3871736419,
"user": 1.3723512000000002,
"system": 1.7796500999999996,
"min": 1.3537755514,
"max": 1.4154824994000001,
"times": [
1.3632296754000002,
1.3896623634,
1.3537755514,
1.3608952064000002,
1.4154824994000001,
1.4108369584,
1.3922845334,
1.3846849204,
1.3645458514000002,
1.3898404844
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6802279774000001,
"stddev": 0.061677160915642695,
"median": 0.6625482954000002,
"user": 0.3591016,
"system": 1.2863685999999999,
"min": 0.6479106784,
"max": 0.8541173484000001,
"times": [
0.6531344494000001,
0.6599870164000001,
0.6479106784,
0.8541173484000001,
0.6651095744000001,
0.6676680024000001,
0.6672385534,
0.6528021034000001,
0.6581560804000001,
0.6761559674000001
]
},
{
"command": "pnpr@main",
"mean": 0.6604440957000002,
"stddev": 0.009926648619702121,
"median": 0.6595302739000001,
"user": 0.36346599999999996,
"system": 1.2778006999999998,
"min": 0.6459449854000001,
"max": 0.6797123634000001,
"times": [
0.6646221314,
0.6590637234000001,
0.6797123634000001,
0.6615144414,
0.6702242584000001,
0.6471135844000001,
0.6576090924000001,
0.6459449854000001,
0.6586395524,
0.6599968244000001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 3.02155928662,
"stddev": 0.09320734944569661,
"median": 2.99994719482,
"user": 1.79050692,
"system": 1.9637064799999997,
"min": 2.95935628582,
"max": 3.28066671782,
"times": [
2.97079634882,
2.98385010182,
3.0029421738199997,
3.02372636382,
3.00218886382,
2.97847765782,
2.99770552582,
3.28066671782,
3.01588282682,
2.95935628582
]
},
{
"command": "pacquet@main",
"mean": 3.01792057252,
"stddev": 0.034095834592730266,
"median": 3.0228185518200004,
"user": 1.7933430199999996,
"system": 1.95453968,
"min": 2.9665029128200002,
"max": 3.07312172682,
"times": [
3.03239892782,
2.9665029128200002,
2.99409877782,
3.05505854982,
2.97111943482,
3.01555448682,
3.03008261682,
3.07312172682,
3.03196713082,
3.0093011608199998
]
},
{
"command": "pnpr@HEAD",
"mean": 0.67143447042,
"stddev": 0.013357793082436883,
"median": 0.6707865348200002,
"user": 0.36120341999999994,
"system": 1.26751868,
"min": 0.6530197148200001,
"max": 0.69473675582,
"times": [
0.6718370738200001,
0.6663774628200001,
0.6618796858200001,
0.69473675582,
0.6697359958200001,
0.65410433582,
0.6530197148200001,
0.6760716968200001,
0.6826313528200001,
0.6839506298200001
]
},
{
"command": "pnpr@main",
"mean": 0.6587389389200002,
"stddev": 0.009311777291842172,
"median": 0.6590412443200001,
"user": 0.35992201999999995,
"system": 1.27516308,
"min": 0.6454392938200001,
"max": 0.6718852848200001,
"times": [
0.6454392938200001,
0.6636011808200001,
0.66947257682,
0.6718852848200001,
0.6547488628200001,
0.6669033888200001,
0.6596881698200001,
0.6472554188200002,
0.6583943188200001,
0.6500008938200001
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/12563 |
| 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 | 4,229.75 ms(+0.05%)Baseline: 4,227.50 ms | 5,073.00 ms (83.38%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 3,021.56 ms(+0.51%)Baseline: 3,006.25 ms | 3,607.50 ms (83.76%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,354.76 ms(+1.55%)Baseline: 1,334.02 ms | 1,600.82 ms (84.63%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 3,784.11 ms(-5.84%)Baseline: 4,018.67 ms | 4,822.41 ms (78.47%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 641.82 ms(+2.17%)Baseline: 628.17 ms | 753.81 ms (85.14%) |
Contributor
|
| Branch | pr/12563 |
| 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 | 2,196.68 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 671.43 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 680.23 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,075.74 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 722.65 ms |
- Skip uplink resolution/validation when the registry is disabled, so a resolver-only tier doesn't fail config load on a registry-shaped config whose uplink auth tokens (e.g. NPM_TOKEN) aren't present — it never mounts a route that uses them. - Enforce the "at least one surface enabled" invariant in serve() and serve_listener(), not just at YAML load and in the CLI, so embedders calling these entrypoints directly can't start a ping-only server.
|
Code review by qodo was updated up to the latest commit e95ae84 |
1 similar comment
|
Code review by qodo was updated up to the latest commit e95ae84 |
…uilders Addresses PR review: - `--disable-registry` / `--disable-resolver` are now threaded into config parsing via `FeatureOverrides` and `Config::resolve_with_overrides`, so a CLI-disabled surface skips its parse-time work (notably strict uplink auth-token resolution) — not just its routes. Previously the flags were applied after `resolve()`, so a resolver-only tier driven by the flag could still fail to load, or retain upstream secrets, from a registry-shaped config. - The "at least one surface enabled" invariant is now also enforced in the public router builders (`try_router_with_auth`, and via it `router`/`try_router`/`router_with_auth`), so embedders that build a router directly can't get a ping-only server.
|
Code review by qodo was updated up to the latest commit 20caa23 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/crates/pnpr/src/config.rs`:
- Around line 1069-1076: The `.expect()` call in the
`from_default_yaml_with_overrides` method causes a panic when
`from_yaml_str_with_overrides` returns an error (such as when both CLI disable
flags are set), while file-backed branches properly return `Err`. Replace the
`.expect()` call with the `?` operator to propagate the error, ensuring the
bundled DEFAULT_CONFIG_YAML branch handles errors consistently with file-backed
branches instead of panicking on invalid configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fab8abf-59aa-4383-9b85-f2460c587f07
📒 Files selected for processing (5)
pnpr/crates/pnpr/src/config.rspnpr/crates/pnpr/src/config/tests.rspnpr/crates/pnpr/src/lib.rspnpr/crates/pnpr/src/main.rspnpr/crates/pnpr/src/server.rs
✅ Files skipped from review due to trivial changes (1)
- pnpr/crates/pnpr/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- pnpr/crates/pnpr/src/server.rs
…onfig `from_default_yaml_with_overrides` kept the `expect(...)` on the parse result, so `--disable-registry --disable-resolver` with no config file (the bundled branch) panicked on the "at least one surface" validation error instead of returning it like the file-backed branches. It now returns `Result`, `resolve_with_overrides` maps the error to an io::Error, and the override-free `from_default_yaml` keeps the `expect` (safe: default overrides leave both surfaces enabled).
|
Code review by qodo was updated up to the latest commit 20caa23 |
|
Code review by qodo was updated up to the latest commit a9ea91e |
1 similar comment
|
Code review by qodo was updated up to the latest commit a9ea91e |
The config parser is intentionally lenient about unknown top-level keys (so a verdaccio config can be loaded verbatim), which means a typo'd `registry:` / `resolver:` block name is silently ignored and the surface stays at its default-enabled state. Logging the effective surface state at startup makes such a misconfiguration immediately visible instead of only discoverable by probing endpoints.
|
Code review by qodo was updated up to the latest commit 30e6d28 |
1 similar comment
|
Code review by qodo was updated up to the latest commit 30e6d28 |
…paths When the resolver is disabled, only `/-/pnpr` is stubbed now — it's the capability-probe path that overlaps the registry catch-all and must 404 cleanly for detection. `/v1/resolve` and `/v1/verify-lockfile` are no longer stubbed: they are POST-only resolver endpoints with no capability probe, and stubbing every method wrongly 404'd the legitimate npm version-manifest paths `GET /v1/resolve` (package `v1`, tag `resolve`) in registry-only mode. Left unmounted, the registry serves their GET as usual and a POST falls to a 405.
|
Code review by qodo was updated up to the latest commit e56c000 |
1 similar comment
|
Code review by qodo was updated up to the latest commit e56c000 |
This was referenced Jun 22, 2026
This was referenced Jul 2, 2026
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
Adds two independently toggleable surfaces to the pnpr server, configured via per-surface blocks (both enabled by default):
/-/pingis always served. At least one surface must stay enabled (validated at config load and again after CLI overrides). CLI flags--disable-registry/--disable-resolveroverride the config.OSV stays a top-level setting rather than nesting under either surface, because it is a cross-cutting policy. The registry serving path does not yet apply OSV screening; that gap is tracked in #12561.
Squash Commit Body
Checklist
pacquet/port, or the description notes what still needs porting. — N/A: this is a change to the standalonepnprserver (its own subproject), which has no TypeScript-CLI or pacquet counterpart.pnpris the Rust server binary; no published JS package is affected, so the JS changeset flow does not apply.resolver_only/registry_onlyserver tests, plus config unit tests for feature parsing and the both-disabled validation error.registry:/resolver:blocks in the bundledconfig.yaml.Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
/-/pingis always served.--disable_registryand--disable_resolverto override toggles at startup.nullbehavior and unknown/typo keys) and for “strict” registry-side handling when registry is disabled.