ci(pacquet): pass --workspace --all-targets to clippy and check#11735
Merged
Conversation
`cargo clippy --locked` and `cargo check --locked` were silently skipping every test, example, and bench target in the workspace because neither command was given `--all-targets`. A virtual workspace defaults to all member packages, so the package set was fine; the target set was not. Nine clippy warnings in three crates' test code (`pacquet-package-manager`, `pacquet-config`, `pacquet-package-is-installable`) had drifted in undetected. The dylint job already used `--all-targets --workspace`; this brings the clippy and check commands in line. Fix the nine warnings (struct-default tightening, type-complexity aliases, `contains_key` over `get(...).is_none()`, doc lazy continuation), then add `--workspace --all-targets` to `pacquet-ci.yml`'s Clippy step, to the `lint` and `check` recipes in `justfile`, and to the documentation in `CONTRIBUTING.md` and `AGENTS.md` that quotes those recipes. `cargo nextest run`, `cargo doc --workspace`, `cargo fmt --all`, `cargo dylint --all -- --all-targets --workspace`, and `cargo deny check` already cover the full workspace, so they need no change.
|
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:
📝 WalkthroughWalkthroughExpands CI and local Rust checks to run across the workspace and all targets; refactors several test helpers with named type aliases and assertion simplifications; applies small docstring and test-callsite adjustments. ChangesCI and build task expansion
Test helper readability
Docstring and small test tweaks
Sequence DiagramsequenceDiagram
participant Justfile as justfile
participant Docs as AGENTS/CONTRIBUTING.md
participant Workflow as .github/workflows/pacquet-ci.yml
participant Clippy as cargo clippy
Justfile->>Clippy: just lint -> cargo clippy --locked --workspace --all-targets -- --deny warnings
Justfile->>Justfile: just check -> cargo check --locked --workspace --all-targets
Workflow->>Clippy: run cargo clippy --locked --workspace --all-targets -- -D warnings
Docs->>Justfile: document updated commands for maintainers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens Pacquet’s Rust CI and local developer workflows so Clippy/check run across the full Cargo workspace and all targets, preventing warnings in tests/examples/benches from being silently skipped.
Changes:
- Update Pacquet CI Clippy step to run with
--workspace --all-targets. - Update
just check/just lintrecipes and corresponding contributor docs to match the full-target invocation. - Fix the newly-surfaced Clippy warnings in affected Pacquet crates/tests (type complexity, map lookups, struct update syntax, doc comment formatting).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/pacquet-ci.yml |
Runs cargo clippy with --workspace --all-targets so all targets are linted in CI. |
justfile |
Aligns check and lint recipes with workspace-wide, all-target Rust validation. |
pacquet/CONTRIBUTING.md |
Updates contributor guidance to reflect the new just check / just lint behavior. |
pacquet/AGENTS.md |
Updates documented just commands to match the workspace/all-target flags. |
pacquet/crates/package-manager/src/link_hoisted_modules/tests.rs |
Introduces type aliases to reduce type complexity in test helpers. |
pacquet/crates/package-manager/src/hoist/tests.rs |
Adds type aliases and replaces get(...).is_none() with !contains_key(...) in assertions. |
pacquet/crates/package-manager/src/build_modules/tests.rs |
Uses struct update syntax to satisfy clippy while keeping test intent intact. |
pacquet/crates/package-is-installable/src/tests.rs |
Adjusts crate-level doc comments to satisfy doc_lazy_continuation. |
pacquet/crates/config/src/defaults/tests.rs |
Tweaks doc comment formatting to satisfy clippy/doc lint expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11735 +/- ##
=======================================
Coverage 89.85% 89.85%
=======================================
Files 145 145
Lines 16424 16424
=======================================
+ Hits 14757 14758 +1
+ Misses 1667 1666 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The Windows clippy job started failing once `--all-targets` began linting test code: `default_store_dir_windows` takes `&Path` and the local bindings were already `&Path` (returned by `Path::new`), so passing `&home_dir` and `¤t_dir` was a needless double borrow.
Diagnostic only: the windows-latest clippy run is failing with no visible annotation. Pipe stdout/stderr to GITHUB_STEP_SUMMARY so the failing diagnostics are readable from the workflow run page. Will revert once the windows-only lint is identified and fixed.
Diagnostic only: the step summary doesn't render through unauthenticated HTTP fetches of the run page, so the bot-readable PR comment is the fallback that surfaces the actual clippy output. Will revert once the windows-only lint is identified and fixed.
Contributor
Clippy log on windows-latest |
`safe_read_package_json_from_dir` is used only by `safe_read_surfaces_non_not_found_io_errors`, a `#[cfg(unix)]`-only test. The unconditional import tripped `-D unused-imports` on windows-latest once `--all-targets` started linting test code. Gate the import with `#[cfg(unix)]` so it lands only when the consumer is compiled. Also revert the workflow diagnostic that mirrored clippy output to a step summary and to a PR comment — the lint is identified and fixed, so the pacquet-ci.yml Clippy step returns to its single-line form.
The earlier fix landed the unix-only gate on `safe_read_package_json_from_dir` but windows-latest still fails. Re-enable the PR-comment diagnostic on the Clippy step so the actual lint output is readable without sign-in. Will revert once the residual lint is fixed.
Contributor
Clippy log on windows-latest |
`LifecycleStdio` and `LogLevel` from `pacquet_reporter`, and `pretty_assertions::assert_eq`, are only used by the `#[cfg(unix)]`-gated lifecycle tests (`lifecycle_emits_script_stdio_and_exit_in_order`). On Windows the unconditional imports tripped `-D unused-imports` once `--all-targets` started linting test code. Split them into `#[cfg(unix)]` imports so they land only when their consumers compile.
Contributor
Clippy log on windows-latest |
| name: Lint and Test | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write |
Contributor
Author
There was a problem hiding this comment.
Agreed on all three points. The Post Clippy log on failure step, the pull-requests: write permission, and the unmodified PR description are all temporary scaffolding — only there to surface the Windows-only clippy errors that --all-targets exposed, because the actual lint diagnostics aren't reachable on the runs page without GitHub auth. The diagnostic will be reverted in the same PR as soon as the windows-latest job is green. Once it's reverted, the workflow goes back to its single-line Clippy step with no extra permissions, and the PR description matches the diff.
Written by an agent (Claude Code, claude-opus-4-7).
Generated by Claude Code
Comment on lines
+88
to
+100
| - name: Clippy | ||
| run: cargo clippy --locked -- -D warnings | ||
| id: clippy | ||
| shell: bash | ||
| run: | | ||
| set +e | ||
| cargo clippy --locked --workspace --all-targets -- -D warnings >clippy.log 2>&1 | ||
| status=$? | ||
| cat clippy.log | ||
| exit "$status" | ||
|
|
||
| - name: Post Clippy log on failure | ||
| if: failure() && steps.clippy.outcome == 'failure' && github.event_name == 'pull_request' | ||
| shell: bash |
Contributor
Author
There was a problem hiding this comment.
Same context as the permissions thread — this comment-posting step is temporary diagnostic scaffolding to surface the Windows clippy errors, and will be reverted in this same PR once the windows-latest job is green. Once reverted, the PR description matches the diff again (just --workspace --all-targets plus the test-import fixes it surfaced).
Written by an agent (Claude Code, claude-opus-4-7).
Generated by Claude Code
Comment on lines
+98
to
+116
| - name: Post Clippy log on failure | ||
| if: failure() && steps.clippy.outcome == 'failure' && github.event_name == 'pull_request' | ||
| shell: bash | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| PR: ${{ github.event.pull_request.number }} | ||
| OS: ${{ matrix.os }} | ||
| run: | | ||
| if [ ! -s clippy.log ]; then | ||
| echo "clippy.log empty or missing — skipping comment" | ||
| exit 0 | ||
| fi | ||
| { | ||
| printf '<!-- clippy-diagnostic:%s -->\n' "$OS" | ||
| printf '## Clippy log on %s\n\n```\n' "$OS" | ||
| head -c 60000 clippy.log | ||
| printf '\n```\n' | ||
| } > comment.md | ||
| gh pr comment "$PR" --repo "${{ github.repository }}" --body-file comment.md |
Contributor
Author
There was a problem hiding this comment.
Same context as the permissions thread — the comment-posting step is temporary diagnostic scaffolding to surface Windows-only clippy errors and will be reverted in this same PR once windows-latest is green. The "new comment per run" and "no fork PR guard" observations are correct, but since the step is short-lived (a handful of runs) I'd rather keep it minimal and pull it entirely than harden it. If the diagnostic ever becomes a permanent fixture I'll switch it to update-in-place via the marker and add the head.repo.full_name == github.repository guard, with continue-on-error to absorb 403s on fork PRs.
Written by an agent (Claude Code, claude-opus-4-7).
Generated by Claude Code
Three more imports were unused on Windows once `--all-targets` started linting test code: - `pacquet-cmd-shim`: `std::fs::metadata` is only used by the unix- permission assertions in `writes_shim_for_bin_string`. - `pacquet-fs`: `symlink_dir` and `std::path::PathBuf` are only used in `unix_symlink_contents_are_relative_to_link_parent` (which is `#[cfg(unix)]`). - `pacquet-package-manager`: `LINK_STATE_COPY` is only used in `auto_respects_cached_copy_state` (`#[cfg(unix)]`). Gate each one with `#[cfg(unix)]` so it compiles only when its consumer does.
Contributor
Clippy log on windows-latest |
Five more items were unused on Windows once `--all-targets` started
linting test code:
- `pacquet-package-manager` build_modules tests:
- `SkippedOptionalPackage` / `SkippedOptionalReason` imports
(only used in `do_not_fail_on_optional_dep_with_failing_postinstall`,
which is `#[cfg(unix)]`).
- `create_failing_postinstall_fixture` helper (called only by
`#[cfg(unix)]` tests).
- `sha512_hex` helper (same).
- `pacquet-cli`:
- `std::fs` in tests/add.rs (only used by the `#[cfg(unix)]`
`should_symlink_correctly` test).
- `pipe_trait::Pipe` in tests/install.rs (only used by a
`#[cfg(unix)] { ... }` block inside `should_install_exec_files`).
Gate each one with `#[cfg(unix)]` so it compiles only when its
consumer does.
| name: Lint and Test | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write |
Contributor
Author
There was a problem hiding this comment.
Acknowledged — same context as the earlier thread on this line. The diagnostic step and the pull-requests: write permission will both be removed from pacquet-ci.yml in this PR before merge, once the windows-latest job is green. I won't merge with either still in place.
Written by an agent (Claude Code, claude-opus-4-7).
Generated by Claude Code
The windows-latest clippy job is now green, so the diagnostic scaffolding added to surface its failures is no longer needed. Drop: - the `pull-requests: write` permission on the test job, - the multi-line Clippy step that mirrored output to `clippy.log`, - the `Post Clippy log on failure` step that posted that log as a PR comment. The Clippy step returns to its single-line form. The workflow now matches the PR description: a one-line change to add `--workspace --all-targets`, plus the test-import gating fixes the new lint scope surfaced.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
pacquet/crates/package-manager/src/build_modules/tests.rs:486
- This test calls
create_failing_postinstall_fixture, which is now#[cfg(unix)](and it also asserts onSkippedOptional*types that are currently only imported on Unix). As written, the test will not compile on non-Unix targets. Either add#[cfg(unix)]to this test (and keep the helper Unix-only), or remove the Unix-only gating from the helper/imports if the behavior should be tested cross-platform.
let pkg_key = key("@pnpm.e2e/failing-postinstall", "1.0.0");
let optional_snapshot = SnapshotEntry { optional: true, ..Default::default() };
let snapshots = HashMap::from([(pkg_key.clone(), optional_snapshot)]);
let importers = root_importers(&[("@pnpm.e2e/failing-postinstall", "1.0.0")]);
pacquet/crates/package-manager/src/build_modules/tests.rs:790
create_failing_postinstall_fixtureis now#[cfg(unix)], but it is invoked from at least one test that is not Unix-gated in this file. That combination breaks non-Unix builds because the helper is not defined. Either remove the#[cfg(unix)]here, or ensure all callers are also behind#[cfg(unix)](and that any related imports are gated consistently).
/// registry, without dragging the lockfile-with-real-integrity
/// machinery into a `BuildModules`-unit test.
#[cfg(unix)]
fn create_failing_postinstall_fixture(virtual_store_dir: &Path, key: &PackageKey) -> PathBuf {
let key_str = key.without_peer().to_string();
Comment on lines
+9
to
+11
| use pacquet_reporter::{IgnoredScriptsLog, LogEvent, Reporter, SilentReporter}; | ||
| #[cfg(unix)] | ||
| use pacquet_reporter::{SkippedOptionalPackage, SkippedOptionalReason}; |
Contributor
Author
There was a problem hiding this comment.
False positive — all three flagged sites are inside #[cfg(unix)] #[test] fn … blocks, so they aren't compiled on Windows and the gated imports/helper match their consumers exactly:
- Line 482 / 486: inside
fn do_not_fail_on_optional_dep_with_failing_postinstall(#[cfg(unix)]at line 470). - Line 786: inside
fn fail_when_failing_postinstall_is_required(#[cfg(unix)]at line 735).
Empirically: Windows CI is green on the latest commit (1a9c8e0). If any reference to SkippedOptionalPackage / SkippedOptionalReason or create_failing_postinstall_fixture weren't gated, the cargo clippy --locked --workspace --all-targets -- -D warnings step would have caught it as cannot find type / cannot find function.
Written by an agent (Claude Code, claude-opus-4-7).
Generated by Claude Code
Contributor
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.3319914096399996,
"stddev": 0.11653751219629327,
"median": 2.28412687204,
"user": 2.7728019999999995,
"system": 3.52210848,
"min": 2.2217431245399997,
"max": 2.5528269345399996,
"times": [
2.3287540665399997,
2.27978834254,
2.2217431245399997,
2.5338390775399997,
2.5528269345399996,
2.24685058254,
2.28609280954,
2.28216093454,
2.25461247754,
2.33324574654
]
},
{
"command": "pacquet@main",
"mean": 2.33265849324,
"stddev": 0.10103405887139433,
"median": 2.2941143240399997,
"user": 2.7101880999999994,
"system": 3.58049928,
"min": 2.23267882254,
"max": 2.51890308854,
"times": [
2.25621607354,
2.31585037654,
2.38570518054,
2.2334114065399997,
2.27237827154,
2.4651737265399998,
2.51890308854,
2.26387764454,
2.38239034154,
2.23267882254
]
},
{
"command": "pnpm",
"mean": 4.57826819464,
"stddev": 0.06116766642701975,
"median": 4.55993436504,
"user": 7.7135105,
"system": 4.03874278,
"min": 4.48268977254,
"max": 4.67031211454,
"times": [
4.67031211454,
4.48268977254,
4.54186324354,
4.58328170454,
4.54537038354,
4.6426307185399995,
4.53536047554,
4.5663518095399995,
4.66130480354,
4.55351692054
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7030940615000001,
"stddev": 0.02280217681784022,
"median": 0.6946537531000001,
"user": 0.38888577999999996,
"system": 1.5990955799999997,
"min": 0.6808642986000001,
"max": 0.7460986536,
"times": [
0.7425654896000001,
0.6915752896,
0.6826337006000001,
0.6808642986000001,
0.6945013076000001,
0.6941560306000001,
0.7026392356000001,
0.7460986536,
0.6948061986,
0.7011004106000001
]
},
{
"command": "pacquet@main",
"mean": 0.7198807119000001,
"stddev": 0.05298543018123544,
"median": 0.6974497761,
"user": 0.39562147999999997,
"system": 1.5928687799999999,
"min": 0.6817591696,
"max": 0.8488599936000001,
"times": [
0.7712973116,
0.6907869876000001,
0.7353233956,
0.6959923436000001,
0.6863638236,
0.6817591696,
0.8488599936000001,
0.6999269336,
0.6989072086000001,
0.6895899516
]
},
{
"command": "pnpm",
"mean": 2.4077490053,
"stddev": 0.1085707323990717,
"median": 2.3808234991,
"user": 2.9547783799999996,
"system": 2.1841407799999994,
"min": 2.3025187166,
"max": 2.6844514016,
"times": [
2.4716607906,
2.3025187166,
2.3391360356,
2.6844514016,
2.4269443516,
2.3524884706,
2.3717160796,
2.3404514036,
2.3899309186,
2.3981918846
]
}
]
} |
zkochan
approved these changes
May 19, 2026
This was referenced May 21, 2026
This was referenced Jun 13, 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
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
cargo clippy --locked -- -D warningswas missing--all-targets, so test, example, and bench targets across the workspace were silently skipped. Virtual-workspace defaults already covered every member package, but the target gap had let nine clippy warnings drift in undetected acrosspacquet-package-manager,pacquet-config, andpacquet-package-is-installable.This PR:
--workspace --all-targetsto the Clippy step in.github/workflows/pacquet-ci.yml.--workspace --all-targetsto thelintandcheckrecipes injustfile, plus the references to them inpacquet/CONTRIBUTING.mdandpacquet/AGENTS.md.type_complexityaliases, threecontains_keyrewrites in place ofget(...).is_none(), and threedoc_lazy_continuationdoc-comment fixes).The
dylintjob already used--all-targets --workspace; this brings clippy and check in line.cargo nextest run,cargo doc --workspace,cargo fmt --all, andcargo deny checkalready cover the full workspace and need no change.Test plan
cargo clippy --locked --workspace --all-targets -- -D warningsis clean.cargo check --locked --workspace --all-targetsis clean.cargo fmt --all -- --checkis clean.hoist::tests,failing_postinstall_*,resolve_child_concurrency_*,package_is_installable::*) all pass.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Documentation
Chores
Refactor
Tests