feat(policy): Clippy lint ledgers + check-lint-policy / check-clippy-exceptions (#179)#225
Conversation
…exceptions (#179) First PR in the post-file-policy 0.4.0 rollout. Adds the Clippy ledger infrastructure called for by docs/CLIPPY_POLICY.md. Ledger-only — no lint activation; that's PR 7 (#191). ## Three TOML ledgers under policy/ - **clippy-lints.toml** — authoritative record of active + planned Clippy lints. Mirrors the tables in docs/CLIPPY_POLICY.md: 4 [[active]] entries: rust::unsafe_code, clippy::dbg_macro, clippy::todo, clippy::unimplemented 10 [[planned]] entries: 4 at min_msrv 1.94, 6 at min_msrv 1.95 (the full PR 7 lint set) - **clippy-debt.toml** — empty. Workspace is currently Clippy-clean; real entries arrive when PR 7 activates new lints with measured fallout. - **clippy-exceptions.toml** — empty. Real entries are added intentionally when a specific suppression is judged durable. ## Two xtask checkers ### `cargo xtask check-lint-policy` Validates that MSRV agrees across: - Cargo.toml `[workspace.package] rust-version` - clippy.toml `msrv` - policy/clippy-lints.toml `msrv` Plus: every lint in `[workspace.lints.clippy]` (currently 0) must have a ledger entry. Reverse direction (ledger entries not in [workspace.lints.clippy]) is intentionally not enforced — [[active]] entries are aspirational until PR 7 activates them. Live output: msrv aligned across all three: 1.95 check-lint-policy: workspace.lints.clippy=0 active_in_ledger=4 planned_in_ledger=10 ### `cargo xtask check-clippy-exceptions` For each entry in clippy-exceptions.toml: - validates required fields (lint, path, owner, reason, expires) - fails on expired entries Plus a shallow regex scan for bare `#[allow(clippy::...)]` attributes in tracked Rust source. Each occurrence is reported as an **informational note** (the policy doc requires `#[expect(clippy::lint, reason = "...")]` form), but the scan does NOT fail the check in this PR. PR 5 is ledger-only; converting bare-allows is a future PR. Live output on current main: check-clippy-exceptions: entries=0 expired=0 schema_errors=0 bare_allow_informational=10 The 10 surfaced bare-allow sites in shipper-core/src/engine/ and shipper-core/src/runtime/ are recorded for future conversion in a follow-up PR. ## Decisions - **Debt expiry semantics: fail for exceptions, warn for debt.** This PR only adds the exceptions checker (which fails on expired). Debt expiry handling lands when PR 7 begins populating the debt ledger. - **Bare-allow scan: informational only.** Honest to the ledger-only scope. Future PR converts existing bare-allows to expect+reason and flips the scan to blocking. - **`[[planned]]` reverse-direction not enforced.** Lets PR 7 activate lints from the ledger one at a time without first having to remove their `[[planned]]` entries here. ## Acceptance - cargo check --workspace --locked passes. - cargo clippy -p xtask --all-targets --locked -- -D warnings clean. - cargo fmt --all -- --check clean. - cargo xtask check-lint-policy exits 0. - cargo xtask check-clippy-exceptions exits 0. - cargo xtask policy-report still produces unified artifact. - All three policy/clippy-*.toml files parse as valid TOML. ## Out of scope - PR 7 (#191): activate Clippy 1.94/1.95 ratchet lints (the 10 [[planned]] entries here). - Convert existing bare `#[allow(clippy::...)]` to `#[expect(..., reason = "...")]`. - Wire check-lint-policy / check-clippy-exceptions into CI as blocking checks. Currently they're available via xtask only. Closes #179.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ 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 |
|
Droid finished @EffortlessSteven's task —— View job Review Validation Complete | PR #225 | c221630 Validated Findings: 1 approved, 1 rejected
Review Summary: PR #225 is well-structured with proper TOML schemas and a clean implementation of two xtask checkers. One finding was validated: the Validation methodology: Full diff inspection + SKILL.md Pass 2 validation procedure. All candidate findings cross-referenced against source lines in xtask/src/clippy_checks.rs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2216306f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| l.clippy | ||
| .keys() | ||
| .map(|k| format!("clippy::{k}")) |
There was a problem hiding this comment.
Enforce the recorded rust lint too
When [workspace.lints.rust] unsafe_code is removed or downgraded, cargo xtask check-lint-policy still passes because the check builds its workspace set only from l.clippy.keys(). The new ledger records rust::unsafe_code as an active lint and the repo invariant says unsafe code is forbidden workspace-wide, so this command currently cannot catch the main active lint regressing unless actual unsafe code is introduced later.
Useful? React with 👍 / 👎.
| match (parsed, today_parsed) { | ||
| (Some(d), Some(t)) => d < t, | ||
| _ => date.trim() < today, | ||
| } |
There was a problem hiding this comment.
Reject malformed exception expiry dates
For an exception entry with a mistyped expires value such as never or 2027/05/12, both date parses fail and this falls back to lexicographic comparison, so the schema/expiry check can accept an expiry that will never be reviewed correctly. Since check-clippy-exceptions is the gate that makes exception expiry enforceable, parse failures should be reported as schema errors instead of compared as strings.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| println!( | ||
| "check-clippy-exceptions: entries={} expired=0 schema_errors={} bare_allow_informational={}", |
There was a problem hiding this comment.
Testing inline comment at line 230
Summary
First PR in the post-file-policy 0.4.0 rollout. Adds the Clippy ledger infrastructure called for by
docs/CLIPPY_POLICY.md. Ledger-only — no lint activation; that's PR 7 (#191).Issue
Closes #179. Builds on the now-complete file-policy ladder (#176/#180). Refines #109. Blocks PR 7 (#191 Clippy 1.94/1.95 ratchet activation).
Three TOML ledgers under
policy/clippy-lints.toml[[active]]+ 10[[planned]]lints mirroringdocs/CLIPPY_POLICY.mdtablesclippy-debt.tomlclippy-exceptions.tomlTwo xtask checkers
cargo xtask check-lint-policy— validates MSRV agrees acrossCargo.toml[workspace.package] rust-version,clippy.tomlmsrv, andpolicy/clippy-lints.tomlmsrv. Plus every lint in[workspace.lints.clippy](currently 0) must have a ledger entry. Reverse direction (ledger entries not in[workspace.lints.clippy]) is intentionally not enforced —[[active]]entries are aspirational until PR 7 activates them.cargo xtask check-clippy-exceptions— validatespolicy/clippy-exceptions.tomlschema (lint, path, owner, reason, expires), fails on expired entries, plus runs a shallow regex scan for bare#[allow(clippy::...)]attributes in tracked Rust source. Bare-allow scan is informational only in this PR. The 10 occurrences currently inshipper-core/src/engine/andshipper-core/src/runtime/are recorded for a future conversion PR.Live local output
Decisions
[[planned]]reverse-direction not enforced. Lets PR 7 activate lints from the ledger one at a time without first having to remove their[[planned]]entries here.Acceptance
cargo check --workspace --lockedpasses.cargo clippy -p xtask --all-targets --locked -- -D warningsclean.cargo fmt --all -- --checkclean.cargo xtask check-lint-policyexits 0.cargo xtask check-clippy-exceptionsexits 0.cargo xtask policy-reportstill produces the unified artifact (no regression on PR 9's command).policy/clippy-*.tomlfiles parse as valid TOML.Follow-ups
[[planned]]ledger entries into live[workspace.lints.clippy]entries and measure fallout intoclippy-debt.toml.#[allow(clippy::...)]occurrences to#[expect(..., reason = "...")]. Once clean, flip the bare-allow scan to blocking.check-lint-policyandcheck-clippy-exceptionsinto the CIPolicyjob (separate from the file-policy half).