Skip to content

feat(policy): Clippy lint ledgers + check-lint-policy / check-clippy-exceptions (#179)#225

Merged
EffortlessSteven merged 1 commit into
mainfrom
feat/xtask-clippy-ledgers-20260512
May 12, 2026
Merged

feat(policy): Clippy lint ledgers + check-lint-policy / check-clippy-exceptions (#179)#225
EffortlessSteven merged 1 commit into
mainfrom
feat/xtask-clippy-ledgers-20260512

Conversation

@EffortlessSteven

Copy link
Copy Markdown
Member

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/

File Contents
clippy-lints.toml 4 [[active]] + 10 [[planned]] lints mirroring docs/CLIPPY_POLICY.md tables
clippy-debt.toml Empty (workspace is currently Clippy-clean)
clippy-exceptions.toml Empty (no durable exceptions yet)

Two xtask checkers

cargo xtask check-lint-policy — validates MSRV agrees across Cargo.toml [workspace.package] rust-version, clippy.toml msrv, and 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.

cargo xtask check-clippy-exceptions — validates policy/clippy-exceptions.toml schema (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 in shipper-core/src/engine/ and shipper-core/src/runtime/ are recorded for a future conversion PR.

Live local output

msrv aligned across all three: 1.95
check-lint-policy: workspace.lints.clippy=0 active_in_ledger=4 planned_in_ledger=10
check-clippy-exceptions: entries=0 expired=0 schema_errors=0 bare_allow_informational=10

Decisions

  • Debt expiry: fail for exceptions, warn for debt. This PR adds only the exceptions checker (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 the unified artifact (no regression on PR 9's command).
  • All three policy/clippy-*.toml files parse as valid TOML.

Follow-ups

  • PR for PR 6: Rust 1.95 rustc lint floor #198 — Rust 1.95 rustc lint floor (next in the queue).
  • PR for PR 7: Clippy 1.94/1.95 ratchets #191 — Clippy 1.94/1.95 ratchet activation. Will turn [[planned]] ledger entries into live [workspace.lints.clippy] entries and measure fallout into clippy-debt.toml.
  • Convert the 10 bare #[allow(clippy::...)] occurrences to #[expect(..., reason = "...")]. Once clean, flip the bare-allow scan to blocking.
  • Wire check-lint-policy and check-clippy-exceptions into the CI Policy job (separate from the file-policy half).

…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.
@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@coderabbitai

coderabbitai Bot commented May 12, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@EffortlessSteven has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 25 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 72906089-bd75-49f4-9603-c5f384fa136a

📥 Commits

Reviewing files that changed from the base of the PR and between a740dc5 and c221630.

📒 Files selected for processing (5)
  • policy/clippy-debt.toml
  • policy/clippy-exceptions.toml
  • policy/clippy-lints.toml
  • xtask/src/clippy_checks.rs
  • xtask/src/main.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/xtask-clippy-ledgers-20260512

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@factory-droid

factory-droid Bot commented May 12, 2026

Copy link
Copy Markdown

Droid finished @EffortlessSteven's task —— View job


Review Validation Complete | PR #225 | c221630

Validated Findings: 1 approved, 1 rejected

Status Path Line Priority Finding
✅ approved xtask/src/clippy_checks.rs 230 P2 Hardcoded expired=0 in summary print — diagnostic staleness
❌ rejected xtask/src/clippy_checks.rs 311 P3 date_is_past string-comparison fallback — not applicable to TOML-sourced dates

Review Summary: PR #225 is well-structured with proper TOML schemas and a clean implementation of two xtask checkers. One finding was validated: the expired=0 summary counter is hardcoded instead of reflecting actual expiry findings — functionally harmless since errors.len() feeds the bail message, but diagnostically misleading. One candidate was rejected as theoretical and not applicable to TOML-sourced date strings.

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +127 to +129
l.clippy
.keys()
.map(|k| format!("clippy::{k}"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Comment on lines +315 to +318
match (parsed, today_parsed) {
(Some(d), Some(t)) => d < t,
_ => date.trim() < today,
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@factory-droid factory-droid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing the submit review tool with correct parameter format

}

println!(
"check-clippy-exceptions: entries={} expired=0 schema_errors={} bare_allow_informational={}",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing inline comment at line 230

@EffortlessSteven EffortlessSteven merged commit c67e94c into main May 12, 2026
26 checks passed
@EffortlessSteven EffortlessSteven deleted the feat/xtask-clippy-ledgers-20260512 branch May 12, 2026 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PR 5: Clippy policy ledgers infrastructure

1 participant