Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent 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). (5)
🧰 Additional context used📓 Path-based instructions (2)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
pacquet/**/{tests,test}/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds pnpm-style workspace catalog support: new catalog type crate, protocol parser, config normalizer, resolver crate, workspace manifest fields, install/resolver plumbing, and unit + end-to-end tests validating resolution and misconfiguration errors. ChangesCatalog resolution support
Possibly related PRs
🎯 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 docstrings
🧪 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 |
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 `@pacquet/crates/catalogs-resolver/src/lib.rs`:
- Around line 156-167: The code currently treats any string before a ':' as a
protocol by using protocol_of_lookup =
catalog_lookup.split(':').next().unwrap_or(""), which incorrectly flags bare
values like "workspace" or "file" as protocols; change the detection to require
the explicit prefix form (e.g., "workspace:", "link:", "file:") by replacing the
split-based check with starts_with checks on catalog_lookup (e.g.,
catalog_lookup.starts_with("workspace:"), catalog_lookup.starts_with("link:") ||
catalog_lookup.starts_with("file:")) and update the branches that return
CatalogResolutionResult::Misconfiguration (CatalogResolutionMisconfiguration /
CatalogResolutionError::EntryInvalidWorkspaceSpec) and the matches!(...) branch
so they only run when the string actually has the protocol prefix.
🪄 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: e4723fb5-7ab4-4cb6-9237-d78e5660d024
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlpacquet/crates/catalogs-config/Cargo.tomlpacquet/crates/catalogs-config/src/lib.rspacquet/crates/catalogs-config/src/tests.rspacquet/crates/catalogs-protocol-parser/Cargo.tomlpacquet/crates/catalogs-protocol-parser/src/lib.rspacquet/crates/catalogs-protocol-parser/src/tests.rspacquet/crates/catalogs-resolver/Cargo.tomlpacquet/crates/catalogs-resolver/src/lib.rspacquet/crates/catalogs-resolver/src/tests.rspacquet/crates/catalogs-types/Cargo.tomlpacquet/crates/catalogs-types/src/lib.rspacquet/crates/workspace/Cargo.tomlpacquet/crates/workspace/src/manifest.rspacquet/crates/workspace/src/manifest/tests.rs
📜 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). (2)
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (macos-latest)
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/catalogs-protocol-parser/src/tests.rspacquet/crates/catalogs-config/src/tests.rspacquet/crates/catalogs-types/src/lib.rspacquet/crates/catalogs-resolver/src/tests.rspacquet/crates/catalogs-protocol-parser/src/lib.rspacquet/crates/workspace/src/manifest/tests.rspacquet/crates/workspace/src/manifest.rspacquet/crates/catalogs-resolver/src/lib.rspacquet/crates/catalogs-config/src/lib.rs
🧠 Learnings (1)
📚 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/catalogs-protocol-parser/src/tests.rspacquet/crates/catalogs-config/src/tests.rspacquet/crates/catalogs-types/src/lib.rspacquet/crates/catalogs-resolver/src/tests.rspacquet/crates/catalogs-protocol-parser/src/lib.rspacquet/crates/workspace/src/manifest/tests.rspacquet/crates/workspace/src/manifest.rspacquet/crates/catalogs-resolver/src/lib.rspacquet/crates/catalogs-config/src/lib.rs
🔇 Additional comments (14)
pacquet/crates/catalogs-types/Cargo.toml (1)
1-17: LGTM!pacquet/crates/catalogs-types/src/lib.rs (1)
12-30: LGTM!Cargo.toml (1)
16-19: LGTM!pacquet/crates/workspace/Cargo.toml (1)
14-14: LGTM!pacquet/crates/catalogs-protocol-parser/Cargo.toml (1)
1-18: LGTM!pacquet/crates/catalogs-protocol-parser/src/lib.rs (1)
9-24: LGTM!Also applies to: 26-27
pacquet/crates/catalogs-protocol-parser/src/tests.rs (1)
4-25: LGTM!pacquet/crates/workspace/src/manifest.rs (1)
26-27: LGTM!Also applies to: 61-70
pacquet/crates/workspace/src/manifest/tests.rs (1)
5-5: LGTM!Also applies to: 69-102
pacquet/crates/catalogs-config/Cargo.toml (1)
1-21: LGTM!pacquet/crates/catalogs-config/src/lib.rs (1)
1-82: LGTM!pacquet/crates/catalogs-config/src/tests.rs (1)
1-68: LGTM!pacquet/crates/catalogs-resolver/Cargo.toml (1)
1-21: LGTM!pacquet/crates/catalogs-resolver/src/tests.rs (1)
1-187: LGTM!
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11787 +/- ##
==========================================
+ Coverage 87.20% 87.28% +0.08%
==========================================
Files 190 193 +3
Lines 22525 22636 +111
==========================================
+ Hits 19642 19758 +116
+ Misses 2883 2878 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.4859350611199997,
"stddev": 0.09283392618464369,
"median": 2.49261433302,
"user": 2.74078248,
"system": 3.7682987799999994,
"min": 2.3650666575200003,
"max": 2.6246971665200003,
"times": [
2.6246971665200003,
2.40831743852,
2.39636764352,
2.61340181652,
2.39480132152,
2.3650666575200003,
2.48837943452,
2.5383850325200004,
2.53308486852,
2.49684923152
]
},
{
"command": "pacquet@main",
"mean": 2.45273394212,
"stddev": 0.07701080737153443,
"median": 2.42379126452,
"user": 2.7797429800000004,
"system": 3.7432415800000003,
"min": 2.3939300235200003,
"max": 2.6432428965200003,
"times": [
2.6432428965200003,
2.39890137352,
2.4846596505200003,
2.3939300235200003,
2.4597739305200004,
2.40335039652,
2.39640543452,
2.49949318652,
2.43338918952,
2.41419333952
]
},
{
"command": "pnpm",
"mean": 4.8950438069199995,
"stddev": 0.031929295098169705,
"median": 4.8970558230200005,
"user": 8.305616079999998,
"system": 4.27526368,
"min": 4.85028775552,
"max": 4.95913795452,
"times": [
4.89788594052,
4.87186001152,
4.86666162752,
4.95913795452,
4.90541144152,
4.87029306452,
4.92284622252,
4.89622570552,
4.909828345519999,
4.85028775552
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6874626044000001,
"stddev": 0.04312440265635045,
"median": 0.6748557775000001,
"user": 0.37214291999999993,
"system": 1.5690749400000001,
"min": 0.6689246075,
"max": 0.8098015325000001,
"times": [
0.8098015325000001,
0.6724887755000001,
0.6699208805000001,
0.6762909355000001,
0.6758231895000001,
0.6707976545000001,
0.6689246075,
0.6738883655000001,
0.6765413845000001,
0.6801487185000001
]
},
{
"command": "pacquet@main",
"mean": 0.7122716305000001,
"stddev": 0.06640839286212849,
"median": 0.6887535145000001,
"user": 0.38859732,
"system": 1.58013774,
"min": 0.6728854885000001,
"max": 0.8930219035000001,
"times": [
0.7413084255000001,
0.6728854885000001,
0.6838209995000001,
0.8930219035000001,
0.6988824615000001,
0.6808291675000001,
0.6816090465000001,
0.6779234125000001,
0.6936860295000001,
0.6987493705000001
]
},
{
"command": "pnpm",
"mean": 2.6940487583,
"stddev": 0.10203990429441027,
"median": 2.7014742160000003,
"user": 3.2648885200000004,
"system": 2.28780054,
"min": 2.5152544865,
"max": 2.8822053075000005,
"times": [
2.6979711595,
2.5587714555,
2.7523891865000003,
2.7377413005,
2.7049772725000003,
2.6634356575,
2.5152544865,
2.7347106845000004,
2.8822053075000005,
2.6930310725
]
}
]
} |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/cli/tests/install.rs (1)
385-389: ⚡ Quick winAssert the pnpm error code in addition to the message.
This test validates the message text but not the contract error code (
ERR_PNPM_CATALOG_ENTRY_NOT_FOUND_FOR_SPEC), so a code regression could pass unnoticed.Proposed assertion addition
assert!( flattened.contains( "Nocatalogentry'`@pnpm.e2e/hello-world-js-bin-parent`'wasfoundforcatalog'default'.", ), "stderr did not mention the missing-catalog-entry error: {stderr}", ); + assert!( + stderr.contains("ERR_PNPM_CATALOG_ENTRY_NOT_FOUND_FOR_SPEC"), + "stderr did not include the expected pnpm error code: {stderr}", + );🤖 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 `@pacquet/crates/cli/tests/install.rs` around lines 385 - 389, The current assertion in the test checks the human-readable message but not the pnpm error code; update the test in install.rs (near the assert that inspects flattened and stderr) to also assert that the stderr (or flattened) contains the literal error code "ERR_PNPM_CATALOG_ENTRY_NOT_FOUND_FOR_SPEC" so the contract is verified; add a second assert using the same stderr/flattened variable (or extend the existing assertion) to fail if the error code string is not present alongside the message.
🤖 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.
Nitpick comments:
In `@pacquet/crates/cli/tests/install.rs`:
- Around line 385-389: The current assertion in the test checks the
human-readable message but not the pnpm error code; update the test in
install.rs (near the assert that inspects flattened and stderr) to also assert
that the stderr (or flattened) contains the literal error code
"ERR_PNPM_CATALOG_ENTRY_NOT_FOUND_FOR_SPEC" so the contract is verified; add a
second assert using the same stderr/flattened variable (or extend the existing
assertion) to fail if the error code string is not present alongside the
message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86d140d6-a73b-4c23-bb2b-b517a21b761b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
pacquet/crates/cli/tests/install.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-deps-resolver/Cargo.tomlpacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
✅ Files skipped from review due to trivial changes (1)
- pacquet/crates/resolving-deps-resolver/Cargo.toml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/cli/tests/install.rspacquet/crates/package-manager/src/install.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
pacquet/**/{tests,test}/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/{tests,test}/**/*.rs: Prefer real fixtures; reach for the dependency-injection seam only when they can't cover the branch. Most happy paths and error paths should be tested withtempfile::TempDir, the mocked registry, or an integration test that spawns the actual binary.
Use the DI seam — a capability trait on theHostprovider, threaded asSys: <Bounds>— only for branches a real fixture can't reach portably: filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths.
Log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!. Follow the test-logging guidance in CODE_STYLE_GUIDE.md.
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found. If the test needs a tool, let it panic when the tool is absent.
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or#[cfg(unix)]) over a runtime probe-and-skip helper for platform-locked tools.
Port relevant tests from pnpm when porting behavior. Matching test coverage is the easiest way to prove behavioral parity.
When porting behavior from pnpm, consult plans/TEST_PORTING.md before adding ported tests. Useallow_known_failure!at the not-yet-implemented boundary and update checkboxes as items land.
Files:
pacquet/crates/cli/tests/install.rs
🧠 Learnings (2)
📚 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/install_without_lockfile.rspacquet/crates/cli/tests/install.rspacquet/crates/package-manager/src/install.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.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/install_without_lockfile.rspacquet/crates/cli/tests/install.rspacquet/crates/package-manager/src/install.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
🔇 Additional comments (6)
pacquet/crates/package-manager/Cargo.toml (1)
14-15: LGTM!pacquet/crates/package-manager/src/install_without_lockfile.rs (1)
10-10: LGTM!Also applies to: 79-81, 152-152, 243-243
pacquet/crates/cli/tests/install.rs (1)
314-353: LGTM!pacquet/crates/package-manager/src/install.rs (1)
10-12: LGTM!Also applies to: 172-182, 256-273, 551-551
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
7-12: LGTM!Also applies to: 43-48, 68-73, 117-117, 269-296
pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
3-3: LGTM!Also applies to: 120-120, 188-188, 219-219, 232-301, 307-307, 335-335, 386-386, 435-435, 487-487, 547-547
Adds four new crates mirroring upstream's `catalogs/*` packages: - pacquet-catalogs-types: `Catalog`/`Catalogs` map aliases plus `DEFAULT_CATALOG_NAME`. - pacquet-catalogs-protocol-parser: `parse_catalog_protocol`; folds `catalog:` shorthand into `"default"` to match upstream. - pacquet-catalogs-resolver: `resolve_from_catalog` with the four upstream `PnpmError` codes (`CATALOG_ENTRY_NOT_FOUND_FOR_SPEC`, `CATALOG_ENTRY_INVALID_RECURSIVE_DEFINITION`, `CATALOG_ENTRY_INVALID_WORKSPACE_SPEC`, `CATALOG_ENTRY_INVALID_SPEC`). Rust callers `match` on the result enum directly instead of porting the TS-ergonomic `matchCatalogResolveResult` visitor. - pacquet-catalogs-config: `get_catalogs_from_workspace_manifest` plus the `INVALID_CATALOGS_CONFIGURATION` mutual-exclusion check. `pacquet-workspace`'s `WorkspaceManifest` now actually deserializes the `catalog` and `catalogs` fields (previously dropped). The resolver is not yet wired into the install path — `deps-installer` hasn't been ported — but the crates are ready for that next step. Tests are 1:1 ports of the upstream Jest suites.
- catalogs-resolver: drop the intra-doc-link form on the `pacquet_resolving_resolver_base::WantedDependency` reference; the crate isn't a dependency, so rustdoc failed to resolve it under `-D rustdoc::broken-intra-doc-links`. - catalogs-resolver, catalogs-config: reorder the `CatalogResolutionError` / `InvalidCatalogsConfigurationError` derive lists to `Debug, Display, Error, Diagnostic, Clone, PartialEq, Eq` so they match the `prefix_then_alphabetical` rule that the CI-only Perfectionist dylint enforces. `just ready` doesn't surface this lint locally.
Wires the catalogs port into the install path:
- `install.rs`: read `pnpm-workspace.yaml` after `find_workspace_dir`
and normalize via `get_catalogs_from_workspace_manifest` into a
`Catalogs` map. Adds `InstallError::{ReadWorkspaceManifest,
InvalidCatalogsConfiguration}` so the upstream
`ERR_PNPM_INVALID_CATALOGS_CONFIGURATION` propagates verbatim.
- `install_without_lockfile.rs`: threads the `Catalogs` map into
`ResolveDependencyTreeOptions`. (Frozen-lockfile catalog handling
needs a lockfile-snapshot pass and is a separate slice.)
- `resolve_dependency_tree`: replaces direct (importer-level)
`catalog:` bare specifiers with the catalog's recorded version
before the resolver chain dispatches. Catalog resolution does NOT
run on transitive deps, matching upstream's importer-only scope.
Misconfigured entries surface as `CatalogMisconfiguration` with
the upstream `ERR_PNPM_CATALOG_ENTRY_*` code instead of leaking
through to `SPEC_NOT_SUPPORTED_BY_ANY_RESOLVER`.
Tests:
- deps-resolver: two new unit tests prove direct-dep rewriting and
the misconfiguration code path.
- cli (e2e): `install_resolves_catalog_protocol` runs the binary
against a workspace with a `catalog:` entry and checks the
virtual-store layout; `install_surfaces_catalog_misconfiguration`
asserts the upstream message is surfaced when the catalog has no
matching alias.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/catalogs-config/src/lib.rs (1)
41-43: ⚡ Quick winAdd an explicit unit test for the
Nonemanifest branch.The early-return path (
None→ emptyCatalogs) is part of the public behavior and appears to be the remaining uncovered line; a focused test will lock this contract and prevent regressions.🤖 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 `@pacquet/crates/catalogs-config/src/lib.rs` around lines 41 - 43, Add a unit test that exercises the early-return branch where workspace_manifest is None to assert that the function returns an empty Catalogs (i.e., Catalogs::new()). In your test module for lib.rs, construct the minimal inputs or test harness so workspace_manifest evaluates to None, call the public entrypoint that contains the let Some(manifest) = workspace_manifest else { return Ok(Catalogs::new()); } logic, and assert the returned value equals Catalogs::new() (or matches expected empty state). Name the test clearly (e.g., returns_empty_catalogs_when_manifest_none) and keep it focused only on the None branch to lock the public contract.
🤖 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.
Nitpick comments:
In `@pacquet/crates/catalogs-config/src/lib.rs`:
- Around line 41-43: Add a unit test that exercises the early-return branch
where workspace_manifest is None to assert that the function returns an empty
Catalogs (i.e., Catalogs::new()). In your test module for lib.rs, construct the
minimal inputs or test harness so workspace_manifest evaluates to None, call the
public entrypoint that contains the let Some(manifest) = workspace_manifest else
{ return Ok(Catalogs::new()); } logic, and assert the returned value equals
Catalogs::new() (or matches expected empty state). Name the test clearly (e.g.,
returns_empty_catalogs_when_manifest_none) and keep it focused only on the None
branch to lock the public contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7830f019-63d7-4bb9-88d3-0faaf30cb8d6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.tomlpacquet/crates/catalogs-config/Cargo.tomlpacquet/crates/catalogs-config/src/lib.rspacquet/crates/catalogs-config/src/tests.rspacquet/crates/catalogs-protocol-parser/Cargo.tomlpacquet/crates/catalogs-protocol-parser/src/lib.rspacquet/crates/catalogs-protocol-parser/src/tests.rspacquet/crates/catalogs-resolver/Cargo.tomlpacquet/crates/catalogs-resolver/src/lib.rspacquet/crates/catalogs-resolver/src/tests.rspacquet/crates/catalogs-types/Cargo.tomlpacquet/crates/catalogs-types/src/lib.rspacquet/crates/cli/tests/install.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-deps-resolver/Cargo.tomlpacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/workspace/Cargo.tomlpacquet/crates/workspace/src/manifest.rspacquet/crates/workspace/src/manifest/tests.rs
✅ Files skipped from review due to trivial changes (3)
- pacquet/crates/catalogs-config/Cargo.toml
- pacquet/crates/catalogs-protocol-parser/Cargo.toml
- pacquet/crates/catalogs-types/Cargo.toml
📜 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). (7)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/workspace/src/manifest/tests.rspacquet/crates/cli/tests/install.rspacquet/crates/catalogs-types/src/lib.rspacquet/crates/catalogs-config/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/catalogs-config/src/tests.rspacquet/crates/catalogs-protocol-parser/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/catalogs-protocol-parser/src/tests.rspacquet/crates/catalogs-resolver/src/lib.rspacquet/crates/catalogs-resolver/src/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/workspace/src/manifest.rs
pacquet/**/{tests,test}/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/{tests,test}/**/*.rs: Prefer real fixtures; reach for the dependency-injection seam only when they can't cover the branch. Most happy paths and error paths should be tested withtempfile::TempDir, the mocked registry, or an integration test that spawns the actual binary.
Use the DI seam — a capability trait on theHostprovider, threaded asSys: <Bounds>— only for branches a real fixture can't reach portably: filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths.
Log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!. Follow the test-logging guidance in CODE_STYLE_GUIDE.md.
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found. If the test needs a tool, let it panic when the tool is absent.
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or#[cfg(unix)]) over a runtime probe-and-skip helper for platform-locked tools.
Port relevant tests from pnpm when porting behavior. Matching test coverage is the easiest way to prove behavioral parity.
When porting behavior from pnpm, consult plans/TEST_PORTING.md before adding ported tests. Useallow_known_failure!at the not-yet-implemented boundary and update checkboxes as items land.
Files:
pacquet/crates/cli/tests/install.rs
🧠 Learnings (2)
📚 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/workspace/src/manifest/tests.rspacquet/crates/cli/tests/install.rspacquet/crates/catalogs-types/src/lib.rspacquet/crates/catalogs-config/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/catalogs-config/src/tests.rspacquet/crates/catalogs-protocol-parser/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/catalogs-protocol-parser/src/tests.rspacquet/crates/catalogs-resolver/src/lib.rspacquet/crates/catalogs-resolver/src/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/workspace/src/manifest.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/workspace/src/manifest/tests.rspacquet/crates/cli/tests/install.rspacquet/crates/catalogs-types/src/lib.rspacquet/crates/catalogs-config/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/catalogs-config/src/tests.rspacquet/crates/catalogs-protocol-parser/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/catalogs-protocol-parser/src/tests.rspacquet/crates/catalogs-resolver/src/lib.rspacquet/crates/catalogs-resolver/src/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/workspace/src/manifest.rs
🔇 Additional comments (20)
pacquet/crates/catalogs-types/src/lib.rs (1)
1-30: LGTM!Cargo.toml (1)
16-19: LGTM!Also applies to: 23-26, 35-35
pacquet/crates/workspace/Cargo.toml (1)
14-14: LGTM!pacquet/crates/catalogs-protocol-parser/src/lib.rs (1)
1-27: LGTM!pacquet/crates/catalogs-protocol-parser/src/tests.rs (1)
1-25: LGTM!pacquet/crates/workspace/src/manifest.rs (1)
26-26: LGTM!Also applies to: 61-70
pacquet/crates/workspace/src/manifest/tests.rs (1)
5-5: LGTM!Also applies to: 69-101
pacquet/crates/catalogs-config/src/lib.rs (1)
1-40: LGTM!Also applies to: 45-81
pacquet/crates/catalogs-config/src/tests.rs (1)
1-68: LGTM!pacquet/crates/catalogs-resolver/Cargo.toml (1)
1-21: LGTM!pacquet/crates/catalogs-resolver/src/lib.rs (1)
1-184: LGTM!pacquet/crates/catalogs-resolver/src/tests.rs (1)
1-187: LGTM!pacquet/crates/package-manager/Cargo.toml (1)
14-15: LGTM!pacquet/crates/package-manager/src/install.rs (1)
10-12: LGTM!Also applies to: 172-182, 256-273, 551-552
pacquet/crates/package-manager/src/install_without_lockfile.rs (1)
10-11: LGTM!Also applies to: 21-34, 89-92, 113-133, 177-178, 205-210, 214-276, 301-329, 342-343, 378-382, 511-513, 630-657
pacquet/crates/resolving-deps-resolver/Cargo.toml (1)
14-15: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
7-12: LGTM!Also applies to: 57-63, 92-99, 101-204, 222-222, 288-288, 321-348, 364-373
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
47-84: LGTM!Also applies to: 94-105, 109-226, 243-344
pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs (1)
85-94: LGTM!Also applies to: 96-827
pacquet/crates/cli/tests/install.rs (1)
314-447: LGTM!
Perfectionist's `single_letter_closure_param` lint (CI-only via Dylint) flagged `|c|` in the box-drawing-strip filter.
Summary
Ports the four upstream
catalogs/*packages to pacquet:Catalog/Catalogsmap aliases plus theDEFAULT_CATALOG_NAMEconstant.parse_catalog_protocol; folds thecatalog:shorthand into"default"to match upstream.resolve_from_catalogwith the four upstreamPnpmErrorcodes:ERR_PNPM_CATALOG_ENTRY_NOT_FOUND_FOR_SPECERR_PNPM_CATALOG_ENTRY_INVALID_RECURSIVE_DEFINITIONERR_PNPM_CATALOG_ENTRY_INVALID_WORKSPACE_SPECERR_PNPM_CATALOG_ENTRY_INVALID_SPECget_catalogs_from_workspace_manifestplus theERR_PNPM_INVALID_CATALOGS_CONFIGURATIONmutual-exclusion check betweencatalog:andcatalogs.default.pacquet-workspace'sWorkspaceManifestnow actually deserializes thecatalogandcatalogsfields — previously they were silently dropped at parse time.Tests are 1:1 ports of the upstream Jest suites:
catalogs/protocol-parser/test/parseCatalogProtocol.test.tscatalogs/resolver/test/resolveFromCatalog.test.tscatalogs/config/test/getCatalogsFromWorkspaceManifest.test.tsNotes
matchCatalogResolveResultvisitor is intentionally not ported. Rust's exhaustivematchonCatalogResolutionResultgives the same exhaustiveness guarantee for free.installing/deps-installer(the upstream caller) hasn't been ported. The crates are ready for that next step.Test plan
cargo nextest run -p pacquet-catalogs-types -p pacquet-catalogs-protocol-parser -p pacquet-catalogs-resolver -p pacquet-catalogs-config -p pacquet-workspacejust checkjust lintjust test(full 1736-test suite, registry mock running)cargo fmtWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests