fix(pacquet): limit settings-only workspaces to root project#12133
Conversation
|
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 (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (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). (9)
🧰 Additional context used📓 Path-based instructions (2)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
pacquet/**/tests/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-22T00:08:44.646ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (7)
📝 WalkthroughWalkthroughThe PR changes callers to compute workspace package glob patterns with workspace_package_patterns(manifest) (re-exported) instead of directly using manifest.packages, clarifies rustdoc for packages/pattern fallbacks, and adds unit and integration tests that assert settings-only workspace manifests enumerate only the root package. ChangesWorkspace Project Discovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoLimit settings-only workspaces to root project enumeration
WalkthroughsDescription• Maps missing packages field to root-only pattern ['.'] • Prevents vendored fixtures from being enumerated as workspace projects • Aligns with pnpm's config-reader default behavior • Adds test for settings-only workspace manifest handling Diagramflowchart LR
A["Settings-only pnpm-workspace.yaml<br/>no packages field"] -->|Previously| B["Passed None to<br/>find_workspace_projects"]
B -->|Used recursive default| C["Enumerated vendored<br/>fixtures as projects"]
A -->|Now| D["Maps None to<br/>root-only pattern"]
D -->|Matches pnpm behavior| E["Enumerates only<br/>root project"]
File Changes1. pacquet/crates/package-manager/src/install.rs
|
There was a problem hiding this comment.
Pull request overview
This PR fixes pacquet workspace project enumeration during install when pnpm-workspace.yaml exists but has no packages: field, ensuring only the workspace root importer is enumerated (instead of falling back to a recursive scan that can mistakenly treat vendored fixtures as workspace projects).
Changes:
- Clarify the tri-state semantics of
WorkspaceManifest.packagesand document that the install path maps an omittedpackages:field to['.']. - Update the install workspace project loader to map
packages: None→Some(['.'])before callingfind_workspace_projects. - Add a regression test ensuring settings-only workspaces enumerate only the root project and ignore nested vendored
package.jsonfiles.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pacquet/crates/workspace/src/manifest.rs |
Updates documentation on packages tri-state semantics and how install interprets omitted packages:. |
pacquet/crates/package-manager/src/install.rs |
Maps settings-only workspaces to root-only patterns (['.']) when enumerating workspace projects for install. |
pacquet/crates/package-manager/src/install/tests.rs |
Adds a regression test covering root-only enumeration for settings-only workspace manifests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12133 +/- ##
==========================================
+ Coverage 88.03% 88.08% +0.05%
==========================================
Files 309 310 +1
Lines 41595 41847 +252
==========================================
+ Hits 36618 36862 +244
- Misses 4977 4985 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.0946876255599998,
"stddev": 0.05909449925187366,
"median": 2.0854762222599996,
"user": 2.69412834,
"system": 3.30280396,
"min": 2.02273372276,
"max": 2.22346660476,
"times": [
2.13762206576,
2.0753928947599998,
2.03565969276,
2.11600018476,
2.0537294087599998,
2.02273372276,
2.06309063776,
2.22346660476,
2.09555954976,
2.12362149376
]
},
{
"command": "pacquet@main",
"mean": 2.07060200066,
"stddev": 0.060944141597729186,
"median": 2.07174814476,
"user": 2.6776213399999995,
"system": 3.29441286,
"min": 1.9929044817600001,
"max": 2.18830870176,
"times": [
2.10215225376,
2.07665814676,
1.9929044817600001,
2.18830870176,
2.06683814276,
2.03679849676,
2.10369083176,
2.00018610776,
2.01781179176,
2.12067105176
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7042388449000001,
"stddev": 0.025009799178858146,
"median": 0.698946724,
"user": 0.40252576000000007,
"system": 1.3620364600000001,
"min": 0.681583515,
"max": 0.7714972050000001,
"times": [
0.7714972050000001,
0.711147822,
0.7006524030000001,
0.6977700440000001,
0.7028395150000001,
0.681583515,
0.69589343,
0.7001234040000001,
0.6872241060000001,
0.693657005
]
},
{
"command": "pacquet@main",
"mean": 0.7489025696,
"stddev": 0.08196222645718236,
"median": 0.7209075185,
"user": 0.40185035999999996,
"system": 1.34982526,
"min": 0.678239186,
"max": 0.9643094860000001,
"times": [
0.765408414,
0.7051165770000001,
0.714991989,
0.702027461,
0.678239186,
0.6995302010000001,
0.766596245,
0.765983089,
0.9643094860000001,
0.7268230480000001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5212394063400003,
"stddev": 0.07841769888979969,
"median": 2.49628303424,
"user": 3.9964573999999997,
"system": 3.1532349399999995,
"min": 2.44069022724,
"max": 2.72189209724,
"times": [
2.53994730424,
2.72189209724,
2.48090717724,
2.50351664124,
2.4807923832400003,
2.46975484924,
2.44069022724,
2.48904942724,
2.53842239924,
2.5474215572400003
]
},
{
"command": "pacquet@main",
"mean": 2.4721689187400004,
"stddev": 0.04596351033346586,
"median": 2.47498540674,
"user": 3.9557124999999997,
"system": 3.12636364,
"min": 2.42483782624,
"max": 2.58250408024,
"times": [
2.47798547424,
2.42933030524,
2.47198533924,
2.48980268224,
2.58250408024,
2.43982746524,
2.48447621924,
2.48224812624,
2.4386916692400002,
2.42483782624
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.68546063486,
"stddev": 0.0213398723569869,
"median": 1.6892905978600001,
"user": 1.8992098799999997,
"system": 1.9225873,
"min": 1.65683424536,
"max": 1.7192684283600002,
"times": [
1.69170239536,
1.70592238036,
1.65683424536,
1.6889908273600003,
1.6717008323600002,
1.68959036836,
1.7047804613600002,
1.65703016636,
1.6687862433600003,
1.7192684283600002
]
},
{
"command": "pacquet@main",
"mean": 1.6922290508600004,
"stddev": 0.0208497940332485,
"median": 1.6947787013600002,
"user": 1.8817086800000002,
"system": 1.9023466,
"min": 1.6615568843600002,
"max": 1.7195528773600002,
"times": [
1.6622277273600001,
1.6862267693600002,
1.6765863883600003,
1.6615568843600002,
1.71421996536,
1.70513204236,
1.70333063336,
1.7074623773600002,
1.7195528773600002,
1.6859948433600003
]
}
]
} |
|
Bencher Report| Branch | pr/12133 |
| 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 | 2,521.24 ms(+6.37%)Baseline: 2,370.16 ms | 2,844.20 ms (88.65%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,685.46 ms(+10.10%)Baseline: 1,530.80 ms | 1,836.96 ms (91.75%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,094.69 ms(+0.16%)Baseline: 2,091.34 ms | 2,509.61 ms (83.47%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 704.24 ms(+6.21%)Baseline: 663.04 ms | 795.65 ms (88.51%) |
7277c82 to
1dbaded
Compare
Address review feedback: the settings-only-workspace root limiting was not applied at every workspace-enumeration entry point. exec_recursive still passed the raw manifest.packages through to find_workspace_projects, falling back to the lower-level ['.', '**'] recursive default. Route it through workspace_package_patterns like install and run_recursive, and cover it with an integration test. Also fix doc-comment placement (read_workspace_manifest doc was stranded above workspace_package_patterns; the explicit packages: [] doc was detached from its test) and add #[must_use] to workspace_package_patterns.
1dbaded to
717da92
Compare
Code Review by Qodo
1. Benchmark test work inflation
|
…stalls slow_start_ramps_per_connection_throughput compares the best-of-N flat and ramped transfer times. The structural ramp gap is well over 100 ms, but with only 3 samples a bursty CI runner can stall every flat sample at once, inflating the flat minimum to within the 25 ms assertion margin and failing the test. Raise the sample count so a near-clean flat reading is effectively guaranteed; the minimum is already the test's noise-resistant estimator.
|
Code review by qodo was updated up to the latest commit 5b7697c |
Fixes pacquet workspace project enumeration when
pnpm-workspace.yamlexists but does not definepackages.A settings-only workspace manifest like this:
should still make the project a workspace, but it should only enumerate the root importer. Pacquet was passing the missing packages field through to the lower-level workspace project finder, which uses the recursive
['.', '**']default. That could accidentally treat vendored fixture packages as workspace projects.This maps absent packages to
['.']in the install path, matching pnpm’s config-readerworkspacePackagePatternsdefault:Summary by CodeRabbit
Bug Fixes
Tests
Documentation