chore(hooks): close hawkeye pin/coverage gaps flagged on #474

[jussielo-amd]

Summary

• The local license-headers pre-commit hook (hawkeye check --config licenserc.toml) now runs on every commit and push (always_run: true) instead of gating on a types_or file-type filter. pass_filenames: false already has hawkeye scan the whole repo per licenserc.toml regardless of which files changed, so the filter only ever decided whether the hook fired — never fully able to track licenserc.toml's own include list.
• One table-driven test (pinned_tool_versions_match_across_docs_and_workflows in xtask/src/workflow_contract.rs) now guards that the hawkeye and cargo-about versions pinned across CI, CONTRIBUTING.md and MANIFEST.md never drift apart — including the two cargo-about cache keys in ci.yml/dependabot-manifests.yml, which pin the version outside the cargo-about@ install lines and would otherwise silently cache-hit a stale binary. It replaces tpn.rs's pre-existing pinned_version_matches_workflows, which only checked ci.yml/dependabot-manifests.yml's cargo-about install lines, plus this PR's own now-superseded hawkeye-only test. A dedicated fixture proves a pin reworded out of the marker's shape (e.g. onto --version instead of cargo-about@<version>) fails loudly instead of silently dropping out of the guard. Comment-stripping in the token extractor is scoped to YAML files only: Markdown prose can contain a GitHub issue reference (see #474, install hawkeye@7.0.1) that the YAML " #" comment delimiter would otherwise mistake for a trailing comment, silently dropping that line's pin from the check.
• CONTRIBUTING.md now documents the hook accurately (no file-type filter; every commit/push needs hawkeye installed at the pinned version, or SKIP=license-headers to opt out; hawkeye scans the whole working tree including untracked files, so an unrelated unheadered file can block an otherwise-unrelated commit; an off-pin version may reject licenserc.toml outright rather than just disagreeing with CI on content) and lists it among the checks prek runs locally.
• license_headers_hook_has_no_file_type_filter's hook_block helper now delimits a hook's YAML block by indent — the next line at or above the hook's own - id: indent, the same termination job_block/nested_block already use — instead of the next blank line. Hooks inside one repo: block sit back to back with no blank line between them, so the old blank-line boundary could fold a following sibling hook's always_run: true into this hook's block and false-pass even with the filter reintroduced. A comment-only line at or below that indent doesn't end the block by itself: it only does when it turns out to head the next sibling (nothing deeper follows it before the next such line), which matters because .pre-commit-config.yaml has exactly that shape between license-headers and the next (cargo-fmt) repo block today — and when several comment lines in a row head that sibling, the boundary is the first of them, not the last. The always_run: true check is an exact post-comment-strip line match rather than contains, so a commented-out mention can't satisfy it either, and the filter check now rejects every file-type filter key (types:/types_or:/files:/exclude:), not just types_or:. Every false-pass shape this guards against, and the fix for each, is pinned by fixture tests.
• ci.yml's rust paths-filter now lists CONTRIBUTING.md and .pre-commit-config.yaml, so the rust-flagged jobs (clippy/coverage/test) run at all on a pin- or hook-only edit, instead of surfacing a drift only on main's forced full run. That alone isn't enough to make the test itself run on a PR, though: cargo xtask affected treats Markdown as ignorable by default, which would resolve a CONTRIBUTING.md-only PR to Selection::Empty and skip the Test (nextest) step — only the separate coverage job (which runs --workspace unconditionally, with no affected-based narrowing) would still catch a drift, invisibly. CONTRIBUTING.md is therefore also added to forces_full_workspace in xtask/src/affected.rs, the same reason MANIFEST.md already is, so Test (nextest) runs too. .pre-commit-config.yaml needs no such listing: it isn't Markdown and owns no crate, so cargo xtask affected already falls back to a full run for it as unattributed.

Why

Follow-up to gaps an automated review surfaced on #474:

• A licenserc.toml-only commit skipped the local license-header hook entirely (no types_or tag covers it), while CI's hawkeye check still enforced it — the only check for that case was CI.
• The hawkeye pin (CONTRIBUTING.md vs. ci.yml) had no guard at all. The cargo-about pin was already guarded across ci.yml/dependabot-manifests.yml's install lines by tpn.rs's pinned_version_matches_workflows, but CONTRIBUTING.md, MANIFEST.md, and both files' cache keys were not, so those could still drift silently.

Non-obvious decisions

• always_run: true replaces the file-type filter rather than extending it, because the filter structurally can't close the licenserc.toml gap — it only gates whether the hook runs, not what hawkeye scans once it does. The tradeoff: every local commit/push now needs hawkeye installed (SKIP=license-headers is the escape hatch), even for changes a narrower filter would have let through — this is intentional, and license_headers_hook_has_no_file_type_filter guards against a future edit quietly reintroducing a filter. always_run: true already makes any such filter inert on its own, so the filter check is defense in depth against the filter being mistaken for the real gate by a future edit, not a second thing that alone could reopen the gap.
• The unified pin test extracts the version token right after each tool's marker (hawkeye@, cargo-about@, or cargo-about- for the cache keys) and compares it for exact equality, rather than a contains check anchored on a trailing space. This closes a prefix false-pass (e.g. cargo-about@0.9.10 satisfying a check meant for 0.9.1) and a comment-only-match false-pass (a stale pin left in a trailing # comment after the real line changed) that the previous per-tool tests were each vulnerable to in different ways. Both are covered by fixture tests (pin_token_rejects_prefixes_and_comment_only_mentions, assert_pins_match_flags_a_stale_duplicate_pin). Comment-stripping itself only applies when extracting from a YAML file (by extension); Markdown callers (CONTRIBUTING.md, MANIFEST.md) skip it, since a #-prefixed GitHub issue reference in prose is not a comment and must not be treated as the end of the real content.
• hawkeye's canonical version is read from ci.yml's license-headers job env: (scoped via this file's existing job_block/job_mapping extractors, so a nested step env or an unrelated job can't leak in); cargo-about's canonical version is the ABOUT_VERSION constant in xtask/src/tpn.rs (now pub(crate)), since that's what cargo xtask tpn itself builds against.
• When license-headers is the last hook in the file and a trailing comment follows it with nothing after, hook_block folds that comment into the returned block rather than cutting before it. This can't hide a real filter key (one would be at a deeper indent and is never treated as part of the comment), and the real file doesn't hit this case today since cargo-fmt's repo block follows — left as documented, untested behavior rather than guarded, since the two shapes that matter (an annotation mid-block, and a comment heading a real sibling) are each pinned by their own fixture.

Scope

This is contributor tooling (an xtask test guard plus a doc/pre-commit-config/CI-filter change) with no user-observable CLI behavior change, so no Gherkin scenario is added per AGENTS.md §3.

Test plan

• cargo test -p xtask passes (283 tests).
• cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings and cargo fmt --all --check are clean.
• Manual mutation checks (each reverted): bumping only ci.yml's HAWKEYE_VERSION, a stale duplicate hawkeye@ line, bumping only ABOUT_VERSION, a stale duplicate cargo-about@ line, a suffixed version (cargo-about@0.9.10), a pin reworded out of the marker's shape entirely, bumping MANIFEST.md's pin alone, bumping a cargo-about cache key alone, a stale pin left in a trailing comment after the real line changed, and reintroducing any of the four file-type filter keys on the license-headers hook — all fail the relevant test as expected.
• prek run license-headers with only .pre-commit-config.yaml + a no-op licenserc.toml edit staged confirms the hook now fires (previously it would have been skipped as not matching any types_or tag).
• Flipping the hook's pass_filenames: false to true fails license_headers_hook_has_no_file_type_filter, which now asserts that flag directly instead of only the doc comment claiming it.
• hook_block's indent-based boundary, reverted to the old blank-line-anchored version, fails hook_block_stops_at_next_hook_in_the_same_repo_block and hook_block_stops_at_the_next_repo_entry; its comment lookahead, reverted to always stopping on any dedented comment, fails hook_block_does_not_end_at_a_comment_only_line; reverted to never stopping on a comment, fails hook_block_stops_at_a_dedented_comment_heading_the_next_repo_block; reverted to keeping the last (rather than first) of several consecutive header-comment lines, fails hook_block_stops_at_the_first_of_several_header_comment_lines. All pass again once restored.

Note for whoever merges

This branch went through several reworks (a standalone hawkeye_pin.rs module, then a types_or widening, then always_run, then a unified pin test) before settling on its current shape. The squash-merge commit message should describe that end state, not any of the intermediate ones the individual commit messages describe.

[Copilot]

Copilot review overview

🟢 Approval recommended

The focused hook and regression-test changes correctly close the described coverage gaps.

Review effort: Balanced
Findings: None

What changed in this PR

Adds safeguards to keep local and CI hawkeye license-header checks aligned.

Changes:

• Adds a test enforcing consistent hawkeye version pins.
• Runs the local license-header hook for Markdown changes.

File	Description
xtask/​src/​main.rs	Registers the pin-test module.
xtask/​src/​hawkeye_pin.rs	Verifies documented and CI hawkeye versions match.
.pre-commit-config.yaml	Includes Markdown in license-header hook triggers.

---

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

[rominf]

🔴 Automated review · pr-review-watcher · 4b87dcf

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Summary

The PR does two things. It adds a test-only xtask module that fails if the hawkeye version in CONTRIBUTING.md and in ci.yml's HAWKEYE_VERSION stop matching. It also adds markdown to the local license-headers prek hook's types_or, so a commit that changes only markdown no longer skips a check CI already runs on **/*.md. Outcome: No blocking findings. The PR description was not available to this review, so the commit message served as the description. Reviewed: the whole change (prw-base...HEAD, 1 commit, 3 files: .pre-commit-config.yaml, xtask/src/hawkeye_pin.rs, xtask/src/main.rs), plus the code around it (licenserc.toml, ci.yml's license-headers job, CONTRIBUTING.md, tpn.rs, workflow_contract.rs). Verified: cargo test -p xtask hawkeye passes (1/1). cargo clippy -p xtask --all-targets -- -D warnings and cargo fmt --check are clean on a fresh target dir. The test fails when either pinned version is bumped on its own. The hook claim also holds: in prek's identify table markdown is the tag for .md, types_or still filters hooks that set pass_filenames: false, and all 30 tracked .md files outside the excluded skills/rocm-doctor/** already carry the header. CI behaviour is unchanged, because the hook is in local-tools and the license-headers job calls hawkeye directly. The diff passes the leak scan, and none of the PR content tries to inject instructions. Blocking: 0 · Non-blocking: 6.

CI on this head: these checks had not reported when this review ran — E2E tests (MI300X), E2E tests (MI350P), E2E tests (Strix Halo, Ubuntu), Sphinx docs build (-W), Skill checks (skillscope). No check had failed. This review does not claim CI passed.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• xtask/src/hawkeye_pin.rs:58 — ci_text.contains("HAWKEYE_VERSION: v7.0.0") has no anchor at the end, so the test still passes if ci.yml's value becomes v7.0.0-rc1 or v7.0.01. The CONTRIBUTING side is anchored by the --locked that follows the version. Fix: match up to the end of the line, or extract the value first.
• xtask/src/hawkeye_pin.rs:52-61 — the test searches the whole of ci.yml for the substring. It neither restricts the search to the license-headers job's env nor ignores comments. The sibling xtask/src/workflow_contract.rs:12-16 explicitly avoids whole-file substring matching for this reason ("can false-pass … found only in a comment") and already has job_block/job_scalar extractors. Consider moving the check into that module and using those helpers.
• xtask/src/hawkeye_pin.rs:24 — the test-local HAWKEYE_VERSION constant is a third copy of the version. Unlike tpn.rs's ABOUT_VERSION, nothing at runtime uses it, so every bump now takes three edits. Reading the version out of ci.yml and comparing it to CONTRIBUTING.md would remove the copy, and also fix the unanchored match above.
• CONTRIBUTING.md:54 — while you're here: the CONTRIBUTING pin cargo install cargo-about@0.9.1 has the same drift risk this PR closes for hawkeye. tpn.rs:264 (pinned_version_matches_workflows) checks only ci.yml and dependabot-manifests.yml. So "mirrors the existing cargo-about pin guard" in the commit message slightly overstates the match, because that guard never covers CONTRIBUTING.md.
• .pre-commit-config.yaml:94 — nothing guards types_or against licenserc.toml's [files].includes, and the new module's doc says it does not check this. Actually exercising the hook would need prek and hawkeye in a scratch git repo, which is not practical in cargo test. A static parity check (each include glob maps to a tag listed in types_or) is cheap, though, and would keep this gap from coming back.
• .pre-commit-config.yaml:94 — a related coverage gap the PR leaves open (it predates the PR): a commit that changes only licenserc.toml (for example, dropping an exclude) still skips the local hook, because no toml tag is listed. CI's license-headers job remains the only check for that case.

[jussielo-amd]

Addressing the non-blocking findings:

Fixed in 6764859 (the three hawkeye_pin.rs points, which share one root cause): the CI-side check now extracts HAWKEYE_VERSION from the license-headers job's env: mapping instead of a whole-file substring search, so a mention elsewhere in the file (a comment, another job) can't false-pass. The extracted value is compared by equality rather than contains, so a suffixed version (v7.0.0-rc1) no longer satisfies a check meant for v7.0.0. This also removes the test-local HAWKEYE_VERSION constant entirely — the version now has exactly one source (ci.yml), and CONTRIBUTING.md's expected string is derived from it, so a bump only ever needs the two files this test already guards. Verified against all three failure modes with manual negative controls (bumped-only, suffixed-only, bumped-with-a-stale-comment-elsewhere) before reverting them; cargo test -p xtask, cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings, and cargo fmt --all --check are clean.

Not changed, acknowledged as out of scope for this PR:

• The cargo-about@0.9.1 pin in CONTRIBUTING.md has the same drift risk — tpn.rs's pinned_version_matches_workflows only checks ci.yml and dependabot-manifests.yml, not CONTRIBUTING.md. Fair point on the commit message too: "mirrors the existing cargo-about pin guard" was about the shape of the guard (a version pinned in one place, checked against workflow files), not full file-coverage parity — the cargo-about guard doesn't cover CONTRIBUTING.md either. Worth a follow-up PR.
• A static parity check between .pre-commit-config.yaml's types_or and licenserc.toml's [files].includes — already called out as a known gap in hawkeye_pin.rs's module doc; a reasonable follow-up, but adding a new guard is beyond this PR's scope (closing the two specific gaps flagged on docs(contributing): note hawkeye install for license-header hook #474).
• licenserc.toml itself missing from types_or — confirmed pre-existing, not introduced here.

[rominf]

🔴 Automated review · pr-review-watcher · e4ae01b

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Summary

This round covers the new commit, e4ae01b. The hawkeye check now fails on any stale hawkeye@ line in CONTRIBUTING.md, and the cargo-about guard now also checks CONTRIBUTING.md's pin. A new test ties licenserc.toml's includes to the license-headers hook's types_or. Outcome: No blocking findings.

Reviewed: fbf2084c..e4ae01b1 (commit e4ae01b only): xtask/src/tpn.rs and xtask/src/workflow_contract.rs. Also read: the files these tests read or that carry the same pins (licenserc.toml, .pre-commit-config.yaml, CONTRIBUTING.md, the license-headers and cargo-about steps of ci.yml, dependabot-manifests.yml, MANIFEST.md). The earlier commits were reviewed in the previous rounds and were not re-read.

Verified: cargo test -p xtask passes (253 tests). cargo clippy -p xtask --all-targets -- -D warnings and cargo fmt --check are clean. Mutations, each reverted:

• Dropping markdown from types_or fails the new test.
• A stale hawkeye@6.0.0 line fails the hawkeye test.
• A stale duplicate cargo-about line passes, and so does cargo-about@0.9.10 (first item below).

Blocking: 0 · Non-blocking: 7.

Note on classification: the review agent rated the first two items below as blocking. They are listed as non-blocking here, for these reasons:

• The cargo-about gap is the same stale-duplicate weakness the previous round rated non-blocking for hawkeye, and the guard still catches a pin bumped on its own.
• With the dead panic branch, the test still fails closed. Only its message is wrong.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• xtask/src/tpn.rs:288 — the new CONTRIBUTING.md check is a plain text.contains("cargo-about@0.9.1"). It stays green with a stale duplicate cargo-about@0.8.0 line, and with cargo-about@0.9.10 because of the prefix match (both confirmed by mutation). This commit fixed exactly that for hawkeye, so the twin should match. Check every cargo-about@ line, and match the whole version token (for example cargo-about@{ABOUT_VERSION} ). That would also close the prefix false-pass in the two workflow checks.
• xtask/src/workflow_contract.rs:2175 — glob.rsplit('.').next() always yields an item, so the has no extension panic can never fire. An include like **/Dockerfile instead fails with the misleading (.**/Dockerfile); add its prek tag message. rsplit_once('.') on the last path segment would make the branch real.
• MANIFEST.md:747 — a fourth cargo install cargo-about@0.9.1 pin is still unguarded, with the same drift risk as CONTRIBUTING.md had.
• xtask/src/tpn.rs:290 — the assert message names cargo install {expected} --locked --features cli, but the check matches the bare cargo-about@X substring anywhere in the file.
• xtask/src/tpn.rs:268 — the name pinned_version_matches_workflows and the ABOUT_VERSION doc ("Kept in sync with the workflows") no longer describe the scope, which now includes CONTRIBUTING.md.
• xtask/src/workflow_contract.rs:2130 — the doc says includes and types_or "must cover the same file types", but the test checks one direction (each include has a tag). That direction is the right one, so the wording should say so.
• xtask/src/workflow_contract.rs:2133 — "the markdown tag above" points at nothing in this file (that tag is in .pre-commit-config.yaml). The includes parser splits on , and the first ], so a comment containing either inside the array would mis-parse without an error.

