diff --git a/.dockerignore b/.dockerignore index a40610d17..364182f0e 100644 --- a/.dockerignore +++ b/.dockerignore @@ -45,9 +45,11 @@ # ── Dev tooling (not needed in any build stage) ─────────────────────────────── # su-exec is compiled in the gcc stage: COPY ./contrib/dev-tools/su-exec/ # workspace-coupling/Cargo.toml is copied in the recipe stage for cargo chef prepare +# clippy-allow-reasons/Cargo.toml is copied in the recipe stage for cargo chef prepare /contrib/dev-tools/ !/contrib/dev-tools/su-exec/ !/contrib/dev-tools/analysis/workspace-coupling/Cargo.toml +!/contrib/dev-tools/checks/clippy-allow-reasons/Cargo.toml # ── Build artifacts and runtime state ───────────────────────────────────────── /bin/ diff --git a/.github/agents/clippy-fixer.agent.md b/.github/agents/clippy-fixer.agent.md index e41761195..75e39842c 100644 --- a/.github/agents/clippy-fixer.agent.md +++ b/.github/agents/clippy-fixer.agent.md @@ -13,7 +13,9 @@ You are the repository's Clippy warning fixer agent. Your job is to analyze clip - Follow `AGENTS.md` for repository-wide behavior - Always prefer applying clippy suggestions over adding `#[allow(...)]` attributes -- When allowances are needed, **always document the reason** in a clear comment +- For new or modified `#[allow(clippy::...)]` attributes, use a specific native + `reason = "..."` parameter. Temporary reasons also need a stable issue reference or explicit + non-empty removal condition. - Create **atomic commits** for each clippy type warning (e.g., one commit per `explicit_iter_loop` issue) - Link to the specific clippy warning in commit messages for traceability - Use the `Committer` agent for final commits @@ -23,7 +25,7 @@ You are the repository's Clippy warning fixer agent. Your job is to analyze clip 1. **Analyze clippy output**: Receive clippy warnings from user or `linter clippy` 2. **Identify fixable warnings**: Determine which warnings can be fixed with clippy suggestions 3. **Apply fixes**: Modify source code to apply clippy suggestions properly -4. **Document exceptions**: Add clear comments for any `#[allow(...)]` attributes +4. **Document exceptions**: Use the native `reason = "..."` parameter for changed Clippy allows 5. **Commit fixes**: Use `Committer` agent to create properly formatted commits 6. **Verify**: Ensure `linter all` passes after fixes diff --git a/.github/skills/dev/git-workflow/run-pre-commit-checks/SKILL.md b/.github/skills/dev/git-workflow/run-pre-commit-checks/SKILL.md index c82b8a684..0f8df9e0d 100644 --- a/.github/skills/dev/git-workflow/run-pre-commit-checks/SKILL.md +++ b/.github/skills/dev/git-workflow/run-pre-commit-checks/SKILL.md @@ -63,10 +63,13 @@ The script runs these steps in order: 1. `./contrib/dev-tools/git/format-project-words.sh` - formats `project-words.txt` with `LC_ALL=C sort -u` -2. `cargo machete --with-metadata` - unused dependency check -3. `cargo deny check bans` - workspace layer-boundary dependency check -4. `linter all` - all linters (markdown, lychee local links, YAML, TOML, clippy, rustfmt, shellcheck, cspell) -5. `cargo test --doc --workspace` - documentation tests +2. `cargo run --quiet --package clippy-allow-reasons -- --staged` - prospective native-reason + check for changed staged Clippy allow attributes. The command resolves the first available + base reference from `origin/develop`, `upstream/develop`, `torrust/develop`, or local `develop`. +3. `cargo machete --with-metadata` - unused dependency check +4. `cargo deny check bans` - workspace layer-boundary dependency check +5. `linter all` - all linters (markdown, lychee local links, YAML, TOML, clippy, rustfmt, shellcheck, cspell) +6. `cargo test --doc --workspace` - documentation tests If the formatter changes the dictionary, the hook exits non-zero before the verification steps. Stage `project-words.txt` and retry the commit. Run the formatter independently with: diff --git a/.github/skills/dev/maintenance/add-workspace-member/SKILL.md b/.github/skills/dev/maintenance/add-workspace-member/SKILL.md new file mode 100644 index 000000000..4be6d5e27 --- /dev/null +++ b/.github/skills/dev/maintenance/add-workspace-member/SKILL.md @@ -0,0 +1,51 @@ +--- +name: add-workspace-member +description: Add or remove an explicit Cargo workspace member in Torrust Tracker. Use when editing the root workspace members list, adding a developer-tool crate, or registering a new standalone workspace package. +metadata: + author: torrust + version: "1.0" +semantic-links: + related-artifacts: + - Cargo.toml + - Containerfile + - .dockerignore + - .github/skills/dev/maintenance/add-rust-dependency/SKILL.md +--- + +# Add a Cargo Workspace Member + +Use this workflow when changing the root `Cargo.toml` `[workspace].members` list. It applies to +explicit members only; path dependencies can be auto-discovered separately by Cargo. + +## Required Review + +1. Add or remove the member in root `Cargo.toml`. +2. Read the semantic link above that list and review `Containerfile` at the cargo-chef recipe + stage. Add or remove its manifest copy and all target stubs required by `cargo metadata`. +3. Decide whether the member has value in container test archives. + - Production-relevant members remain included. + - Developer-only analysis, checks, benchmarks, clients, and host-only E2E tools are normally + excluded from every `cargo nextest archive` invocation. + - Keep the explanation and all four archive exclusion lists synchronized. +4. Review `.dockerignore`; a manifest copied in the recipe stage must not be excluded from the + build context. + +## Verification + +Run the narrow validation appropriate to the change before the normal repository gate: + +```bash +docker build --target recipe --file Containerfile . +``` + +For a changed archive inclusion/exclusion, also run: + +```bash +docker build --target test_debug --file Containerfile . +``` + +Then run `linter all`, `cargo test --doc --workspace`, and the mandatory pre-commit workflow. + +## Related Skills + +- [`add-rust-dependency`](../add-rust-dependency/SKILL.md) — add an external dependency. diff --git a/.github/skills/dev/maintenance/update-github-workflow-actions/SKILL.md b/.github/skills/dev/maintenance/update-github-workflow-actions/SKILL.md index 4892b2915..267de89cf 100644 --- a/.github/skills/dev/maintenance/update-github-workflow-actions/SKILL.md +++ b/.github/skills/dev/maintenance/update-github-workflow-actions/SKILL.md @@ -38,9 +38,11 @@ For Cargo dependency updates, use - Prefer a scoped, stable pattern over a moving `owner/action@v2` tag when Dependabot updates exact versions. - Confirm that the configured pattern matches the full `uses:` reference, including its version. 5. Add one semantic `skill-link: update-github-workflow-actions` comment near the workflow's top-level metadata and review the related skills when updating the workflow policy. -6. Run `linter yaml`, `git diff --check`, and the relevant repository checks before committing. -7. Commit with a signed Conventional Commit, push the branch to the fork remote, and open a PR targeting `develop`. -8. Confirm affected workflow runs are queued and pass. If a run is blocked by the allowlist, correct the organization policy and rerun the failed jobs; do not weaken the workflow pin. +6. When a workflow command compares Git revisions, configure `actions/checkout` with + `fetch-depth: 0` so its merge base is available in CI. +7. Run `linter yaml`, `git diff --check`, and the relevant repository checks before committing. +8. Commit with a signed Conventional Commit, push the branch to the fork remote, and open a PR targeting `develop`. +9. Confirm affected workflow runs are queued and pass. If a run is blocked by the allowlist, correct the organization policy and rerun the failed jobs; do not weaken the workflow pin. ## Allowlist Failure Diagnosis diff --git a/.github/skills/dev/rust-code-quality/fix-clippy-warnings/SKILL.md b/.github/skills/dev/rust-code-quality/fix-clippy-warnings/SKILL.md index 6c15652aa..826603603 100644 --- a/.github/skills/dev/rust-code-quality/fix-clippy-warnings/SKILL.md +++ b/.github/skills/dev/rust-code-quality/fix-clippy-warnings/SKILL.md @@ -35,12 +35,42 @@ Only add `#[allow(...)]` when: ## How to Document Exceptions -When adding `#[allow(...)]` attributes, always include a clear comment explaining why: +When adding or modifying `#[allow(clippy::...)]` or `#[expect(clippy::...)]` attributes, use Rust's +native `reason` parameter. The prospective Rust validator checks changed attributes against the +branch merge base, so existing allows remain the separate remediation scope of #2158. ```rust -// This is a temporary workaround during refactoring of the announce response parser -// TODO: Remove this allowance when the parser is fully refactored -#[allow(clippy::unnecessary_wraps)] +#[allow( + clippy::unnecessary_wraps, + reason = "Temporary parser compatibility shim; remove when #2158 is complete." +)] +``` + +The reason must be specific. A temporary reason must also contain either a stable issue reference +such as `#2158` or a non-empty `remove when`, `remove after`, `remove by`, `removed when`, +`removed after`, `removed by`, or `until` condition. The validator treats `temporary`, +`temporarily`, `TODO`, `for now`, and `workaround` as temporary wording and normalizes whitespace +before checking the condition. + +Do not enable `clippy::allow_attributes_without_reason` workspace-wide until #2158 has remediated +the historical attributes that lack native reasons. It is the correct eventual compiler-aware +enforcement mechanism, but enabling it now would violate this issue's prospective-baseline scope. + +For a temporary item-level suppression, prefer `#[expect(..., reason = "...")]` when it is useful +to learn that the underlying lint no longer fires. Do not force `expect` for crate-level policy. + +The validator also checks changed `cfg_attr(..., allow(clippy::...))` and +`cfg_attr(..., expect(clippy::...))` controls. Attributes written inside a `macro_rules!` token body +are not visited by the Rust AST and are out of scope for this prospective check; do not use macros +to conceal a lint suppression. + +For example: + +```rust +#[expect( + clippy::unnecessary_wraps, + reason = "Temporary parser compatibility shim; remove when #2158 is complete." +)] fn parse_announce_response(data: &[u8]) -> Result { // implementation } @@ -93,7 +123,7 @@ for item in &items { 1. **Identify the warning**: Run `linter clippy` to see specific clippy errors 2. **Apply suggestion**: Try the suggested fix first 3. **Verify functionality**: Ensure the change doesn't break existing behavior -4. **Document exceptions**: Add clear comments for any allowances +4. **Document exceptions**: Use the native `reason = "..."` parameter for changed Clippy allows 5. **Run full linters**: Confirm `linter all` passes ## Related Skills diff --git a/.github/workflows/testing.yaml b/.github/workflows/testing.yaml index 49c7b9797..9623cf126 100644 --- a/.github/workflows/testing.yaml +++ b/.github/workflows/testing.yaml @@ -98,6 +98,34 @@ jobs: name: Run Unit Tests run: cargo test --tests --benches --examples --workspace --all-targets --all-features + documented-clippy-allows: + name: Documented Clippy Allows + runs-on: ubuntu-latest + timeout-minutes: 15 + env: + BASE_REF: ${{ github.base_ref || 'develop' }} + + steps: + - id: checkout + name: Checkout Repository + uses: actions/checkout@v7 + with: + fetch-depth: 0 + + - id: setup + name: Setup Toolchain + uses: dtolnay/rust-toolchain@stable + with: + toolchain: stable + + - id: fetch-base + name: Fetch Base Branch + run: git fetch origin "$BASE_REF" + + - id: check + name: Check Documented Clippy Allows + run: cargo run --quiet --package clippy-allow-reasons -- --base-ref "origin/$BASE_REF" + layer-bans: name: Layer Boundary Bans runs-on: ubuntu-latest diff --git a/Cargo.lock b/Cargo.lock index 289275b46..be2152eca 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -725,6 +725,16 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" +[[package]] +name = "clippy-allow-reasons" +version = "0.1.0" +dependencies = [ + "proc-macro2", + "serde", + "serde_json", + "syn 2.0.119", +] + [[package]] name = "cmake" version = "0.1.58" diff --git a/Cargo.toml b/Cargo.toml index cf633dbc5..c2c625b8d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -120,9 +120,13 @@ url = { version = "2", features = [ "serde" ] } nix = { version = "0.31.3", default-features = false, features = [ "signal" ] } [workspace] +# skill-link: add-workspace-member +# related-artifact: Containerfile — when adding or removing an explicit member, +# review its cargo-chef manifest/stub lists and nextest archive exclusions. members = [ "console/tracker-client", "contrib/dev-tools/analysis/workspace-coupling", + "contrib/dev-tools/checks/clippy-allow-reasons", "packages/e2e-tools", "packages/persistence-benchmark", "packages/rest-api-application", diff --git a/Containerfile b/Containerfile index a247a0b0e..70981d0b1 100644 --- a/Containerfile +++ b/Containerfile @@ -1,7 +1,10 @@ # syntax=docker/dockerfile:latest # # semantic-links: +# skill-links: +# - add-workspace-member # related-artifacts: +# - Cargo.toml # explicit workspace members; review this recipe when they change # - .hadolint.yaml # hadolint global linting rules and ignore policies with rationale # Torrust Tracker @@ -72,6 +75,7 @@ COPY console/tracker-client/Cargo.toml console/tracker-client/ # Build stages below) because they are not part of the production tracker service # and do not need to be tested inside the container image: # - workspace-coupling (analysis/coupling tool, no production value) +# - clippy-allow-reasons (prospective source-quality check, no production value) # - torrust-tracker-torrent-repository-benchmarking (benchmarking only) # - torrust-tracker-client (CLI dev tools: tracker_client, tracker_checker, etc.) # - torrust-tracker-e2e-tools (E2E runners + profiling tool, GHA host-only) @@ -82,6 +86,7 @@ COPY console/tracker-client/Cargo.toml console/tracker-client/ # or declared target file is missing. `cargo chef prepare` has no `--exclude` # flag (only `--bin`), so these stubs cannot be omitted from the recipe stage. COPY contrib/dev-tools/analysis/workspace-coupling/Cargo.toml contrib/dev-tools/analysis/workspace-coupling/ +COPY contrib/dev-tools/checks/clippy-allow-reasons/Cargo.toml contrib/dev-tools/checks/clippy-allow-reasons/ COPY packages/e2e-tools/Cargo.toml packages/e2e-tools/ COPY packages/persistence-benchmark/Cargo.toml packages/persistence-benchmark/ COPY packages/axum-health-check-api-server/Cargo.toml packages/axum-health-check-api-server/ @@ -127,6 +132,7 @@ RUN mkdir -p \ packages/e2e-tools/src/bin \ packages/persistence-benchmark/src/bin \ contrib/dev-tools/analysis/workspace-coupling/src \ + contrib/dev-tools/checks/clippy-allow-reasons/src \ console/tracker-client/src/bin \ packages/axum-health-check-api-server/src \ packages/axum-http-server/src \ @@ -163,6 +169,8 @@ RUN mkdir -p \ packages/e2e-tools/src/bin/qbittorrent_e2e_runner.rs \ packages/persistence-benchmark/src/bin/persistence_benchmark_runner.rs \ contrib/dev-tools/analysis/workspace-coupling/src/main.rs \ + contrib/dev-tools/checks/clippy-allow-reasons/src/lib.rs \ + contrib/dev-tools/checks/clippy-allow-reasons/src/main.rs \ console/tracker-client/src/lib.rs \ console/tracker-client/src/bin/http_tracker_client.rs \ console/tracker-client/src/bin/tracker_checker.rs \ @@ -222,7 +230,7 @@ COPY --from=recipe /build/recipe.json /build/recipe.json # Note: `cargo chef cook` does not support `--exclude` (the cargo-chef CLI only # exposes `--workspace` and `--package`, not `--exclude`). The excluded workspace # members (workspace-coupling, torrust-tracker-torrent-repository-benchmarking, -# torrust-tracker-client, torrust-tracker-contrib-bencode, +# clippy-allow-reasons, torrust-tracker-client, torrust-tracker-contrib-bencode, # torrust-tracker-e2e-tools, torrust-tracker-persistence-benchmark) are therefore # still compiled as part of the cook skeleton (their Cargo.toml manifests are in # the recipe, so cargo-chef cooks them). The build-time savings come from the @@ -235,6 +243,7 @@ RUN cargo chef cook --tests --workspace --all-features --recipe-path /build/reci # by pre-faulting the linker phases, avoiding redundant linking work in later stages. RUN cargo nextest archive --tests --workspace --all-features \ --exclude workspace-coupling \ + --exclude clippy-allow-reasons \ --exclude torrust-tracker-torrent-repository-benchmarking \ --exclude torrust-tracker-client \ --exclude torrust-tracker-contrib-bencode \ @@ -261,6 +270,7 @@ RUN cargo chef cook --tests --workspace --all-features --recipe-path /build/reci # by pre-faulting the linker phases, avoiding redundant linking work in later stages. RUN cargo nextest archive --tests --workspace --all-features \ --exclude workspace-coupling \ + --exclude clippy-allow-reasons \ --exclude torrust-tracker-torrent-repository-benchmarking \ --exclude torrust-tracker-client \ --exclude torrust-tracker-contrib-bencode \ @@ -275,6 +285,7 @@ WORKDIR /build/src COPY . /build/src RUN cargo nextest archive --tests --workspace --all-features \ --exclude workspace-coupling \ + --exclude clippy-allow-reasons \ --exclude torrust-tracker-torrent-repository-benchmarking \ --exclude torrust-tracker-client \ --exclude torrust-tracker-contrib-bencode \ @@ -288,6 +299,7 @@ WORKDIR /build/src COPY . /build/src RUN cargo nextest archive --tests --workspace --all-features \ --exclude workspace-coupling \ + --exclude clippy-allow-reasons \ --exclude torrust-tracker-torrent-repository-benchmarking \ --exclude torrust-tracker-client \ --exclude torrust-tracker-contrib-bencode \ diff --git a/contrib/dev-tools/checks/clippy-allow-reasons/Cargo.toml b/contrib/dev-tools/checks/clippy-allow-reasons/Cargo.toml new file mode 100644 index 000000000..b875bbabd --- /dev/null +++ b/contrib/dev-tools/checks/clippy-allow-reasons/Cargo.toml @@ -0,0 +1,18 @@ +[package] +description = "Prospective validation for documented Clippy allow attributes." +name = "clippy-allow-reasons" +publish = false + +authors.workspace = true +edition.workspace = true +rust-version.workspace = true +version = "0.1.0" + +[lints] +workspace = true + +[dependencies] +proc-macro2 = { version = "1", features = [ "span-locations" ] } +serde = { version = "1", features = [ "derive" ] } +serde_json = "1" +syn = { version = "2", features = [ "full", "visit" ] } diff --git a/contrib/dev-tools/checks/clippy-allow-reasons/src/lib.rs b/contrib/dev-tools/checks/clippy-allow-reasons/src/lib.rs new file mode 100644 index 000000000..6df828a1e --- /dev/null +++ b/contrib/dev-tools/checks/clippy-allow-reasons/src/lib.rs @@ -0,0 +1,305 @@ +//! Validation primitives for documented Clippy `allow` attributes. + +use std::collections::BTreeSet; + +use syn::parse::Parser as _; +use syn::punctuated::Punctuated; +use syn::spanned::Spanned as _; +use syn::visit::{self, Visit}; +use syn::{Attribute, Meta, Token}; + +/// A validation error for a changed Clippy `allow` attribute. +#[derive(Debug, Eq, PartialEq)] +pub struct Violation { + /// The one-based source line containing the relevant attribute. + pub line: usize, + /// The actionable reason for rejecting the attribute. + pub message: &'static str, +} + +/// Validates changed Clippy lint-suppression attributes in a Rust source file. +/// +/// Only attributes whose source span overlaps a line in `changed_lines` are checked. This lets a +/// caller enforce a prospective policy without requiring an inventory of historical attributes. +/// +/// # Errors +/// +/// Returns a [`syn::Error`] when `source` is not valid Rust syntax. +pub fn validate_changed_allows(source: &str, changed_lines: &BTreeSet) -> Result, syn::Error> { + let file = syn::parse_file(source)?; + let mut visitor = AllowVisitor { + changed_lines, + violations: Vec::new(), + }; + + visitor.visit_file(&file); + Ok(visitor.violations) +} + +struct AllowVisitor<'a> { + changed_lines: &'a BTreeSet, + violations: Vec, +} + +impl<'ast> Visit<'ast> for AllowVisitor<'_> { + fn visit_attribute(&mut self, attribute: &'ast Attribute) { + self.validate(attribute); + visit::visit_attribute(self, attribute); + } +} + +impl AllowVisitor<'_> { + fn validate(&mut self, attribute: &Attribute) { + let span = attribute.span(); + let start = span.start().line; + let end = span.end().line; + + if self.changed_lines.range(start..=end).next().is_none() { + return; + } + + for lint_control in clippy_lint_controls(attribute) { + self.validate_lint_control(start, lint_control); + } + } + + fn validate_lint_control(&mut self, line: usize, lint_control: LintControl) { + let Some(reason) = lint_control.reason else { + self.violations.push(Violation { + line, + message: "Clippy allow and expect attributes require `reason = \"\"`.", + }); + return; + }; + + if reason.trim().is_empty() { + self.violations.push(Violation { + line, + message: "Clippy allow and expect reasons must not be empty.", + }); + } else if is_temporary(&reason) && !has_temporary_removal_information(&reason) { + self.violations.push(Violation { + line, + message: "Temporary Clippy allow and expect reasons require an issue reference or non-empty `remove when`, `remove after`, `remove by`, or `until` condition.", + }); + } + } +} + +struct LintControl { + reason: Option, +} + +fn clippy_lint_controls(attribute: &Attribute) -> Vec { + clippy_lint_controls_in_meta(&attribute.meta) +} + +fn clippy_lint_controls_in_meta(meta: &Meta) -> Vec { + let Meta::List(list) = meta else { + return Vec::new(); + }; + + let Ok(items) = parse_lint_items(list.tokens.clone()) else { + return Vec::new(); + }; + + if list.path.is_ident("allow") || list.path.is_ident("expect") { + let has_clippy_lint = items.iter().any( + |item| matches!(item, Meta::Path(path) if path.segments.first().is_some_and(|segment| segment.ident == "clippy")), + ); + if !has_clippy_lint { + return Vec::new(); + } + + return vec![LintControl { + reason: lint_reason(&items), + }]; + } + + if list.path.is_ident("cfg_attr") { + return items.iter().skip(1).flat_map(clippy_lint_controls_in_meta).collect(); + } + + Vec::new() +} + +fn lint_reason(items: &Punctuated) -> Option { + items.iter().find_map(|item| match item { + Meta::NameValue(name_value) if name_value.path.is_ident("reason") => match &name_value.value { + syn::Expr::Lit(expression) => match &expression.lit { + syn::Lit::Str(reason) => Some(reason.value()), + _ => None, + }, + _ => None, + }, + _ => None, + }) +} + +fn parse_lint_items(tokens: proc_macro2::TokenStream) -> Result, syn::Error> { + Punctuated::::parse_terminated.parse2(tokens) +} + +fn is_temporary(reason: &str) -> bool { + let normalized = normalize_reason(reason); + + ["temporary", "temporarily", "todo", "for now", "workaround"] + .iter() + .any(|marker| normalized.contains(marker)) +} + +fn has_temporary_removal_information(reason: &str) -> bool { + let normalized = normalize_reason(reason); + let has_issue = normalized + .match_indices('#') + .any(|(index, _)| normalized[index + 1..].chars().next().is_some_and(char::is_numeric)); + let has_condition = [ + "remove when", + "remove after", + "remove by", + "removed when", + "removed after", + "removed by", + "until", + ] + .iter() + .any(|prefix| { + normalized.split_once(prefix).is_some_and(|(_, suffix)| { + !suffix + .trim_matches(|character: char| character == ':' || character.is_whitespace()) + .is_empty() + }) + }); + + has_issue || has_condition +} + +fn normalize_reason(reason: &str) -> String { + reason.split_whitespace().collect::>().join(" ").to_ascii_lowercase() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn changed(lines: &[usize]) -> BTreeSet { + lines.iter().copied().collect() + } + + #[test] + fn it_should_reject_a_changed_allow_without_a_reason() { + let source = "#[allow(clippy::too_many_lines)]\nfn example() {}\n"; + + let violations = validate_changed_allows(source, &changed(&[1])).unwrap(); + + assert_eq!(violations.len(), 1); + assert_eq!(violations[0].line, 1); + } + + #[test] + fn it_should_reject_a_changed_expect_without_a_reason() { + let source = "#[expect(clippy::too_many_lines)]\nfn example() {}\n"; + + let violations = validate_changed_allows(source, &changed(&[1])).unwrap(); + + assert_eq!(violations.len(), 1); + assert_eq!(violations[0].line, 1); + } + + #[test] + fn it_should_reject_a_changed_cfg_attr_allow_without_a_reason() { + let source = "#[cfg_attr(test, allow(clippy::too_many_lines))]\nfn example() {}\n"; + + let violations = validate_changed_allows(source, &changed(&[1])).unwrap(); + + assert_eq!(violations.len(), 1); + } + + #[test] + fn it_should_accept_a_documented_changed_cfg_attr_allow() { + let source = "#[cfg_attr(test, allow(clippy::too_many_lines, reason = \"Test fixture is intentionally verbose.\"))]\nfn example() {}\n"; + + assert_eq!(validate_changed_allows(source, &changed(&[1])).unwrap(), [] as [Violation; 0]); + } + + #[test] + fn it_should_reject_a_changed_allow_with_an_empty_reason() { + let source = "#[allow(clippy::too_many_lines, reason = \"\")]\nfn example() {}\n"; + + let violations = validate_changed_allows(source, &changed(&[1])).unwrap(); + + assert_eq!(violations.len(), 1); + assert_eq!(violations[0].message, "Clippy allow and expect reasons must not be empty."); + } + + #[test] + fn it_should_accept_a_documented_item_allow() { + let source = "#[allow(clippy::struct_field_names, reason = \"The wire schema uses external names.\")]\nstruct Schema { field_name: String }\n"; + + assert_eq!(validate_changed_allows(source, &changed(&[1])).unwrap(), [] as [Violation; 0]); + } + + #[test] + fn it_should_accept_a_documented_crate_allow() { + let source = "#![allow(clippy::module_name_repetitions, reason = \"The generated compatibility module is intentionally named.\")]\n"; + + assert_eq!(validate_changed_allows(source, &changed(&[1])).unwrap(), [] as [Violation; 0]); + } + + #[test] + fn it_should_reject_a_temporary_reason_without_removal_information() { + let source = "#[allow(clippy::too_many_arguments, reason = \"Temporary compatibility shim.\")]\nfn example() {}\n"; + + let violations = validate_changed_allows(source, &changed(&[1])).unwrap(); + + assert_eq!(violations.len(), 1); + } + + #[test] + fn it_should_accept_a_temporary_reason_with_a_removal_condition() { + let source = "#[allow(clippy::too_many_arguments, reason = \"Temporary compatibility shim; remove when the v4 migration completes.\")]\nfn example() {}\n"; + + assert_eq!(validate_changed_allows(source, &changed(&[1])).unwrap(), [] as [Violation; 0]); + } + + #[test] + fn it_should_accept_a_temporary_reason_with_an_issue_reference() { + let source = + "#[allow(clippy::too_many_arguments, reason = \"Temporary compatibility shim; see #2158.\")]\nfn example() {}\n"; + + assert_eq!(validate_changed_allows(source, &changed(&[1])).unwrap(), [] as [Violation; 0]); + } + + #[test] + fn it_should_require_removal_information_for_common_temporary_wording() { + let source = "#[allow(clippy::too_many_arguments, reason = \"TODO: workaround for now.\")]\nfn example() {}\n"; + + let violations = validate_changed_allows(source, &changed(&[1])).unwrap(); + + assert_eq!(violations.len(), 1); + } + + #[test] + fn it_should_accept_normalized_temporary_removal_conditions() { + for reason in [ + "Temporarily retained; remove when the migration completes.", + "Workaround; removed after: the compatibility layer is deleted.", + "TODO: remove by the next release.", + "For now, retain until the upstream fix is released.", + ] { + let source = format!("#[allow(clippy::too_many_arguments, reason = \"{reason}\")]\nfn example() {{}}\n"); + + assert_eq!( + validate_changed_allows(&source, &changed(&[1])).unwrap(), + [] as [Violation; 0] + ); + } + } + + #[test] + fn it_should_ignore_an_unchanged_legacy_allow() { + let source = "#[allow(clippy::too_many_lines)]\nfn legacy() {}\n"; + + assert_eq!(validate_changed_allows(source, &changed(&[2])).unwrap(), [] as [Violation; 0]); + } +} diff --git a/contrib/dev-tools/checks/clippy-allow-reasons/src/main.rs b/contrib/dev-tools/checks/clippy-allow-reasons/src/main.rs new file mode 100644 index 000000000..4f3a494c1 --- /dev/null +++ b/contrib/dev-tools/checks/clippy-allow-reasons/src/main.rs @@ -0,0 +1,357 @@ +//! Command-line adapter for prospective Clippy `allow` rationale validation. + +use std::collections::{BTreeMap, BTreeSet}; +use std::io::{self, Write}; +use std::path::PathBuf; +use std::process::{Command, ExitCode}; +use std::{env, fs}; + +use clippy_allow_reasons::validate_changed_allows; +use serde::Serialize; + +const EXIT_VIOLATIONS: u8 = 1; +const EXIT_USAGE_ERROR: u8 = 2; + +fn main() -> ExitCode { + match run() { + Ok(()) => ExitCode::SUCCESS, + Err(error) => { + let exit_code = error.exit_code(); + for diagnostic in error.diagnostics() { + if emit_diagnostic(&diagnostic).is_err() { + emit_output_failure(exit_code); + break; + } + } + ExitCode::from(exit_code) + } + } +} + +#[derive(Serialize)] +struct CliDiagnostic { + kind: &'static str, + message: String, + #[serde(skip_serializing_if = "Option::is_none")] + file: Option, + #[serde(skip_serializing_if = "Option::is_none")] + line: Option, + exit_code: u8, +} + +enum CliError { + Usage(String), + Runtime(String), + Violations(Vec), +} + +impl CliError { + const fn exit_code(&self) -> u8 { + match self { + Self::Usage(_) => EXIT_USAGE_ERROR, + Self::Runtime(_) | Self::Violations(_) => EXIT_VIOLATIONS, + } + } + + fn diagnostics(self) -> Vec { + match self { + Self::Usage(message) => { + vec![diagnostic("usage_error", message, None, None, EXIT_USAGE_ERROR)] + } + Self::Runtime(message) => { + vec![diagnostic("runtime_error", message, None, None, EXIT_VIOLATIONS)] + } + Self::Violations(diagnostics) => diagnostics, + } + } +} + +fn run() -> Result<(), CliError> { + let arguments = arguments().map_err(CliError::Usage)?; + let workspace_root = workspace_root().map_err(CliError::Runtime)?; + let base_ref = match arguments.base_ref { + Some(base_ref) => base_ref, + None => default_base_ref(&workspace_root).map_err(CliError::Runtime)?, + }; + let base_commit = git_output(&workspace_root, ["merge-base", "HEAD", &base_ref]).map_err(CliError::Runtime)?; + let changed_lines = changed_rust_lines(&workspace_root, &base_commit, arguments.staged).map_err(CliError::Runtime)?; + let mut violations = Vec::new(); + + for (file, lines) in changed_lines { + let source = source_for_validation(&workspace_root, &file, arguments.staged).map_err(CliError::Runtime)?; + let file_violations = validate_changed_allows(&source, &lines) + .map_err(|error| CliError::Runtime(format!("{}: failed to parse Rust source: {error}", file.display())))?; + + violations.extend(file_violations.into_iter().map(|violation| { + diagnostic( + "validation_error", + violation.message.to_owned(), + Some(file.display().to_string()), + Some(violation.line), + EXIT_VIOLATIONS, + ) + })); + } + + if violations.is_empty() { + Ok(()) + } else { + Err(CliError::Violations(violations)) + } +} + +fn workspace_root() -> Result { + let root = git_output( + &env::current_dir().map_err(|error| error.to_string())?, + ["rev-parse", "--show-toplevel"], + )?; + Ok(PathBuf::from(root)) +} + +struct Arguments { + base_ref: Option, + staged: bool, +} + +fn arguments() -> Result { + let mut arguments = env::args().skip(1); + let mut base_ref = None; + let mut staged = false; + + while let Some(argument) = arguments.next() { + match argument.as_str() { + "--base-ref" if base_ref.is_none() => { + let Some(reference) = arguments.next() else { + return Err(String::from("usage error: `--base-ref` requires a Git reference")); + }; + base_ref = Some(reference); + } + "--staged" if !staged => staged = true, + "--base-ref" | "--staged" => return Err(format!("usage error: duplicate argument `{argument}`")), + _ => { + return Err(format!( + "usage error: unexpected argument `{argument}`; use `--base-ref ` and `--staged`" + )); + } + } + } + + Ok(Arguments { base_ref, staged }) +} + +fn default_base_ref(workspace_root: &PathBuf) -> Result { + const CANDIDATES: [&str; 4] = ["origin/develop", "upstream/develop", "torrust/develop", "develop"]; + + for candidate in CANDIDATES { + if git_ref_exists(workspace_root, candidate)? { + return Ok(String::from(candidate)); + } + } + + Err(format!("could not resolve a base reference; tried {}", CANDIDATES.join(", "))) +} + +fn git_ref_exists(workspace_root: &PathBuf, reference: &str) -> Result { + let output = Command::new("git") + .args(["rev-parse", "--verify", "--quiet", reference]) + .current_dir(workspace_root) + .output() + .map_err(|error| format!("failed to run Git: {error}"))?; + + Ok(output.status.success()) +} + +fn changed_rust_lines( + workspace_root: &PathBuf, + base_commit: &str, + staged: bool, +) -> Result>, String> { + let mut command = Command::new("git"); + command.args([ + "-c", + "diff.noprefix=false", + "-c", + "diff.mnemonicPrefix=false", + "-c", + "core.quotePath=false", + "--no-pager", + "diff", + "--no-ext-diff", + "--no-color", + "--unified=0", + ]); + if staged { + command.arg("--cached"); + } + let output = command + .args([base_commit, "--", "*.rs"]) + .current_dir(workspace_root) + .output() + .map_err(|error| format!("failed to run Git diff: {error}"))?; + + if !output.status.success() { + return Err(format!( + "Git diff command failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + )); + } + + let diff = String::from_utf8(output.stdout).map_err(|error| format!("Git diff output was not valid UTF-8: {error}"))?; + parse_changed_rust_lines(&diff) +} + +fn source_for_validation(workspace_root: &PathBuf, file: &PathBuf, staged: bool) -> Result { + if staged { + return git_output(workspace_root, ["show", &format!(":{}", file.display())]); + } + + let source_path = workspace_root.join(file); + fs::read_to_string(&source_path).map_err(|error| format!("{}: failed to read source: {error}", source_path.display())) +} + +fn parse_changed_rust_lines(diff: &str) -> Result>, String> { + let mut changed_lines = BTreeMap::new(); + let mut current_file = None; + + for line in diff.lines() { + if line.starts_with("diff --git ") { + current_file = None; + continue; + } + + if let Some(file) = line.strip_prefix("+++ b/") { + current_file = Some(PathBuf::from(file)); + continue; + } + if let Some(file) = line.strip_prefix("+++ ") { + if file == "/dev/null" { + current_file = None; + continue; + } + return Err(format!("unrecognized Git diff file header `{line}`")); + } + + let Some(hunk) = line.strip_prefix("@@ ") else { + continue; + }; + let Some(file) = ¤t_file else { + return Err(format!("Git diff hunk has no recognized Rust file header `{line}`")); + }; + let Some(range) = hunk.split_whitespace().nth(1) else { + continue; + }; + let Some(added_range) = range.strip_prefix('+') else { + continue; + }; + let (start, count) = added_range.split_once(',').unwrap_or((added_range, "1")); + let start = start + .parse::() + .map_err(|error| format!("failed to parse Git diff line range `{added_range}`: {error}"))?; + let count = count + .parse::() + .map_err(|error| format!("failed to parse Git diff line range `{added_range}`: {error}"))?; + + changed_lines + .entry(file.clone()) + .or_insert_with(BTreeSet::new) + .extend(start..start + count); + } + + Ok(changed_lines) +} + +fn git_output(workspace_root: &PathBuf, arguments: [&str; N]) -> Result { + let output = Command::new("git") + .args(arguments) + .current_dir(workspace_root) + .output() + .map_err(|error| format!("failed to run Git: {error}"))?; + + if !output.status.success() { + return Err(format!( + "Git command failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + )); + } + + String::from_utf8(output.stdout) + .map(|output| output.trim().to_owned()) + .map_err(|error| format!("Git output was not valid UTF-8: {error}")) +} + +const fn diagnostic( + kind: &'static str, + message: String, + file: Option, + line: Option, + exit_code: u8, +) -> CliDiagnostic { + CliDiagnostic { + kind, + message, + file, + line, + exit_code, + } +} + +fn emit_diagnostic(diagnostic: &CliDiagnostic) -> io::Result<()> { + let mut stderr = io::stderr().lock(); + write_diagnostic(&mut stderr, diagnostic) +} + +fn emit_output_failure(exit_code: u8) { + let mut stderr = io::stderr().lock(); + drop(stderr.write_all(b"{\"kind\":\"output_error\",\"message\":\"failed to emit diagnostic\",\"exit_code\":")); + drop(write!(stderr, "{exit_code}")); + drop(stderr.write_all(b"}\n")); +} + +fn write_diagnostic(writer: &mut impl Write, diagnostic: &CliDiagnostic) -> io::Result<()> { + serde_json::to_writer(&mut *writer, diagnostic)?; + writer.write_all(b"\n") +} + +#[cfg(test)] +mod tests { + use super::*; + + struct FailingWriter; + + impl Write for FailingWriter { + fn write(&mut self, _buffer: &[u8]) -> io::Result { + Err(io::Error::other("intentional write failure")) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } + + #[test] + fn it_should_parse_added_lines_from_multiple_rust_hunks() { + let diff = "+++ b/src/lib.rs\n@@ -1 +1,2 @@\n unchanged\n+added_one\n+added_two\n@@ -5 +7 @@\n+added_three\n"; + + let changed_lines = parse_changed_rust_lines(diff).unwrap(); + + assert_eq!(changed_lines[&PathBuf::from("src/lib.rs")], BTreeSet::from([1, 2, 7])); + } + + #[test] + fn it_should_reject_an_unrecognized_git_diff_file_header() { + let diff = "diff --git a/src/lib.rs b/src/lib.rs\n+++ w/src/lib.rs\n@@ -0,0 +1 @@\n+#[allow(clippy::too_many_lines)]\n"; + + let error = parse_changed_rust_lines(diff).unwrap_err(); + + assert!(error.contains("unrecognized Git diff file header")); + } + + #[test] + fn it_should_report_a_diagnostic_write_failure() { + let diagnostic = diagnostic("runtime_error", String::from("failure"), None, None, EXIT_VIOLATIONS); + + let error = write_diagnostic(&mut FailingWriter, &diagnostic).unwrap_err(); + + assert_eq!(error.kind(), io::ErrorKind::Other); + } +} diff --git a/contrib/dev-tools/checks/clippy-allow-reasons/tests/cli.rs b/contrib/dev-tools/checks/clippy-allow-reasons/tests/cli.rs new file mode 100644 index 000000000..ff31a43f0 --- /dev/null +++ b/contrib/dev-tools/checks/clippy-allow-reasons/tests/cli.rs @@ -0,0 +1,297 @@ +use std::fs; +use std::path::Path; +use std::process::Command; +use std::time::{SystemTime, UNIX_EPOCH}; + +use serde_json::Value; + +#[test] +fn it_should_report_a_changed_allow_without_a_native_reason() { + let workspace = FixtureRepository::new(); + workspace.establish_baseline(); + workspace.add_undocumented_allow(); + + let output = Command::new(env!("CARGO_BIN_EXE_clippy-allow-reasons")) + .args(["--base-ref", "develop"]) + .current_dir(workspace.path()) + .output() + .expect("failed to run clippy-allow-reasons"); + + assert!(!output.status.success()); + assert_eq!(output.status.code(), Some(1)); + assert_eq!(output.stdout, b""); + let diagnostic = parse_single_diagnostic(&output.stderr); + assert_eq!(diagnostic["kind"], "validation_error"); + assert_eq!(diagnostic["file"], "src/lib.rs"); + assert_eq!(diagnostic["line"], 3); + assert!(diagnostic["message"].as_str().unwrap().contains("require `reason")); + assert_eq!(diagnostic["exit_code"], 1); +} + +#[test] +fn it_should_detect_an_undocumented_allow_despite_local_git_diff_configuration() { + let workspace = FixtureRepository::new(); + workspace.establish_baseline(); + workspace.add_undocumented_allow(); + + for configuration in [ + ["diff.noprefix", "true"], + ["diff.mnemonicPrefix", "true"], + ["color.diff", "always"], + ["diff.external", "false"], + ] { + git(workspace.path(), ["config", configuration[0], configuration[1]]); + let output = run_validator(workspace.path(), &["--base-ref", "develop"]); + + assert_eq!(output.status.code(), Some(1)); + assert_eq!(parse_single_diagnostic(&output.stderr)["kind"], "validation_error"); + } +} + +#[test] +fn it_should_not_write_output_when_validation_succeeds() { + let workspace = FixtureRepository::new(); + workspace.establish_documented_baseline(); + workspace.add_documented_allow(); + + let output = run_validator(workspace.path(), &["--base-ref", "develop"]); + + assert!(output.status.success()); + assert_eq!(output.stdout, b""); + assert_eq!(output.stderr, b""); +} + +#[test] +fn it_should_use_a_local_develop_branch_when_no_expected_remote_exists() { + let workspace = FixtureRepository::new(); + workspace.establish_documented_baseline(); + workspace.add_documented_allow(); + + let output = run_validator(workspace.path(), &[]); + + assert!(output.status.success()); + assert_eq!(output.stdout, b""); + assert_eq!(output.stderr, b""); +} + +#[test] +fn it_should_validate_staged_content_without_failing_for_unstaged_edits() { + let workspace = FixtureRepository::new(); + workspace.establish_documented_baseline(); + workspace.add_documented_allow(); + git(workspace.path(), ["add", "src/lib.rs"]); + workspace.add_undocumented_allow(); + + let output = run_validator(workspace.path(), &["--staged"]); + + assert!(output.status.success()); + assert_eq!(output.stdout, b""); + assert_eq!(output.stderr, b""); +} + +#[test] +fn it_should_accept_supported_documented_attribute_shapes() { + let workspace = FixtureRepository::new(); + workspace.establish_empty_baseline(); + write_file( + workspace.path().join("src/lib.rs").as_path(), + "#![allow(\n clippy::module_name_repetitions,\n reason = \"The generated compatibility module is intentionally named.\"\n)]\n\nstruct Example;\n\nimpl Example {\n #[allow(clippy::too_many_lines, reason = \"The generated method mirrors the protocol.\")]\n fn method(&self) {}\n}\n\nfn statement() {\n #[allow(clippy::let_and_return, reason = \"The binding keeps the example readable.\")]\n let value = 1;\n let _ = value;\n}\n", + ); + + let output = run_validator(workspace.path(), &["--base-ref", "develop"]); + + assert!(output.status.success()); + assert_eq!(output.stdout, b""); + assert_eq!(output.stderr, b""); +} + +#[test] +fn it_should_report_usage_errors_as_ndjson() { + let directory = FixtureDirectory::new(); + + let output = run_validator(directory.path(), &["--unexpected"]); + + assert_eq!(output.status.code(), Some(2)); + assert_eq!(output.stdout, b""); + let diagnostic = parse_single_diagnostic(&output.stderr); + assert_eq!(diagnostic["kind"], "usage_error"); + assert_eq!(diagnostic["exit_code"], 2); +} + +#[test] +fn it_should_report_runtime_errors_as_ndjson() { + let workspace = FixtureRepository::new(); + + let output = run_validator(workspace.path(), &["--base-ref", "missing-base-reference"]); + + assert_eq!(output.status.code(), Some(1)); + assert_eq!(output.stdout, b""); + let diagnostic = parse_single_diagnostic(&output.stderr); + assert_eq!(diagnostic["kind"], "runtime_error"); + assert_eq!(diagnostic["exit_code"], 1); +} + +fn parse_single_diagnostic(stderr: &[u8]) -> Value { + let lines = std::str::from_utf8(stderr).unwrap().lines().collect::>(); + + assert_eq!(lines.len(), 1); + serde_json::from_str(lines[0]).unwrap() +} + +fn run_validator(directory: &Path, arguments: &[&str]) -> std::process::Output { + Command::new(env!("CARGO_BIN_EXE_clippy-allow-reasons")) + .args(arguments) + .current_dir(directory) + .output() + .expect("failed to run clippy-allow-reasons") +} + +struct FixtureRepository { + root: std::path::PathBuf, +} + +struct FixtureDirectory { + path: std::path::PathBuf, +} + +impl FixtureDirectory { + fn new() -> Self { + let timestamp = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("system clock is before the Unix epoch") + .as_nanos(); + let path = std::env::temp_dir().join(format!("clippy-allow-reasons-no-git-{}-{timestamp}", std::process::id())); + + fs::create_dir_all(&path).expect("failed to create fixture directory"); + + Self { path } + } + + fn path(&self) -> &Path { + &self.path + } +} + +impl Drop for FixtureDirectory { + fn drop(&mut self) { + drop(fs::remove_dir_all(&self.path)); + } +} + +impl FixtureRepository { + fn new() -> Self { + let timestamp = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("system clock is before the Unix epoch") + .as_nanos(); + let root = std::env::temp_dir().join(format!("clippy-allow-reasons-{}-{timestamp}", std::process::id())); + + fs::create_dir_all(root.join("src")).expect("failed to create fixture repository"); + git(root.as_path(), ["init", "--quiet", "--initial-branch=develop"]); + git(root.as_path(), ["config", "user.email", "tests@example.com"]); + git(root.as_path(), ["config", "user.name", "Validator tests"]); + + Self { root } + } + + fn path(&self) -> &Path { + &self.root + } + + fn establish_baseline(&self) { + write_file( + self.path().join("src/lib.rs").as_path(), + "#[allow(clippy::legacy)]\nfn legacy() {}\n", + ); + git(self.path(), ["add", "src/lib.rs"]); + git( + self.path(), + [ + "-c", + "commit.gpgsign=false", + "-c", + "core.hooksPath=/dev/null", + "commit", + "--quiet", + "-m", + "test: establish baseline", + ], + ); + git(self.path(), ["switch", "--quiet", "-c", "feature"]); + } + + fn establish_documented_baseline(&self) { + write_file( + self.path().join("src/lib.rs").as_path(), + "#[allow(clippy::legacy, reason = \"Legacy baseline.\")]\nfn legacy() {}\n", + ); + git(self.path(), ["add", "src/lib.rs"]); + git( + self.path(), + [ + "-c", + "commit.gpgsign=false", + "-c", + "core.hooksPath=/dev/null", + "commit", + "--quiet", + "-m", + "test: establish documented baseline", + ], + ); + git(self.path(), ["switch", "--quiet", "-c", "feature"]); + } + + fn establish_empty_baseline(&self) { + write_file(self.path().join("src/lib.rs").as_path(), ""); + git(self.path(), ["add", "src/lib.rs"]); + git( + self.path(), + [ + "-c", + "commit.gpgsign=false", + "-c", + "core.hooksPath=/dev/null", + "commit", + "--quiet", + "-m", + "test: establish empty baseline", + ], + ); + git(self.path(), ["switch", "--quiet", "-c", "feature"]); + } + + fn add_undocumented_allow(&self) { + write_file( + self.path().join("src/lib.rs").as_path(), + "#[allow(clippy::legacy)]\nfn legacy() {}\n#[allow(clippy::too_many_lines)]\nfn added() {}\n", + ); + } + + fn add_documented_allow(&self) { + write_file( + self.path().join("src/lib.rs").as_path(), + "#[allow(clippy::legacy, reason = \"Legacy baseline.\")]\nfn legacy() {}\n#[allow(clippy::too_many_lines, reason = \"The generated fixture is intentionally verbose.\")]\nfn added() {}\n", + ); + } +} + +impl Drop for FixtureRepository { + fn drop(&mut self) { + drop(fs::remove_dir_all(&self.root)); + } +} + +fn git(directory: &Path, arguments: [&str; N]) { + let status = Command::new("git") + .args(arguments) + .current_dir(directory) + .status() + .expect("failed to run Git"); + + assert!(status.success(), "Git command failed"); +} + +fn write_file(path: &Path, contents: &str) { + fs::write(path, contents).expect("failed to write fixture source"); +} diff --git a/contrib/dev-tools/git/hooks/pre-commit.sh b/contrib/dev-tools/git/hooks/pre-commit.sh index b5472666b..ee5e02020 100755 --- a/contrib/dev-tools/git/hooks/pre-commit.sh +++ b/contrib/dev-tools/git/hooks/pre-commit.sh @@ -50,6 +50,7 @@ ensure_cargo_on_path declare -a STEPS=( "Formatting project dictionary|./contrib/dev-tools/checks/format-project-words.sh" + "Checking documented Clippy allows|cargo run --quiet --package clippy-allow-reasons -- --staged" "Checking for unused dependencies (cargo machete --with-metadata)|cargo machete --with-metadata" "Checking workspace layer boundary bans (cargo deny check bans)|cargo deny check bans" "Running all linters|linter all" diff --git a/docs/adrs/20260519000000_define_global_cli_output_contract.md b/docs/adrs/20260519000000_define_global_cli_output_contract.md index bf8d9962e..551f4f1b5 100644 --- a/docs/adrs/20260519000000_define_global_cli_output_contract.md +++ b/docs/adrs/20260519000000_define_global_cli_output_contract.md @@ -74,6 +74,7 @@ All diagnostics go to stderr via the tracing subscriber or direct JSON stderr wr | `e2e_tests_runner` | `no-stdout-result` | CI orchestrator; pass/fail via exit code | | `qbittorrent_e2e_runner` | `no-stdout-result` | CI orchestrator; pass/fail via exit code | | `tracker_client` | `stdout-result-data` | Announce/scrape results as JSON; monitor progress as NDJSON on stderr | +| `clippy-allow-reasons` | `no-stdout-result` | Prospective repository check; pass/fail via exit code | The `profiling` binary is a developer-only diagnostic harness and is excluded from the normative scope of this contract. diff --git a/docs/copilot-pr-reviews/pr-2177-copilot-suggestions.md b/docs/copilot-pr-reviews/pr-2177-copilot-suggestions.md new file mode 100644 index 000000000..3026de6b9 --- /dev/null +++ b/docs/copilot-pr-reviews/pr-2177-copilot-suggestions.md @@ -0,0 +1,38 @@ +--- +semantic-links: + skill-links: + - process-copilot-suggestions + related-artifacts: + - .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md + - docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md +--- + + + + + +# PR #2177 Copilot Suggestions Tracking + +Source: Copilot PR review threads for . + +## Processing Log + +- 2026-09-10 - Started processing suggestions. Outdated threads remain tracked until each has a recorded decision, reply, and resolution. +- 2026-09-11 10:10 UTC - Recorded decisions for all six original threads before resolution: CI suggestions are addressed by `aea3c0036`; Bash-only suggestions are superseded by the approved Rust replacement. +- 2026-09-11 10:15 UTC - Replied to and resolved all six recorded threads. The two CI suggestions are addressed by `aea3c0036`; the four Bash-only suggestions are superseded by the approved Rust replacement. + +## Suggestions + +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | --------------------- | ------------------------------------------------------------------------- | ----------------------------------------------------------------------------- | --------------------------------------------------------------------- | --------- | ----------------------------------------------------------------------------- | ------ | ------------ | +| 1 | PRRT_kwDOGp2yqc6gX0Ei | `.github/workflows/testing.yaml` | | Ensure the PR base ref is fetched before merge-base validation. | ACTION | | DONE | RESOLVED | +| 2 | PRRT_kwDOGp2yqc6gX0FH | `.github/workflows/testing.yaml` | | Keep checkout history shallow except for the focused validator check. | ACTION | | DONE | RESOLVED | +| 3 | PRRT_kwDOGp2yqc6gX0Fc | `contrib/dev-tools/checks/require-documented-clippy-allows.sh` | | Explain the fallback base reference behavior. | NO_ACTION | | DONE | RESOLVED | +| 4 | PRRT_kwDOGp2yqc6gX0Fv | `contrib/dev-tools/checks/require-documented-clippy-allows.sh` | | Avoid relying on merge-base with a missing ref. | NO_ACTION | | DONE | RESOLVED | +| 5 | PRRT_kwDOGp2yqc6gX0GB | `contrib/dev-tools/checks/require-documented-clippy-allows.sh` | | Handle temporary-rationale parsing edge cases. | NO_ACTION | | DONE | RESOLVED | +| 6 | PRRT_kwDOGp2yqc6gX0Gh | `contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh` | | Document that shell fixture tests are not run by CI or the hook. | NO_ACTION | | DONE | RESOLVED | + +## Notes + +- These threads are outdated because the Bash implementation was deliberately replaced. Each still + requires an explicit no-action or action decision, reply, and resolution under the Copilot workflow. diff --git a/docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md b/docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md index da0befbc0..fe383be03 100644 --- a/docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md +++ b/docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md @@ -1,14 +1,14 @@ --- doc-type: issue issue-type: enhancement -status: planned +status: in-progress priority: p2 epic: 2003 github-issue: 2157 spec-path: docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md branch: "2157-2003-require-documented-clippy-allows" related-pr: null -last-updated-utc: 2026-09-07 11:20 +last-updated-utc: 2026-09-11 10:25 semantic-links: skill-links: - create-issue @@ -17,6 +17,7 @@ semantic-links: - .github/agents/implementer.agent.md - .github/skills/dev/rust-code-quality/ - contrib/dev-tools/ + - docs/adrs/20260519000000_define_global_cli_output_contract.md - docs/issues/open/2003-overhaul-guardrails-and-automation/EPIC.md --- @@ -26,46 +27,60 @@ semantic-links: ## Goal -Require every newly introduced or modified Clippy `allow` attribute to have a nearby, specific -rationale, and add focused enforcement that does not block on pre-existing undocumented allows. +Require every newly introduced or modified Clippy `allow` attribute to use Rust's native, specific +`reason = "..."` parameter, with prospective enforcement that does not block on pre-existing +undocumented allows. ## Background A Clippy suppression may reflect an intentional design choice, a false positive, or a temporary -deferral. Without a nearby explanation, future maintainers cannot determine why it exists or -whether it should be removed. The repository already contains approximately 215 Clippy allows, so -this policy needs a prospective baseline rather than silently expanding into a bulk remediation. +deferral. Without a rationale, future maintainers cannot determine why it exists or whether it +should be removed. Rust supports a native `reason = "..."` parameter on lint-level attributes, +and Clippy's `allow_attributes_without_reason` lint detects absent reasons. The workspace MSRV is +1.88, so the native form is available for all maintained code. The repository already contains +approximately 215 Clippy allows, so this policy needs a prospective baseline rather than silently +expanding into a bulk remediation. ## Scope ### In Scope -- Define the accepted rationale format for item- and crate-level `allow(clippy::...)` attributes. -- Require temporary suppressions to include a stable issue reference or explicit removal condition. -- Add a focused, testable validator that detects undocumented additions or modifications. -- Establish and commit a baseline representing pre-existing attributes, or use another reviewed - change-detection mechanism that cannot silently grandfather new undocumented attributes. +- Define the native rationale form for item- and crate-level `allow(clippy::...)` attributes: + `reason = ""`. +- Use native `reason = "..."` enforcement prospectively; defer workspace-wide + `clippy::allow_attributes_without_reason` activation until #2158 remediates historical allows. +- Require temporary suppressions to include a stable issue reference or explicit removal condition + inside their native reason string. +- Add a focused Rust validator that detects newly added or modified attributes lacking native + reasons or the repository-specific temporary-removal information. +- Use a reviewed change-detection mechanism that cannot silently grandfather a newly added or + modified undocumented attribute while #2158 remediates the historical inventory. +- Keep the validator's Rust module and command narrowly scoped, independently testable, and + suitable for extraction into the later approved harness; do not define the EPIC's final harness + architecture in this issue. - Update relevant Rust code-quality guidance and agent instructions. ### Out of Scope - Documenting or removing existing Clippy allows; that is the separate existing-allow inventory issue. -- Changing Clippy lint levels or remediating the underlying lint findings. +- Remediating historical allows or their underlying lint findings; that belongs to #2158. - Replacing the repository's existing linter runner or CI architecture. +- Selecting the unified guardrail/sensor harness architecture planned by EPIC #2003. ## Architectural Decisions - Related ADR: `docs/adrs/20260821172000_establish_ai_agent_context_capability_and_portability_governance.md` +- Related ADR: `docs/adrs/20260519000000_define_global_cli_output_contract.md` - ADRs to create: None unless the selected baseline or enforcement mechanism changes repository-wide automation architecture. ## Implementation Plan -| ID | Status | Task | Notes / Expected Output | -| --- | ------ | ------------------------------------- | -------------------------------------------------------------------------------- | -| T1 | TODO | Define rationale policy | Cover intentional, false-positive, and temporary cases. | -| T2 | TODO | Select prospective baseline strategy | Record how existing allows are excluded without admitting new undocumented ones. | -| T3 | TODO | Implement and test focused validation | Support item and crate attributes with actionable diagnostics. | -| T4 | TODO | Integrate and document | Select a current validation tier without redesigning the runner. | +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | ------------------------------------ | --------------------------------------------------------------------------------------------------- | +| T1 | DONE | Define native rationale policy | Uses `reason = "..."`; covers intentional, false-positive, and temporary cases. | +| T2 | DONE | Select prospective baseline strategy | Merge-base diff excludes legacy allows without accepting changed attributes lacking native reasons. | +| T3 | DONE | Implement and test Rust validation | Pure `syn` module and narrow Git adapter remain independently testable and extractable. | +| T4 | DONE | Integrate and document | Uses current validation tiers without choosing the final EPIC harness architecture. | ## Progress Tracking @@ -74,61 +89,126 @@ this policy needs a prospective baseline rather than silently expanding into a b - [x] Folder-style spec drafted in `docs/issues/drafts/2003-require-documented-clippy-allows/ISSUE.md` - [x] Spec reviewed and approved by user/maintainer - [x] GitHub issue #2157 created and issue number added to this spec -- [ ] Implementation completed and verified -- [ ] Acceptance criteria reviewed after implementation and updated with evidence +- [x] Revised specification approved and committed +- [x] Bash implementation removed in a separate commit +- [x] Replacement implementation completed and verified +- [x] Acceptance criteria reviewed after replacement implementation and updated with evidence + +### PR #2177 Remediation + +- [ ] Preserve, commit, or discard pre-existing uncommitted changes before review fixes. +- [x] Repair the current failed workflow and record verification against the branch tip. +- [x] Process Cameron's maintainer feedback using + `docs/pr-review-feedback/pr-2177-review-feedback.md` and the review-feedback workflow. +- [x] Process Copilot suggestions using + `docs/copilot-pr-reviews/pr-2177-copilot-suggestions.md` and the Copilot-suggestions workflow. +- [ ] Run final repository validation and independent reviews after all findings are processed. +- [ ] Rebase onto the latest `torrust/develop`, rerun required checks, and push with + `--force-with-lease`. +- [ ] Confirm required CI checks pass and request refreshed maintainer review. ### Progress Log - 2026-09-07 10:45 UTC - GitHub Copilot - Created as an immediately implementable EPIC #2003 child; existing-allow remediation is explicitly separate - This spec - 2026-09-07 11:05 UTC - josecelano - Approved this subissue specification - Chat approval - 2026-09-07 11:10 UTC - GitHub Copilot - Created GitHub issue #2157, linked it to EPIC #2003, and promoted this specification to `docs/issues/open/` - https://github.com/torrust/torrust-tracker/issues/2157 +- 2026-09-08 17:00 UTC - GitHub Copilot - Implemented the prospective merge-base validator, rationale policy, focused Git-fixture tests, pre-commit and CI integration - Pending final verification +- 2026-09-08 17:25 UTC - GitHub Copilot - Independent complexity and task reviews passed; `linter all`, focused tests, documentation tests, and all pre-commit steps passed - Ready to commit +- 2026-09-09 12:00 UTC - josecelano - Replaced the Bash approach with Rust and native lint reasons as the approved direction; the Bash implementation is superseded and will be deleted before replacement work begins - Chat decision +- 2026-09-09 12:20 UTC - GitHub Copilot - Removed the superseded Bash validator, tests, integrations, guidance, and obsolete evidence in dedicated reset commit `d1900cde` before starting the Rust replacement - Committed as `refactor(quality): remove superseded Bash Clippy guard` +- 2026-09-09 13:10 UTC - GitHub Copilot - Implemented native-reason prospective validation as a small Rust crate with a pure `syn` module, Git-diff adapter, unit tests, and end-to-end Git fixture - Pending full validation +- 2026-09-09 13:40 UTC - GitHub Copilot - Full lint and documentation tests plus independent complexity and acceptance reviews passed; restored prior append-only review evidence after the reset - Ready to commit +- 2026-09-09 14:05 UTC - GitHub Copilot - Corrected `clippy-allow-reasons` to the global CLI output contract: `no-stdout-result`, silent success, and NDJSON stderr failure diagnostics; added CLI contract tests - `linter all`, focused tests, strict Clippy, and documentation tests passed +- 2026-09-09 14:15 UTC - GitHub Copilot - Repaired PR #2177 container recipe context for the new workspace crate after `cargo chef prepare` failed in CI; focused uncached recipe build and `linter all` passed - Pending focused commit +- 2026-09-09 15:10 UTC - GitHub Copilot - Correcting Cameron's PR #2177 review findings: exclude the dev-only checker from nextest archives, parse CLI arguments before Git access, and append validation-evidence corrections - Pending validation +- 2026-09-09 15:20 UTC - GitHub Copilot - Completed Cameron's requested corrections: all four nextest archive commands exclude `clippy-allow-reasons`; invalid CLI arguments are parsed before Git access; corrected earlier lint-evidence claims - Focused tests, `linter all`, and `docker build --no-cache --target test_debug --file Containerfile .` passed +- 2026-09-10 09:00 UTC - GitHub Copilot - Started a structured PR #2177 remediation pass; the issue spec tracks merge readiness while separate Cameron and Copilot audit records track individual findings, commits, replies, and thread states - In progress +- 2026-09-10 09:20 UTC - GitHub Copilot - Fixed Cameron finding M25 / BB1: replaced five `assert_is_empty` assertions in the Rust validator library tests; nightly Clippy and `linter all` pass on the branch tip - Pending focused commit, reply, and thread resolution +- 2026-09-10 09:30 UTC - GitHub Copilot - Completed Cameron finding M25 / BB1 in `65a8d1b4`; nightly Clippy and `linter all` passed, reply posted, and review thread resolved - Done +- 2026-09-10 10:05 UTC - GitHub Copilot - Addressed Cameron findings M15 / BB2 and M19: pinned and fail-closed Git diff parsing; disposable Git fixtures disable signing and hooks; hostile configuration tests, nightly Clippy, and `linter all` passed - Pending focused commits, replies, and thread resolutions +- 2026-09-10 10:15 UTC - GitHub Copilot - Completed Cameron findings M15 / BB2 and M19 in `067f379b`; validation passed, replies posted, and both review threads resolved - Done +- 2026-09-10 10:30 UTC - GitHub Copilot - Addressed Cameron findings M16 / BB3 and M23: native-reason validation now covers changed direct and conditional Clippy `allow` and `expect` controls; the skill documents the macro token-body limitation - Pending focused commits, replies, and thread resolutions +- 2026-09-10 10:40 UTC - GitHub Copilot - Completed Cameron findings M16 / BB3 and M23 in `415f16f6` and `b2db0d1c`; validation passed, replies posted, and both review threads resolved - Done +- 2026-09-10 11:00 UTC - GitHub Copilot - Addressed Cameron finding M17: temporary-reason detection recognizes common intent wording and normalizes removal conditions without weakening required issue/removal evidence - Pending focused commits, reply, and thread resolution +- 2026-09-10 11:10 UTC - GitHub Copilot - Completed Cameron finding M17 in `bb1c7e45` and `856d3d33`; validation passed, reply posted, and the review thread resolved - Done +- 2026-09-10 11:30 UTC - GitHub Copilot - Addressed Cameron finding M20: diagnostic output failures now trigger a fixed NDJSON fallback and are covered by a failing-writer unit test - Pending focused commit, reply, and thread resolution +- 2026-09-10 11:40 UTC - GitHub Copilot - Completed Cameron finding M20 in `8e21a7e4`; validation passed, reply posted, and the review thread resolved - Done +- 2026-09-11 08:35 UTC - GitHub Copilot - Completed implementation for Cameron finding M21 in `aea3c0036`: moved the check into a single fail-fast CI job, explicitly fetches the selected base ref, and passes it through `BASE_REF`; the mandatory pre-commit gate passed - Pending review reply and thread resolution; upstream `develop` advanced to `0255a201` +- 2026-09-11 08:40 UTC - GitHub Copilot - Completed Cameron finding M21: reply posted and thread resolved after `aea3c0036` moved the focused check out of the duplicated unit-test matrix - Done +- 2026-09-11 08:50 UTC - GitHub Copilot - Addressed Cameron finding M22: the checker resolves a portable default base reference, and pre-commit validates staged/index content with `--staged`; focused tests, strict Clippy, ShellCheck, Markdown lint, and whitespace checks passed - Pending focused commit, review reply, and thread resolution +- 2026-09-11 09:05 UTC - GitHub Copilot - Completed implementation for Cameron finding M22 in `6ff50131`; the mandatory pre-commit JSON gate passed - Pending review reply and thread resolution +- 2026-09-11 09:10 UTC - GitHub Copilot - Completed Cameron finding M22: reply posted and thread resolved after `6ff50131` made base selection portable and pre-commit index-backed - Done +- 2026-09-11 09:15 UTC - GitHub Copilot - Addressed Cameron finding M27: normalized the `clippy-allow-reasons` exclusion indentation across all four nextest archive blocks; structural and whitespace checks passed - Pending focused commit, review reply, and thread resolution +- 2026-09-11 09:20 UTC - GitHub Copilot - Completed implementation for Cameron finding M27 in `57b6ac68`; the mandatory pre-commit JSON gate passed - Pending review reply and thread resolution +- 2026-09-11 09:25 UTC - GitHub Copilot - Completed Cameron finding M27: reply posted and thread resolved after `57b6ac68` aligned all archive exclusion blocks - Done +- 2026-09-11 09:30 UTC - GitHub Copilot - Addressed Cameron finding M24: clarified the append-only independent-review report contract so it accurately preserves immutable append order and explicitly records corrective causality rather than promising timestamp order - Pending focused commit, review reply, and thread resolution +- 2026-09-11 09:35 UTC - GitHub Copilot - Completed implementation for Cameron finding M24 in `8ff5139b`; the mandatory pre-commit gate passed - Pending review reply and thread resolution +- 2026-09-11 09:40 UTC - GitHub Copilot - Completed Cameron finding M24: reply posted and thread resolved after `8ff5139b` clarified immutable append-order semantics - Done +- 2026-09-11 09:45 UTC - GitHub Copilot - Completed historical Cameron findings M1-M14: each received a specific action or approved-supersession response and was resolved - Done +- 2026-09-11 09:50 UTC - GitHub Copilot - Verified Cameron finding M26 against the current branch tip: `linter all` passed after all five `assert_is_empty` corrections; historical evidence remains append-only - Pending review reply and thread resolution +- 2026-09-11 09:55 UTC - GitHub Copilot - Completed Cameron finding M26: fresh lint evidence was replied and the thread resolved; append-only validation corrections now name all three stale claims and both affected test areas - Done +- 2026-09-11 10:00 UTC - GitHub Copilot - Completed the Cameron maintainer-feedback task: the audit records specific outcomes, replies, and resolved status for M1-M27 - Done +- 2026-09-11 10:15 UTC - GitHub Copilot - Completed the Copilot-suggestions task: all six original review threads received specific action or approved-supersession replies and were resolved - Done +- 2026-09-11 10:25 UTC - GitHub Copilot - Posted and recorded consolidated responses for all five Cameron reviews after their associated findings were resolved - Done +- 2026-09-10 11:20 UTC - GitHub Copilot - Addressed Cameron finding M18: CLI integration tests now exercise a real accepted changed allow plus method, statement, and multiline crate-level forms - Pending focused commits, reply, and thread resolution ## Acceptance Criteria -- [ ] Guidance defines nearby rationale requirements for all supported Clippy allow attribute forms. -- [ ] Temporary allows identify a removal condition or stable follow-up issue. -- [ ] A committed prospective baseline or reviewed equivalent excludes existing attributes without accepting new undocumented attributes. -- [ ] Focused tests prove undocumented new attributes fail and documented ones pass for item and crate forms. -- [ ] The enforcement runs in a documented existing validation tier and produces actionable diagnostics. -- [ ] `linter all` exits with code `0` and relevant tests pass. +- [x] Temporary native reasons identify a removal condition or stable follow-up issue. +- [x] A reviewed prospective baseline excludes existing attributes without accepting changed attributes that lack native reasons. +- [x] Focused Rust tests prove undocumented new attributes fail and documented item and crate forms pass. +- [x] Enforcement runs in a documented existing validation tier and produces actionable diagnostics. +- [x] `linter all` exits with code `0` and relevant tests pass. ## Verification Plan ### Automatic Checks - `linter all` -- Focused validator tests +- Focused Rust validator tests - `cargo test --doc --workspace` ### Manual Verification Scenarios -| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | -| --- | --------------------- | ---------------------------------------------------------------------------- | ------------------------------------------------------------------------ | ------ | ---------------------- | -| M1 | Undocumented addition | Add an isolated undocumented fixture allow and run the validator. | The validator fails and identifies the attribute and required rationale. | TODO | Pending implementation | -| M2 | Documented exceptions | Run fixtures for intentional, false-positive, and temporary rationale types. | Each passes only with complete required information. | TODO | Pending implementation | +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | --------------------- | ---------------------------------------------------------------------------- | ------------------------------------------------------------- | ------ | ----------------------- | +| M1 | Undocumented addition | Add an isolated undocumented fixture allow and run the validator. | The validator fails and identifies the missing native reason. | DONE | End-to-end Git fixture | +| M2 | Documented exceptions | Run fixtures for intentional, false-positive, and temporary rationale types. | Each passes only with complete required native information. | DONE | Focused Rust unit tests | ### Acceptance Verification -| AC ID | Status (`TODO`/`DONE`) | Evidence | -| ----- | ---------------------- | ---------------------- | -| AC1 | TODO | Pending implementation | -| AC2 | TODO | Pending implementation | -| AC3 | TODO | Pending implementation | -| AC4 | TODO | Pending implementation | -| AC5 | TODO | Pending implementation | -| AC6 | TODO | Pending implementation | +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ----------------------------------------------------------------------------------------------------------------------------------------- | +| AC1 | DONE | Rust code-quality guidance requires native reasons for changed Clippy allows. | +| AC2 | DONE | Unit tests cover native reasons with issue references and non-empty removal conditions. | +| AC3 | DONE | The Rust command validates only changed attribute spans from the Git merge-base diff. | +| AC4 | DONE | Unit and end-to-end Git-fixture tests cover missing, empty, item, crate, and temporary native reasons. | +| AC5 | DONE | Pre-commit and CI run the Rust command; CI fetches history for merge-base computation and it follows the `no-stdout-result` CLI contract. | +| AC6 | DONE | `linter all`, focused Rust tests including CLI output contracts, strict Clippy, and `cargo test --doc --workspace` passed. | ## Risks and Trade-offs -- Text parsing can be brittle. Support a small explicit syntax and reject unrecognized forms visibly. +- Native lint reasons prevent custom syntax for the rationale itself. Any temporary-policy check + must inspect only the native reason string and reject unrecognized forms visibly. - A stale baseline can become a loophole. Version it, test it, and make its update reviewable. +- A standalone Rust validator could prematurely become a harness. Keep its module and command + boundary narrow, and defer its final location and interface to the EPIC decision. ## Implementation Completion Review -After implementation, record material findings about the baseline or validator in an issue-local +After replacement implementation, record material findings about the baseline or validator in an issue-local `implementation-retrospective.md`. If none occurred, add a concise progress-log entry explaining why no retrospective is needed. +## Validation Evidence Corrections + +- 2026-09-09 - The 13:40, 14:05, and 14:15 entries predate the current branch's final lint + state and incorrectly state that `linter all` passed. The relevant + `clippy::assert_is_empty` failures spanned both the validator library and CLI-test coverage. + Commit `65a8d1b4` corrected the remaining library assertions in response to PR #2177 review; + a fresh `linter all` run on 2026-09-11 passed. Do not treat the earlier entries as final + acceptance evidence. + ## References - Parent EPIC: #2003 diff --git a/docs/issues/open/2157-2003-require-documented-clippy-allows/agent-review-reports.md b/docs/issues/open/2157-2003-require-documented-clippy-allows/agent-review-reports.md new file mode 100644 index 000000000..a277df7f2 --- /dev/null +++ b/docs/issues/open/2157-2003-require-documented-clippy-allows/agent-review-reports.md @@ -0,0 +1,176 @@ +--- +semantic-links: + related-artifacts: + - .github/agents/complexity-auditor.agent.md + - .github/agents/task-reviewer.agent.md + - .github/agents/pr-reviewer.agent.md + - docs/agents/orchestration.md +--- + +# Agent Review Reports - Issue #2157 - Require Documented Clippy Allows + +> Append one completed independent-review entry at a time. Do not modify, reorder, or remove +> earlier entries. The report preserves append order, which may differ from wall-clock order when +> a completed review or correction is recorded after an earlier timestamped entry. A correction or +> follow-up entry names the earlier conclusion it changes or resolves. + +## Reports + +### 2026-09-08 17:03 UTC - Complexity Auditor + +- Invocation scope: Complexity and maintainability audit of all functions added in `contrib/dev-tools/checks/require-documented-clippy-allows.sh` and `contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh`. +- Inputs: Issue #2157 specification; working-tree diff; both changed shell scripts; canonical agent-review report template. +- Evidence: `bash -n contrib/dev-tools/checks/require-documented-clippy-allows.sh contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh && bash contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh` passed. `shellcheck --severity=warning contrib/dev-tools/checks/require-documented-clippy-allows.sh contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh` passed with no output. Cargo Clippy cognitive-complexity validation is not applicable: the reviewed functions are Bash, with no affected Rust package. +- Findings: + - None. `resolve_base_ref` complexity=4, nesting=2, lines=18; `require_rationale` complexity=7, nesting=1, lines=32; the five simple test helpers have complexity=1 and 6-17 lines; the two rejection tests have complexity=2 and 15 lines. No function exceeds the cyclomatic, nesting, or length thresholds. +- Verdict: AUDIT PASSED +- Follow-up actions: + - None. The Implementer may proceed to the next step. + +### 2026-09-08 17:24 UTC - Task Reviewer (Follow-up) + +- Invocation scope: Independent re-review of issue #2157's current working-tree implementation after repair of the temporary removal-condition validation blocker, against all six acceptance criteria. +- Inputs: `docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md`; prior 2026-09-08 Task Reviewer report; working-tree diff; validator and Git-fixture test scripts; rationale guidance; pre-commit hook; testing workflow; implementation retrospective; existing review reports. +- Evidence: `bash -n contrib/dev-tools/checks/require-documented-clippy-allows.sh contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh && bash contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh && shellcheck --severity=warning contrib/dev-tools/checks/require-documented-clippy-allows.sh contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh && CLIPPY_ALLOW_BASE_REF=develop ./contrib/dev-tools/checks/require-documented-clippy-allows.sh` passed. The fixture suite proves `// clippy-allow: temporary: remove when` exits non-zero and reports the temporary-allow diagnostic; it also proves a stable `#2158` reference and `remove when the refactor reaches this module` pass. `linter all`, `cargo test --doc --workspace`, `git diff --check`, and `TORRUST_GIT_HOOKS_LOG_DIR=.tmp ./contrib/dev-tools/git/hooks/pre-commit.sh` passed. The pre-commit gate passed all seven steps, including cargo machete, cargo deny, linter all, hadolint, and documentation tests. +- Findings: + - None. The prior blocker is resolved: `REMOVAL_CONDITION_REGEX` requires a non-empty token after `remove when`, `remove after`, or `remove by`, while the fixture tests cover the formerly accepted incomplete condition and both accepted temporary-rationale alternatives. + - The merge-base comparison remains a reviewed prospective baseline: it detects added and modified single-line item and crate `allow(clippy::...)` attributes without requiring legacy remediation. CI fetches complete history and supplies the PR base reference; pre-commit runs the same validator. The implementation retrospective records this material architectural decision. +- Acceptance criteria: + - AC1 PASS - `fix-clippy-warnings` and the Clippy fixer agent specify the adjacent intentional, false-positive, and temporary rationale forms for changed item and crate attributes; documented item and crate fixtures pass. + - AC2 PASS - The exact incomplete `remove when` fixture fails; fixtures with `#2158` and a non-empty `remove when` condition pass. + - AC3 PASS - The validator derives a merge base and scans `git diff --unified=0` additions, including modified attributes; a legacy-modification fixture without a rationale fails. + - AC4 PASS - Focused Git-fixture tests prove undocumented item attributes fail, documented item and crate attributes pass, and temporary rationale variants are enforced. + - AC5 PASS - The validator emits file-and-line diagnostics, is documented in the pre-commit skill, runs in the pre-commit hook, and runs in CI with `fetch-depth: 0` and the PR base reference. + - AC6 PASS - `linter all`, focused validator tests, ShellCheck, and `cargo test --doc --workspace` passed; the full mandatory pre-commit gate also passed. +- Repository-convention findings: + - None. The work is scoped to the stated guardrail, test coverage, integration, guidance, and required issue-local retrospective; `git diff --check` passed. +- Completion-review finding: + - PASS. `implementation-retrospective.md` exists in the folder-style specification and records the reusable merge-base baseline finding and CI full-history requirement. +- Issue-spec updates: + - None. All six acceptance criteria were already checked and are now independently verified as PASS; no unverified item was marked complete. +- Verdict: REVIEW PASSED +- Follow-up actions: + - The current working tree is ready for the normal implementation commit and pre-PR workflow. This report is review documentation only and does not authorize unrelated changes. + +### 2026-09-08 17:12 UTC - Task Reviewer + +- Invocation scope: Independent pre-commit review of issue #2157's current working-tree implementation against all six acceptance criteria, including the prospective baseline, supported rationale syntax, fixtures, pre-commit and CI integration, workflow Git-history configuration, and documentation. +- Inputs: `docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md`; working-tree diff; validator and fixture scripts; pre-commit hook; testing workflow; updated Clippy and workflow skills; implementation retrospective; prior independent review reports. +- Evidence: `bash -n contrib/dev-tools/checks/require-documented-clippy-allows.sh contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh && bash contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh && shellcheck --severity=warning contrib/dev-tools/checks/require-documented-clippy-allows.sh contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh` passed. `CLIPPY_ALLOW_BASE_REF=develop ./contrib/dev-tools/checks/require-documented-clippy-allows.sh` passed. `linter all` passed. `cargo test --doc --workspace` passed. `git diff --check` passed. A disposable Git fixture containing `// clippy-allow: temporary: reason; remove when` immediately above a new `#[allow(clippy::too_many_arguments)]` exited 0, which falsifies the claimed temporary-removal validation. +- Findings: + - BLOCKER: `contrib/dev-tools/checks/require-documented-clippy-allows.sh` accepts a `temporary` rationale ending in `remove when` without a condition. The current `REMOVAL_CONDITION_REGEX='remove[[:space:]](when|after|by)'` only detects the phrase, although the documented policy requires `remove when `. This permits a temporary allow with neither a stable issue reference nor an explicit removal condition, so AC2 is not satisfied. Add a non-empty condition requirement after `when`, `after`, or `by`, and add a rejecting fixture for the incomplete phrase. + - INFO: The prospective mechanism is a reviewed merge-base diff, not a committed inventory. It correctly includes current working-tree changes in pre-commit, detects added and modified single-line item and crate attributes, and CI uses `fetch-depth: 0` plus the PR base reference. This satisfies AC3 without silently grandfathering newly changed attributes. +- Verdict: REVIEW FAILED +- Follow-up actions: + - Implementer: Tighten temporary removal-condition validation, add the negative fixture, rerun the focused script checks and repository gates, then request a new independent task review. Do not proceed to an implementation commit or pull request from this review. + +### 2026-09-08 17:03 UTC - Complexity Auditor (Correction) + +- Invocation scope: Corrects the changed-test-function inventory in the 2026-09-08 17:03 UTC Complexity Auditor report; the original complexity conclusion remains unchanged. +- Inputs: The two reviewed shell scripts and the original audit report entry. +- Evidence: Source review confirms nine changed test functions. The original focused Bash syntax, fixture-test, and ShellCheck commands passed. Cargo Clippy cognitive-complexity validation remains not applicable because no Rust functions or package were reviewed. +- Findings: + - None. `resolve_base_ref` complexity=4, nesting=2, lines=18; `require_rationale` complexity=7, nesting=1, lines=32; `create_fixture` complexity=1, nesting=0, lines=17; `run_validator` complexity=1, nesting=0, lines=6; `it_should_reject_an_undocumented_item_allowance` complexity=2, nesting=1, lines=15; `it_should_accept_a_documented_item_allowance` complexity=1, nesting=0, lines=10; `it_should_accept_a_documented_crate_allowance` complexity=1, nesting=0, lines=10; `it_should_require_a_removal_condition_for_temporary_allowances` complexity=2, nesting=1, lines=15; `it_should_accept_a_temporary_allowance_with_a_stable_issue_reference` complexity=1, nesting=0, lines=10; `it_should_accept_a_temporary_allowance_with_an_explicit_removal_condition` complexity=1, nesting=0, lines=10; `it_should_reject_a_modified_legacy_allowance_without_a_rationale` complexity=2, nesting=1, lines=15. No function exceeds the cyclomatic, nesting, or length thresholds. +- Verdict: AUDIT PASSED +- Follow-up actions: + - None. The Implementer may proceed to the next step. + +### 2026-09-09 11:14 UTC - Complexity Auditor + +- Invocation scope: Post-reset replacement implementation for #2157: `contrib/dev-tools/checks/clippy-allow-reasons/src/lib.rs`, `src/main.rs`, `tests/cli.rs`, workspace membership, pre-commit hook, and CI integration. Assessed all changed Rust functions, cyclomatic complexity, nesting depth, and function length. +- Inputs: `docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md`; reset commit `d1900cde`; current working-tree replacement files and integration diff; `implementation-retrospective.md`. +- Evidence: + - `cargo clippy --package clippy-allow-reasons -- -W clippy::cognitive_complexity -D warnings` exited 0 with no cognitive-complexity warnings. + - `cargo test --package clippy-allow-reasons` exited 0: 10 tests passed (8 library unit, 1 binary unit, 1 CLI integration); 0 failed. + - `git diff --check` exited 0. + - Editor diagnostics for `src/lib.rs`, `src/main.rs`, and `tests/cli.rs`: no errors. + - The pure `syn` validation library and Git CLI adapter are separate. The existing pre-commit and CI tiers invoke the narrow command; the implementation introduces no #2003 harness interface, orchestration, or output-contract architecture. +- Findings: + - None. No assessed function exceeds cyclomatic complexity 10, nesting depth 3, or 50 lines. The largest orchestration function, `run`, remains a 32-line adapter with complexity 8. + - Assessed functions: `validate_changed_allows` (2/0/14), `visit_attribute` (1/0/4), `validate` (5/1/29), `is_clippy_allow` (3/1/15), `allow_reason` (5/2/18), `parse_allow_items` (1/0/3), `is_temporary` (1/0/3), `has_temporary_removal_information` (3/2/15), 8 library test functions (1/0/5–8 each), `main` (3/1/12), `CliError::from` (1/0/3), `CliError::fmt` (2/1/6), `run` (8/1/32), `workspace_root` (2/0/8), `base_ref` (4/1/23), `changed_rust_lines` (2/0/4), `parse_changed_rust_lines` (8/2/35), `git_output` (3/1/19), `write_stdout` (1/0/3), `write_stderr` (1/0/3), the binary parser test (1/0/7), the CLI integration test (1/0/24), `FixtureRepository::new` (1/0/15), `FixtureRepository::path` (1/0/3), `FixtureRepository::drop` (1/0/3), test `git` (1/0/10), and test `write_file` (1/0/3). Values are complexity/nesting/lines. +- Verdict: AUDIT PASSED +- Follow-up actions: + - Implementer may proceed to the next step. Preserve the pure-library/CLI boundary when the #2003 harness architecture is decided later. + +### 2026-09-09 11:16 UTC - Task Reviewer + +- Invocation scope: Independent post-reset review of the current #2157 Rust replacement implementation against every acceptance criterion in `ISSUE.md`; reviewed native Rust lint-reason syntax, temporary-removal policy, merge-base change detection, unit and end-to-end Git fixtures, pre-commit and CI integration, the #2158 historical-remediation boundary, and the approved Bash-removal/Rust-replacement sequence. +- Inputs: Folder-style issue specification; prior current report entry; `implementation-retrospective.md`; reset commit `d1900cde`; the current worktree diff; `contrib/dev-tools/checks/clippy-allow-reasons/`; `Cargo.toml`; `Cargo.lock`; pre-commit hook; testing workflow; relevant skills and agent guidance. +- Evidence: + - `git log` shows `d1900cde refactor(quality): remove superseded Bash Clippy guard` immediately after the policy/spec commits and before the current uncommitted Rust replacement. Its file inventory deletes the Bash validator and Bash fixture suite; the worktree adds `clippy-allow-reasons`, restores integrations and documentation for the Rust command, and contains no Bash replacement validator. + - `cargo test --package clippy-allow-reasons` passed: 8 library unit tests, 1 binary parser test, and 1 end-to-end disposable-Git-repository CLI test. + - `cargo clippy --package clippy-allow-reasons -- -W clippy::cognitive_complexity -D warnings`, `cargo run --quiet --package clippy-allow-reasons -- --base-ref develop`, and `git diff --check` passed. + - An independent disposable Git fixture changed a baseline undocumented allow and confirmed a line-specific missing-native-reason failure; it then confirmed a documented crate-level native allow passes. + - `TORRUST_GIT_HOOKS_LOG_DIR=.tmp ./contrib/dev-tools/git/hooks/pre-commit.sh` passed all seven steps, including the new validator, `cargo machete`, `cargo deny check bans`, `linter all`, Containerfile linting, and workspace documentation tests. + - CI uses `actions/checkout` with `fetch-depth: 0` and runs `cargo run --quiet --package clippy-allow-reasons -- --base-ref "origin/${{ github.base_ref || 'develop' }}"`. The pre-commit hook invokes the same Rust command with `torrust/develop`. +- Acceptance criteria: + - AC1 PASS - The guidance and Clippy-fixer instructions require Rust-native `reason = "..."`; pure `syn` tests accept documented item and crate forms. + - AC2 PASS - The validator rejects temporary reasons lacking a stable numeric issue reference or non-empty `remove when`, `remove after`, `remove by`, or `until` condition; unit tests cover rejection and both accepted alternatives. + - AC3 PASS - The Git adapter calculates `git merge-base HEAD `, parses zero-context added-line hunks for Rust files, and validates an attribute whenever its span overlaps a changed line. Unit coverage preserves unchanged legacy attributes, and independent Git-fixture execution confirms a modified legacy allow is not grandfathered. + - AC4 PASS - Focused Rust tests cover missing and empty reasons, documented item/crate attributes, temporary variants, and an unchanged legacy allow. The CLI integration fixture verifies an undocumented addition fails with an actionable file-and-line diagnostic. + - AC5 PASS - The command is documented, invoked by pre-commit and CI, and emits actionable diagnostics. CI obtains complete history for reliable merge-base calculation. + - AC6 PASS - Focused tests, strict focused Clippy, `linter all`, `cargo test --doc --workspace`, whitespace validation, and the full pre-commit gate passed. The latter three are included in the successful pre-commit evidence. +- Findings: + - BLOCKER - The approved reset commit deleted the prior `agent-review-reports.md`, including completed historical review entries. The current untracked re-created report contains only the new Complexity Auditor entry, so it does not preserve all prior entries in chronological order as the issue-local report contract requires. Restore the deleted entries unchanged from `d1900cde^:docs/issues/open/2157-2003-require-documented-clippy-allows/agent-review-reports.md`, retain the current Complexity Auditor entry in chronological order, and retain this report as the final entry. + - INFO - `clippy::allow_attributes_without_reason` is correctly not enabled in workspace lints. Guidance and `implementation-retrospective.md` explicitly defer it to #2158 because historical attributes would otherwise fail; the focused prospective validator preserves that boundary. + - INFO - The issue progress log records the Rust implementation at `2026-09-09 13:10 UTC`, later than this review's `11:16 UTC` timestamp. Correct the timestamp if it is not an intended future/planned entry, so issue evidence remains chronological. +- Repository-convention findings: + - The report-history loss is blocking documentation-process noncompliance. No production-code, diagnostics, formatting, dependency, or validation-gate failure was found. +- Completion-review finding: + - PASS - Folder-style `implementation-retrospective.md` exists and records the material reusable decisions: native reasons, span-aware merge-base validation, pure `syn`/Git-adapter boundary, and the deliberate #2158 deferral of `clippy::allow_attributes_without_reason`. +- Issue-spec updates: + - None. All six acceptance criteria were already checked and independently verified as PASS. The replacement-completion/checklist milestones remain unchecked because the report-history blocker prevents a clean completion verdict. +- Verdict: REVIEW FAILED +- Follow-up actions: + - Restore and preserve the complete earlier issue-local report history, append rather than replace entries, correct the future progress-log timestamp if applicable, then request a new independent review. Do not open a pull request or proceed with an implementation commit from this failed review. + +### 2026-09-09 12:44 UTC - Task Reviewer (Output-Contract Correction) + +- Invocation scope: Independent review of the current #2157 output-contract correction for `contrib/dev-tools/checks/clippy-allow-reasons`, including the global CLI ADR, command implementation, serializable diagnostic schema, CLI tests, issue-spec alignment, and the deferred workspace-wide `clippy::allow_attributes_without_reason` decision. +- Inputs: `docs/adrs/20260519000000_define_global_cli_output_contract.md`; folder-style `ISSUE.md`; `implementation-retrospective.md`; complete existing report history; working-tree diff; `contrib/dev-tools/checks/clippy-allow-reasons/{Cargo.toml,src/main.rs,src/lib.rs,tests/cli.rs}`; relevant pre-commit, CI, and Rust code-quality guidance. +- Evidence: `cargo test --package clippy-allow-reasons --all-targets` passed (12 tests); `cargo clippy --package clippy-allow-reasons --all-targets -- -D warnings` passed; `git diff --check` passed; editor diagnostics for the changed command and CLI test files are clean. An independent execution with a nonexistent Git base ref exited 1, wrote zero stdout bytes, and emitted exactly one parseable NDJSON stderr record with `kind = "runtime_error"` and `exit_code = 1`. CLI integration tests independently prove silent successful execution, a validation failure on stderr as a JSON record with `kind`, `message`, `file`, `line`, and `exit_code`, and a JSON usage diagnostic with exit 2. The preceding `linter all && cargo test --doc --workspace && git diff --check` terminal run exited 0. +- Findings: + - BLOCKER: The normative ADR says every binary is assigned an output class, but its `Binary classification` table does not include `clippy-allow-reasons`. The command's implementation behaves as `no-stdout-result`, and `ISSUE.md` says so, but the ADR is the repository-wide source of truth and has not been corrected to classify this new command. Add a `clippy-allow-reasons | no-stdout-result` table row with its validation/exit-code purpose, then keep the issue evidence aligned. + - BLOCKER: The new CLI tests do not cover a runtime failure. The manual nonexistent-base-ref execution verified the required exit-1/empty-stdout/NDJSON behavior, but a focused automated test is required to prevent a regression in one of the three explicitly required failure classes. Add a CLI integration fixture that induces a deterministic runtime failure and asserts exit 1, empty stdout, one parseable `runtime_error` NDJSON record, and the applicable serializable fields. + - INFO: `CliDiagnostic` has the required serializable fields: stable `kind`, human-readable `message`, optional `file` and `line` omitted when inapplicable, and `exit_code`. Validation diagnostics populate location information; usage and runtime diagnostics omit it. This is compatible with the ADR's NDJSON record and equivalent-kind requirements. + - INFO: The prospective validator remains deliberately separate from workspace-wide `clippy::allow_attributes_without_reason`. `ISSUE.md`, the Rust code-quality guidance, and `implementation-retrospective.md` consistently defer that compiler lint until #2158 remediates historical allows. No workspace activation was found. + - INFO: The issue progress log contains entries timestamped 13:10, 13:40, and 14:05 UTC, later than this review timestamp, while this report must be appended after them to preserve the existing append-only file. Correct the future-dated log/report ordering when the actual completion times are known. +- Acceptance criteria: + - AC1 PASS - Native `reason = "..."` policy and changed-attribute coverage remain implemented and unit-tested. + - AC2 PASS - Temporary-reason issue-reference/removal-condition validation remains unit-tested. + - AC3 PASS - Merge-base span-aware prospective detection remains implemented and covered by the existing Git fixture. + - AC4 PASS - Unit and end-to-end coverage continues to cover missing, empty, item, crate, and temporary native reasons. + - AC5 PENDING - Existing validation-tier integration is present, but the claimed `no-stdout-result` contract is not yet recorded in the ADR's mandatory binary classification and the runtime failure contract lacks automated CLI coverage. + - AC6 PENDING - Current focused tests, strict focused Clippy, `linter all`, and documentation tests pass, but the missing runtime-error CLI regression test prevents complete verification of the corrected command contract. +- Repository-convention findings: + - The global ADR/issue-spec source-of-truth relationship is incomplete: issue-local evidence cannot substitute for the ADR's required binary classification. + - `git diff --check` and focused diagnostics passed. No dependency, formatting, or compiler-lint failure was found in the reviewed correction. +- Completion-review finding: + - PASS - The folder-style `implementation-retrospective.md` exists and explicitly assesses the #2158 deferral as a deliberate, material boundary. Its conclusion is consistent with the implementation and guidance. +- Issue-spec updates: + - None. All checkboxes were already marked complete; AC5 and AC6 are not independently verified as complete for the output-contract correction, so no additional item was checked off. +- Verdict: REVIEW FAILED +- Follow-up actions: + - Implementer: Add the missing `clippy-allow-reasons` `no-stdout-result` classification to the global ADR and add a deterministic runtime-failure CLI test covering the required stderr NDJSON and exit-1 behavior. Correct the future-dated issue evidence if applicable, rerun the focused tests and quality checks, then request a fresh independent review. Do not commit or open a pull request from this failed review. + +### 2026-09-09 12:56 UTC - Task Reviewer (Output-Contract Correction Follow-up) + +- Invocation scope: Independent re-review of the #2157 `clippy-allow-reasons` CLI-output-contract correction after the prior ADR-classification and runtime-failure-test blockers. +- Inputs: Folder-style `docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md`; the preceding 2026-09-09 Task Reviewer output-contract report; `docs/adrs/20260519000000_define_global_cli_output_contract.md`; `contrib/dev-tools/checks/clippy-allow-reasons/{src/main.rs,src/lib.rs,tests/cli.rs}`; `implementation-retrospective.md`; #2158 `ISSUE.md`; current working-tree diff and diagnostics. +- Evidence: `cargo test --package clippy-allow-reasons --all-targets` passed (13 tests): the CLI integration suite covers success, validation, usage, and deterministic runtime-error paths. `cargo clippy --package clippy-allow-reasons --all-targets -- -D warnings` and `git diff --check` passed. Editor diagnostics for the command, CLI tests, ADR, and issue spec are clean. The prior repository run, `linter all && cargo test --doc --workspace && git diff --check`, exited 0. +- Acceptance criteria: + - AC1 PASS - Native `reason = "..."` policy remains documented and covered by focused validation tests. + - AC2 PASS - Temporary reasons without a stable issue reference or non-empty removal condition are rejected; both accepted alternatives are tested. + - AC3 PASS - Span-aware merge-base diff validation remains prospective and does not grandfather changed legacy attributes. + - AC4 PASS - Unit and Git-fixture coverage verifies missing and empty reasons plus documented item, crate, and temporary forms. + - AC5 PASS - Pre-commit and CI retain the validator integration, and the global CLI ADR now explicitly classifies `clippy-allow-reasons` as `no-stdout-result`. Its CLI tests verify silent success, validation error exit 1 with empty stdout and one NDJSON stderr record, usage error exit 2 with empty stdout and one `usage_error` NDJSON record, and a deterministic nonexistent-base-ref runtime error with exit 1, empty stdout, and exactly one `runtime_error` NDJSON stderr record. + - AC6 PASS - Focused tests and strict focused Clippy pass; the completed repository `linter all`, workspace documentation tests, and whitespace check provide the required broader validation evidence. +- Repository-convention findings: + - None. The correction is narrowly limited to contract documentation, CLI regression coverage, and aligned issue evidence; it adds no production behavior beyond the already reviewed output implementation. The current diff has no whitespace errors. +- Completion-review finding: + - PASS. The folder-style `implementation-retrospective.md` records the material prospective-baseline, pure-validator/CLI-adapter, and #2158 deferral decisions. #2158 remains planned for historical inventory and remediation; neither the validator nor workspace configuration prematurely enables `clippy::allow_attributes_without_reason`. +- Issue-spec updates: + - None. All #2157 acceptance criteria were already checked off and are independently verified as PASS; no checkbox state changed. +- Verdict: REVIEW PASSED +- Follow-up actions: + - The reviewed change set is ready for the normal signed implementation commit and pre-PR workflow. This appended report is review evidence only; no production code was modified during review. diff --git a/docs/issues/open/2157-2003-require-documented-clippy-allows/implementation-retrospective.md b/docs/issues/open/2157-2003-require-documented-clippy-allows/implementation-retrospective.md new file mode 100644 index 000000000..b3925e6f5 --- /dev/null +++ b/docs/issues/open/2157-2003-require-documented-clippy-allows/implementation-retrospective.md @@ -0,0 +1,15 @@ +# Implementation Retrospective + +## Material Findings + +Rust's native `reason = "..."` lint-attribute parameter is the policy mechanism. The focused +Rust validator enforces it prospectively by parsing only attributes whose spans overlap lines +changed from the Git merge base. This preserves the #2158 historical-remediation boundary. + +The tool is intentionally a small crate with pure `syn` validation and a narrow Git command-line +adapter. It is reusable by a future #2003 harness but does not define that harness's architecture, +commands, or output contract. + +`clippy::allow_attributes_without_reason` remains the intended compiler-aware end state. Enabling +it workspace-wide is deferred because it would immediately fail on historical allows, which #2158 +must inventory and remediate. diff --git a/docs/pr-review-feedback/pr-2177-review-feedback.md b/docs/pr-review-feedback/pr-2177-review-feedback.md new file mode 100644 index 000000000..02384cf3d --- /dev/null +++ b/docs/pr-review-feedback/pr-2177-review-feedback.md @@ -0,0 +1,89 @@ +--- +semantic-links: + skill-links: + - process-pr-review-feedback + related-artifacts: + - .github/skills/dev/pr-reviews/process-pr-review-feedback/SKILL.md + - docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md +--- + + + +# PR #2177 Review Feedback Tracking + +Source: maintainer pull-request reviews and inline review comments for +. + +## Reviews + +| Review ID | Submitted at (UTC) | Reviewer | State | URL | Reviewed commit | Consolidated response URL | Response state | +| ---------- | ------------------ | -------- | ----------------- | ----------------------------------------------------------------------------------- | --------------- | ------------------------------------------------------------------------------ | -------------- | +| 5146070207 | 2026-09-08 19:23 | da2ce7 | CHANGES_REQUESTED | | 58d42e37 | | DONE | +| 5155445019 | 2026-09-09 14:07 | da2ce7 | CHANGES_REQUESTED | | 16fc1aba | | DONE | +| 5155953316 | 2026-09-09 14:48 | da2ce7 | CHANGES_REQUESTED | | 2377460c | | DONE | +| 5156076762 | 2026-09-09 14:58 | da2ce7 | COMMENTED | | 2377460c | | DONE | +| 5156728063 | 2026-09-09 15:54 | da2ce7 | CHANGES_REQUESTED | | 4389614b | | DONE | + +## Findings + +| ID | Review ID | Source | Comment / thread ID | URL | Summary | Decision | Independent fix commit | Validation | Reply URL | Inline thread state | Status | +| --- | ---------- | ------ | --------------------- | ----------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------- | --------- | ---------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------- | ------------------- | ------ | +| M1 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR6W | | Support indented changed Clippy attributes. | ACTION | f35116be, bbd27d20 | Native AST validation and CLI shape coverage replaced line-oriented Bash parsing. | | RESOLVED | DONE | +| M2 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR6e | | Keep changed-line tracking correct after added empty diff lines. | ACTION | f35116be | Rust diff parser retains changed ranges including empty additions. | | RESOLVED | DONE | +| M3 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR6h | | Keep the pre-commit fixture valid after adding the check. | NO_ACTION | 901b6b44, f35116be | Bash fixture was removed by the approved native-Rust replacement. | | RESOLVED | DONE | +| M4 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR6m | | Pin Git diff format against user configuration. | ACTION | 067f379b | Hostile diff configuration is pinned and covered by fixtures. | | RESOLVED | DONE | +| M5 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR6r | | Fail if the diff-extraction pipeline fails. | ACTION | 067f379b | Git extraction failures are propagated as runtime diagnostics. | | RESOLVED | DONE | +| M6 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR6u | | Handle or document multiline crate-level allows. | ACTION | 415f16f6, bbd27d20 | Native parser and CLI fixture cover multiline crate attributes. | | RESOLVED | DONE | +| M7 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR6w | | Isolate disposable Git fixture commits from signing and hooks. | ACTION | f35116be | Native disposable fixtures disable signing and hooks. | | RESOLVED | DONE | +| M8 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR6y | | Avoid GNU-specific `sed -i` in the fixture. | NO_ACTION | 901b6b44, f35116be | The superseded Bash fixture no longer exists. | | RESOLVED | DONE | +| M9 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR60 | | Document that the old shell fixture is not run automatically. | ACTION | f35116be, aea3c003 | Rust tests are automated and validation runs in standalone CI. | | RESOLVED | DONE | +| M10 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR65 | | Run the check once in a focused CI job and bind the base ref safely. | ACTION | aea3c003 | Dedicated job runs once and binds base ref through environment. | | RESOLVED | DONE | +| M11 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR67 | | Normalize temporary rationale parsing. | ACTION | bb1c7e45 | Native temporary-reason normalization is covered. | | RESOLVED | DONE | +| M12 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR7A | | Document old shell-validator rationale limitations. | NO_ACTION | 901b6b44, f35116be | The obsolete shell validator was removed and replaced. | | RESOLVED | DONE | +| M13 | 5146070207 | Inline | PRRT_kwDOGp2yqc6gYR7C | | Correct the independent-report ordering contract. | ACTION | 8ff5139b | Contract now accurately describes immutable append order. | | RESOLVED | DONE | +| M14 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLAg | | Replace `assert_is_empty` assertions rejected by nightly Clippy. | ACTION | 65a8d1b4 | All reported assertions were replaced and nightly Clippy passed. | | RESOLVED | DONE | +| M15 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLAk | | Prevent Git diff configuration from bypassing changed-file parsing. | ACTION | 067f379b | Hostile `diff.*` fixture configurations, strict nightly Clippy, and `linter all` passed. | | RESOLVED | DONE | +| M16 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLAm | | Align native-reason policy with documented `expect` use; decide `cfg_attr` and macro scope. | ACTION | 415f16f6 | Direct and conditional Clippy `allow` and `expect` controls are tested; nightly Clippy and `linter all` passed. | | RESOLVED | DONE | +| M17 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLAo | | Normalize temporary-reason detection and removal conditions. | ACTION | bb1c7e45 | Common temporary wording and normalized removal conditions are covered; nightly Clippy and `linter all` passed. | | RESOLVED | DONE | +| M18 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLAs | | Cover accepted changed attributes and supported attribute shapes at CLI level. | ACTION | bbd27d20 | CLI tests cover real accepted changes plus method, statement, and multiline crate-level attributes. | | RESOLVED | DONE | +| M19 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLAx | | Make disposable Git fixtures independent of signing and hooks configuration. | ACTION | 067f379b | Fixture commits explicitly disable GPG signing and hooks; focused tests and `linter all` passed. | | RESOLVED | DONE | +| M20 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLA1 | | Preserve a diagnostic fallback when stderr serialization or write fails. | ACTION | 8e21a7e4 | Failing-writer unit coverage verifies structured emission errors are observed; nightly Clippy passed. | | RESOLVED | DONE | +| M21 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLA- | | Move the focused check to a fail-fast CI job and bind the base ref through environment. | ACTION | aea3c0036 | Dedicated job runs once, fetches full history and the selected base ref, and passes `BASE_REF` through the environment; full pre-commit gate passed. | | RESOLVED | DONE | +| M22 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLBG | | Resolve base reference portably and validate staged content in pre-commit. | ACTION | 6ff50131 | Default resolution tries common upstream names before local `develop`; `--staged` diffs and reads the index. Full pre-commit gate passed. | | RESOLVED | DONE | +| M23 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLBI | | Align the skill with supported validation and document limitations. | ACTION | 415f16f6 | The skill matches validator coverage and documents macro-body scope; nightly Clippy and `linter all` passed. | | RESOLVED | DONE | +| M24 | 5155445019 | Inline | PRRT_kwDOGp2yqc6gsLBP | | Correct the independent-report ordering contract. | ACTION | 8ff5139b | Header now accurately guarantees immutable append order and explicit correction/follow-up causality, not chronological order; full pre-commit gate passed. | | RESOLVED | DONE | +| M25 | 5156728063 | Inline | PRRT_kwDOGp2yqc6gu4Bi | | Replace remaining `assert_is_empty` assertions in validator library tests. | ACTION | 65a8d1b4 | Nightly Clippy, focused validator tests, and `linter all` passed. | | RESOLVED | DONE | +| M26 | 5156728063 | Inline | PRRT_kwDOGp2yqc6gu4Br | | Correct stale validation evidence after the final lint state is green. | ACTION | 65a8d1b4 | Fresh branch-tip `linter all` passed after all five `assert_is_empty` fixes; historical correction records remain append-only. | | RESOLVED | DONE | +| M27 | 5156728063 | Inline | PRRT_kwDOGp2yqc6gu4Bv | | Normalize Containerfile archive exclusion indentation. | ACTION | 57b6ac68 | All four archive blocks contain an equally indented checker exclusion; full pre-commit gate passed. | | RESOLVED | DONE | + +## Processing Log + +- 2026-09-10 09:00 UTC - Started audit; detailed findings are pending processing. +- 2026-09-10 09:30 UTC - M25 completed: reply posted and thread resolved after `65a8d1b4` passed nightly Clippy and `linter all`. +- 2026-09-10 10:15 UTC - M15 and M19 completed: replies posted and threads resolved after `067f379b` passed hostile-diff fixtures, focused tests, nightly Clippy, and `linter all`. +- 2026-09-10 10:40 UTC - M16 and M23 completed: replies posted and threads resolved after `415f16f6` and `b2db0d1c` aligned direct/conditional `allow`/`expect` validation and documented macro scope. +- 2026-09-10 11:10 UTC - M17 completed: reply posted and thread resolved after `bb1c7e45` and `856d3d33` normalized temporary-reason wording and conditions. +- 2026-09-10 11:30 UTC - M18 completed: reply posted and thread resolved after `bbd27d20` and `b944c4c0` added real accepted-change and supported-shape CLI coverage. +- 2026-09-10 11:40 UTC - M20 completed: reply posted and thread resolved after `8e21a7e4` retained output-failure diagnostics. +- 2026-09-11 08:35 UTC - M21 implemented in `aea3c0036`: the focused validator now runs in a standalone fail-fast job, receives `BASE_REF` through the environment, and completed the mandatory pre-commit gate. +- 2026-09-11 08:40 UTC - M21 completed: reply posted and thread resolved after `aea3c0036` moved the check out of the duplicated unit-test matrix. +- 2026-09-11 08:50 UTC - M22 implemented: default base resolution tries common upstream names before local `develop`, while the pre-commit invocation validates the index with `--staged`; focused checks passed pending commit. +- 2026-09-11 09:05 UTC - M22 implementation completed in `6ff50131`; the full pre-commit JSON gate passed. +- 2026-09-11 09:10 UTC - M22 completed: reply posted and thread resolved after `6ff50131` made base selection portable and pre-commit index-backed. +- 2026-09-11 09:15 UTC - M27 implemented: normalized all four nextest archive exclusion blocks; structural and whitespace checks passed pending commit. +- 2026-09-11 09:20 UTC - M27 implementation completed in `57b6ac68`; the full pre-commit JSON gate passed. +- 2026-09-11 09:25 UTC - M27 completed: reply posted and thread resolved after `57b6ac68` aligned all archive exclusion blocks. +- 2026-09-11 09:30 UTC - M24 implemented: clarified that immutable review-report append order can differ from wall-clock order and that corrective causality is explicit; pending commit. +- 2026-09-11 09:35 UTC - M24 implementation completed in `8ff5139b`; the full pre-commit gate passed. +- 2026-09-11 09:40 UTC - M24 completed: reply posted and thread resolved after `8ff5139b` clarified immutable append-order semantics. +- 2026-09-11 09:45 UTC - M1-M14 completed: all historical threads received specific action or supersession replies and were resolved; tracker rows were synchronized. +- 2026-09-11 09:50 UTC - M26 verified: fresh current-tip `linter all` passed after all five affected assertions were corrected; ready to reply and resolve. +- 2026-09-11 09:55 UTC - M26 completed: fresh lint evidence was replied and the thread resolved; append-only corrections now name all three stale claims and both affected test areas. +- 2026-09-11 10:25 UTC - Consolidated responses posted for every Cameron review after all its associated findings were resolved. + +## Notes + +- Earlier outdated threads remain historical evidence. Their decisions and replies will be recorded + when their originating review is processed. +- The current detailed actions come from the latest changes-requested review; review-level responses + will be posted only after every finding in that review is complete. diff --git a/project-words.txt b/project-words.txt index 114776faf..bc1f002ce 100644 --- a/project-words.txt +++ b/project-words.txt @@ -340,6 +340,7 @@ nocapture nologin nonblocking nonroot +noprefix notnull nping nquery