[jussielo-amd]

Closes ROCMAI-904.

Summary

The unit test renderers_take_the_pr_gating_phrases_from_the_shared_constants in crates/e2e-report/src/consolidated/mod.rs is meant to guarantee that the markdown and HTML renderers build the PR-gating sentences from the shared MOCK_GATES_PR and OTHER_LANES_NON_BLOCKING constants instead of re-typing the words. Its positive half only checked that the production part of markdown.rs and html.rs contained the constant's name as a substring. Both files import the constants and markdown.rs also names them in a comment, so a renderer could stop using a constant entirely and the check would still pass on the import or the comment.

The test now strips use statements and comments (line, doc and block) from the production source with a small pure helper, strip_comments_and_uses, and then requires each constant to appear in a usage position with a second helper, references_constant. It accepts the two shapes the renderers use today: a {NAME} (or {NAME:spec}) interpolation inside a string literal (markdown.rs) and a (NAME) splice outside any string in a maud block (html.rs). An import, a comment or doc comment, a bare mention such as let _ = NAME;, a (NAME) written inside a string, a {NAME} written outside one, and a call argument such as foo(NAME) no longer count. The negative half is unchanged: neither source may contain the phrase literal.

What the test still does not prove is placement: an interpolation in some other string, or a splice in the wrong block, would satisfy it. That guarantee is carried by the verbatim pins summary_explanation_paragraphs_are_unchanged and legend_mock_and_other_platform_items_are_unchanged, and the test comment now says so; this test only guards that the words keep coming from the constants rather than from a re-typed copy or an unused import.

The stripper is a hand-rolled scanner, not a lexer. It reads plain strings and one-character char literals but not raw strings, and its doc comment lists that gap. A new helper, contains_raw_string, makes the test fail with a clear message if the production part of either renderer ever contains a raw string (r, br or cr, with any number of #), instead of trusting a scan it cannot do.

The helpers have their own unit tests: import-only, comment-only, doc-comment-only, import plus comment with a re-typed phrase, a bare mention, call arguments, a splice inside a string and an interpolation outside one (all rejected), a format-string interpolation, a format spec and macro splices (accepted), comment and char-literal handling in the stripper, and every raw-string form for the guard, including the cr forms, with plain b"..." and c"..." strings and identifiers merely ending in br or cr as near misses.

Mutation evidence, run locally and reverted with targeted edits. In the renderers: replacing **{MOCK_GATES_PR}** in markdown.rs with **gates** (import and comment left in place, which the old check accepted) turns the test red; appending It gates the PR. after the (MOCK_GATES_PR) splice in html.rs turns it red on the negative half; wrapping that splice as (str::trim(MOCK_GATES_PR)) turns it red on the positive half; adding a raw string to markdown.rs turns it red with the raw-string message. In the helpers: dropping the identifier-before-( check, looking the splice up in text that still contains strings, accepting {NAME} outside strings, making contains_raw_string always false, and dropping cr from its prefix list each turn exactly their own helper tests red. The real tree is green. No renderer output changes, and both verbatim pins pass untouched. No README or docs sentence describes this test's claim, so no doc needed updating.

Test plan

Ran locally: cargo test -p e2e-report (91 passed), cargo fmt --all --check, cargo clippy --locked -p e2e-report --all-targets -- -D warnings, cargo xtask manifest --check, a leak scan of the diff against origin/main, and the mutations above. Not run: the full workspace gate, scripts/smoke_local.py and the e2e harness, since the change touches only a unit-test module in one crate and no code outside #[cfg(test)]. Windows-target clippy was not needed as nothing is platform-gated.

This is a test-only change with no user-observable behavior, so no Gherkin scenario applies.

[Copilot]

🟢 Approval recommended

No active review issues were identified for the test-only changes strengthening the shared PR-gating source checks.

No open issues 🎉

What changed in this PR

Strengthens the e2e-report source test so imports and comments alone cannot satisfy the shared PR-gating constant checks. This is a test-only change; renderer output is unchanged.

Changes:

• Strip comments and use statements before checking for constant interpolation or macro splices.
• Add helper tests covering rejected mentions, accepted usage shapes, and string preservation.
• Clarify the source check’s guarantees while retaining the phrase-literal rejection checks.

File	Description
crates/​e2e-report/​src/​consolidated/​mod.rs	Adds source-check helpers and tests, then strengthens the shared gating-phrase assertions.

_{Review effort: Balanced}

[juhovainio]

Thanks for tightening this. The usage-position check matches ROCMAI-904's scope: the negative half is kept, the change is test-only, and the mutation evidence is in the PR body. I found nothing blocking in the diff, but I'm requesting changes for the following before approval.

To address

• references_constant accepts {NAME}, {NAME: or (NAME) anywhere in non-comment code. A constant interpolated into an unrelated string, or a stray foo(MOCK_GATES_PR) call, would still satisfy it. The PR text acknowledges this limit; please either narrow the match or state explicitly in the test comment that the pins carry the placement guarantee.
• The comment/string stripper is a hand-rolled lexer with known gaps (raw strings, and only the exact '"' char literal). I checked that the production sections of markdown.rs and html.rs contain no raw strings and use only the shapes the test accepts, so the current tree should pass. A future raw string in those files would silently change the result, so please document that limitation next to the helper.

CI

• docs/readthedocs.com is failing ("Read the Docs build failed!"). That looks unrelated to a test-only Rust change, but please confirm it is a pre-existing or infra failure.
• clippy, Ubuntu and Windows build/test, coverage and commit-signatures were still pending when I looked. I did not run the claimed cargo test, clippy or mutation runs locally, so I'm relying on CI and your PR-body evidence for those.

[jussielo-amd]

Thanks for the review. Both points are addressed in aaff06e.

On references_constant being too loose: I narrowed it where that was cheap and robust, and stated the remaining limit in the test. A {NAME} (or {NAME:spec}) now has to sit inside a string literal and a (NAME) splice has to sit outside one, and a splice that directly follows an identifier is refused, so foo(MOCK_GATES_PR) and out.push_str(MOCK_GATES_PR) no longer count, nor does (MOCK_GATES_PR) written inside a string or {MOCK_GATES_PR} written outside one. I did not try to anchor on the sentence text around the constant, because that would duplicate the verbatim pins and break on any rewording. The test comment and the helper's doc comment now say plainly that placement in the right sentence is carried by summary_explanation_paragraphs_are_unchanged and legend_mock_and_other_platform_items_are_unchanged, and that this test only guards that the words keep coming from the constants. Each refused and accepted shape has a test, and I checked each guard by mutation (dropping the identifier check, mixing the string and non-string sides, and wrapping the real html.rs splice as a call argument each turn the matching test red).

On the stripper's gaps: its doc comment now says it is a hand-rolled scanner that reads plain strings and one-character char literals (I also taught it the escaped forms such as '\"') but not raw strings. Rather than only documenting that, the test now asserts that the production parts of markdown.rs and html.rs contain no raw string (r", r#", br"), with a message telling the next person to teach the helper raw strings first. That guard has its own test over every raw form and the near misses, and adding a raw string to markdown.rs turns the test red with that message.

The docs/readthedocs.com check is not touched by this change, which only edits a unit-test module in one Rust file. That build failed on the previous head and passes on the other open pull requests I checked; a fresh build is running on this push, so I will not call it confirmed until that one finishes.

[Copilot]

🟡 Changes recommended

The usage scanner still counts escaped format braces, import-comment content, and raw C-string content as actual references, allowing unused MOCK_GATES_PR checks to pass.

1 open issue

• Raw C strings bypass the raw-string guard · New

2 previously missed issues

In code that hasn't changed since last review

Import scan stops at semicolons inside comments

crates/​e2e-report/​src/​consolidated/​mod.rs:1853

A valid import such as use super::X /* reason; (MOCK_GATES_PR) */; stops this loop at the semicolon inside the block comment. Scanning then resumes inside that comment, leaving (MOCK_GATES_PR) in code, so references_constant returns true even though the constant appears only in a comment. Please make the import-terminator scan comment-aware and add a regression case for an inline import comment containing a semicolon.

Escaped braces falsely count as constant usage

crates/​e2e-report/​src/​consolidated/​mod.rs:1951

The substring check accepts write!(out, "{{MOCK_GATES_PR}}") and "{{MOCK_GATES_PR:>4}}", although escaped braces emit literal text without reading the constant. This leaves an unused-name false positive in the check intended to require actual usage, independently of the documented placement limitation. Ignore escaped opening braces and add rejection cases for these forms, while retaining acceptance of "{{{MOCK_GATES_PR}}}".

_{Review effort: Balanced}

Fixed: the usage check is narrowed and the stripper's raw-string gap is documented and guarded in aaff06e; both points are answered on the PR and the Read the Docs build passes on the new head.

[Copilot]

🔵 Needs a closer look

The constant-use guard can falsely accept names found only in import comments or escaped format text, allowing code with no actual reference to the constant to pass.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

Import skipper mistakes semicolons in comments for code

crates/​e2e-report/​src/​consolidated/​mod.rs:1853

The import-skipping loop stops at semicolons inside comments: use super::{/* rationale; (MOCK_GATES_PR) */ MOCK_GATES_PR}; leaves (MOCK_GATES_PR) to be scanned as code. The usage check therefore passes even though the constant appears only in an import and a comment; a line comment containing a semicolon has the same problem. Strip comments before processing imports, or share comment-state handling with the import skipper, and add regression fixtures for both comment forms.

Escaped format braces falsely satisfy the constant-use check

crates/​e2e-report/​src/​consolidated/​mod.rs:1955

This substring check accepts write!(out, "{{MOCK_GATES_PR}}"), which prints the name literally without referencing the constant; escaped format-spec text also passes. This lets literal text satisfy the new constant-use guard. Ignore escaped opening-brace pairs and add rejection cases for escaped names and specs, while retaining acceptance of {{{MOCK_GATES_PR}}}, which contains a real capture.

1 resolved since last review

• Raw C strings bypass the raw-string guard

_{Review effort: Balanced}