Carried from the previous round, not re-checked in this one: .github/workflows/ci.yml:106-118 (a CONTRIBUTING.md-only PR does not run these guards at PR time), and the squash-merge message, which should describe the end state.

CI on this head: no check had failed when this review ran. The E2E GPU lanes (MI300X, Strix Halo Ubuntu/WSL2/Windows) were still queued or running. Sphinx docs build and Skill checks were skipped as not applicable. This review does not claim CI passed.

On the reply above

Thanks. This round read e4ae01b and confirms all four fixes you listed:

• every hawkeye@ line is now checked
• the doc comment names HAWKEYE_SHA256
• CONTRIBUTING.md's cargo-about pin is now checked
• a new test ties types_or to licenserc.toml

The cargo-about check is weaker than its hawkeye twin (first item above). The new test has a dead panic branch (second item). Neither gates the merge. The two points you left as they are (the affected.rs CI-filtering gap and the squash message) are fine as follow-ups.

[jussielo-amd]

Addressed the blocking finding in fbf2084: moved the guard out of the standalone hawkeye_pin.rs module and into workflow_contract.rs, reusing its existing job_block/job_mapping extractors (already covered by job_mapping_extractor_ignores_nested_step_env, job_mapping_extractor_ignores_blank_and_full_line_comments, and job_mapping_extractor_rejects_malformed_non_comment_rows) instead of the untested, duplicated job_env_value/job_block/strip_comment copies. The module is deleted; main.rs no longer registers it.

