feat(pacquet/cli): promote --filter/--filter-prod to recursive mode#12726
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughPromotes ChangesFilter promotion and recursive root exclusion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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.
🧹 Nitpick comments (3)
pacquet/crates/cli/tests/exec_recursive.rs (1)
107-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse regular comments here instead of rustdoc.
This
///block documents a private test and mostly restates what the assertions already cover. As per coding guidelines, "Use doc comments for rustdoc-visible API contracts, and use regular//comments only for non-obvious implementation rationale" and "Do not duplicate tests in prose: if a behavior is already covered by a test, keep the doc comment focused on the contract rather than re-narrating the test case." As per path instructions, "Doc/comment guidance: write rustdoc on public item contracts; use normal//for implementation-only rationale" and "Doc comments are contracts: don’t duplicate what the test already proves; keep tests as the 'behavior documentation.'"🤖 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/exec_recursive.rs` around lines 107 - 109, Replace the rustdoc-style `///` comment in `exec_recursive` with a regular `//` comment, since this is private test-only rationale rather than a public API contract. Keep the comment brief and focused on the non-obvious behavior around bare `--filter` promoting recursive mode in the CLI, and avoid restating what the test assertions already verify.Sources: Coding guidelines, Path instructions
pacquet/crates/cli/tests/pack_recursive.rs (1)
62-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse regular comments here instead of rustdoc.
This
///block documents a private test and mostly restates what the assertions already cover. As per coding guidelines, "Use doc comments for rustdoc-visible API contracts, and use regular//comments only for non-obvious implementation rationale" and "Do not duplicate tests in prose: if a behavior is already covered by a test, keep the doc comment focused on the contract rather than re-narrating the test case." As per path instructions, "Doc/comment guidance: write rustdoc on public item contracts; use normal//for implementation-only rationale" and "Doc comments are contracts: don’t duplicate what the test already proves; keep tests as the 'behavior documentation.'"🤖 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/pack_recursive.rs` around lines 62 - 65, The comment above the private test in pack_recursive.rs is using rustdoc syntax where a regular comment should be used; replace the current `///` block with `//` comments, or remove it if the assertions already document the behavior. Keep the explanation as implementation/test rationale only, and avoid restating what `test_pack_recursive` and its assertions already verify.Sources: Coding guidelines, Path instructions
pacquet/crates/cli/tests/run_recursive.rs (1)
205-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse regular comments here instead of rustdoc.
These
///blocks annotate private test helpers/tests and mostly restate behavior that the test bodies already document. As per coding guidelines, "Use doc comments for rustdoc-visible API contracts, and use regular//comments only for non-obvious implementation rationale" and "Do not duplicate tests in prose: if a behavior is already covered by a test, keep the doc comment focused on the contract rather than re-narrating the test case." As per path instructions, "Doc/comment guidance: write rustdoc on public item contracts; use normal//for implementation-only rationale" and "Doc comments are contracts: don’t duplicate what the test already proves; keep tests as the 'behavior documentation.'"Also applies to: 230-232, 264-267, 285-289
🤖 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/run_recursive.rs` around lines 205 - 208, Replace the rustdoc-style `///` comments in the recursive run tests with regular `//` comments, since these private test helpers and test cases are not public API contracts. Update the comment blocks around the workspace setup and recursive run scenarios in `run_recursive` to be implementation rationale only, and avoid restating what the test bodies already make clear; keep the focus on the intent of helpers like the workspace fixture setup and the recursive selection behavior.Sources: Coding guidelines, Path instructions
🤖 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/exec_recursive.rs`:
- Around line 107-109: Replace the rustdoc-style `///` comment in
`exec_recursive` with a regular `//` comment, since this is private test-only
rationale rather than a public API contract. Keep the comment brief and focused
on the non-obvious behavior around bare `--filter` promoting recursive mode in
the CLI, and avoid restating what the test assertions already verify.
In `@pacquet/crates/cli/tests/pack_recursive.rs`:
- Around line 62-65: The comment above the private test in pack_recursive.rs is
using rustdoc syntax where a regular comment should be used; replace the current
`///` block with `//` comments, or remove it if the assertions already document
the behavior. Keep the explanation as implementation/test rationale only, and
avoid restating what `test_pack_recursive` and its assertions already verify.
In `@pacquet/crates/cli/tests/run_recursive.rs`:
- Around line 205-208: Replace the rustdoc-style `///` comments in the recursive
run tests with regular `//` comments, since these private test helpers and test
cases are not public API contracts. Update the comment blocks around the
workspace setup and recursive run scenarios in `run_recursive` to be
implementation rationale only, and avoid restating what the test bodies already
make clear; keep the focus on the intent of helpers like the workspace fixture
setup and the recursive selection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6186e92e-c4c9-44d5-9152-66feeeb43dd0
📒 Files selected for processing (10)
pacquet/crates/cli/src/cli_args/cli_command.rspacquet/crates/cli/src/cli_args/exec/recursive.rspacquet/crates/cli/src/cli_args/pack.rspacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/src/cli_args/run/recursive.rspacquet/crates/cli/src/cli_args/tests.rspacquet/crates/cli/src/lib.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/tests/pack_recursive.rspacquet/crates/cli/tests/run_recursive.rs
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12726 +/- ##
==========================================
+ Coverage 85.29% 85.31% +0.02%
==========================================
Files 404 405 +1
Lines 61572 61633 +61
==========================================
+ Hits 52515 52580 +65
+ Misses 9057 9053 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Integrated-Benchmark Report (Linux)Commit: 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": 4.02209224996,
"stddev": 0.19013289814294485,
"median": 4.02071929476,
"user": 3.73462958,
"system": 3.4123263800000005,
"min": 3.79756688826,
"max": 4.32181794226,
"times": [
4.20261006826,
4.00013498526,
4.20246347626,
3.80732970226,
4.32181794226,
3.89731209926,
4.04130360426,
3.81233678926,
4.13804694426,
3.79756688826
]
},
{
"command": "pacquet@main",
"mean": 3.9202123776600004,
"stddev": 0.16815620336951734,
"median": 3.8713173342600005,
"user": 3.6754533800000004,
"system": 3.42675968,
"min": 3.70079030526,
"max": 4.15702621226,
"times": [
3.8733833842600003,
3.70079030526,
3.95718944526,
4.11548035626,
3.73383667026,
3.8629227202600003,
4.15702621226,
4.14464554226,
3.86925128426,
3.78759785626
]
},
{
"command": "pnpr@HEAD",
"mean": 2.1927552207599996,
"stddev": 0.15948108046503987,
"median": 2.13684302176,
"user": 2.71546258,
"system": 2.98847508,
"min": 1.9898620252600001,
"max": 2.4105352942600002,
"times": [
2.38312096926,
2.4105352942600002,
2.06888909826,
2.13137872426,
2.3023409632600003,
2.14230731926,
2.07999280926,
2.04054954326,
2.37857546126,
1.9898620252600001
]
},
{
"command": "pnpr@main",
"mean": 2.17411892546,
"stddev": 0.1009650622216487,
"median": 2.20209060426,
"user": 2.6252548799999995,
"system": 3.01411638,
"min": 2.02835451726,
"max": 2.32166324326,
"times": [
2.32166324326,
2.20374101526,
2.22720429626,
2.03824717126,
2.22111521326,
2.07141273326,
2.1442911752600002,
2.02835451726,
2.2847196962600003,
2.20044019326
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6529260981,
"stddev": 0.01902950197238337,
"median": 0.6570836883,
"user": 0.38940648,
"system": 1.3259572399999997,
"min": 0.6286145863,
"max": 0.6831973143,
"times": [
0.6507654993,
0.6637526643,
0.6708052243,
0.6368700473,
0.6371418513,
0.6634018773,
0.6295877083,
0.6286145863,
0.6831973143,
0.6651242083
]
},
{
"command": "pacquet@main",
"mean": 0.6417052634999999,
"stddev": 0.029948170326403848,
"median": 0.6309464083,
"user": 0.37658188,
"system": 1.32526214,
"min": 0.6197803803,
"max": 0.7171245633,
"times": [
0.6665047363,
0.6197803803,
0.6210931423,
0.6328797313,
0.6362052313000001,
0.6227342613,
0.6267511343,
0.7171245633,
0.6449663693,
0.6290130853
]
},
{
"command": "pnpr@HEAD",
"mean": 0.7103463969000001,
"stddev": 0.02414131225660412,
"median": 0.7113005638000001,
"user": 0.39781837999999997,
"system": 1.3693848400000002,
"min": 0.6800526223,
"max": 0.7579014283000001,
"times": [
0.6852107603,
0.6855797703000001,
0.6800526223,
0.7151885843,
0.7306911923,
0.7178845233,
0.7243011953,
0.6992413493,
0.7074125433,
0.7579014283000001
]
},
{
"command": "pnpr@main",
"mean": 0.7257302360999999,
"stddev": 0.08986442250754326,
"median": 0.7036430203,
"user": 0.39796378,
"system": 1.35215474,
"min": 0.6599898873,
"max": 0.9731281773,
"times": [
0.6599898873,
0.7047140293,
0.7025720113,
0.6796297453,
0.6764013883,
0.6971841943,
0.7157089953,
0.7053450703,
0.7426288623,
0.9731281773
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.3253521822400005,
"stddev": 0.025450058508069657,
"median": 4.31464130174,
"user": 3.8242467400000004,
"system": 3.3925098,
"min": 4.29224934924,
"max": 4.36004658424,
"times": [
4.36004658424,
4.35584822324,
4.34380761424,
4.30767031024,
4.31094159324,
4.30782073124,
4.354511995239999,
4.31834101024,
4.29224934924,
4.30228441124
]
},
{
"command": "pacquet@main",
"mean": 4.29139221184,
"stddev": 0.0301245333090638,
"median": 4.28859030574,
"user": 3.7674450400000006,
"system": 3.3674414999999995,
"min": 4.25399551624,
"max": 4.34369879124,
"times": [
4.327430886239999,
4.28911063224,
4.320523866239999,
4.34369879124,
4.25399551624,
4.26736430024,
4.26504907924,
4.266942376239999,
4.288069979239999,
4.29173669124
]
},
{
"command": "pnpr@HEAD",
"mean": 2.22880207614,
"stddev": 0.12059681615061908,
"median": 2.1803460147400004,
"user": 2.54063104,
"system": 2.9142432,
"min": 2.10880186724,
"max": 2.47908625224,
"times": [
2.1818964382400003,
2.10880186724,
2.22695732024,
2.3867809702400002,
2.1310446032400003,
2.28638944724,
2.17879559124,
2.16322139824,
2.14504687324,
2.47908625224
]
},
{
"command": "pnpr@main",
"mean": 2.20264279444,
"stddev": 0.09087019759356965,
"median": 2.1639497742400002,
"user": 2.4846591400000007,
"system": 2.9224412,
"min": 2.06433787424,
"max": 2.32766885424,
"times": [
2.1538863312400003,
2.31563069724,
2.14694521524,
2.06433787424,
2.32144230024,
2.17401321724,
2.1520646072400003,
2.14513941924,
2.32766885424,
2.22529942824
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.36305188684,
"stddev": 0.02535472555000429,
"median": 1.35839582474,
"user": 1.3430307400000001,
"system": 1.6978165,
"min": 1.3388074192400001,
"max": 1.42707695624,
"times": [
1.36169398724,
1.3500186222400001,
1.35929105624,
1.3641301782400002,
1.42707695624,
1.3801542772400002,
1.3388074192400001,
1.34682429824,
1.34502148024,
1.35750059324
]
},
{
"command": "pacquet@main",
"mean": 1.37259850964,
"stddev": 0.08060461172327053,
"median": 1.34713060524,
"user": 1.33422374,
"system": 1.6898985,
"min": 1.33655770024,
"max": 1.59978359124,
"times": [
1.35484326224,
1.3394179482400002,
1.59978359124,
1.3603369412400002,
1.36683107124,
1.33740104724,
1.35515970124,
1.33854141724,
1.33711241624,
1.33655770024
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6757896546400001,
"stddev": 0.03518762514637291,
"median": 0.66666265824,
"user": 0.34202104,
"system": 1.2998647,
"min": 0.6528873822400001,
"max": 0.7723726582400001,
"times": [
0.6549958462400001,
0.7723726582400001,
0.6768185882400001,
0.6528873822400001,
0.6555928332400001,
0.6591362772400001,
0.6775798442400001,
0.6751878002400001,
0.66569541324,
0.66762990324
]
},
{
"command": "pnpr@main",
"mean": 0.6650502307400001,
"stddev": 0.029253798588574208,
"median": 0.65872777674,
"user": 0.33916154000000004,
"system": 1.2806324999999998,
"min": 0.63902515924,
"max": 0.7400335562400001,
"times": [
0.65902522624,
0.6617315882400001,
0.6431989572400001,
0.67830212024,
0.67209571424,
0.6568839782400001,
0.6584303272400001,
0.6417756802400001,
0.63902515924,
0.7400335562400001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 3.0542110576199994,
"stddev": 0.042788220929443156,
"median": 3.0531736271199996,
"user": 1.8183598800000003,
"system": 1.9612393000000001,
"min": 3.00939495612,
"max": 3.1562265091199997,
"times": [
3.0100819891199997,
3.0211783681199997,
3.00939495612,
3.06188434212,
3.03989164412,
3.05540243812,
3.0532452381199997,
3.05310201612,
3.1562265091199997,
3.0817030751199996
]
},
{
"command": "pacquet@main",
"mean": 3.0352990549199994,
"stddev": 0.052713047387347015,
"median": 3.03230362862,
"user": 1.81053518,
"system": 1.9670457,
"min": 2.96639409912,
"max": 3.1673851861199998,
"times": [
3.0375447201199997,
3.04613918512,
3.03662336712,
3.02214651812,
3.03768676312,
2.96639409912,
2.98702254012,
3.0240642801199997,
3.1673851861199998,
3.02798389012
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6789485432200001,
"stddev": 0.015887903183536327,
"median": 0.6768272806200002,
"user": 0.34293007999999997,
"system": 1.3283521999999999,
"min": 0.6598869621200001,
"max": 0.7018451751200001,
"times": [
0.7018451751200001,
0.66626281212,
0.6624625961200001,
0.7014568791200001,
0.6598869621200001,
0.6817924021200001,
0.6665871961200001,
0.6839223471200001,
0.6934069031200001,
0.6718621591200001
]
},
{
"command": "pnpr@main",
"mean": 0.6697162191200001,
"stddev": 0.01629191166171556,
"median": 0.6673411336200001,
"user": 0.34064468,
"system": 1.2892111,
"min": 0.6519251061200001,
"max": 0.7100180641200001,
"times": [
0.6630028361200001,
0.6654791621200001,
0.6519251061200001,
0.6692031051200001,
0.7100180641200001,
0.6554468001200001,
0.6611453451200001,
0.6706105241200001,
0.6800401291200001,
0.6702911191200001
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/12726 |
| 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,325.35 ms(-8.99%)Baseline: 4,752.45 ms | 5,702.95 ms (75.84%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 3,054.21 ms(-0.42%)Baseline: 3,067.07 ms | 3,680.49 ms (82.98%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,363.05 ms(+0.04%)Baseline: 1,362.51 ms | 1,635.02 ms (83.37%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,022.09 ms(-16.44%)Baseline: 4,813.48 ms | 5,776.18 ms (69.63%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 652.93 ms(+1.40%)Baseline: 643.90 ms | 772.69 ms (84.50%) |
Contributor
|
| Branch | pr/12726 |
| 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,228.80 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 678.95 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 675.79 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,192.76 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 710.35 ms |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cli/tests/exec_recursive.rs`:
- Around line 145-158: The recursive exec test is currently vacuous because
using `true` only checks that the command failed and stderr mentioned the
selector error, but it does not prove `exec` was not dispatched. Update the test
in `exec_recursive.rs` to use a marker-writing command instead of `true`, then
assert the marker file or side effect was not created so the failure path is
observable. Keep the existing selector error assertions around the `pacquet`
invocation and ensure the regression is anchored on the `exec` path and its
selector validation behavior.
In `@pacquet/crates/cli/tests/run_recursive.rs`:
- Around line 462-475: The current regression only checks stderr for the
unsupported selector path, but it does not prove that run_recursive::run
actually stops before invoking build. Add a negative assertion in this test that
uses a unique marker from the executed command path to verify build never starts
when the [main] filter is rejected, so the test covers the contract in
run_recursive.rs rather than only the error message.
🪄 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: 0dba16f4-7e10-48bb-8da5-5ef42eeb8f89
📒 Files selected for processing (4)
pacquet/crates/cli/src/cli_args/recursive.rspacquet/crates/cli/tests/exec_recursive.rspacquet/crates/cli/tests/pack_recursive.rspacquet/crates/cli/tests/run_recursive.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- pacquet/crates/cli/tests/pack_recursive.rs
- pacquet/crates/cli/src/cli_args/recursive.rs
A bare `--filter` / `--filter-prod` (without `-r`/`--recursive`) now puts
the command into recursive mode CLI-wide, matching pnpm's parse-cli-args
promotion. This is the prerequisite for driving release.yml, which runs
`pn publish --filter=<pkg>` without `-r`.
Also ports the `!{<workspace-root>}` augmentation from pnpm's main
dispatch: for run/exec, an all-exclusion (or unfiltered) selection
auto-excludes the workspace root unless the workspace is root-only
(patterns === ['.']). The recursive selection now filters with glob
directory matching (pnpm's useGlobDirFiltering default), which is
load-bearing for that augmentation: it excludes only the project at the
workspace root, not every nested package. pack stays out of the
auto-exclusion command set, matching pnpm.
Refs #12723
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SBAmoo35Kr6K96ie7vG8tF
Strengthens coverage of the recursive root auto-exclusion, surfaced by review:
- `recursive_pack_includes_workspace_root` pins the
`AutoExcludeRoot::Disabled` contract — a recursive `pack` keeps the
workspace root. This was previously indistinguishable from `Enabled`
because no test set up a root package.
- `recursive_run_from_subdirectory_still_excludes_root` exercises the
non-trivial relative-path branch of the `!{<workspace-root>}`
augmentation (running `-r run` with `--dir packages/<pkg>`), which had
no coverage.
Also documents that pnpm's `--include-workspace-root` / `--workspace-root`
gates are not ported because pacquet surfaces neither flag yet.
Refs #12723
Pins the load-bearing `follow_prod_deps_only` flag on the
`!{<workspace-root>}` augmentation, surfaced by review. A
`--filter-prod`-only all-exclusion must route the root exclusion into the
same production-only selection pass as the user's exclusion; otherwise the
union of the prod and non-prod passes re-adds both the workspace root and
the excluded package, silently running the script everywhere.
Refs #12723
The `select_recursive_projects(...)?` in the recursive `run` / `exec` runners propagates the filter engine's error, which a coverage report flagged as uncovered. A `[<since>]` changed-packages selector is rejected with `UnsupportedDiffSelector` (pacquet has not ported git-diff project selection), and that error travels out through the `?`. Add integration tests that pass `--filter [main]` to recursive `run` / `exec` and assert the error surfaces (non-zero exit plus the explanatory message) rather than being swallowed. A real fixture (a `[<since>]` selector string) reaches this branch, so per pacquet/AGENTS.md these are plain integration tests, not the dependency- injection seam (which is reserved for branches a real fixture cannot reach, e.g. filesystem error kinds or time). Refs #12723
…cted selector Strengthen the `[<since>]` diff-selector regression tests so they prove the command short-circuits before dispatch, not merely that stderr names the error. `run` asserts the build script's `ran.txt` marker is absent, and `exec` now runs a marker-writing command (`touch ran.txt`) and asserts the marker is absent. Per REVIEW_GUIDE.md, regression assertions must observe the changed behavior rather than only the error message. Refs #12723
The comment above `promote_recursive_for_filter()` in main restated the function's own doc comment clause-for-clause (the recursive promotion, the `parse-cli-args` parity, and the place-once-before-dispatch rationale all already live on the `///`). Per the repo comment convention, a call site must not re-explain what the callee documents; the method name carries the intent and the doc comment carries the why.
77369a2 to
b65e82a
Compare
zkochan
approved these changes
Jun 29, 2026
Code Review by Qodo
Context used 1. Outdated fails with filter
|
Contributor
Author
|
Thanks Qodo. I investigated both findings — both describe faithful ports of pnpm's own behavior, so per pacquet's cardinal rule (match pnpm exactly; fix any divergence upstream first) neither is changed here. 1. Root-exclusion selector const relativeWSDirPath = () => path.relative(process.cwd(), wsDir) || '.'
filters.push({ filter: `!{${relativeWSDirPath()}}`, followProdDepsOnly: Boolean(config.filterProd.length) })So adding escaping in pacquet would deviate from pnpm. It's also not reachable in normal use: 2. Recursive selection routes the no-filter case through the filter engine (perf). This also mirrors pnpm: upstream pushes the synthetic Written by an agent (Claude Code). Generated by Claude Code |
|
Code review by qodo was updated up to the latest commit b65e82a |
Contributor
Author
|
On the new finding #1 "Outdated fails with filter" (findings #2 and #3 are addressed in my earlier comment): This is the intended, pnpm-faithful behavior, not a regression introduced by the promotion:
The one fair sub-point is wording: the guard message names only Written by an agent (Claude Code). Generated by Claude Code |
3 tasks
KSXGitHub
pushed a commit
that referenced
this pull request
Jun 29, 2026
Brings in the filter->recursive promotion (#12726), the `bin` (#12703) and `repo` (#12702) commands, and a pnpr cold-store perf fix (#12709). Resolved one conflict in cli_args/dispatch_query.rs: the publish and bin handlers and their imports were both added at the same spot, so both were kept. Adapted recursive publish to #12726's new shared-function signatures: `discover_workspace_projects` now returns `(projects, patterns)` and `select_recursive_projects` takes an `AutoExcludeRoot`. Publish passes `AutoExcludeRoot::Disabled` because it is not in pnpm's run/exec/add/test root-auto-exclusion set; its private/unnamed eligibility check drops the workspace root instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KtBQzmLLDU3RcGzzCMopPB
KSXGitHub
pushed a commit
that referenced
this pull request
Jun 29, 2026
With the filter->recursive promotion merged (#12726), `pacquet publish --filter=<sel>` enters recursive mode without `-r` — the shape pnpm's `release.yml` publishes the monorepo with. Add two integration tests mirroring that: a bare `--filter` that matches nothing is a recursive no-op, and an exclusion selector (`--filter=!<pkg>`) over an all-private workspace records an empty `publishedPackages`. Both fall through to the single-package path and fail if the promotion is missing or the publish handler ignores the promoted recursive flag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KtBQzmLLDU3RcGzzCMopPB
3 tasks
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
4 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
Ports pnpm's CLI-wide
--filter/--filter-prod→ recursive promotion to pacquet. A bare--filter(or--filter-prod), without-r/--recursive, now puts the command into recursive mode for every command, matching pnpm'sparse-cli-args. This is the prerequisite for drivingrelease.yml, which publishes throughpn publish --filter=<pkg>with no-r.It also ports the
!{<workspace-root>}augmentation from pnpm'smain.ts: forrun/exec, an all-exclusion selection (e.g.--filter=!pnpm --filter=!@pnpm/exe) or an unfiltered recursive run auto-excludes the workspace root unless the workspace is root-only (packages === ['.']). To match that faithfully, the recursive selection now filters with pnpm's default glob directory matching (useGlobDirFiltering), so!{<root>}drops only the project at the workspace root, not every nested package.packstays out of the auto-exclusion command set, matching pnpm.--include-workspace-root/--workspace-rootare not handled because pacquet does not surface either flag yet; this is noted in code.Resolves #12723.
Squash Commit Body
Checklist
pacquet/port. The TypeScript pnpm CLI already implements this behavior(
cli/parse-cli-argspromotion and thepnpm/src/main.tsaugmentation);this PR ports it to pacquet to restore parity.
--filterpromotion for
run/exec/pack;runroot auto-exclusion (default,all-exclusion,
--filter-prodall-exclusion, and from a subdirectory);packkeeping the root; and the unsupported-[since]-selector error pathout of the recursive runners.
PR description maintained by an agent (Claude Code).
Summary by CodeRabbit
--filterand--filter-prodnow automatically enable recursive execution, even without-r.pnpm-workspace.yamlpatterns to more accurately handle workspace roots and exclusions.pack,run, andexecno longer include the workspace root when it shouldn’t be selected.exec/runwith unsupported diff-style selectors now fails reliably with clear errors.--dir), and selector error handling.