ci: run clippy as a single-OS job and add it to the pre-push hook#12389
Merged
Conversation
The put_json/put_json_with_token test helpers took the JSON body by value but only borrowed it for serde_json::to_vec, tripping clippy's needless_pass_by_value under --all-targets. Take &Value instead, which also drops an unnecessary body.clone() at one call site.
Clippy was a step inside the three-OS Lint-and-Test matrix, so it ran once per OS even though it lints the same platform-independent source each time. Move it to its own ubuntu-only job, mirroring the existing single-OS doc, format, and dylint jobs (platform-gated cfg blocks are still type-checked per-OS by the test build). It was also missing from pacquet/scripts/pre-push-rust.sh, so a clippy lint that only fires under --all-targets — like the one that just reached main — slipped past local pushes and surfaced only in CI. Add the same --all-targets workspace clippy gate to the hook.
|
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 (4)
📜 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). (6)
🧰 Additional context used📓 Path-based instructions (2)pacquet/**/AGENTS.md📄 CodeRabbit inference engine (pacquet/CLAUDE.md)
Files:
pnpr/**/pnpr/**/*.rs📄 CodeRabbit inference engine (pnpr/AGENTS.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-05-14T17:38:34.573ZApplied to files:
📚 Learning: 2026-06-06T18:58:37.156ZApplied to files:
📚 Learning: 2026-06-12T20:41:57.558ZApplied to files:
🪛 Shellcheck (0.11.0)pacquet/scripts/pre-push-rust.sh[info] 24-24: Expressions don't expand in single quotes, use double quotes for that. (SC2016) 🔇 Additional comments (6)
📝 WalkthroughWalkthroughThe PR restructures Clippy linting by moving it from the main CI test job to a dedicated job, adds matching Clippy checks to the local pre-push script, documents these changes, and refactors test helpers to accept borrowed JSON references instead of owned values. ChangesClippy CI and Local Check Alignment
Test Helper JSON Borrowing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 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 |
Code Review by Qodo
1. Pre-push clippy not locked
|
PR Summary by QodoCI: move clippy to ubuntu-only job and enforce it in pre-push WalkthroughsDescription• Fix a pnpr integration-test helper to avoid a clippy needless_pass_by_value lint. • Add workspace clippy (--all-targets, -D warnings) to the Rust pre-push hook. • Split clippy into a dedicated ubuntu-only CI job to avoid triple matrix cost. Diagramgraph TD
Dev([Developer]) --> Hook["pre-push-rust.sh"] --> ClippyCmd["cargo clippy (--all-targets)"]
GHA(["GitHub Actions"]) --> WF["pacquet-ci.yml"] --> ClippyJob["Clippy job (ubuntu)"] --> ClippyCmd
TestSrc["batch_publish.rs (tests)"] --> ClippyCmd
High-Level AssessmentThe following are alternative approaches to this PR: 1. Keep clippy in the OS matrix but guard it with a conditional
2. Extract common disk-cleanup + Rust toolchain setup into a reusable composite action
Recommendation: The PR’s approach is the best tradeoff: a dedicated ubuntu-only clippy job actually removes redundant work (unlike conditionally skipping within the matrix) and aligns with existing single-OS lint jobs. Consider a follow-up composite action only if disk-cleanup/toolchain setup duplication spreads further. File ChangesTests (1)
Documentation (1)
Other (2)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12389 +/- ##
==========================================
- Coverage 88.32% 88.32% -0.01%
==========================================
Files 298 298
Lines 39034 39034
==========================================
- Hits 34477 34475 -2
- Misses 4557 4559 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Jun 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
After #12299 merged, the Pacquet CI
Clippystep failed onmainwith aclippy::needless_pass_by_valuelint in a pnpr integration test. This PR fixes that lint and closes the two process gaps that let it reachmain.Why it slipped through
pacquet/scripts/pre-push-rust.shranrustfmt,taplo,cargo doc, andcargo dylint— but nevercargo clippy. So a clippy lint could only be caught in CI.cargo clippy -p <crate>doesn't catch it. The failing lint only fires under--all-targets(it's in atests/crate). Without that flag, integration-test lints are invisible locally.Changes
Fix the lint (
pnpr/crates/pnpr/tests/batch_publish.rs): theput_json/put_json_with_tokentest helpers took the JSON body by value but only borrowed it forserde_json::to_vec. Take&Valueinstead (also drops an unnecessarybody.clone()).Add clippy to the pre-push hook with the load-bearing
--all-targets --workspace -- -D warnings, so the same lint CI runs is enforced before a push.Run clippy as its own single-OS job. Clippy was a step inside the three-OS
Lint and Testmatrix, so it ran once per OS (windows + ubuntu + macos) even though it lints the same platform-independent source each time. Move it to a dedicatedubuntu-latestjob, mirroring the existing single-OSdoc,format, anddylintjobs. Platform-gated#[cfg]blocks are still type-checked per-OS by the matrix test build.Update the
pacquet/AGENTS.mddescription of what the hook checks.Verification
Locally, all pre-push Rust gates pass on this branch:
cargo fmt --check,cargo clippy --all-targets --workspace -- -D warnings,cargo doc -D warnings, andcargo dylint. Thebatch_publishintegration tests still pass (8/8).Notes
Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
Chores
Documentation