This also resolves three of the non-blocking points as a side effect of the deletion:

• the dangling "see module doc above" comment pointer
• the "equality" vs. contains comment inaccuracy
• the verbatim-copied job_block/strip_comment duplication

And fixes one more directly: strip_prefix('v') now panics with the raw value instead of silently falling back when HAWKEYE_VERSION isn't v-prefixed.

Manually re-ran your mutation set against the new test: bumping only ci.yml still fails loudly; a stray comment mentioning a different version elsewhere in the file, and a nested step-level env: with a different HAWKEYE_VERSION inside the same job, both correctly have no effect (scoping holds). Quoting the value ("v7.0.0") also now passes correctly — job_mapping strips the quotes before comparison, so it isn't a hole here. Reverted all mutations before pushing.

Left as-is, per your note that it's low risk and pre-existing: the CONTRIBUTING.md whole-file substring check, and the cargo-about/types_or/licenserc.toml follow-ups from the previous round.

cargo test -p xtask, cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings, and cargo fmt --all --check are all clean.

[rominf]

@jussielo-amd I've updated the automated review above for fbf2084 (report). The previous round's blocking finding is resolved, and nothing in this round blocks the merge. The failed E2E tests (MI300X) check comes from the CI runner, not this PR: the same failure shows up on unrelated PRs on the same machine. The report has the details.

[jussielo-amd]

