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 08af251ec..f7574f828 100644 --- a/.github/skills/dev/maintenance/update-github-workflow-actions/SKILL.md +++ b/.github/skills/dev/maintenance/update-github-workflow-actions/SKILL.md @@ -66,18 +66,15 @@ git push {your-fork-remote} "$UPDATE_BRANCH" 2. Start from an up-to-date `develop` branch and create that dedicated branch. 3. Identify every matching action reference and review the action's release notes for compatibility or security implications. If no workflow reference needs an update, stop without committing. 4. Update all intended `.github/workflows/*.yaml` references consistently. Dependabot manages GitHub Actions updates through `.github/dependabot.yaml`; preserve its explicit version format. -5. Before opening the pull request, obtain the complete current Torrust organization allowed-actions list and save it in `"$ALLOWLIST_CURRENT"`. Prepare the complete revised list in `"$ALLOWLIST_NEW"`, retaining every current entry. The allowlist is organization-wide: never replace it with an inventory from this repository alone. A missing entry from the configured list may be authorized by a broader organization policy, such as GitHub-owned or verified Marketplace actions; do not infer that it must be added from a repository scan. If a complete replacement list is requested, obtain an organization-wide inventory first; otherwise, provide only the required additions and replacements. If the required reference is not allowed and the agent cannot change the organization policy, tell the user that a GitHub organization administrator must update the allowed-actions list before the workflow can run. - - Add an allowlist pattern that permits the versioned reference, such as `owner/action@v2.*`. - - 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. - - Compare the lists with a set-based comparison. Verification must prove that `"$ALLOWLIST_CURRENT"` is a subset of `"$ALLOWLIST_NEW"` for an additive change and that every third-party `uses:` reference is matched by the new list. - - Have an organization administrator apply the revised list at [Organization Actions settings](https://github.com/organizations/torrust/settings/actions), then confirm the update. Do not remove old entries merely because this repository no longer uses them; other Torrust repositories may still rely on them. +5. Before opening the pull request, obtain the complete current Torrust organization allowed-actions list and save it in `"$ALLOWLIST_CURRENT"`. Prepare the complete revised list in `"$ALLOWLIST_NEW"`, retaining every current entry. The allowlist is organization-wide: never replace it with an inventory from this repository alone. A missing entry from the configured list may be authorized by a broader organization policy, such as GitHub-owned or verified Marketplace actions; do not infer that it must be added from a repository scan. If a complete replacement list is requested, obtain an organization-wide inventory first; otherwise, provide only the required additions and replacements. If the required reference is not allowed and the agent cannot change the organization policy, tell the user that a GitHub organization administrator must update the allowed-actions list before the workflow can run. Add an allowlist pattern that permits the versioned reference, such as `owner/action@v2.*`, and 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. Compare the lists with a set-based comparison: for an additive change, `"$ALLOWLIST_CURRENT"` must be a subset of `"$ALLOWLIST_NEW"`, and every third-party `uses:` reference must be matched by the new list. Have an organization administrator apply the revised list at [Organization Actions settings](https://github.com/organizations/torrust/settings/actions), then confirm the update. Do not remove old entries merely because this repository no longer uses them; other Torrust repositories may still rely on them. 6. 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. -7. Run `linter yaml`, `git diff --check`, and the mandatory pre-commit checks before committing. -8. Commit with a signed Conventional Commit and push the branch to the fork remote. When step 5 required an organization policy update, include the complete contents of both `"$ALLOWLIST_CURRENT"` and `"$ALLOWLIST_NEW"` verbatim in the commit body. Use headings that identify the lists as before and after the update; do not summarize or omit unchanged entries. -9. Open a PR targeting `develop` with [the GitHub workflow-actions update PR template](../../../../../docs/templates/GITHUB-WORKFLOW-ACTIONS-UPDATE-PR.md). Replace both allowlist placeholders with the complete captured contents verbatim. State the administrator's confirmation only when it was received. -10. 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. +7. When a workflow command compares Git revisions, configure `actions/checkout` with + `fetch-depth: 0` so its merge base is available in CI. +8. Run `linter yaml`, `git diff --check`, and the mandatory pre-commit checks before committing. +9. Commit with a signed Conventional Commit and push the branch to the fork remote. When step 5 required an organization policy update, include the complete contents of both `"$ALLOWLIST_CURRENT"` and `"$ALLOWLIST_NEW"` verbatim in the commit body. Use headings that identify the lists as before and after the update; do not summarize or omit unchanged entries. +10. Open a PR targeting `develop` with [the GitHub workflow-actions update PR template](../../../../../docs/templates/GITHUB-WORKFLOW-ACTIONS-UPDATE-PR.md). Replace both allowlist placeholders with the complete captured contents verbatim. State the administrator's confirmation only when it was received. +11. 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 d53f1012a..1f2a7613d 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.0.1 + 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 a77329000..11123fff9 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..331c695c1 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-14 12:40 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,132 @@ 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. +- [x] 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-14 12:15 UTC - GitHub Copilot - Rebased PR #2177 onto `torrust/develop` at `8d93b691`; upstream workflow-skill conflict resolution introduced a Markdown lint regression now repaired locally - Pending focused signed commit, push, and fresh CI +- 2026-09-14 12:20 UTC - GitHub Copilot - Found a second rebase-conflict regression: the unit-job checkout `uses:` key in `testing.yaml` had incorrect indentation, causing YAML parsing to fail; repaired locally - Pending validation, focused signed commit, push, and fresh CI +- 2026-09-14 12:25 UTC - GitHub Copilot - Corrected the issue-progress wording after CSpell rejected a nonstandard indentation term; rerunning the complete validation gate - Pending focused signed commits, push, and fresh CI +- 2026-09-14 12:30 UTC - GitHub Copilot - Removed the remaining CSpell-rejected term from the issue progress record - Pending complete validation, focused signed commits, push, and fresh CI +- 2026-09-14 12:35 UTC - GitHub Copilot - Validated both rebase-conflict repairs: the full mandatory pre-commit JSON gate passed, including Markdown, YAML, CSpell, Clippy, formatting, ShellCheck, and documentation tests - Pending focused signed commits, push, and fresh CI +- 2026-09-14 12:40 UTC - GitHub Copilot - Committed both rebase-conflict CI repairs in `94c47af5`: restored valid unit-job YAML and Markdownlint-compliant workflow-actions guidance; full pre-commit gate passed - Pending issue tracking commit, push, and fresh CI +- 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