feat(quality): [#2157] require documented Clippy allows

[josecelano]

Summary

Adds prospective enforcement for documented Clippy allow attributes. The new modular Rust validator compares changed Rust attribute spans with the merge base and requires Rust's native reason = "..." parameter for added or modified item and crate allowances, without making the existing inventory a blocker.

Temporary native reasons must provide either a stable issue reference or a non-empty removal condition. The validator separates a pure syn validation module from a narrow Git command adapter, with unit coverage and an end-to-end disposable-Git-repository fixture for undocumented changed attributes.

The check runs in the existing pre-commit hook and testing workflow. CI fetches full history so it can compute the merge base reliably. clippy-allow-reasons is classified as a no-stdout-result command: success is silent and failures emit NDJSON diagnostics on stderr. The cargo-chef recipe stage includes the tool's manifest and targets, while all nextest archives exclude this Git-dependent development check. CLI arguments are parsed before Git access so usage errors reliably retain exit code 2. Rust quality guidance, the ClippyFixer agent instructions, workflow guidance, and #2157 completion evidence now describe the policy and validation design. Workspace-wide clippy::allow_attributes_without_reason remains deferred until #2158 remediates historical attributes.

Root Cargo.toml and Containerfile now have reciprocal semantic links, and the new add-workspace-member skill requires contributors to review cargo-chef recipe inputs, all archive exclusions, and .dockerignore whenever an explicit workspace member changes.

Validation

• cargo test --package clippy-allow-reasons
• cargo clippy --package clippy-allow-reasons --all-targets -- -D warnings
• cargo +nightly fmt --check
• cargo run --quiet --package clippy-allow-reasons -- --base-ref torrust/develop
• linter all
• CLI tests for silent success plus NDJSON validation, runtime, and usage errors
• docker build --no-cache --target recipe --file Containerfile .
• docker build --no-cache --target test_debug --file Containerfile .

Closes #2157

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Adds prospective enforcement that newly added or modified Rust #[allow(clippy::...)] suppressions must include an adjacent rationale comment, without blocking on the legacy inventory.

Changes:

• Introduces a merge-base diff validator (require-documented-clippy-allows.sh) plus Git-fixture integration tests.
• Integrates the check into the pre-commit hook and CI workflow (including full-history checkout).
• Updates Rust quality guidance and agent/workflow documentation to describe the rationale policy.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
docs/issues/open/2157-2003-require-documented-clippy-allows/implementation-retrospective.md	Records key architectural decision (merge-base baseline) and CI history requirement.
docs/issues/open/2157-2003-require-documented-clippy-allows/agent-review-reports.md	Captures independent reviews and evidence for implementation/acceptance criteria.
docs/issues/open/2157-2003-require-documented-clippy-allows/ISSUE.md	Marks plan, progress, and acceptance criteria as completed with evidence.
contrib/dev-tools/git/hooks/pre-commit.sh	Adds the documented-Clippy-allows validator to pre-commit steps.
contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh	Adds Git-fixture integration tests for documented/undocumented and temporary cases.
contrib/dev-tools/checks/require-documented-clippy-allows.sh	Implements the merge-base diff validator and rationale enforcement.
.github/workflows/testing.yaml	Ensures full history checkout and runs the new validator in CI.
.github/skills/dev/rust-code-quality/fix-clippy-warnings/SKILL.md	Documents the required rationale formats and temporary suppression rules.
.github/skills/dev/maintenance/update-github-workflow-actions/SKILL.md	Adds guidance about fetch-depth: 0 when workflows need merge-base computation.
.github/skills/dev/git-workflow/run-pre-commit-checks/SKILL.md	Updates pre-commit step list to include the new validator.
.github/agents/clippy-fixer.agent.md	Updates agent instructions to require adjacent rationale comments for changed allows.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.93%. Comparing base (0e61ec8) to head (a45c18c).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2177      +/-   ##
===========================================
+ Coverage    84.86%   84.93%   +0.06%     
===========================================
  Files          351      352       +1     
  Lines        30131    30337     +206     
  Branches     30131    30337     +206     
===========================================
+ Hits         25572    25766     +194     
- Misses        4186     4195       +9     
- Partials       373      376       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[da2ce7]

Review (review-pr skill)

Verdict: changes requested. The validator cannot see indented #[allow(clippy::…)] attributes, which are 56% of this repository's allows (120 of 215 on this head), so newly added undocumented allowances pass silently and AC3 is not met; separately, the new pre-commit step breaks an existing test.

This is a well-shaped change: the merge-base approach is the right call for a prospective policy, the retrospective records why, the Git fixtures are real integration tests rather than mocks, and the skill-link discipline on testing.yaml is exactly right. The blocking problem is in the attribute matcher: ^\+#!?\[allow\(clippy:: is anchored at column 0, so every allow on a method inside an impl or a mod is skipped without any diagnostic. A second defect in the same awk block, /^\+[^+]/, does not count added blank lines, which shifts reported line numbers and lets a rationale separated from its attribute by a blank line pass. Both are fixed by a few characters (^\+[[:space:]]*#!?\[allow\(clippy:: and incrementing on every added line), and both want fixtures. Separately, adding the validator to pre-commit.sh breaks it_should_continue_pre_commit_checks_when_dictionary_is_already_formatted in test-format-project-words.sh, whose fixture root does not contain the new script. The remaining inline items are suggestions and nits.

Acceptance criteria for #2157, as the diff stands

AC	Assessment
AC1 rationale requirements for all supported attribute forms	Partial: the syntax is well defined; the forms are not (indentation, multi-line #![allow(, #[expect] are unaddressed and only column-0 single-line is enforced).
AC2 temporary allows name a removal condition or issue	Satisfied (lines 54-62, fixtures 84-115).
AC3 excludes existing attributes without accepting new undocumented ones	Not satisfied: B1 accepts any new indented allow, B3 accepts a non-adjacent rationale, S1 accepts additions to multi-line blocks.
AC4 focused tests for item and crate forms	Partial: eight real fixtures, all column-0; the dominant indented form is neither tested nor enforced.
AC5 enforcement in a documented tier with actionable diagnostics	Partial: tier integration and docs are done; B3 makes the diagnostic name a line holding no attribute.
AC6 linter all and relevant tests pass	Not fully: linter all and the new suite pass (verified: 18.3 s, 0.6 s), but test-format-project-words.sh now fails (B2).

Two items not attachable inline: the spec frontmatter still reads status: planned, related-pr: null, last-updated-utc: 2026-09-07 11:20 while the body marks every AC done and this PR exists (siblings with an open PR use in-review and a populated related-pr); and under AGENTS.md Engineering Policy #3 a diff parser with a hand-maintained line counter and its own test suite is the kind of component that policy points at Rust for — not a reason to hold this PR, but the retrospective is the right place to record it (#1843 owns that migration).

Verification performed on a server lane against this head: bash contrib/dev-tools/checks/tests/test-require-documented-clippy-allows.sh passes (0.6 s); the validator itself passes against its merge base; linter all passes (18.3 s). The B1 and B3 findings were confirmed by running the awk extractor against synthetic diffs in a scratch repository, not by inference.

Worth follow-up issues rather than this PR

1. Cover the remaining suppression forms deliberately: multi-line #![allow(, #[expect(clippy::…)], #[cfg_attr(…, allow(clippy::…))] (pairs with #2158).
2. Move the validator to Rust under Engineering Policy #3 / #1843; a token-aware implementation removes the whole regex defect class and the fixtures port directly.
3. Run contrib/dev-tools/checks/tests/* in CI; three scripts exist and none is executed by any tier.
4. Refresh docs/issues/open/2003-overhaul-guardrails-and-automation/initial-inventory.md:42, which already omitted the dictionary formatter and hadolint and is now one step further behind.
5. Fix the stale path in run-pre-commit-checks/SKILL.md:63 (git/format-project-words.sh vs the hook's checks/format-project-words.sh).

[da2ce7]

Blocker (B1). ^\+#!?\[allow\(clippy:: is anchored at column 0, so an indented attribute — every #[allow(clippy::…)] on a method inside an impl, an item inside a mod, or anything under #[cfg(test)] — is skipped without a diagnostic. On this PR's head the repository has 120 indented allow(clippy:: attributes against 95 at column 0 (grep -rEn '^[[:space:]]+#!?\[allow\(clippy::' --include='*.rs'), e.g. packages/configuration/src/v2_0_0/tracker_api.rs:51. Concrete failing input on a feature branch, which the validator accepts with exit 0:

impl Thing {
    #[allow(clippy::too_many_lines)]
    fn big(&self) {}
}

This defeats #2157's Goal ("every newly introduced or modified") and AC3 ("without accepting new undocumented attributes"), and contradicts the spec's own mitigation "reject unrecognized forms visibly". Fix: /^\+[[:space:]]*#!?\[allow\(clippy::/ plus fixtures for the indented item form.

[da2ce7]

Blocker (B3). /^\+[^+]/ requires a character after the +, so an added empty line (a bare + in --unified=0 output) never increments line_number. Every added allow after it in the same hunk is reported one line early per preceding blank line, and require_rationale then re-reads the wrong line: the diagnostic quotes a blank line as the attribute, and — worse — a rationale separated from its attribute by a blank line is accepted, because the shifted line number lands on the comment. Measured layout: // clippy-allow: … at line 2, blank line 3, #[allow(clippy::c)] at line 4 → reported as :3, rationale read from line 2, returns 0. That is exactly the non-adjacent form the updated fix-clippy-warnings skill forbids ("immediately above"). Fix: count every added line (/^\+/, with the +++ header handled by its own rule and next); validated together with the B1 fix, both cases then report the true lines.

[da2ce7]

Blocker (B2). This step breaks an existing test: contrib/dev-tools/checks/tests/test-format-project-words.sh builds a fixture root that copies only format-project-words.sh and pre-commit.sh (its create_fixture, lines 26-27) and stubs only cargo and linter. it_should_continue_pre_commit_checks_when_dictionary_is_already_formatted (lines 188-207) runs the real hook there: step 2 now executes a path that does not exist (exit 127), run_step returns 127, the loop at pre-commit.sh:377 breaks, commands.log has 0 lines instead of 4 and the success line is never printed. Even with the script copied in, resolve_base_ref fails in the non-Git fixture and exits 2. Fix: copy the validator into the fixture and git init it, or stub the validator alongside cargo/linter.

[da2ce7]

Suggestion (S5). The parse depends on the developer's local diff.* configuration: under diff.mnemonicPrefix=true the header is +++ w/src/lib.rs, file_path is never set, the extractor emits :3, and sed -n 3p "${PROJECT_ROOT}/" fails with Is a directory — reported as a bogus rationale failure because require_rationale runs as an if ! condition where set -e is suppressed. diff.noprefix=true, color.diff=always, and a diff.external driver break it the same way. Pin the format: git -c diff.noprefix=false -c diff.mnemonicPrefix=false -c core.quotePath=false --no-pager diff --no-ext-diff --no-color --unified=0 …, or match /^\+\+\+ ./ and strip the one-letter prefix.

[da2ce7]

Suggestion (S9). The loop is fed from process substitution, whose exit status is never checked and which pipefail does not reach: if git diff or awk fails, the loop sees no input and line 95 prints the success message. Capture the output and check the status (or run the pipeline in an explicit subshell that propagates failure) so a broken pipeline is an error, not a pass.

[da2ce7]

Suggestion (S8). Nothing runs this file: neither CI nor the hook executes contrib/dev-tools/checks/tests/*. The sibling script says so in its header ("NOTE: These tests are NOT automatically run by the pre-commit hook or CI…", test-format-project-words.sh:6-9); adding the same note keeps the caveat discoverable and is the honest counterweight to AC4.

[da2ce7]

Suggestion (S6/S7). A 0.0 s check that needs only git, awk and sed sits behind toolchain install, Node, sccache, cargo fetch, cargo install torrust-linting, cargo fmt and linter all, runs once per matrix leg, and fetch-depth: 0 now adds a full-history clone to the heaviest job. A small standalone job modelled on layer-bans would fail fast, run once, and confine the full clone. Separately, ${{ github.base_ref || 'develop' }} is interpolated straight into the run: string; binding it through env: and reading "$BASE_REF" is the standard hygiene and survives odd branch names.

[da2ce7]

Nit (N1-N3). remove[[:space:]](when|after|by)[[:space:]]+… requires exactly one space after remove, so "removed when …", "remove when …" and "to be removed once …" fail; UNTIL_CONDITION_REGEX accepts until <x> but no updated doc mentions that form; RATIONALE_REGEX requires exactly one space after //, so //clippy-allow: falls through to the generic message with no near-miss hint.

[da2ce7]

Nit (N4/N5). Only the single line above the attribute is inspected, so a two-line rationale fails (worth a sentence in the skill), and a #![allow(clippy::…)] on line 1 of a file can never pass — a targeted message ("move the rationale comment above it") beats the generic one. substr($0, 7) on a +++ /dev/null header yields ev/null; harmless today, a trap if the matcher is relaxed (N6).

[da2ce7]

Suggestion (S12). Entries are out of chronological order (17:03, 17:24, 17:12, 17:03-correction) while the file's own header says entries are appended and never reordered, and the sibling 2160 file is strictly ascending. A reader meets the 17:24 resolution before the 17:12 blocker it resolves.

[da2ce7]

Round-2 review of 16fc1ab (merge base 7abc30b2b9fb85b235e7b2ef2a40f5d5f7ed1555).

Verdict: Request changes. The Bash guard was replaced by a native Rust validator, and that rewrite fixes the two round-1 blockers about the matcher (B1, B3) plus S1, S8, S9 and N4-N6 outright. What remains is one hard blocker that is already red in CI, two enforcement holes that let the gate pass on input it is supposed to reject, and five round-1 findings the new head carries over unchanged.

Blockers

BB1 - CI is red on this PR's own new code. linter all fails on nightly: clippy::assert_is_empty (pedantic, denied by [workspace.lints.clippy] pedantic = "deny") fires five times in contrib/dev-tools/checks/clippy-allow-reasons/src/lib.rs (175, 182, 198, 206, 213) and five times in contrib/dev-tools/checks/clippy-allow-reasons/tests/cli.rs (31, 58, 59, 70, 87). Run 34354675664, job Unit (nightly), step Run All Linters; Unit (stable) was cancelled behind it. Reproduced independently: linter all exit 1 in 43.1 s and cargo clippy --workspace --all-targets --all-features -- -D warnings exit 101 in 3.6 s on nightly rustc 1.100.0-nightly (a69a63265 2026-09-03); the same clippy passes on rustc 1.98.0 (88d9e12ae 2026-08-18) in 38.1 s, so this is nightly-only and real. assert_eq!(output.stdout, [] as [u8; 0]) is the suggested rewrite. Worth noting that the other mechanical escape - #[allow(clippy::assert_is_empty)] - is the exact form this PR's own gate would then reject for lacking a reason.

BB2 - the gate silently passes under ordinary local Git settings (round-1 S5, not addressed and now worse). parse_changed_rust_lines recognises a file only via line.strip_prefix("+++ b/"), and changed_rust_lines invokes git diff with no format pinning. Measured against this head, with an undocumented #[allow(clippy::too_many_lines)] present in src/lib.rs:

local config	header the tool sees	exit
(none)	+++ b/src/lib.rs	1 (correct)
diff.noprefix=true	+++ src/lib.rs	0
diff.mnemonicPrefix=true	+++ w/src/lib.rs	0
color.diff=always	ANSI-wrapped	0

In round 1 this class of misconfiguration produced a visible (if bogus) error; now it produces a clean pass, so the developer-side hook reports success while enforcing nothing. diff.external and core.quotePath are the same shape. Two things are needed: pin the invocation (git -c diff.noprefix=false -c diff.mnemonicPrefix=false -c core.quotePath=false --no-pager diff --no-ext-diff --no-color --unified=0 ...), and make an unrecognised +++/@@ line an error rather than a skip, so "I understood nothing" can never be reported as "nothing to report". CI is not affected today (runners set none of these), which is exactly why only the pinning plus a hard failure will catch it.

BB3 - #[expect(...)] is unchecked, and the updated skill now recommends it (round-1 S2, not addressed and now contradicted). is_clippy_allow returns early unless the attribute's own path is allow, so #[expect(clippy::too_many_lines)] with no reason passes with exit 0 (measured). Meanwhile fix-clippy-warnings/SKILL.md gains "For a temporary item-level suppression, prefer #[expect(..., reason = \"...\")]" - the documentation steers authors at precisely the form the validator does not inspect, and #[expect] is the more likely home for a temporary suppression, which is the case the reason policy exists for. Either accept expect alongside allow (a one-line change to is_clippy_allow plus fixtures), or drop the expect recommendation from the skill until it is covered.

Suggestions

S-a - #[cfg_attr(test, allow(clippy::...))] is not checked (measured: exit 0). Same root cause as BB3.

S-b - attributes inside macro_rules! bodies are not checked (measured: exit 0), because syn::visit does not descend into macro token streams. This one is inherent to the AST approach rather than a defect; the honest fix is a stated limitation in fix-clippy-warnings/SKILL.md, next to the "prospective only" caveat.

S-c - near-miss temporary detection (round-1 N1-N3, restated in the new mechanism). is_temporary is contains("temporary"), so "temporarily", "TODO", "for now" and "workaround" skip the removal-condition requirement entirely; and the condition prefixes are matched with exactly one trailing space ("remove when "), so "remove when", "removed when" and "remove when:" all fail a reason the author believes is compliant. The skill documents only the exact spellings, so the two disagree at the edges.

S-d - the happy-path CLI test proves nothing (round-1 N7/N8, not addressed). it_should_not_write_output_when_validation_succeeds creates the feature branch and then changes no file, so it exercises an empty diff, not an accepted documented allow. There is no CLI-level coverage for the accepted case, for an indented attribute inside an impl, or for a multi-line #![allow(...)] block - the three cases the syn rewrite exists to handle. (All three do work: I verified each against this head.) Arrange/Act/Assert comments used by the sibling suites are also absent.

S-e - Git fixtures still lack the repository's isolation flags (round-1 S3, carried over into the Rust fixture). FixtureRepository::new sets user.email/user.name but not commit.gpgsign=false or core.hooksPath=/dev/null, and git() asserts on success. The precedent is contrib/dev-tools/git/tests/test-merge-pull-request.sh:43. A contributor with a global commit.gpgsign=true and no signing key inside the fixture - plausible under Essential Rule #2 - fails the suite for reasons unrelated to the validator.

S-f - CI placement and interpolation (round-1 S6/S7, not addressed). The check still sits inside the unit matrix job behind toolchain install, Node, sccache, cargo fetch, cargo install torrust-linting, cargo fmt and linter all, running once per matrix leg; fetch-depth: 0 now adds a full-history clone to the heaviest job in the workflow. A standalone job modelled on layer-bans would fail fast, run once, and confine the deep clone. Separately, ${{ github.base_ref || 'develop' }} is still interpolated directly into the run: string rather than bound through env: and read as "$BASE_REF".

S-g - the two callers disagree about the upstream remote's name, and neither falls back. The hook passes --base-ref torrust/develop and the binary defaults to torrust/develop; CI passes origin/<base>. A torrust remote is assumed by release-new-version/SKILL.md but nowhere established for contributors, and there is no git remote add guidance in the repository. A contributor whose upstream is origin or upstream gets a runtime_error and exit 1 on every commit. Resolve the base ref against a small candidate list, or state the required remote name in run-pre-commit-checks/SKILL.md.

S-h - the hook diffs the working tree, not the index. git diff --unified=0 <base> (no --cached) compares the merge base to the working tree, so a pre-commit run can fail on edits that are not being committed, and it never evaluates exactly the content that is. --cached plus reading the staged blob would match what a pre-commit gate is asked to certify.

S-i - a whole-file reformat re-arms every legacy allow in that file. "Changed" is any line overlapping the attribute's span, so a rustfmt or import-ordering pass across a file holding legacy undocumented allows converts all of them into blockers in a single commit. That is defensible, but it is the sharp edge of the prospective baseline and belongs in the skill so it is met as a documented rule rather than as a surprise, and it interacts with #2158's sequencing.

S-j - agent-review-reports.md is still not chronological (round-1 S12, not addressed). The file reads 2026-09-08 17:03, 17:24, 17:12, 17:03 (Correction), then 2026-09-09 11:14, 11:16, 12:44, 12:56, while its own header says entries are appended and never reordered. Two of the newer entries additionally record that the issue progress log carries later timestamps (13:10, 13:40, 14:05) than the reports appended after them, so the file now documents its own ordering problem instead of fixing it.

Nit

N-a - emit_diagnostic discards both write results (drop(serde_json::to_writer(...)), drop(stderr.write_all(...))). Behaviour stays safe because the exit code is set independently, but a validator that fails to print why it failed is worth one if let Err(...) fallback line.

Round-1 findings: status at this head

Thread state says nothing either way, so each was re-derived from the tree. Eight can be resolved as they stand.

#	Round-1 finding	Status at 16fc1ab
B1	column-0 anchor skips indented attributes	Addressed - AST walk; an #[allow] on a method inside an impl is rejected (verified). Resolvable.
B3	blank lines shift the reported line number	Superseded - awk line counting is gone; reasons are attribute-internal. Reported lines were exact in every probe. Resolvable.
B2	new hook step breaks test-format-project-words.sh	Superseded - the step is now a cargo invocation, which the fixture already stubs; measured, step 2 PASSes and commands.log goes 3 to 4 lines. That suite is red on 7abc30b2 too, for two pre-existing reasons unrelated to this PR (its PROJECT_ROOT uses three .. for a four-level path, and the lint-containerfile.sh step is neither copied into the fixture nor stubbed). Not this PR's regression; worth its own issue. Resolvable.
S5	parse depends on local diff.* configuration	Not addressed - see BB2, and the failure mode degraded from a visible error to a silent pass.
S9	unchecked process substitution can pass on a broken pipeline	Addressed - git_output checks status.success() and propagates. Resolvable. (The residual "understood nothing, reported nothing" path is BB2, not this.)
S1	multi-line crate-level allow blocks invisible	Addressed - a multi-line #![allow(\n clippy::...,\n)] is rejected (verified). Resolvable.
S2	#[expect(...)] not handled	Not addressed - see BB3, and now contradicted by the skill.
S3	fixture commits lack commit.gpgsign/core.hooksPath isolation	Not addressed - see S-e; carried over into the new Rust fixture.
S4	GNU-only sed -i	Superseded - the Bash test is deleted. Resolvable.
N7/N8	happy path asserts only the exit code	Not addressed - see S-d, in a stronger form.
S8	nothing runs the test file	Addressed - the tests are cargo test targets in a workspace member, run by CI's Run Unit Tests step. Resolvable.
S6/S7	CI placement, cost, and ${{ }} interpolation	Not addressed - see S-f.
N1-N3	rationale near-misses	Superseded in form (the // comment regexes are gone); the same class survives in is_temporary and the condition prefixes - see S-c.
N4-N6	one-line rationale window; line-1 crate allow; substr on /dev/null	Addressed - reasons are attribute-internal, so none of the three can arise. Resolvable.
S12	agent-review-reports.md out of order	Not addressed - see S-j.

Enforcement analysis

The design question the issue turns on is answered clearly and, I think, correctly: clippy::allow_attributes_without_reason is the eventual compiler-aware mechanism but would flag the whole historical inventory, so this PR enforces prospectively with a custom checker and defers the inventory to #2158. fix-clippy-warnings/SKILL.md states that trade-off explicitly, which is the right way to carry it. The scope is honest about itself: at this head the workspace holds 226 #[allow(clippy::...)] attribute lines and the only seven carrying a reason are string literals inside the validator's own tests, so no production reason text exists yet to check for truthfulness.

Enforcement is real rather than documentary - a workspace member, wired into both CI and the hook, that rejects on exit code - and the AST rewrite genuinely closed the round-1 matcher holes. The gap is coverage of the attribute surface, not of the idea: expect, cfg_attr, and macro bodies are all silent, and BB2 means the whole check can be silent. Once BB1 is fixed and BB2/BB3 are closed or explicitly scoped out in writing, the shape of this is sound.

Verification performed

All work read-only against refs/quarantine/pr-2177; gates on a dedicated worktree with a per-lane target directory, wall times measured.

• linter all - FAIL, exit 1, 43.1 s (nightly clippy, BB1).
• cargo clippy --workspace --all-targets --all-features -- -D warnings - FAIL, exit 101, 3.6 s, nightly rustc 1.100.0-nightly (a69a63265 2026-09-03).
• cargo +stable clippy --workspace --all-targets --all-features -- -D warnings - PASS, exit 0, 38.1 s, rustc 1.98.0 (88d9e12ae 2026-08-18).
• cargo test --package clippy-allow-reasons - PASS, 13 tests (8 lib + 1 bin + 4 CLI), exit 0, 0.3 s.
• cargo fmt --check - PASS, exit 0, 0.7 s.
• Negative tests, each injected into the worktree and reverted: undocumented top-level allow -> exit 1; indented allow in an impl -> exit 1; allow on a statement inside a function body -> exit 1; multi-line #![allow(...)] -> exit 1; empty and bare-"Temporary" reasons -> exit 1; documented and issue-referencing reasons -> exit 0; #[expect], #[cfg_attr(test, allow(...))] and a macro_rules! body -> exit 0 (BB3, S-a, S-b); the diff.*/color.diff matrix above.
• CI cross-checked against the workflow runs for this head, not only pr checks: Testing failure, Container failure (Build Tracker Image), OS Compatibility/Coverage/Docs Lint/Copilot Setup Steps success. The Container failure is separate from this PR's subject matter and I have not attributed it here.

[da2ce7]

Blocker (BB1). clippy::assert_is_empty is denied through [workspace.lints.clippy] pedantic = "deny" and fires on nightly here and at lines 58, 59, 70 and 87, plus src/lib.rs 175, 182, 198, 206 and 213 - ten errors, so linter all and cargo clippy --workspace --all-targets --all-features -- -D warnings both fail on this head. That is the Unit (nightly) / Run All Linters failure in run 34354675664; I reproduced it independently (linter all exit 1 in 43.1 s, clippy exit 101 in 3.6 s on rustc 1.100.0-nightly (a69a63265 2026-09-03)). The same clippy passes on rustc 1.98.0, so it is nightly-only rather than a flake. The suggested rewrite is assert_eq!(output.stdout, [] as [u8; 0]). Please do not reach for #[allow(clippy::assert_is_empty)] instead - that is the form this PR's own gate rejects without a reason, and using it here would read badly.

[da2ce7]

Suggestion (S-c; round-1 N1-N3 restated in the new mechanism). contains("temporary") misses the ordinary spellings of the same intent - "temporarily", "TODO", "for now", "workaround" - so a reason that is plainly a temporary suppression skips the removal-condition requirement entirely. In the other direction, the prefixes at line 130 are matched with exactly one trailing space, so "remove when", "removed when" and "remove when:" fail a reason the author believes is compliant, and the message names only the exact spellings. The skill documents the exact forms too, so both disagree with what an author will actually type. A small normalisation (collapse whitespace, match remove(d)? (when|after|by) and until as a pattern) would close both sides.

[da2ce7]

Suggestion (S-d; round-1 N7/N8 unaddressed). This test creates the feature branch and then changes no file, so it asserts the empty-output behaviour over an empty diff, not over an accepted documented allow - it would still pass if validate_changed_allows were stubbed to unreachable!(). Writing a second revision of src/lib.rs that adds #[allow(clippy::too_many_lines, reason = "...")] would make the name true. While you are here, the three cases the syn rewrite exists to fix have no CLI-level coverage at all: an indented attribute on a method inside an impl, an attribute on a statement inside a function body, and a multi-line #![allow(...)] block. All three do work correctly on this head - I verified each - which is exactly why they are worth pinning down before the next refactor.

[da2ce7]

Nit (N-a). Both write results are discarded here, so a validator that cannot print its diagnostics still exits 1 with an empty stderr and the reader has nothing to act on. The exit code keeps the behaviour safe, so this is only worth one fallback line - e.g. keep the Result and, on error, write a fixed-string NDJSON record - rather than a redesign.

[da2ce7]

Suggestion (S-f; round-1 S6/S7 unaddressed). A check that needs only git and a small binary still sits inside the unit matrix job behind toolchain install, Node, sccache, cargo fetch, cargo install --locked torrust-linting, cargo fmt and linter all, so it runs twice (nightly and stable) and only after the slowest steps have already passed; and the fetch-depth: 0 added at line 46 puts a full-history clone on the heaviest job in the workflow. A standalone job modelled on layer-bans would fail fast, run once, and confine the deep clone to a cheap job. Separately, ${{ github.base_ref || 'develop' }} is interpolated straight into the run: string; binding it through env: and reading "$BASE_REF" is the standard hygiene here and survives odd branch names.

[da2ce7]

Suggestion (S-g and S-h). Two separate points on this line. (1) The hook hardcodes torrust/develop and the binary's own default is torrust/develop, while the CI step passes origin/<base> - the two callers disagree about the upstream remote's name and neither falls back. A torrust remote is assumed by release-new-version/SKILL.md but is nowhere established for contributors, and the repository carries no git remote add guidance; a contributor whose upstream is origin or upstream gets a runtime_error and exit 1 on every commit, which reads as the gate being broken. Resolving the base ref against a short candidate list, or naming the required remote in run-pre-commit-checks/SKILL.md, would fix it. (2) The validator diffs the merge base against the working tree (git diff --unified=0 <base>, no --cached), so this pre-commit step can fail on edits that are not being committed and never evaluates exactly the content that is.

[da2ce7]

Suggestion (S-j; round-1 S12 unaddressed). This header says entries are appended and never reordered, but the file below still reads 2026-09-08 17:03, 17:24, 17:12, 17:03 (Correction) before the 2026-09-09 block, so a reader still meets the 17:24 resolution ahead of the 17:12 blocker it resolves; the sibling 2160 file is strictly ascending. The 2026-09-09 entries then note, twice, that the issue progress log carries entries at 13:10, 13:40 and 14:05 - later than the reports appended after them - so the file now documents its own ordering problem rather than resolving it. Either restore ascending order in one explicit corrective commit, or amend the header to describe what the file actually guarantees.

[da2ce7]

Round-3 review at 2377460ca48bb404347314aac12a5ffc1c7e782c (merge base f6b73e2960c9b94c88d500c793ccfb87731ccd52, 12 commits). REQUEST_CHANGES.

git range-diff 7abc30b2..16fc1aba f6b73e29..2377460c shows commits 1-10 as = — the round-2 series rebased onto current develop with identical patches — plus two new commits, 135bea32 and 2377460c. Blob ids confirm it: src/lib.rs, src/main.rs, tests/cli.rs, Cargo.toml, testing.yaml, pre-commit.sh, clippy-fixer.agent.md and both SKILL.md files are byte-identical to the round-2 head. The round-2 review therefore still applies in full; its 11 inline threads are open and are not re-posted here. Status of each item, re-measured against this head rather than inferred from the patch:

item	status at this head	evidence
BB1 clippy::assert_is_empty	open	linter all FAIL (19.5 s), nightly clippy -D warnings FAIL exit 101 (3.1 s); 10 errors at tests/cli.rs 31/58/59/70/87 and src/lib.rs 175/182/198/206/213. Testing run 34362856339 fails the same way at Run All Linters; Unit (stable) cancelled.
BB2 diff-format sensitivity	open	diff.noprefix=true → exit 0; diff.mnemonicPrefix=true → exit 0; color.diff=always → exit 0, with a real undocumented allow present in each row.
BB3 #[expect] unchecked	open	#[expect(clippy::too_many_lines)] with no reason → exit 0, while fix-clippy-warnings/SKILL.md:56-57 still recommends #[expect(..., reason = "…")].
S-a cfg_attr, S-b macro bodies	open	both probes exit 0.
S-c … S-j, N-a	open	the files they anchor to are unchanged.

cargo test -p clippy-allow-reasons passes (13 tests, 0.4 s) and cargo-fmt --check passes; stable clippy passes because assert_is_empty does not exist in stable 1.98 — the failure is real on nightly, not server drift.

The two new commits, and a correction to my round-2 review. Round 2 saw Container red at 16fc1aba and declined to attribute it to this PR. That was wrong. The job log for Test (Docker) (release) (job 102476226035) fails at cargo chef prepare with cargo metadata … failed to read /build/src/contrib/dev-tools/checks/clippy-allow-reasons/Cargo.toml — the new workspace member without a matching Containerfile entry. 135bea32 is the right fix and the minimal one: it mirrors the workspace-coupling pattern exactly (dockerignore negation, manifest COPY, src/ stub dir, lib.rs + main.rs stubs — both needed, since the crate declares neither target explicitly). hadolint passes on it. Cost is negligible: no new external package enters Cargo.lock, syn/serde/serde_json at these features are already workspace-coupling dependencies, and the runtime image is untouched, so image size does not change. The one real cost is a single cache invalidation: proc-macro2 with span-locations is a new feature toggle on an external dependency, which by the Containerfile's own note at lines 165-172 invalidates the dependencies_thirdparty cook layer once.

2377460c records the repair in the issue progress log, in order and in the file's format. One inline comment on each new commit follows.

Nothing here suggests the approach should change; the syn rewrite remains the right mechanism and the draft status is appropriate. The three blockers are unchanged from round 2, and item 2 remains the one that needs real design attention, because a gate that silently exits 0 reports success while enforcing nothing.

[da2ce7]

Round-4 review of 4389614bad20da4fa8d010bedde40f1c397f8cbc — REQUEST_CHANGES.

Round-4 review at 4389614bad20da4fa8d010bedde40f1c397f8cbc (rebased onto develop 0e61ec8a; git merge-base confirms it). git range-diff f6b73e29..2377460c 0e61ec8a..4389614b shows all twelve earlier commits = and three new ones: 3c326bdb, c3e00ede, 4389614b. Every item below was re-measured against this head on a clean detached worktree, not inferred from thread state.

Two things are genuinely fixed, and both are good fixes.

c3e00ede closes C1 properly: all four RUN cargo nextest archive invocations (lines 243, 270, 285, 299 — grep confirms four RUN cargo nextest archive lines in the file and four excludes) now carry --exclude clippy-allow-reasons, and the cook-stage note at line 230 was updated too, so the Containerfile comment at line 75 now describes what the recipe actually does. 3c326bdb closes the parse-order point: run() now calls base_ref() before workspace_root(). Measured with git removed from PATH in a non-repository directory: --unexpected → exit 2 with {"kind":"usage_error",…}; with git present, also exit 2; valid arguments with git hidden still give exit 1 runtime_error, which is the right split. The new FixtureDirectory makes it_should_report_usage_errors_as_ndjson pass under a git-less PATH as well.

The three round-2 blockers are still open, and BB1 is only half fixed.

3c326bdb replaced assert!(x.is_empty()) with assert_eq!(x, b"") at the five sites in tests/cli.rs and left the five in src/lib.rs untouched (git rev-parse 2377460c:…/src/lib.rs = 4389614b:…/src/lib.rs, byte-identical). linter all at this head is still exit 1 (21.6 s; rustfmt and shellcheck pass, clippy fails), and cargo clippy --workspace --all-targets --all-features -- -D warnings on nightly is still exit 101, now with 5 errors instead of 10:

error: used `assert!` to check that a value is empty
   --> contrib/dev-tools/checks/clippy-allow-reasons/src/lib.rs:175:9
… also 182:9, 198:9, 206:9, 213:9
error: could not compile `clippy-allow-reasons` (lib test) due to 5 previous errors

I measured the remedy rather than predicting it. Applying clippy's own suggestion at all five sites as a throwaway patch (1 file changed, 5 insertions(+), 5 deletions(-)) makes linter all pass — exit 0, 26.7 s, All linters passed, every linter running to completion instead of aborting at clippy — with all 13 package tests still green and cargo-fmt --all --check clean. So BB1 is the only thing keeping the gate red at this head, and it closes in five one-line edits with no behavioural or formatting fallout. The patch was reverted and the worktree confirmed clean.

The reason the fix stopped halfway is written down in the PR itself: the new Validation Evidence Corrections section says the failure is clippy::assert_is_empty "in the new CLI tests". Half of it was never in the CLI tests. Clippy prints the fix for each remaining site (assert_eq!(…, [] as [Violation; 0])).

BB2 and BB3 were not touched by any of the three commits and were re-measured open at this head:

probe	injected into src/lib.rs	exit	expected
#[allow(clippy::too_many_lines)]	undocumented allow	1	1 — correct
#[allow(…, reason = "…")]	documented allow	0	0 — correct
#[expect(clippy::too_many_lines)]	BB3	0	1
#[cfg_attr(test, allow(clippy::too_many_lines))]	S-a	0	1
#[allow(…)] inside a macro_rules! body	S-b	0	(inherent)

local git config (BB2)	header the tool receives	exit	expected
(none)	+++ b/contrib/…/src/lib.rs	1	1
diff.noprefix=true	+++ contrib/…/src/lib.rs	0	1
diff.mnemonicPrefix=true	+++ w/contrib/…/src/lib.rs	0	1
color.diff=always	ANSI-escaped, no plain +++ match	0	1

Each row ran with a real undocumented #[allow(clippy::too_many_lines)] present; every injection was reverted and the worktree confirmed clean afterwards, with no residual diff.*/color.* configuration.

Status of every standing item at this head

id	label	status	evidence
BB1	Blocker	partially fixed — still red	5 of 10 assert_is_empty sites fixed (all in tests/cli.rs); src/lib.rs 175/182/198/206/213 unchanged. linter all exit 1, nightly clippy exit 101.
BB2	Blocker	not fixed	src/main.rs parse_changed_rust_lines still keys on the literal +++ b/; git diff still invoked with no --no-ext-diff, no --no-color, no -c core.… pinning. Matrix above.
BB3	Blocker	not fixed, both sides	#[expect] probe exits 0; fix-clippy-warnings/SKILL.md blob unchanged and still recommends #[expect(…, reason = "…")] at lines 56-57.
C1	Blocker (r3)	FIXED	c3e00ede: four excludes for four archive invocations; line 230 cook note updated.
parse order (r3 note)	note	FIXED	3c326bdb; measured exit 2 without Git.
C2	Suggestion (r3)	not fixed — see D1	The correction section states linter all now passes; it does not.
S-a	Suggestion	not fixed	cfg_attr probe exits 0.
S-b	Suggestion	not fixed	macro_rules! probe exits 0.
S-c	Suggestion	not fixed	is_temporary is still contains("temporary"); the four condition prefixes still require exactly one trailing space.
S-d	Suggestion	not fixed	it_should_not_write_output_when_validation_succeeds still creates feature and writes no file after it — still an empty diff.
S-e	Suggestion	not fixed	Fixture still sets only user.email/user.name (lines 145-146); no commit.gpgsign=false, no core.hooksPath=/dev/null.
S-f	Suggestion	not fixed	.github/workflows/testing.yaml blob unchanged.
S-g	Suggestion	not fixed	src/main.rs:109 still defaults to torrust/develop; hook line 53 unchanged.
S-h	Suggestion	not fixed	pre-commit.sh blob unchanged; still diffs the working tree, not the index.
S-i	Suggestion	not fixed	SKILL.md blob unchanged.
S-j	Suggestion	not fixed	agent-review-reports.md blob unchanged; entry order still 17:03, 17:24, 17:12, 17:03.
N-a	Nit	not fixed	src/main.rs:210-211 still drop(serde_json::to_writer(…)) / drop(stderr.write_all(b"\n")).

One new Blocker and one new Nit are raised inline, plus a fresh anchor for BB1's remaining half — its round-2 thread anchors on tests/cli.rs:31, which this force-push fixed, so that thread now reads as outdated even though the blocker is live.

A process note, offered as an aid rather than a complaint. Both threads resolved on this head were resolved with a reply stating that linter all passes, and the 15:20 UTC progress entry and the new Validation Evidence Corrections section say the same. It does not pass — linter all is exit 1 at this commit, and CI's Run All Linters step agrees. That claim has now been recorded four times across three heads. Running linter all once on the actual branch tip before writing the evidence line would catch all four; the half-fix above is what a run would have shown.

CI at this head agrees with every measurement above. Testing 34370597138 is failure: Unit (nightly) (job 102530522759) fails at step 11 Run All Linters with those same five src/lib.rs lines and nothing else, and steps 12-14 — including Check Documented Clippy Allows, the step that would exercise this PR's own validator — are skipped as a consequence, so the new check has still never run green on its own branch. Layer Boundary Bans passes; Docker E2E is skipped (issue #2179, not this PR). Lint Containerfile with hadolint in the Container run passes, so N-b is cosmetic as stated. Docs Lint, Generate Coverage Report (PR) and OS Compatibility pass. The Container run at this head (34370597232) is still building at the time of writing; Lint Containerfile with hadolint in it has already passed. I have not waited it out because Testing already settles the recommendation and C1's fix is verifiable structurally — worth a glance when it lands.

Gates on a clean detached worktree at this head, nightly rustc 1.100.0-nightly (a69a63265 2026-09-03) / stable rustc 1.98.0 (88d9e12ae 2026-08-18), warm target dirs: linter all FAIL exit 1 (21.6 s); nightly clippy … -D warnings FAIL exit 101 (2.3 s); stable clippy … -D warnings PASS (2.8 s — assert_is_empty does not exist on stable 1.98, which is why only the nightly leg is red); cargo test -p clippy-allow-reasons PASS, 13 tests (0.9 s); cargo-fmt --all --check PASS (0.7 s). No container build was run — the review host has no Docker; the Containerfile is assessed from its diff plus the Container workflow at this head.

The approach is still sound and the two fixes in this force-push are the right ones. Requesting changes for BB1-BB3 and the new D1.

[da2ce7]

Blocker (D1, new). This line states that linter all now passes. It does not, at this commit:

$ linter all          # clean detached worktree @ 4389614b
ERROR clippy: Clippy linting failed. Please fix the issues above. (2.094s)
INFO  rustfmt: Rust formatting check passed! (0.725s)
INFO  shellcheck: shellcheck passed (1.327s)
ERROR Some linters failed                                              # exit 1, 21.6 s

Unit (nightly) at this head fails at the same step, on the same five lines.

The section is otherwise exactly the right instrument — separating the correction from the historical entries rather than rewriting them is the correct call, and naming the entries it retracts is the correct scope. Three things keep it from doing its job:

1. The claim it adds is the class of claim it exists to correct. With the 15:20 UTC progress entry and the two review-thread replies on this head, linter all … passed has now been recorded four times against trees where it fails.
2. It mislocates the failure. "clippy::assert_is_empty in the new CLI tests" covers five of the ten sites; the other five are in src/lib.rs and are still there. That description is very likely why the fix stopped halfway — see the comment on src/lib.rs:175.
3. It corrects the 13:40 and 14:05 entries but not the 14:15 one (line 109), which also ends "focused uncached recipe build and linter all passed" and is equally untrue of the tree it was written against.

This is a Blocker rather than a Suggestion because this file is the issue's acceptance evidence: as written it would certify a red gate as green at closure time. The fix is mechanical once BB1 is finished — run linter all on the branch tip and write down what it printed.

[da2ce7]

Nit (N-b, new). This continuation line is indented with two spaces; every other --exclude line in all four archive invocations uses four, including the one this commit added at line 243. Same at lines 285 and 299. hadolint does not check continuation indentation (its step passes at this head) and the shell does not care, so this is purely cosmetic — but the four invocations are otherwise character-identical blocks, which is exactly what makes them cheap to keep in sync by eye, and three of them now break that.

The substance of C1 is fixed here and I confirmed it independently: grep -c '^RUN cargo nextest archive' gives 4, grep -n 'exclude clippy-allow-reasons' gives 243/270/285/299, and the cook-stage note at line 230 was updated too — so the manifest-stage comment at line 75 is now accurate. Choosing to keep a Git-dependent, dev-only crate out of the archive was the right repair rather than installing git into the tester image.