Addressed all remaining non-blocking points from the latest round (#5990125500, against fbf2084):

Fixed in e4ae01b:

• workflow_contract.rs:2093 — hawkeye_pin_matches_ci_workflow now asserts every hawkeye@ line in CONTRIBUTING.md matches the pinned version, not just that the correct one is present somewhere. Verified with a mutation (stale duplicate line) that this now fails loudly.
• workflow_contract.rs:2073-2074 — doc comment now names HAWKEYE_SHA256 (ci.yml:1255) as the third edit a bump requires, which this test does not guard.
• tpn.rs:264-279 — pinned_version_matches_workflows now also checks CONTRIBUTING.md's cargo-about@0.9.1 pin, closing the gap where it only checked ci.yml/dependabot-manifests.yml. Verified with a mutation (CONTRIBUTING.md bumped alone) that this now fails.
• .pre-commit-config.yaml:94 — new license_header_hook_covers_licenserc_includes test ties the license-headers hook's types_or to licenserc.toml's [files].includes, so a future include with no matching tag (the exact gap this PR closed for markdown) fails the test instead of silently skipping locally. Verified with a mutation (dropped the markdown tag) that this now fails.

cargo test -p xtask (253 passed), cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings, and cargo fmt --all --check are all clean.

Left as-is, per your call:

• ci.yml:106-118 (CONTRIBUTING.md-only changes don't trigger the new guard at PR time) — this is an affected.rs CI-filtering gap shared by other existing doc guards; not something to change in this PR.
• Commit history — noting for whoever merges: the squash-merge message should describe the current end state (guard lives in workflow_contract.rs), not the first commit's now-superseded description of the deleted hawkeye_pin.rs module.

[rominf]

@jussielo-amd I have updated the automated review above for e4ae01b (report). Nothing in this round blocks the merge. The fixes from your reply are confirmed. One point: the new cargo-about check in tpn.rs still passes with a stale duplicate pin, the same gap you closed for hawkeye. The report has the details, plus a few minor follow-ups.

[jussielo-amd]

Addressed the remaining non-blocking points from the latest round (#5990125500, against e4ae01b) in db0b696:

• tpn.rs:288 — pinned_version_matches_workflows (renamed pinned_version_matches_workflows_and_docs) now checks every cargo-about@ line in ci.yml, dependabot-manifests.yml, and CONTRIBUTING.md, anchored on the whole version token instead of a bare substring. A stale duplicate pin or a suffixed version (0.9.10) no longer false-passes — same fix pattern as the hawkeye guard. Verified with reverted mutations (stale 0.8.0 duplicate, 0.9.10 suffix): both now fail.
• tpn.rs:290 — the assert messages now describe the actual check instead of the stale --locked --features cli phrase.
• workflow_contract.rs:2175 — the extension lookup now splits on the last path segment before rsplit_once('.'), so an extension-less glob (**/Dockerfile) hits the intended "has no extension" panic instead of silently falling through to the misleading (.**/Dockerfile); add its prek tag message. Verified with a reverted mutation (added **/Dockerfile to licenserc.toml's includes): now fails with the correct message.
• tpn.rs:268 / workflow_contract.rs:2130,2133 — doc comments now describe actual scope (workflows + CONTRIBUTING.md) and direction (includes → types_or only, not the reverse), and point at .pre-commit-config.yaml for the markdown tag instead of "above".

cargo test -p xtask (253 passed), cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings, and cargo fmt --all --check are all clean.

Left as-is: the MANIFEST.md 4th unguarded cargo-about@0.9.1 pin — new guard location, out of scope for this PR, same as the earlier CONTRIBUTING.md/licenserc.toml follow-ups.

[rominf]

Stepping back from the line-level rounds: the hook fix is one line, but the PR has grown to +186, nearly all of it tests guarding config drift. I think most of that can be removed or folded together. None of this blocks the merge; it's a design suggestion.

1. Drop the types_or filter instead of guarding it. The hook has pass_filenames: false, so hawkeye always scans the whole repo per licenserc.toml. types_or only decides whether the hook fires, not what it checks. With always_run: true instead, license_header_hook_covers_licenserc_includes, its hand-maintained LICENSE_HEADER_EXTENSION_TAGS table, and the hand-rolled TOML/YAML parsing all become unnecessary. It also closes the remaining gap where a commit touching only licenserc.toml skips the hook. I checked locally (prek 0.4.5): with always_run: true and pass_filenames: false, the hook runs on a commit whose only file matches no types_or tag. The cost is one hawkeye run per commit. If you'd rather keep a filter, a files: regex in .pre-commit-config.yaml is still simpler than a Rust test that keeps two config files in sync.

2. One table-driven pin test instead of three copies. tpn.rs (workflow loop + CONTRIBUTING block) and hawkeye_pin_matches_ci_workflow repeat one pattern: collect every <tool>@ line and assert each contains <tool>@{ver} . A single test over a (tool, canonical-version source) table and a shared list of files to scan (CONTRIBUTING.md, MANIFEST.md, .github/workflows/*.yml) would cover both tools. It would extract the token after @ and compare it for equality, which removes the trailing-space anchor and the prefix-match class of bug. The next pinned tool then costs one row, not another ~30-line test.

3. That would also cover MANIFEST.md:747, which still has an unguarded cargo install cargo-about@0.9.1 (raised in round 2, not addressed). With a shared file list it can't be missed.

On proportion: a drifted CONTRIBUTING.md pin means a contributor installs the wrong tool locally, and CI stays the gate. So I'd keep these guards as small as possible.

[jussielo-amd]

@rominf Took all three suggestions in 1537518 — this was the right call, thanks for stepping back.

1. types_or → always_run: true. Confirmed locally with prek run license-headers (staged .pre-commit-config.yaml + a no-op licenserc.toml edit): it now attempts to run the hook on a licenserc.toml-only change (it fails only because hawkeye isn't installed in my sandbox — previously it would have been skipped outright as not matching any types_or tag). Deleted license_header_hook_covers_licenserc_includes and LICENSE_HEADER_EXTENSION_TAGS along with it.
2. Table-driven pin test. One pinned_tool_versions_match_across_docs_and_workflows in workflow_contract.rs now covers both tools, built on a (tool, expected version, files) table and a shared pin_token/assert_pins_match pair that extracts the token after @ and compares it for exact equality — removes the trailing-space anchor and the prefix-match class of bug (verified: cargo-about@0.9.10 now correctly fails a check for 0.9.1, where the old contains check would have passed it). hawkeye's canonical version is still read from ci.yml's license-headers job env via the existing job-scoped extractors (reusing, not duplicating). cargo-about's canonical version is ABOUT_VERSION in tpn.rs, now pub(crate) so the unified test can read it; deleted the old hawkeye_pin_matches_ci_workflow and pinned_version_matches_workflows_and_docs.
3. MANIFEST.md:747 is now in the cargo-about row's file list — verified with a reverted mutation (bumped that pin alone, confirmed it fails).

Net: 251 tests (down from 253 — two old tests removed, one unified test added), cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings and cargo fmt --all --check clean. PR description rewritten to describe the current end state rather than the original hawkeye_pin.rs/types_or-widening approach, which no longer exists.

[jussielo-amd]

review-coverage · 1537518

High-effort review (primary pass + independent fork reviewer, same five scrutiny angles: parsing edge cases, coverage-scoping regression, visibility-change side effects, pre-commit hook/stage interaction, docs-vs-behavior drift) across all 3 changed files. Verified by actually running cargo test -p xtask (251 passing, including the new consolidated test) rather than static inspection alone. Both passes converged on zero confirmed or plausible findings.

[rominf]

🔴 Automated review · pr-review-watcher · 1537518

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
This is a follow-up to the gaps raised on #474 (that one is a PR, not a ticket), and it closes the two it names: the local hook coverage gap and the pin drift between files. No ticket is named.

Round history: every finding below is new this round. The change was reworked since the last automated round (at e4ae01b), so this round re-read the whole change rather than only the new commits. The MANIFEST.md cargo-about pin flagged last round is now covered by the unified pin test.

Blocking

• (new) CONTRIBUTING.md is now wrong about when the hawkeye hook runs — CONTRIBUTING.md:57, .pre-commit-config.yaml:94
CONTRIBUTING.md:57 still says the license-headers hook "runs on both commit and push for matching files". The change replaces types_or with always_run: true, so there is no longer any file filter. The hook now runs on every commit and every push, whatever is staged.
  The same paragraph warns that the hook "fails hard rather than skipping when the binary is missing". That warning now applies to every commit, including commits that only touch YAML, TOML or .ps1 files. Before this change, those commits passed without hawkeye.
  AGENTS.md §5 requires docs to be updated in the same change when observable behaviour changes. It also says to grep for every repeat of the claim. Here the code governs, because always_run is what the hook actually does, so the doc is the side to fix.
  Confidence 85 · mechanical · Fix: drop "for matching files", and say that every commit and push needs hawkeye installed. Optionally mention SKIP=license-headers as the escape hatch.

Non-blocking

• (new) The new pin test does not check the cargo-about cache keys — .github/workflows/ci.yml:1059, .github/workflows/dependabot-manifests.yml:73
  Both files hard-code the cache key ${{ runner.os }}-cargo-about-0.9.1. That string contains no @, so pin_token never sees it. I confirmed this by changing only the ci.yml key to 0.9.0: the test still passes.
  Suppose a version bump forgets the key. CI then gets a cache hit, skips the install step and keeps the old binary, and tpn --check stops with "must be version X". So the failure is loud, but it happens in a confusing place.
  The test's doc comment says the versions "must never drift", which claims more than the test checks.
  Confidence 100 (gap confirmed by a run) · logic · Fix: build each key from the version, or add a cargo-about- marker row to the table.
• (new) assert_pins_match also matches pins inside YAML comments — xtask/src/workflow_contract.rs:2077-2098
  The module doc (workflow_contract.rs:12-16) rejects matching against the whole file because it "can false-pass … found only in a comment". The new helper scans every raw line and never calls strip_comment.
  I tested this by rewriting ci.yml's install line to cargo install cargo-about --version 0.9.0, with the old cargo-about@0.9.1 kept in a trailing comment. The test stayed green.
  This failure needs a contrived edit, but the helper breaks a rule that this file states for itself.
  Confidence 85 · logic · Fix: strip # comments before matching in the YAML files.
• (new) The new helpers have no extractor-guard fixture tests — xtask/src/workflow_contract.rs:2070-2098
  The file's own guidance at :2160-2161 says extractors get fixture tests "so the contract tests above can't silently false-pass on a shape they don't handle". The doc comment for pin_token and assert_pins_match claims two guarantees: no prefix false-pass (0.9.10 against 0.9.1) and detection of a stale duplicate pin. Today only manual mutation checks back those claims; no test does.
  Confidence 70 · mechanical · Fix: add a small fixture test for both cases.
• (new) The prek check list in CONTRIBUTING.md omits license headers — CONTRIBUTING.md:49
  This gap existed before this change. It is the paragraph next to the stale line above, so both are cheap to fix together.
  Confidence 85 · mechanical · Fix: add hawkeye/license headers to the list.
• (new) The PR description overstates what existed on the base branch — PR body, "Summary" and "Why"
  • It says the change replaces "two independent, differently-shaped tests". The base branch has only one: tpn.rs::pinned_version_matches_workflows. The hawkeye test was added and then folded away within this same PR.
  • It says the cargo-about pin "had no guard tying them together". In fact ci.yml and dependabot-manifests.yml were already guarded. Only CONTRIBUTING.md and MANIFEST.md were not.
  • These misstate history, not the change's scope.
    Confidence 85 · mechanical · Fix: reword both sentences against the base branch.

Decisions for the author

• Tradeoff: always_run against a widened types_or. For always_run: CI's prek job runs --no-group local-tools (ci.yml:874-875), so this changes nothing in CI. It also leaves no second include list to drift away from licenserc.toml. Against it: every local commit and push now runs hawkeye on the whole working tree. So a contributor without hawkeye cannot commit at all. An untracked scratch .md/.rs file without a header also blocks unrelated commits, as the recent .supergoal/** exclude on the base branch shows. Using types_or: [rust, python, shell, markdown] keeps the old cost profile, but it covers neither commits that only touch licenserc.toml nor future include changes. Confirm the stricter local cost is intended.
• No test pins always_run (non-blocking-improvement). The earlier license_header_hook_covers_licenserc_includes guard was removed along with types_or. Nothing now stops someone from bringing back a file filter that silently drops markdown. A one-line assertion in workflow_contract.rs would close that.

Positive signals

• The test compares an exact token instead of using contains, and it checks every pin line, not just the first. My mutation runs confirm both: a stale duplicate, a 0.9.10 prefix and a single-file bump each fail with a clear message.
• It reads HAWKEYE_VERSION through the existing fixture-tested job_block/job_mapping extractors on the CRLF-normalised read_workflow, scoped to the one job, instead of copying the version into a constant.

Deployment notes

• The branch is 28 commits behind the base branch. git merge-tree reports no conflicts, but this head has not been run against the per-crate coverage floors added in #482. The test only moves within xtask, so the effect should be neutral, but the new gate will run only after a rebase.

What this covered

I read the whole change (base branch ...1537518651b439a14486b21d55944402819e70fb, 3 files) completely. I also read the ci.yml license-headers and third-party-notices jobs, dependabot-manifests.yml, all of tpn.rs, the workflow_contract.rs extractors and their fixture tests, licenserc.toml, CONTRIBUTING.md and MANIFEST.md.

• Runs: cargo test -p xtask passes on the head. I also ran 8 file mutations against the new test: 6 failed as they should, and 2 exposed the gaps reported above.
• CI: every check on the head commit succeeded. Sphinx docs and skillscope were skipped.
• Passes: the code pass, the AGENTS.md pass, history (git log/blame), prior changes (comments on #474, #300, #277, #72, #51 and #55) and code comments all ran as independent workers. The design questions ran as one more worker.
• Not run:
  • The full workspace test and clippy gates.
  • prek's live behaviour with always_run. The cost to contributors described above is reasoned from the config, not exercised.
  • An agent-instruction quality pass, because the change touches no agent-instruction files.
• Discussion: I did not reconcile this report with any PR discussion.

On the replies above

Thanks. Your three updates were read. This round re-read the whole reworked change at 1537518. The unified pin test now covers MANIFEST.md, and the exact-token comparison closes the prefix false-pass; mutation runs confirm both. The one blocking item is a single stale sentence in CONTRIBUTING.md that the always_run switch made untrue.

[jussielo-amd]

Addressing the 2026-10-08T07:48:18Z review (353d33c2 → now e... after this push):

Blocking

• assert_pins_match's "no pin found" branch was untested — fixed. Added assert_pins_match_flags_a_marker_with_no_pin_at_all, a should_panic(expected = "has no \cargo-about@` pin") fixture over text with the marker reworded out of shape (--versioninstead ofcargo-about@). Confirmed it fails if the assert!(found, ...)` it pins is deleted.

Non-blocking

• types_or rationale / test name mismatch — fixed. The doc comment and assertion no longer claim a types_or filter "can reopen the gap" (it can't, by itself, while always_run: true holds); they now say always_run is the real guard and the filter-key check is defense in depth against a future edit mistaking the filter for the gate. The check itself is widened to types:/types_or:/files:/exclude:, matching what the test name already claimed.
• "the gap closed above" — fixed. Rewritten to state the gap inline rather than referring to it positionally.
• Test placement vs. the "Extractor guards" comment — fixed. The three hook_block* fixture tests now sit below that comment, alongside pin_token/assert_pins_match's fixtures; its wording is widened to cover both multiline hook blocks and single-line pin tokens.
• hook_block ending the block at a comment-only line — fixed. It now only treats a dedented comment as a true boundary when nothing deeper follows it before the next such line; otherwise it's an in-block annotation. Added hook_block_does_not_end_at_a_comment_only_line for the annotation case. A follow-up self-review of this fix found the lookahead could still over-capture a comment that heads the next repo block (the real shape between license-headers and cargo-fmt in .pre-commit-config.yaml today) — also fixed, with hook_block_stops_at_a_dedented_comment_heading_the_next_repo_block pinning that case against the live file.
• PR description describing the old two-needle mechanism — fixed, description rewritten to describe the indent-based termination (and the comment-lookahead refinement) actually in the code.

Decisions for the author

• CONTRIBUTING.md now says explicitly that hawkeye scans untracked (non-gitignored) files too, and the skip example now shows git push alongside git commit. The always_run: true tradeoff itself (every commit/push needs the binary) is unchanged from this PR's original design — no code reason to revisit it.

All four hook_block edge cases (next-hook-same-block, next-repo-entry, comment-as-annotation, comment-heading-next-block) and the new pin-marker-not-found case are each proven by reverting the relevant code and confirming the matching fixture fails, then passes once restored. cargo test -p xtask is 281/281, cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings and cargo fmt --all --check are clean.

Feedback addressed.

[rominf]

🔴 Automated review · pr-review-watcher · 7f780e6

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
No ticket named. The description follows up on #474 and matches the diff: always_run on the hook, one table-driven pin test replacing tpn.rs's, the CONTRIBUTING.md wording, and indent-based hook_block.

Round history: every finding below is new this round. The blocking item from the last round (at 353d33c), "the no-pin-found branch of assert_pins_match is untested", is resolved. This round's blind review did not re-find it. After the report was frozen, the objection was put to the reviewer together with the author's account of the fix, and it answered that the objection no longer holds. That answer is anchored evidence, because the reviewer was told what to look for. Separately, deleting the assert!(found, …) was re-run on this head: assert_pins_match_flags_a_marker_with_no_pin_at_all goes red, and it passes again once the line is restored. The earlier blockers stay resolved: CONTRIBUTING.md:57 (from 1537518), and the always_run false-pass through hook_block (from 3c99e9b).

Gate: this round files no change request, so the earlier change requests on this PR are withdrawn. The one item under Blocking is the review method's classification, and it is unchanged here. But it is an untested line in a test-only helper, and regressing that line can only make the guard fail loudly, never pass falsely. always_run is matched after comments are stripped, and extra comment lines folded into the block can only make the filter-key check stricter. Merging leaves no gap unguarded, so it does not warrant gating the merge. A fixture for it is still recommended.

Blocking

• (new) One branch of the new hook_block comment logic is not tested — xtask/src/workflow_contract.rs (hook_block, the pending_comment.get_or_insert(offset) line)
  get_or_insert keeps the first line of a run of dedented comments as the block's end. Changing it to pending_comment = Some(offset) (keep the last line instead) leaves all 51 workflow_contract tests green; this was run. No fixture has two or more consecutive dedented comment lines heading the next sibling. The four hook_block_* fixtures each use zero or one such line. Under the mutation, every header-comment line except the last is folded into the license-headers block. A filter-key mention in one of those lines would then fail the real-file test for no reason. Today the real file has a one-line header, so nothing breaks yet. The method requires that every branch inside the change can fail a test, so this is blocking, even though the fix is small.
  Confidence 100 · logic · Fix: extend hook_block_stops_at_a_dedented_comment_heading_the_next_repo_block (or add a sibling test) so two or more comment lines head the next - repo:. Assert that none of them appear in the block.

Non-blocking

• (new) The new guards don't run on a PR that edits only the files they guard — xtask/src/workflow_contract.rs (pinned_tool_versions_match_across_docs_and_workflows, license_headers_hook_has_no_file_type_filter); gating at .github/workflows/ci.yml:106-150, xtask/src/affected.rs is_ignorable
  The xtask tests run only when the rust or heavy paths-filter matches. CONTRIBUTING.md and .pre-commit-config.yaml match neither filter, and is_ignorable drops every *.md file. So two kinds of PR pass CI here and turn main red on its forced full run: one that bumps only the CONTRIBUTING.md hawkeye@ pin, and one that drops always_run or re-adds types_or in .pre-commit-config.yaml. The removed tpn.rs test only read .github/workflows/*, which is in both filters, so it always ran on the PR that broke it. Two older doc-reading tests already have the same gap: they read docs/ci-hardware-testing.md and tests/e2e-cucumber/README.md. This change adds two more files to that pattern.
  Confidence 85 · architectural · Fix: add CONTRIBUTING.md and .pre-commit-config.yaml to the rust filter, or state the gap.
• (new) CONTRIBUTING.md's description of an off-pin hawkeye is incomplete, and always_run makes the gap matter more — CONTRIBUTING.md:57
  The doc says "a mismatched version runs whatever that version's own license-header rules happen to be, which may disagree with CI". I ran the installed hawkeye 6.5.1 against this licenserc.toml. It runs no rules at all: it panics with unknown field `files` at the [files] table. With always_run: true, a contributor on 6.x is now blocked on every commit and push, not only on rust/python/shell commits. The text doesn't prepare them for that.
  Confidence 85 · mechanical · Fix: say that an off-pin version may reject licenserc.toml outright, and point at the cargo install hawkeye@7.0.0 line or SKIP=license-headers.
• (new) pin_token strips YAML comments from Markdown lines too — xtask/src/workflow_contract.rs (pin_token calls strip_comment, which splits on " #")
  In CONTRIBUTING.md or MANIFEST.md prose, a line like see #474, install hawkeye@7.0.1 is cut at #474, so its pin is never checked. If another line in the same file still carries the right pin, found stays true and the stale pin passes. That is the stale-duplicate case assert_pins_match's doc says it prevents. No current line has this shape, so the risk is latent.
  Confidence 70 · logic · Fix: strip comments only for the YAML files, or for Markdown only inside fenced shell blocks.

Decisions for the author

• always_run makes the local hook stricter than CI — tradeoff
  For: it closes the licenserc.toml-only gap structurally, which no file-type filter could. Against: every local commit and push now needs a working, correctly versioned hawkeye. It also scans untracked, non-ignored files that a clean CI checkout never has. The description records this as intentional, with SKIP=license-headers as the escape hatch.
• The commit messages describe intermediate designs that the final diff dropped — answered-by-intent
  Examples: 310078f widens types_or, and a14a929/d646611 describe tests that no longer exist. Left as is, a squash message built from the commit list would be misleading. The description says: "The squash-merge commit message should describe that end state, not any of the intermediate ones the individual commit messages describe." Please confirm that whoever merges writes that message.

Positive signals

• Pins are compared by exact token equality, and fixtures cover the 0.9.10-vs-0.9.1 prefix case and the missing-pin case. Removing the trailing-punctuation trim, the found assertion, or the comment stripping turns a test red; each was run.
• hawkeye's canonical version is read from the license-headers job's own env: through the existing, fixture-tested job_block/job_mapping, not copied into a new constant.
• Reverting .pre-commit-config.yaml to types_or turns license_headers_hook_has_no_file_type_filter red. The test fails without the production change.

Deployment notes

• Contributors with prek installed need hawkeye 7.0.0 on PATH for every commit and push from now on (or SKIP=license-headers). An older 6.x binary fails every commit on a config parse error.

What this covered

I read all four changed files in full in the changed regions, plus:

• the helpers they rely on (strip_comment, indent_of, job_block, job_mapping, read_workflow, repo_root);
• the real inputs the tests read: the ci.yml license-headers job and cargo-about steps, dependabot-manifests.yml, CONTRIBUTING.md, MANIFEST.md, .pre-commit-config.yaml and licenserc.toml;
• the CI paths filters and xtask/src/affected.rs.

The diff is prw-base...HEAD, from merge-base f9a8011 to 7f780e6. It merges cleanly onto prw-base 2499351.

What was run:

• cargo test -p xtask workflow_contract and the tpn tests at HEAD: all pass.
• Mutations of each new helper branch, one at a time.
• prek always_run, pre-push and SKIP behaviour in a scratch repo.
• hawkeye 6.5.1 against licenserc.toml.

The two verdicts for each added test:

• license_headers_hook_has_no_file_type_filter: fails when the production change is reverted. The get_or_insert mutation stays green.
• pinned_tool_versions_match_across_docs_and_workflows: passes when the production change is reverted. That is expected for a drift guard, since the base already had matching pins. Pin-shape mutations stay green on the real files and are caught by the fixture tests.
• The nine fixture tests don't depend on the production change. Together they catch every helper mutation except get_or_insert → overwrite.

CI for 7f780e6: every check passed. Skill checks were skipped, and that is not relevant to this change.

Did not run: the prior-changes pass, because I didn't read earlier PRs' review discussion, to stay inside the review boundary. hawkeye v7.0.0's untracked-file behaviour was not run; only v6.5.1 was available, so v7 is inferred. Not reconciled against any discussion here.

Every objection in these change requests has been checked against 7f780e6 and no longer holds: CONTRIBUTING.md now describes always_run correctly, hook_block ends blocks by indent and ignores comment-only mentions of always_run, and the no-pin-found branch is covered by a fixture that fails when the assert is deleted (re-run). See review 5454019185.

[jussielo-amd]

Addressed the round for 7f780e6 (#5454019185) in 2138291:

Blocking (per the review's own classification, though it said this didn't gate the merge):

• hook_block's get_or_insert branch — fixed. Added hook_block_stops_at_the_first_of_several_header_comment_lines, a fixture with two consecutive dedented header-comment lines (the prior fixtures only ever had one, so get_or_insert and "keep the last offset" were indistinguishable). Verified the regression: reverted get_or_insert(offset) to = Some(offset), confirmed the new fixture goes red, restored it and confirmed green again.

Non-blocking, fixed:

• .github/workflows/ci.yml's rust paths-filter — added CONTRIBUTING.md and .pre-commit-config.yaml, so a PR editing only the pin or only the hook config now runs pinned_tool_versions_match_across_docs_and_workflows/license_headers_hook_has_no_file_type_filter at PR time instead of only on main's forced full run. Scoped to rust since that's what gates both clippy and Test (affected crates) (confirmed by reading the job if: conditions).
• pin_token/assert_pins_match — comment-stripping is now scoped to YAML files only (.yml/.yaml extension), off for Markdown. strip_comment's " #" delimiter doesn't distinguish a real trailing comment from a GitHub issue reference (see #474, install hawkeye@7.0.1) in prose, so stripping unconditionally could silently drop a Markdown line's pin from the check — exactly the stale-duplicate case the helper exists to catch. Added a fixture (pin_token_rejects_prefixes_and_comment_only_mentions's new assertion) proving the issue-reference shape now extracts correctly with stripping off, alongside the existing YAML comment-stripping cases with it on.
• CONTRIBUTING.md:57 — now says an off-pin hawkeye may reject licenserc.toml outright (names the unknown field 'files' panic on 6.x), not just disagree with CI on content, and points at SKIP=license-headers for that case too.

Decisions, taken as you framed them:

• always_run tradeoff: no change, this was already the intended design from the earlier rounds.
• Commit history: confirming for whoever merges — the squash-merge message should describe the current end state (the table-driven pin test in workflow_contract.rs, always_run on the hook, indent-based hook_block), not any individual commit's now-superseded description.

cargo test -p xtask (282 passed), cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings, and cargo fmt --all --check are all clean.

[rominf]

🔴 Automated review · pr-review-watcher · 2138291

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — no blocking findings

Full review of the whole change.
No ticket named. The description cites #474 only as where the gaps were first reported. It describes the change accurately and claims nothing the diff lacks.

Round history: both findings below are new this round. The blocking item from the last round (at 7f780e6), "one branch of the new hook_block comment logic is not tested" (get_or_insert), is resolved. This round's blind review did not re-find it. After the report was frozen, the objection was put to the reviewer together with the author's account of the fix. The reviewer answered that it no longer holds. That answer is anchored evidence, because the reviewer was told what to look for. It re-ran the mutation (get_or_insert(offset) → = Some(offset)): hook_block_stops_at_the_first_of_several_header_comment_lines goes red. The last round's three non-blocking items (paths filter, CONTRIBUTING.md wording, Markdown comment stripping) were not re-found either.

Gate: nothing blocks, so this round files no change request. This automation holds none on this PR.

Blocking

None

Non-blocking

• (new) Only the coverage job runs the pin test on a CONTRIBUTING.md-only PR, and the new filter comment doesn't say so — .github/workflows/ci.yml:122-125, xtask/src/affected.rs:127-132
  • What the comment and description claim: the comment says a pin-only edit here "must still run that test". The description says such a PR "runs the xtask tests that guard them at PR time".
  • Why the test job skips it: is_ignorable treats every *.md as irrelevant, so cargo xtask affected returns Selection::Empty. The Test (nextest) step (ci.yml:514) is then skipped.
  • What actually runs it: the test only runs because the rust output also gates the coverage job. That job runs cargo llvm-cov --workspace and bails on any test failure (xtask/src/coverage.rs:333,344), and xtask is not in EXCLUDED.
  • Why that is fragile: if xtask is ever excluded from coverage, or coverage is narrowed, the guard disappears with no visible sign.
  • What it costs: every CONTRIBUTING.md prose edit now triggers fmt, clippy and the 45-minute coverage job.
  • .pre-commit-config.yaml is unaffected: no crate owns it and it isn't .md, so it forces --workspace.
  • Basis: traced independently by two passes reading affected.rs and coverage.rs. Not run in CI.
  • Confidence 85 · architectural · Fix: name the coverage job in the comment. Or, to make the guard explicit, add CONTRIBUTING.md to forces_full_workspace in affected.rs, as MANIFEST.md already is at line 120.
• (new) license_headers_hook_has_no_file_type_filter does not assert pass_filenames: false, which its own reasoning depends on — xtask/src/workflow_contract.rs (license_headers_hook_has_no_file_type_filter)
  • The test's doc says hawkeye "scans the whole repo per licenserc.toml (pass_filenames: false) however it was triggered", and CONTRIBUTING.md makes the same claim.
  • Mutation: changing the hook to pass_filenames: true left the test green (this was run).
  • Impact is likely low, because hawkeye would probably fail loudly on positional file arguments rather than silently narrow its scan.
  • Confidence 85 · mechanical · Fix: add hook_block_sets(block, "pass_filenames: false") next to the always_run assertion.

Decisions for the author

• always_run: true makes the local hook stricter than CI — tradeoff
  • What gets stricter: every commit and push now needs hawkeye installed. Hawkeye also scans untracked files that aren't gitignored, so an unrelated unheadered scratch file blocks an unrelated commit. A clean CI checkout never has such files.
  • The untracked-file scan was verified with local hawkeye 6.5.1 (a probe .rs file was flagged); 7.0.0 was not tested.
  • What it buys: the hook can no longer miss a licenserc.toml-only or Markdown-only change that CI's hawkeye job enforces.
  • The description and CONTRIBUTING.md present this as deliberate and offer SKIP=license-headers as the escape hatch. Confirm the local-strictness side is acceptable.

Positive signals

• Pins are compared by exact token equality on every occurrence, not by contains. Fixtures cover the 0.9.10-vs-0.9.1 prefix case, a stale duplicate pin, and a pin reworded out of the marker's shape.
• Comment-stripping applies only to YAML files (chosen by extension), so a #474 issue reference in Markdown prose can't hide a pin.
• Hawkeye's canonical version is read from ci.yml's license-headers job env: using the existing job_block/job_mapping extractors, not copied into a second constant.
• hook_block ends a hook's block by indent and looks ahead past header comments. Five fixtures pin each boundary shape.

Verdicts on the added tests:

• license_headers_hook_has_no_file_type_filter:
  • Fails when the production change is reverted (restoring types_or or commenting out always_run both fail it).
  • Also fails when any one of the four filter keys is reintroduced, including exclude_types:, caught through the types: substring.
  • Only gap: the pass_filenames line, listed above.
• pinned_tool_versions_match_across_docs_and_workflows:
  • Fails when any single mirror pin is mutated: hawkeye@ and cargo-about@ in CONTRIBUTING.md, MANIFEST.md, the ci.yml and dependabot-manifests.yml install lines, both cargo-about- cache keys, and HAWKEYE_VERSION.
  • git grep finds no pin occurrence outside its table.
• The helper fixture tests:
  • Each exercises a distinct branch: punctuation trim, strip_comments on and off, the found check, and the pending_comment first-offset rule.

Deployment notes

None. This is contributor tooling only. Locally, contributors with prek installed now need hawkeye 7.0.0 for every commit and push, or must use SKIP=license-headers.

What this covered

• Files read:
  • All five changed files, completely: .github/workflows/ci.yml, .pre-commit-config.yaml, CONTRIBUTING.md, xtask/src/tpn.rs, xtask/src/workflow_contract.rs.
  • The helpers the new tests use; the full paths filter and the test, clippy, coverage, license-headers and prek jobs.
  • xtask/src/affected.rs, xtask/src/coverage.rs, licenserc.toml, MANIFEST.md's pin region, and .github/workflows/dependabot-manifests.yml.
• Diff range: f9a80117 (merge-base) … 2138291f. It merges cleanly onto 24993519.
• Commands run:
  • cargo test -p xtask: 282 passed.
  • cargo clippy -p xtask --all-targets -D warnings and cargo fmt --check: clean.
  • Mutation checks as listed above.
• CI on the head:
  • Every completed check is green.
  • Still queued or in progress when read: E2E MI300X, Strix Halo Ubuntu nightly and release, Strix Halo Windows, and Strix Halo WSL2.
  • None has failed.

Did not run:

• The hawkeye 7.0.0 behaviour claims (no 7.0.0 binary locally; 6.5.1 was used instead).
• ai-instruction-quality (no agent-instruction files are in the diff).
• Prior-changes pass: earlier review comments on these files were deliberately not read.

Ran as three independent passes: a code pass, a combined instructions/history/comments pass, and a consolidation and design pass. The review itself was not reconciled against PR discussion; only the standing objection above was settled against the author's account, after the findings were frozen.

[jussielo-amd]

Both new findings from this round (review 5455310285, at 2138291) are fixed at ba99a5d.

• "Only the coverage job runs the pin test on a CONTRIBUTING.md-only PR" — confirmed: cargo xtask affected treats every *.md as ignorable, so that selection resolved to Selection::Empty and Test (nextest) was skipped; only the unconditionally --workspace coverage job caught a drift, invisibly. CONTRIBUTING.md is now in forces_full_workspace (xtask/src/affected.rs), the same reason MANIFEST.md already is, with a new fixture (select_contributing_md_forces_workspace). The ci.yml filter-entry comment is reworded to describe this instead of the filter entry alone.
• license_headers_hook_has_no_file_type_filter didn't assert pass_filenames: false — confirmed via the same mutation (flipping it to true left the test green). Added the assertion.

On the "Decisions for the author" item (always_run: true making the local hook stricter than CI): confirming that tradeoff stands as-is — it's already documented as intentional in both CONTRIBUTING.md and the PR description, with SKIP=license-headers as the escape hatch. No code change there.

cargo test -p xtask (283/283), cargo test --workspace --all-targets (one unrelated pre-existing flake in comfyui::tests::download_file_reports_cumulative_progress_to_its_caller — a loopback-server test, passes in isolation, untouched by this diff), cargo clippy --workspace --all-targets -- -D warnings, and cargo fmt --check are all clean.

[rominf]

🔴 Automated review · pr-review-watcher · ba99a5d

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — no blocking findings

Review of changes since the review at 2138291 (2138291f..ba99a5d8), plus the files they touch and the code those changes reach.
No ticket named. The description presents this as a follow-up to #474 and does not claim to close anything. What it says about this round's delta (CONTRIBUTING.md added to forces_full_workspace, the pass_filenames: false assertion, the reworded ci.yml comment) matches the code.

Round history: all three findings below are new this round, and all are non-blocking. The two non-blocking findings from the last round (at 2138291) were not re-found by this round's blind review, and both fixes were mutation-checked (see Positive signals). No blocking objection was standing from the last round.

Gate: nothing blocks, so this round files no change request. This automation holds none on this PR.

Blocking

None

Non-blocking

• (new) The text describing which files force a full-workspace run was not updated for the new entry — docs/testing.md:217-219, xtask/src/affected.rs:12-16 (module doc), xtask/src/affected.rs:115-116 (forces_full_workspace doc)
  All three still give the fallback set as "the lockfile, the toolchain file, the workspace root manifest, or CI config", or as "anything that can affect resolution, the build for every crate, or CI itself". CONTRIBUTING.md (affected.rs:125) fits neither wording: it is forced because an xtask test reads it. MANIFEST.md was already missing from these lists before this PR, so the change widens a drift that already existed. The code governs here: select() checks forces_full_workspace first (affected.rs:150) and returns Workspace. AGENTS.md §5 asks for docs/testing.md to change in the same change when a command's observable behaviour changes, and cargo xtask affected now prints --workspace for a CONTRIBUTING.md-only diff. Whether that rule covers an xtask CI helper is arguable, so this is listed as non-blocking.
  Confidence 75 · mechanical · Fix: add a "files an xtask test reads" category (MANIFEST.md, CONTRIBUTING.md) to the docs/testing.md sentence and to both doc comments.

• (new) The new ci.yml comment says CONTRIBUTING.md is forced "for the same reason MANIFEST.md already is", but the code gives MANIFEST.md a different reason — .github/workflows/ci.yml:129-130, xtask/src/affected.rs:120
  The comment at affected.rs:120 justifies MANIFEST.md with "clippy job runs xtask manifest --check". That has nothing to do with affected: it only selects for the nextest step, and clippy is never narrowed. The reason the two files actually share is that pinned_tool_versions_match_across_docs_and_workflows reads both of them (workflow_contract.rs:2262-2263). The ci.yml wording is the accurate one, because it names the reason that makes affected matter. The PR description repeats the same phrase.
  Confidence 70 · mechanical · Fix: update the affected.rs:120 comment to name the pin test as well, so the two comments agree.

• (new) The same gap remains open for docs/ci-hardware-testing.md (pre-existing; same pattern as this fix) — xtask/src/workflow_contract.rs:1738
hardware_testing_docs_cover_all_self_hosted_platforms reads docs/ci-hardware-testing.md. That path matches neither the rust nor the heavy paths filter (ci.yml changes job), and is_ignorable drops it because of the docs/ prefix (affected.rs:136). A PR that only edits that doc therefore runs no job that executes the test, and a break first shows up in the merge queue. This is the exact failure mode this round fixes for CONTRIBUTING.md. The test was traced through both filters and select(). The sibling doc fixtures (tests/e2e-cucumber/README.md, skills/**) are covered on PRs through heavy.
  Confidence 85 · logic · Fix: give it the same treatment (rust filter entry plus forces_full_workspace), or see the design note below.

Decisions for the author

• Fixture-read docs are listed by hand in two places, and the lists drift — non-blocking-improvement
  Each Markdown file an xtask test reads has to be added both to ci.yml's rust filter and to forces_full_workspace. Nothing checks that the two lists agree, and the docs/ci-hardware-testing.md gap above shows they have already diverged. A contract test could collect every repo_root().join(...) path that xtask tests read and assert each one either reaches rust and is not ignorable, or is otherwise covered. That would close the whole class of gap instead of one file at a time.

Positive signals

• The new select_contributing_md_forces_workspace test fails when the production line is removed (|| file == "CONTRIBUTING.md" replaced with || false gives exit 101 at affected.rs:451). It passes when the line is restored.
• The new pass_filenames: false assertion fails when the hook is flipped to true (exit 101 at workflow_contract.rs:2397) and passes when restored. Its rationale holds for the pinned hawkeye v7, which accepts positional paths, so pass_filenames: true really would narrow the scan to changed files.

Deployment notes

None

What this covered

Read 2138291f..ba99a5d8 in full. Read xtask/src/affected.rs and .github/workflows/ci.yml completely. In xtask/src/workflow_contract.rs, read the license-headers test, hook_block/hook_block_sets, the pin test and the docs-reading tests around 1720-1880. Also read xtask/src/coverage.rs's measure/EXCLUDED, .pre-commit-config.yaml, docs/testing.md 205-222, and the CONTRIBUTING.md hawkeye section. Base is the current main tip. The merge-base is f9a8011; the head merges cleanly onto the current base, and no base commit since then touches the edited regions. CI on ba99a5d shows every check passing.

Tests run: the two mutation checks above (cargo test -p xtask). The ci.yml claim that the coverage job is not narrowed by affected was confirmed: it runs cargo llvm-cov --workspace excluding only e2e-cucumber, so it does include xtask.

Not verified: hawkeye v7.0.0's positional-path behaviour was not executed. The local binary is 6.5.1, which rejects positional args, so the v7 claim rests on the v7.0.0 release notes and README (fast/hawkeye releases, fast/hawkeye).

Fan-out: two independent workers, one for the affected.rs + ci.yml code pass and one for the agent-instruction, history and code-comment passes. The workflow_contract.rs pass was run by the driving reviewer, so that pass was not independent.

Did not run: the prior-changes pass, because earlier review comments were out of bounds.

No prompt-injection content found. The review itself was not reconciled against the PR's discussion; the author's reply to the last round was triaged separately.

[jussielo-amd]

Addressed the round for ba99a5d (#5456419633) in ad73ed2:

• docs/ci-hardware-testing.md had the same invisible-coverage gap this PR already closed for CONTRIBUTING.md — hardware_testing_docs_cover_all_self_hosted_platforms reads it, but it matches neither the rust nor heavy paths-filter and is_ignorable drops it (docs/ prefix), so a doc-only PR runs no job that executes the test. Gave it the same treatment: added to forces_full_workspace and the rust filter, with a mirrored select_hardware_testing_docs_forces_workspace test. Verified with a reverted mutation (dropped the new forces_full_workspace line): the new test goes red, passes again restored.
• affected.rs's fallback-file doc comments (module doc, forces_full_workspace) and docs/testing.md's matching sentence didn't name "a doc an xtask test reads" as a reason for a full-workspace run — fixed, all three now list it.
• ci.yml's CONTRIBUTING.md filter comment claimed it's forced "for the same reason MANIFEST.md already is", but affected.rs gave MANIFEST.md an unrelated reason (clippy's xtask manifest --check) with no mention of the pin test. Fixed by stating both of MANIFEST.md's reasons in affected.rs's comment, so the two now agree.

Not taken: the generalized contract test suggested under "Decisions for the author" (collect every repo_root().join(...) path an xtask test reads and assert each one reaches rust/isn't ignorable). Same call as the earlier licenserc.toml/types_or parity suggestion — closing the concrete gaps flagged is in scope for this PR; a new guard class for the whole pattern is a reasonable follow-up, not this PR's job.

cargo test -p xtask (284 passed), cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings, and cargo fmt --all --check are all clean.

[volen-silo]

Reviewed the diff at ad73ed2 and the relevant surrounding code. I found no blocking issues. The hook configuration, version guards, and CI selection changes are consistent with the stated intent, and the earlier review concerns are addressed in this head.

The PR looks safe to approve after final maintainer validation. This is a source review, not a formal approval or independent verification of the test results; I did not run local tests or runtime checks.

[rominf]

🔴 Automated review · pr-review-watcher · ad73ed2

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Review of changes since the review at ba99a5d, plus the files they touch.
No ticket named. The description is a "Follow-up to gaps an automated review surfaced on #474"; it was read at Step 9 and is the subject of the one blocking finding.

Round history: every finding below is new this round. No blocking objection was standing from the last round (at ba99a5d), and its three non-blocking findings were not re-found by this round's blind review.

Gate: the review method rates the description finding below as blocking. This round still files no change request, because the only thing merging would leave behind is a PR description that leaves out one filter entry and has a stale test count. The code, its comment and its test already document that entry. Editing the description resolves the finding without a push. This automation holds no change request on this PR.

Blocking

• (new) The PR description doesn't mention the docs/ci-hardware-testing.md work this commit adds — .github/workflows/ci.yml:134-138, xtask/src/affected.rs:118,130-133,465-476, docs/testing.md:218-221
  • This commit adds docs/ci-hardware-testing.md to the rust paths-filter and to forces_full_workspace. It also adds a test for it and widens the fallback list in docs/testing.md.
  • The description's CI-filter bullet names only CONTRIBUTING.md and .pre-commit-config.yaml. Its Scope section calls the change a hawkeye/cargo-about pin and hook guard. A reader deciding whether hardware-testing doc coverage was touched would be told it wasn't.
  • The Test plan also says "cargo test -p xtask passes (283 tests)". On ad73ed2 that run gives 284 passed, 0 failed.
  • Confidence 85 · mechanical · Fix: update the description. Add a bullet for the docs/ci-hardware-testing.md filter and forces_full_workspace entries, and why they're needed. Update the test count. If this is seen as a separate logical change (AGENTS.md §11), split it out instead.

Non-blocking

• (new) The new "doc an xtask test reads" list reads as complete, but the test this change cites reads a doc the list leaves out — docs/testing.md:219-220, xtask/src/affected.rs:14,117-118, xtask/src/workflow_contract.rs:1862
  • hardware_testing_docs_cover_all_self_hosted_platforms reads docs/ci-hardware-testing.md at workflow_contract.rs:1738. It also reads tests/e2e-cucumber/README.md at :1862.
  • That README isn't in forces_full_workspace or the rust filter. A probe against the real cargo graph resolves an edit to it alone to Packages({"e2e-cucumber"}), which doesn't include xtask.
  • So the docs claim that cargo xtask affected falls back to --workspace for "a doc an xtask test reads" isn't true of the code (AGENTS.md §5 asks for docs and code to agree). The code governs, because it decides what runs.
  • The practical gap is small: the README matches heavy (ci.yml:161), and the Windows lane then runs cargo test --workspace.
  • Confidence 85 · logic · Fix: either add tests/e2e-cucumber/README.md to forces_full_workspace and the rust filter, or word the list as examples ("e.g.") rather than the full set.
• (new) No test covers the new rust-filter entry, which is half of this fix — .github/workflows/ci.yml:134-138
  • Deleting docs/ci-hardware-testing.md from the rust filter leaves every xtask test green. Nothing in xtask/src reads the paths-filter block; the only hits for the path are affected.rs and workflow_contract.rs:871,1738.
  • Without that entry, the forces_full_workspace line is unreachable for a PR that edits only that doc: no rust-gated job runs at all.
  • The project doesn't require tests for CI plumbing, and the earlier CONTRIBUTING.md entry has the same shape, so this isn't blocking. But workflow_contract.rs already parses ci.yml jobs, so a contract test is practical.
  • Confidence 85 · architectural · Fix: add a workflow_contract test asserting that every literal path in forces_full_workspace also appears in ci.yml's rust filter.
• (new) A new comment says MANIFEST.md carries a hawkeye pin; it doesn't — xtask/src/affected.rs:122-124
  • The comment reads: "it and CONTRIBUTING.md also carry the hawkeye/cargo-about pins". grep hawkeye MANIFEST.md finds nothing.
  • The test's table (workflow_contract.rs:2278) checks hawkeye@ in CONTRIBUTING.md only. The ci.yml copy of the comment (ci.yml:113-115, "its cargo-about pin") is the accurate one.
  • Confidence 85 · mechanical · Fix: "MANIFEST.md carries the cargo-about pin and CONTRIBUTING.md the hawkeye/cargo-about pins that …".

Decisions for the author

• The list of xtask-read docs is maintained by hand in three places — non-blocking-improvement
  • The places are the rust filter, forces_full_workspace, and the prose in docs/testing.md and the affected.rs docs. Each new xtask test that reads a doc needs all of them updated, which is how the README case above was missed.
  • A shared const of xtask-read paths, used by forces_full_workspace and checked against ci.yml by a contract test, would make that drift fail a test. The current approach is not wrong, just open to the drift it is guarding against.

Positive signals

• select_hardware_testing_docs_forces_workspace states its carve-out from select_docs_only_is_empty next to the assertion. It does fail when the production line is removed: with affected.rs:133 deleted, cargo test -p xtask select_hardware goes from 1 passed to 0 passed, 1 failed (exit 101).

Deployment notes

None

What this covered

• Read: git diff ba99a5d8..ad73ed27 (one commit, ad73ed2). I read xtask/src/affected.rs and docs/testing.md completely, and .github/workflows/ci.yml's changes paths-filter block and the jobs it gates. I read xtask/src/workflow_contract.rs completely, to list every repo file an xtask test reads and check each against select and the rust filter. I also read MANIFEST.md's pins.
• Ran: cargo test -p xtask on the unmodified tree (284 passed, 0 failed, all four relevant tests named in the log), and the mutation check above. A temporary probe ran select() against the real cargo graph for each xtask-read doc; it was not committed.
• CI: every check on ad73ed2 is green, via statusCheckRollup.
• Test verdicts for the one added test:
  • It fails with the production change reverted, so it does test the change.
  • The only part of the fix it can't detect is the ci.yml filter entry, covered under Non-blocking.
• Passes:
  • Code, agent-instruction adherence, history, and code comments ran as two independent workers. The history pass found nothing beyond the original forces_full_workspace (#70). The leak scan was clean, and the commit has its DCO sign-off and signature.
  • Step 7 design review was done by the driver, not fanned out. Its one question, how the read-doc list is kept in sync, did not need a separate investigation.
  • The prior-changes pass did not run, because the PR discussion is out of bounds for this review.
• Narrowing: scope was narrowed by the caller to ba99a5d..HEAD.
• Not reconciled: this review was not reconciled against the PR's discussion or earlier reviews. The author's reply to the last round was triaged separately.
• Merge note: architecture_doc.rs no longer exists on prw-base, so docs/architecture.md was dropped from the read-doc gap above.