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 85.11%. Comparing base (618723d) to head (ad5b6be).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2177      +/-   ##
===========================================
+ Coverage    85.05%   85.11%   +0.06%     
===========================================
  Files          351      352       +1     
  Lines        30208    30567     +359     
  Branches     30208    30567     +359     
===========================================
+ Hits         25693    26018     +325     
- Misses        4148     4178      +30     
- Partials       367      371       +4     

☔ 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]

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]

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.

[josecelano]

Review 5146070207 completed. M1-M13 are now resolved: the approved native Rust replacement covers parsing, diff handling, fixtures, temporary rationale policy, CI placement, and report-history semantics; Bash-only points were explicitly closed as superseded. See commits 067f379b, bb1c7e45, aea3c003, and 8ff5139b.

[josecelano]

Review 5155445019 completed. M14-M24 are resolved: nightly assertion cleanup, hardened Git diff parsing, native allow/expect/cfg_attr support, normalized temporary reasons, fixture isolation, CLI shape coverage, output fallback, focused CI, portable staged validation, skill documentation, and append-order clarification are recorded in the audit.

[josecelano]

Review 5155953316 completed. Its carried-forward blockers were resolved by 65a8d1b4 (nightly Clippy), 067f379b (fail-closed pinned Git diff), and 415f16f6 (native expect/cfg_attr controls); container archive policy is completed in 57b6ac68.

[josecelano]

Review 5156076762 completed. The follow-up observations are covered by the resolved historical and current findings: the Bash implementation was deliberately removed, while the Rust replacement has focused tests, CI integration, portable base selection, and staged-index validation.

[josecelano]

Review 5156728063 completed. M25-M27 are resolved: 65a8d1b4 fixes all remaining nightly Clippy empty assertions; fresh branch-tip linter all evidence corrects the historical record; and 57b6ac68 aligns all nextest archive exclusion blocks.

Waiting too long for the re-review

[josecelano]

ACK ad5b6be