[juhovainio]

Summary

• Adds cargo xtask release-readiness, which mechanically computes a GO/NO-GO verdict, categorized commits (New Features / Bug Fixes / Breaking Changes), known issues, and an Area/Evidence/Status table from git history, job results, and open bug-labeled issues.
• Adds a nightly pipeline (release-readiness-nightly → release-readiness-narrative → publish-release-readiness-wiki) that drafts short prose from those facts via actions/ai-inference (using the COPILOT_PAT secret, a Copilot-licensed PAT — the Copilot CLI that action shells out to does not accept a bare GITHUB_TOKEN) and publishes a Release-Readiness wiki page. publish-release-readiness-wiki shares the E2E support-matrix wiki job's branch/repo gate, own concurrency group, and degenerate-output guard, but additionally still runs (with an explicit "evidence unavailable" notice) when release-readiness-nightly itself fails outright, instead of leaving the wiki showing a stale verdict with no sign it's stale. release-readiness-nightly/release-readiness-narrative themselves have no branch/repo guard — they run on any branch this workflow is dispatched on.
• The LLM only ever drafts prose from already-computed facts; it never decides pass/fail, counts, or PR/issue numbers.
• Commits scoped entirely to internal CI/dev tooling (ci, nightly, github, hooks, xtask, build, deps, e2e*) are excluded from the Bug Fixes / New Features summary, so the page stays focused on end-user-facing functionality and a summarized hardware-validation (E2E) result rather than CI housekeeping.

Jira: ROCMAI-568

Changes since the automated review

• The release-readiness verdict's E2E gate now reads the consolidated e2e-report-nightly job's own result (--e2e-report-job-result), not individual per-lane results, so a lane that dies before uploading a report still fails the gate instead of silently vanishing from the consolidated totals.
• E2E evidence that's missing or unparseable (e.g. a truncated/broken report) now fails the gate explicitly ("E2E (report unreadable)") instead of silently reading as zero failures. The status table's header always renders regardless (confirmed by test); this case does not trip publish-release-readiness-wiki's degenerate-output guard, which only trips when the table has no header row at all (e.g. a crash before writing anything).
• A legitimately skipped nightly/windows-nightly job (a no-op night) no longer causes a false NO-GO.
• Untrusted GitHub issue titles are escaped before reaching the generated wiki page.
• extract_pr_number no longer matches a mid-sentence (#NNN) parenthetical.
• Extracted a pure assemble_facts function so the GO/NO-GO logic is directly unit-tested (previously zero coverage on the orchestration/rendering path).
• Rebased onto current main and re-authored 3 commits that had picked up the wrong git identity.
• The Strix Halo Windows E2E pool-label fix that had been bundled in here is split out into fix(nightly): run Strix Halo Windows E2E on the devlab-dispatch pool #682.

Changes since the second round of review

• publish-release-readiness-wiki now runs (instead of being skipped entirely) when release-readiness-nightly fails outright, publishing an explicit "EVIDENCE UNAVAILABLE" notice to the wiki rather than silently leaving a stale verdict in place with no indication it's stale.
• render_table's status table now lists the actual failing_gates reason strings beneath the Area/Evidence/Status table (e.g. E2E (lane report check: cancelled)), since a cancelled lane otherwise renders as a contradictory "0 failure(s)" next to a red ❌.
• The narrative's embedded markdown is now escaped, not link-stripped, before it's pushed to the wiki (publish-release-readiness-wiki's sed doubles \ and escapes [, ], <, > instead of trying to strip [text](url) patterns). The old stripping regex left <img> tags, reference-style [a][1] links, and nested [outer [inner](url)](url) links untouched or still live; escaping brackets/angle-brackets can't be bypassed by nesting, since an escaped bracket is never re-parsed as link/image syntax. This is distinct from the issue-title escaping above: that covers titles as render_table renders them into the wiki's status table, this covers the LLM's own drafted output on its way to the same page. Neither covers facts.json itself, which is what the LLM prompt is actually built from and still carries raw, unescaped titles.
• Extracted a pure build_known_issues mapping/sort function out of collect_known_issues, so its test exercises the real production filter/sort instead of a parallel reimplementation that would pass even if the production logic broke.
• Added test coverage for branches that were previously exercised by no test: the GO/NO-GO verdict headline, PR-number-suffix rendering, skipped treated as passing for the nightly build, the Windows build, and the E2E report job (each asserted separately), the exact n > 0 vs n > 1 failure-count boundary, escape_markdown's backslash-doubling/newline-collapsing/angle-bracket-escaping, and a multibyte-safe guard in split_trailing_pr_reference (removing it could panic by slicing mid-character on a subject ending in a multibyte character after a mid-sentence (#N) reference).
• Fixed the actions/ai-inference pin comment: that action tags full semver releases (v1.0.0, v2.1.1, ...) alongside a floating major alias, not bare majors only — the SHA pin still freezes an exact commit either way.
• Corrected docs/ci-hardware-testing.md's release-readiness section: it previously (incorrectly) scoped the whole feature to main and claimed the wiki-publish guards are identical to publish-e2e-wiki's.
• LANE_REPORTS now requires both the release and nightly leg's uploaded report for every channel-matrixed lane, not just the release leg, in both nightly.yml and e2e-selfhosted.yml. In e2e-selfhosted.yml this means the per-PR "E2E consolidated report (self-hosted)" check now also fails if the Strix Ubuntu nightly leg uploads no report, not just its release leg.

Left unchanged (judgment calls, not oversights)

• The narrative job drafts prose from already-public GitHub issue titles/commit subjects and publishes it to the (public) wiki; this is the same trust boundary the existing publish-e2e-wiki job and this repo's public issue tracker already operate under, so it isn't a new exposure introduced here.
• publish-e2e-wiki and publish-release-readiness-wiki use separate concurrency groups (each only serializes against itself) and neither retries a transient wiki-clone failure; a retry/backoff story for wiki pushes would be a repo-wide improvement, not something specific to this PR's new job.
• A genuinely missing E2E report file (as opposed to a present-but-unparseable one) still fails the release-readiness-nightly job via read_to_string(...)? rather than producing a NO-GO page directly; the wiki-publish job's "evidence unavailable" fallback (above) already covers this case at the publish step, so the page never goes stale or silently shows a wrong verdict — only the job's own logs say why the page didn't update, and that's within the same tolerance the equivalent publish-e2e-wiki path already accepts.
• Bare URLs in issue titles or commit subjects can still be autolinked by the wiki renderer even though bracket/angle-bracket markup is escaped; this is a GitHub-renderer behavior outside what escaping on our side can prevent, not a gap in the escaping itself.
• The CI check failures showing on this PR are pre-existing flakes unrelated to this diff (see individual check runs); not a regression from these changes.

Changes since the third round of review

• total_e2e_failures now scans only the summary table's row range (between the | Platform header and the | **Total** row), instead of every |-delimited line in the document — it was also matching rows from the expectation-grid and command-coverage tables e2e-report prints after the summary, which inflated the failure count roughly 90-fold against a real report and made the verdict unable to ever read GO. Added a regression test against e2e_report::consolidated_summary_markdown's real output, not a hand-written snippet.
• Both build gates (nightly, windows-nightly) now treat cancelled/timed_out as failures instead of reading as GO. Added failure-path tests for collect_commit_subjects and collect_known_issues (behind a current_dir seam) so a git/gh failure is no longer an untested branch.
• Strengthened the workflow-contract tests a prior mutation-testing pass found gaps in: pinned fetch-depth: 0 on release-readiness-nightly's checkout, the publish job's needs on both upstream jobs, the two continue-on-error: true artifact downloads, the distinct wiki page names, and the dynamic --nightly-result read; and made the env-only-reaches-the-shell test fail if any other line in the job also references steps.draft.outputs.response.
• categorize now puts any type!: (not just feat!/fix!) in the Breaking bucket, matching Conventional Commits' actual grammar.
• The Build row now renders "not run" instead of ✅ when windows-nightly was skipped, instead of claiming a pass with no Windows build behind it.
• Fixed a stale comment and a misleading assertion message in workflow_contract.rs (no behavior change).

Left unchanged (judgment calls, not oversights)

• Commit messages describing behavior the code no longer has (a --e2e-lane-result flag, "6 GPU/WSL E2E lane jobs", link-stripping, etc.) are left as-is in this pass; correcting them means rewriting already-pushed history, which this PR doesn't force-push to do. They'll be reworded in the squash-merge body at merge time instead.
• Test (affected crates) failing on preflight_runs_the_real_reclaim_assert_clear is the pre-existing multicall-coreutils decoy issue fixed on main by fix(ci): keep the GPU reclaim decoys alive on multicall coreutils #712; this branch's merge-base predates that fix. A rebase (not a code change in this diff) settles it, and it was already rated non-gating.
• Narrative-steering via bug-labeled issue titles/excerpts (any GitHub user can author text an LLM then drafts into wiki prose) is a real exposure, but fixing it (labeling the section as LLM-drafted from untrusted input, or feeding only titles) is a product decision about how much narrative detail to keep, not a one-line fix; left for a follow-up.
• Whether non-blocking lanes (W7900, MI350P) should count toward NO-GO is a tradeoff for the author/team to confirm, not something to decide unilaterally in this pass.

Changes since the fourth round of review

• facts.json now always ends with a trailing newline, so the $GITHUB_OUTPUT heredoc delimiter in nightly.yml's "Read facts for prompt" step can't land on the same line as the JSON's closing brace even if that step's own echo guard is ever dropped.
• e2e-report-nightly's result no longer accepts skipped as a passing gate; that job is if: always() and has no legitimate no-op-night skip.
• total_e2e_failures no longer panics when a **Total** row precedes the | Platform header; it now searches for Total only after the header index.
• Dropped the unreachable "" status arm in total_e2e_failures (the producer only ever emits PASS, FAIL, or EMPTY).
• collect_known_issues_in_fails_outside_a_github_repo now asserts the specific gh issue list failure message instead of a substring both the bail! and JSON-parse-error paths happen to share.
• Fixed 3 stale comments: the failing_gates doc omitted the lane-report-check gate; nightly.yml's ai-inference pin comment named the wrong AGENTS.md policy; ci-hardware-testing.md pointed "below" at a section that's actually above.
• Added tests for 3 previously-untested branches: the known-issue reaction tie-break, excerpt's exact-character-limit boundary, and build_skipped's skipped half for both the nightly and Windows build gates.

Left unchanged (judgment calls, not oversights)

• Commit-message history-rewrite objection still holds: correcting the 4 commits whose messages describe removed behavior means rewriting already-pushed branch history and force-pushing, which this pass doesn't do. Since this repo's squash setting (COMMIT_MESSAGES) builds the squash body verbatim from these commit messages and the merge queue has no step to edit it, this needs to happen as an actual history rewrite before the PR is queued, not "at merge time" as earlier rounds assumed.
• Whether internal-type breaking commits (ci!:, chore!:, build!:) should be dropped from the Breaking bucket is left as the reviewer explicitly flagged it as intent-dependent; no such commits exist in this branch today, so it doesn't affect the current page's output either way.
• publish-release-readiness-wiki's "EVIDENCE UNAVAILABLE" notice can still be wrong when it's a transient table-download failure rather than an actual release-readiness-nightly failure (both take the same ! -f table.md branch); narrowing this needs threading needs.release-readiness-nightly.result through env: and is left for a follow-up.
• main.rs → release_readiness::run's argument wiring and its 5 result strings remain untested and untyped (a swapped windows_result/e2e_report_job_result or a typo like succes would compile and read as a failed gate rather than an error); a ValueEnum JobResult would be the fix, left for a follow-up since it's a larger refactor of main.rs's argument surface than this pass's scope.
• The sdk_version axis being checked differently from the channel axis in workflow_contract.rs predates this PR; this round only put the two side by side, so it's left as a follow-up rather than a fix bundled into an unrelated pass.

Test plan

• cargo test -p xtask (full suite, 371 tests, including unit tests for the fixes above and workflow-contract tests exercising the real publish guards — including the three-way missing/degenerate/well-formed table guard and the narrative-escaping sed on publish-release-readiness-wiki — against actual extracted shell snippets, not pinned strings)
• cargo clippy -p xtask --all-targets --all-features -- -D warnings
• cargo fmt -p xtask --check
• Ran cargo xtask release-readiness end-to-end locally against this branch's real commit history and a fake E2E report, inspected table.md output
• Dispatched nightly.yml on this branch (run 38396499964) and confirmed release-readiness-nightly and release-readiness-narrative complete successfully; downloaded and read the narrative.md artifact to confirm it's genuine grounded prose citing real PR/issue numbers, not a refusal or empty output
• A follow-up dispatch to confirm the CI-scope exclusion and the new fail-safe/guard behavior end-to-end in the real pipeline (local verification already confirms mechanical correctness deterministically)

[rominf]

🔴 Automated review · pr-review-watcher · 7235dd9

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.

Round 1 — every finding below is new this round.

Review — needs work

Full review of the whole change.
No ticket named. The description misstates the change: see the last two blocking findings.

Blocking

• The GO verdict fails open when the E2E evidence is missing — xtask/src/release_readiness.rs:312-319, xtask/src/release_readiness.rs:495-498, .github/workflows/nightly.yml:1735-1781
total_e2e_failures returns .unwrap_or(0) when there is no | **Total** row. That is what consolidated_summary_markdown (crates/e2e-report/src/consolidated.rs:966-970) writes when no lane uploaded a report. It is also what an empty or partial support-matrix.md yields when the e2e-report-nightly step fails, because the artifact upload there runs if: always(). A probe run returned 0 failures for an empty file, for the real "no report.json files" output, and for an EMPTY-status-only table, so each of these publishes "✅ GO". A lane that times out before uploading is not counted as a failure either, and the six lanes are all continue-on-error. The verdict never receives needs.e2e-report-nightly.result or any per-lane result. The test total_e2e_failures_defaults_to_zero_without_a_totals_row pins this fail-open behaviour.
  Confidence 100 · logic · Fix: return NO-GO (or UNKNOWN) when the Total row is missing or fewer platforms are present than expected. Pass the e2e-report and lane job results into the xtask. Invert the test.

• Nights with no new commits publish a false NO-GO — xtask/src/release_readiness.rs:91-93, .github/workflows/nightly.yml:207
  When nightly sets skip=true, the nightly job succeeds, windows-nightly is skipped, and windows_result != "success" adds the failing gate "windows build (skipped)". The verdict is then NO-GO, and publish-release-readiness-wiki publishes it because release-readiness-nightly succeeded.
  Confidence 85 · logic · Fix: treat skipped as "no build this night" (or skip the whole release-readiness chain when needs.nightly.outputs.skip == 'true'). Do not count it as a failed gate.

• Text from issues any GitHub user can file reaches the public wiki unescaped and unreviewed — xtask/src/release_readiness.rs:253-291, xtask/src/release_readiness.rs:390-395, .github/workflows/nightly.yml:1826-1858, .github/workflows/nightly.yml:1916-1925

  • .github/ISSUE_TEMPLATE/bug_report.md:11 auto-applies labels: bug, so any GitHub user controls entries in known_issues.
  • The raw titles go into table.md as unescaped markdown. A probe rendered injected image and link markdown verbatim. That file is written to $GITHUB_STEP_SUMMARY and to the public Release-Readiness wiki page.
  • Titles and 240-character body excerpts are also the LLM prompt. At the pinned SHA, ai-inference joins the system prompt and the user prompt into one copilot -p string, so the prompt's "Hard rules" carry no more weight than injected text.
  • The model's response is published with cat, with no filtering. An attacker can therefore put links, a fake "GO" claim, or text that drops real blockers from "Known Issues" on the project wiki.
  • Command execution is not possible, because copilot-allow-tools is unset.

  Confidence 85 · architectural · Fix: escape titles or render issues as #NNN links only. Either keep the narrative as a run artifact or step summary, or feed the model only maintainer-authored inputs (commit subjects), never issue bodies.

• The verdict, gate assembly and table rendering are untested — xtask/src/release_readiness.rs:71-116, xtask/src/release_readiness.rs:321-400
  These mutations all left the 6 tests green (cargo test -p xtask release_readiness, 6 run):

  • nightly gate removed
  • E2E gate removed
  • verdict inverted
  • Build-row check broken
  • issue sort reversed
  • rfind→find in extract_pr_number
  • the excerpt boundary <=→<

  No test reaches run, render_table or collect_known_issues. That includes the GO/NO-GO logic the page exists to publish.

  Test verdicts:

  • All six tests are in a new module, so verdict (1) does not apply.
  • (2) The categorize tests catch the scope-filter, ! and suffix-strip mutations.
  • (2) extract_pr_number_reads_trailing_reference misses rfind→find.
  • (2) excerpt_… misses the boundary mutation.
  • (2) total_e2e_failures_reads_the_totals_row catches column-index mutations.
  • (2) The defaults-to-zero test pins the defect above.

  Confidence 100 · logic · Fix: extract the gate/verdict computation into a pure function and test it, including skipped and cancelled results and a missing report. Snapshot-test render_table.

• The wiki "degenerate-output guard" can never trip — .github/workflows/nightly.yml:1903-1909, .github/workflows/nightly.yml:1866-1868
  The guard greps table.md for | Area | Evidence | Status |, but render_table writes that header unconditionally (release_readiness.rs:335). The publish job also only runs when release-readiness-nightly succeeded, so the file always exists. The comment "same degenerate-output guard" as publish-e2e-wiki is false: that guard catches an empty consolidation and is covered by wiki_publish_guard_stops_on_the_generators_empty_output in xtask/src/workflow_contract.rs. The new job has no contract test, unlike publish-e2e-wiki's nightly_publishes_the_consolidated_matrix_to_the_wiki. Together with the first finding, an empty E2E night publishes GO.
  Confidence 85 · logic · Fix: make the guard test real evidence (e.g. that a Total row was parsed, carried as a field in facts.json). Add a workflow-contract test like the one #590 added.

• The Strix Windows nightly runner move is bundled in, unmentioned, and incomplete — .github/workflows/nightly.yml:917-926, .github/workflows/nightly.yml:1115-1118, docs/ci-hardware-testing.md:60-74

  • Out of scope: commit 51d125a moves e2e-gpu-nightly-strix-windows from native to devlab-dispatch, a behaviour change unrelated to the release-readiness feature (AGENTS.md §11: one logical change per PR).
  • Docs left stale: docs/ci-hardware-testing.md still says both nightly Strix lanes "still pin the native label" and that moving them "is a separate, not-yet-made decision" (AGENTS.md §5 docs-sync).
  • Comment left stale: the moved job still says "The persistent runner workspace keeps those paths valid across scenarios and subsequent runs". That is false on the fresh-per-job pool, and the per-PR twin in e2e-selfhosted.yml says "RUNNER_WORKSPACE is fresh every job".
  • No contract test: no test pins the moved lane's labels. The WSL lane has nightly_wsl_lane_shares_the_per_pr_pool_labels.
  • Contradicted commit claim: the message says "no registered runner carries native anymore", but e2e-gpu-nightly-strix (nightly.yml:716) still pins native.

  Confidence 85 · architectural · Fix: move this to its own PR, with the doc, comment and contract-test updates.

• Three commits use a placeholder identity and fail the required signature gate — commits 49070b1, e654b4c, 7235dd9
  These three are authored, committed and signed off as test <test@example.com>. CI "Commit signatures + sign-off" fails on exactly these three ("signature is not GitHub-Verified"). AGENTS.md §2 requires the employer identity and a valid DCO sign-off. The squash body would also carry the bogus Signed-off-by.
  Confidence 100 · mechanical · Fix: re-author and re-sign these commits under the real identity.

• "Test (affected crates)" fails — CI on 7235dd9
  It fails at cargo nextest run --locked -p rocm-core --lib --features e2e-test-hooks with "the package 'rocm-core' does not contain this feature". The merge-base 34fbdfe is 24 commits behind the base tip, which added that feature (#510). The cause is the stale base, not this code, but the required check is red.
  Confidence 100 · mechanical · Fix: rebase onto the current base and re-run.

• The description claims "no new secret — uses permissions: models: read", and the code does the opposite — .github/workflows/nightly.yml:1786-1800, .github/workflows/nightly.yml:1836-1840
  The narrative job needs a new secrets.COPILOT_PAT (a Copilot-licensed PAT, per its own comment) and declares only contents: read. Nothing in the diff has models: read. A maintainer reading the description would not know a new PAT must be provisioned.
  Confidence 100 · mechanical · Fix: correct the description. Also document the secret and its minimum scope.

• The description omits work the change does — PR description
  It does not mention:

  • the Strix Windows runner-label move;
  • the comment rewrites in e2e-wsl-nightly;
  • the new COPILOT_PAT dependency;
  • the unpinned npm install.

  Its "degenerate-output guard" claim is not delivered (see above).
  Confidence 85 · mechanical · Fix: narrow the PR to the feature and update the description to match the code.

Non-blocking

• Unpinned npm install -g @github/copilot runs with COPILOT_PAT in the environment — .github/workflows/nightly.yml:1818, .github/workflows/nightly.yml:1836-1840
  Each night installs the latest version with no version pin, and the job holds a secret. This is the same supply-chain reasoning as AGENTS.md §6's action pinning. Separately, the checkout at :1805 keeps its credentials, which this job does not need.
  Confidence 85 · architectural · Fix: pin @github/copilot@x.y.z, and set persist-credentials: false on that checkout.

• actions/ai-inference pin comment is # v3, not # vX.Y.Z — .github/workflows/nightly.yml:1836
  The SHA matches upstream's v3 tag, which is the release name. No v3.x.y tag exists, so §6's comment form cannot be met literally. Note that v3 is the kind of tag upstream re-points.
  Confidence 70 · mechanical · Fix: keep the SHA. Note in the comment that no patch tag exists, or wait for one.

• always() where !cancelled() fits — .github/workflows/nightly.yml:1746, .github/workflows/nightly.yml:1801
  Both jobs run after a workflow cancel, and the narrative job uses the PAT when it does. The publish job is already gated with !cancelled().
  Confidence 85 · mechanical · Fix: use !cancelled().

• Commit classification misses user-facing changes without saying so — xtask/src/release_readiness.rs:181-224
  These are dropped:

  • ticket-prefixed subjects such as EAI-7404: make runtime activation and rollback transactional (#424) (32 of the 374 commits on the base tip);
  • capitalised Fix … / Feat: and spaced feat (cli):;
  • Revert "…" subjects, so the reverted feat still lists as new.

  extract_pr_number also takes the last (# anywhere in the subject, not only a trailing one, despite its doc. Probe: fix: handle (#12) refs renders … (#12) twice. The table never reports how many commits went unclassified, and --limit 100 caps the issue list while the header says "(N open)".
  Confidence 85 · logic · Fix: match the type case-insensitively, report the unclassified count, anchor the PR match to the end of the subject, and note any truncation.

• The two wiki publish jobs can race — .github/workflows/nightly.yml:1880-1882
publish-e2e-wiki and publish-release-readiness-wiki push to the same wiki repo in the same run. They use different concurrency groups and neither retries, so a non-fast-forward rejection fails one of them.
  Confidence 70 · logic · Fix: share one concurrency group, or pull --rebase and retry on push.

• The prompt contradicts itself — .github/prompts/release-readiness-narrative.txt:25-35
  Section 3 asks the model to highlight "failing gates", and the hard rules forbid "any pass/fail/status judgment".
  Confidence 70 · mechanical · Fix: allow restating failing_gates verbatim and forbid only the model's own verdict.

• Commit history would put false or noisy claims into the squash body — commits 834ae24, e654b4c, 300abb0, cc75a79
  834ae24 ("temporarily narrow") and its revert are noise. 300abb0's "switch … to gpt-4.1" is undone by cc75a79, and the two give different reasons for the model choice. 51d125a's "no registered runner carries native anymore" is contradicted by the Linux nightly lane, as noted above.
  Confidence 85 · mechanical · Fix: squash or reword these before merge so the squash body describes the final state.

Decisions for the author

• Overlapping "release readiness" name — non-blocking-improvement
  scripts/release_readiness.py is already the release gate under that name: AGENTS.md §8, docs/release-trust.md, and nightly.yml:15,154,246,294. The new "GO" ignores what that gate checks:

  • publish-nightly;
  • the production-trust gate state;
  • clippy, smoke, and the required GPU-lane set.

  A different name ("nightly health / release-notes preview"), or listing the unchecked gates on the page, would stop readers mistaking it for the gate.

• New xtask module vs. extending crates/e2e-report — tradeoff
  e2e-report already owns per-platform PASS/FAIL and expected-vs-actual reconciliation. Having it emit a machine-readable verdict would remove the scraping of rendered markdown, which is the root of the first finding. A separate module and page keep the release-preview concern apart from the support matrix. Both are reasonable; confirm the choice was deliberate.

• Release baseline includes pre-release tags — tradeoff
  git describe --match 'v*' matches -rc and -preview tags, so "since last release" can mean "since the last RC". It resolves to v0.2.0 today.

Positive signals

• The verdict, counts and PR numbers are computed outside the LLM step, and the publish job tolerates a missing narrative.
• The GITHUB_OUTPUT heredoc uses a random delimiter for content holding untrusted data, and the LLM response reaches the shell through env:, not ${{ }} interpolation.
• ai-inference is used without copilot-allow-tools, so injected text cannot run commands.

Deployment notes

• Needs a new repository secret, COPILOT_PAT: a Copilot-licensed PAT. Its minimum scope is undocumented.
• Creates and overwrites a new Release-Readiness wiki page nightly. The wiki must already be initialised.
• Moves the nightly Strix Windows lane onto the DevLab Dispatch pool, which adds load to the pool the per-PR lanes use. Commit 834ae24's message reports this lane hanging to its 90-minute timeout there.

What this covered

All four changed files read completely (.github/prompts/release-readiness-narrative.txt, .github/workflows/nightly.yml, xtask/src/main.rs, xtask/src/release_readiness.rs), plus crates/e2e-report consolidation, xtask/src/workflow_contract.rs, e2e-selfhosted.yml, docs/ci-hardware-testing.md, scripts/release_readiness.py, and ai-inference's source at the pinned SHA. Reviewed against prw-base, merge-base 34fbdfe…7235dd94 (merges cleanly with the base tip). What ran: cargo test -p xtask release_readiness (6 tests ran), cargo clippy -p xtask --all-targets -D warnings (clean), and 16 single-branch mutations plus probe inputs. All four passes ran, fanned out to independent workers alongside a design pass.

Did not run:

• Signature verification locally (no keys); CI's result was used instead.
• actionlint (not installed).
• The xtask coverage measurement (the CI coverage check is green).
• Confirmation that the claude-sonnet-5.5 slug is valid, or of the claim that the native Windows runner is unregistered.

"E2E tests (MI300X)" was still queued.

Not reconciled against any PR discussion here.

[juhovainio]

Thanks for the thorough review — addressed everything below. Branch was rebased onto current main and force-pushed; the 3 misauthored commits and the squash-noise pairs no longer exist in history.

Blocking, addressed in bab831ad

• Fail-open GO verdict — total_e2e_failures now returns Option<u64>; a missing/unparseable **Total** row is None and is surfaced as the failing gate "E2E (report unreadable)", not silently read as zero failures.
• False NO-GO on skipped nights — "skipped" now passes the gate the same as "success" for both nightly_result and windows_result.
• Unescaped issue titles reaching the wiki — added escape_markdown(), applied to every known-issue title before it reaches render_table.
• Untested verdict/gate/table logic — extracted a pure assemble_facts(...) (no git/gh I/O) out of run, with direct unit tests for all-success, unreadable-E2E, skipped-windows, and failed-windows cases. Added a render_table test asserting the header-omission and escaped-title behavior.
• Dead wiki publish guard — render_table now omits the | Area | Evidence | Status | header when E2E evidence is unreadable, so publish-release-readiness-wiki's existing grep guard is a real signal. Added release_readiness_wiki_publish_guard_stops_on_unreadable_e2e_evidence in xtask/src/workflow_contract.rs, which runs the actual guard snippet from nightly.yml against both degenerate and well-formed render_table output.
• Bundled Strix Windows runner-pool move — split into its own PR: fix(nightly): run Strix Halo Windows E2E on the devlab-dispatch pool #682 (includes the docs/ci-hardware-testing.md sync).
• 3 commits with a placeholder identity — re-authored as Juho Vainio <juho.vainio@amd.com> with proper DCO sign-off during the rebase.
• Stale merge-base / failing required check — rebased onto current main.
• Inaccurate secrets claim in the description — corrected: the narrative job uses the COPILOT_PAT secret, not permissions: models: read.
• Description omitted scope — rewritten to reflect the final diff and list the fixes made in response to this review.

Non-blocking, addressed in 3eb77057 / 367acd35

• Pinned npm install -g @github/copilot to 1.0.97.
• Added persist-credentials: false to the release-readiness-nightly / release-readiness-narrative checkout steps.
• always() → !cancelled() on both jobs.
• Prompt self-contradiction (section 3 vs. the hard-rule forbidding pass/fail judgments) — reworded to ask for evidence, not a verdict.
• Noisy commit history (834ae24/e654b4c, 300abb0/cc75a79) — dropped the net-zero pair, squashed the model-slug fix into one commit.

Not addressed in this PR (left as follow-ups)

• Commit-classification completeness (ticket-prefixed subjects, Revert "…" handling, case-insensitive type matching, reporting an unclassified count) — real gaps, but a separate change from this review's blocking findings; happy to file a follow-up if useful.
• The two wiki-publish jobs' race potential — pre-existing pattern shared with publish-e2e-wiki, out of scope here.
• actions/ai-inference's # v3 pin comment not matching the vX.Y.Z convention — no patched tag exists upstream to pin to.

Naming overlap with scripts/release_readiness.py

Filed ROCMAI-568 for this feature and cross-referenced the existing ROCMAI-443 (the unrelated Python signing gate) in its description, so the naming overlap is documented rather than silently ambiguous. Elected to keep the cargo xtask release-readiness name rather than rename the shipped command.

feedback addressed

[rominf]

🔴 Automated review · pr-review-watcher · 367acd3

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.

Round 2 — full review again: the round-1 head (7235dd9) no longer exists after the force-push. Each finding is marked new or standing. Round 1's change request was dismissed by the author. This change request restates every objection that still holds.

Round-1 objections settled this round:

• Skipped nights publish a false NO-GO: resolved. The review did not re-find it. When asked afterwards, the reviewer confirmed assemble_facts now passes skipped, and a test pins that.
• Strix Windows runner move: resolved. It is gone from the diff (moved to #682).
• Placeholder commit identity: resolved. All 9 commits are GitHub-Verified under the author's own identity.
• "Test (affected crates)" failure: resolved. It is green after the rebase.
• Description scope and secrets claim: resolved. The rewritten description was not re-found as a mismatch.
• Dead wiki guard: resolved. The guard now trips. What it now causes is the new finding about NO-GO never being published.
• The untrusted-issue-text objection still holds; it is restated at the end of Blocking.
• Unpinned npm install (non-blocking in round 1): mostly resolved. The CLI is now pinned to an exact version, no secret is present at install time, and the checkout no longer keeps credentials. One gap remains: the CLI's dependency detect-libc ^2.1.2 still floats. It is loaded inside the step that holds COPILOT_PAT, so a bad future release would run with that token in reach. Pinning it, --ignore-scripts, or a committed lockfile would close the gap. Not gating.

Self-hosted lanes not reporting at this head: "E2E tests (MI300X)" and "E2E tests (Strix Halo, Ubuntu) (release)" are queued, and "E2E tests (Strix Halo, Ubuntu) (nightly)" is in progress. That is missing evidence, not a defect. Every hosted check passed.

Review — needs work

Full review of the whole change.
The description names ROCMAI-568, which cannot be retrieved from here. The PR adds cargo xtask release-readiness and three nightly jobs that publish a GO/NO-GO page to the repo wiki.

Blocking

• standing (narrowed) — The E2E gate gives GO when most platforms reported nothing — xtask/src/release_readiness.rs:347-353, xtask/src/release_readiness.rs:126-130, .github/workflows/nightly.yml (release-readiness-nightly needs:)
  • total_e2e_failures reads only the Fail cell of the consolidated **Total** row.
  • consolidated_summary_markdown_from_reports (crates/e2e-report/src/consolidated.rs:1032-1076) adds up only the reports that were actually downloaded.
  • The GPU lanes are continue-on-error. A lane that dies before uploading report.json (preflight failure, timeout, runner loss) simply drops out of the Total. A platform with zero scenarios (EMPTY) adds 0.
  • The job never looks at the lane results or at needs.e2e-report-nightly.result. So one passing platform out of six gives GO, and the table says "0 failure(s) across all platforms".
  • This contradicts the module's own rule for e2e_fail_total ("treated as a failing gate rather than silently reading as zero failures"). It also contradicts the description's claim that "E2E evidence that's missing … now fails the gate explicitly". Only a missing Total row fails it.
  • Confidence 85 · logic · Fix: gate on every platform row's Status being PASS and on an expected platform set, or pass the lane job results in as gates.
• new — A NO-GO is never published, so the wiki keeps showing the previous verdict — xtask/src/release_readiness.rs:386-391, .github/workflows/nightly.yml (publish-release-readiness-wiki guard and its result == 'success' condition), xtask/src/workflow_contract.rs (release_readiness_wiki_publish_guard_stops_on_unreadable_e2e_evidence)
  • When E2E evidence is unreadable, render_table leaves out the | Area | Evidence | Status | header on purpose. The guard then exits 0 without publishing, and the new contract test requires exactly that.
  • Every other failure has the same effect, because the publish job only runs when release-readiness-nightly succeeded. Examples: the artifact download fails, git describe fails, or gh fails.
  • An empty report is a realistic case: e2e-report-nightly redirects into support-matrix.md before cargo runs, so a cargo failure leaves an empty file.
  • On exactly the nights when readiness is unknown, the page keeps the last GO. Keeping the last good page makes sense for the support-matrix page this job copies. For a verdict page it fails in the wrong direction.
  • Confidence 85 · logic · Fix: publish a NO-GO / "evidence unavailable" page in these cases. Keep the guard only for a truly empty file, keyed on a line render_table always emits.
• standing (narrowed) — Several of the new gate checks have no test that fails when they break — xtask/src/release_readiness.rs:120, :128, :397, :406
  • The reviewer made four changes at once and all 14 release_readiness tests still passed:
    • the Some(n) if n > 0 E2E check made to never fire
    • the nightly_result check removed
    • the Build cell forced to ✅
    • the E2E cell forced to ✅
  • No test covers E2E failures > 0 giving NO-GO, a Linux nightly failure giving NO-GO, or either ❌ cell. The description says assembly was extracted so "the GO/NO-GO logic is directly unit-tested". That is only partly true.
  • Confidence 100 · mechanical · Fix: add one test for each case, so that each of the four changes above fails a test.
• standing — round 1 rated this non-blocking. This change request does not rest on it, and it gates nothing on its own. — The actions/ai-inference pin is commented # v3, not # vX.Y.Z — .github/workflows/nightly.yml (Draft narrative step)
  • AGENTS.md §6 requires a full SHA "with a trailing # vX.Y.Z comment, never a moving tag".
  • Checked via gh api: 2c43c91… is the lightweight major tag v3. No v3.x.y exists yet, so neither the reader nor Dependabot can tell which release is pinned.
  • Confidence 85 · mechanical · Fix: pin to the latest semver release (v2.1.1), or get a maintainer to accept the exception explicitly and note it in the comment.
• standing (round 1) — Text from outsiders' issues still reaches the public wiki, and the escaping fix can be bypassed — xtask/src/release_readiness.rs (escape_markdown, collect_known_issues), .github/workflows/nightly.yml (Read facts for prompt, Draft narrative, Push page to the wiki)
  This round's blind review did not report it as blocking: it rated the narrative path non-blocking at 70. It was put to the reviewer afterwards as a standing objection, with the author's defence ("added escape_markdown(), applied to every known-issue title"). The verdict below comes from that follow-up question and is anchored evidence, not a blind finding. Most of the objection still holds:
  • Titles: the escaping can be bypassed. escape_markdown does not escape \. A title written as \[click\](https://evil.example) comes out with doubled backslashes, and under CommonMark that renders as a live link in table.md. The reviewer ran escape_markdown on it and read the result against the escape rules, but could not render it (no renderer installed). Bare URLs also pass through and are autolinked. Raw HTML is neutralised.
  • The narrative path is unchanged. Issue titles and 240-character body excerpts are still the LLM prompt. At the pinned SHA the system prompt and the user prompt are one string. narrative.md is still cat'd onto the wiki unfiltered. An outsider can therefore try, every night and unattended, to get links, prose implying GO, or a "Known Issues" list that drops real blockers onto the official project wiki.
  • Limits: whether it works depends on the model following the injected text. The mechanical verdict line and the raw issue list above it cannot be changed. There is no command execution.
  • Unverified: whether GitHub honours the bug template's automatic bug label, since a licence comment comes before it. If it does not, a triager applying bug is the gate. That narrows who can do this but does not remove it.
  • Fix: escape \ first, or render issues as #NNN links only. Either keep the narrative as a run artifact or step summary, or feed the model only maintainer-authored inputs, never issue bodies.

Non-blocking

• new — Untrusted issue text reaches the LLM, and its output is published to the wiki unescaped — xtask/src/release_readiness.rs:288-327, .github/workflows/nightly.yml (Read facts for prompt, Push page to the wiki)
  • Open bug issues come from outside contributors (the bug template asks for labels: bug). Their titles and 240-character body excerpts are inlined as the prompt.
  • At this SHA the action joins the system prompt and the user prompt into one string, so the prompt's rules can be overridden.
  • narrative.md is cat'd onto the official wiki. Issue titles are escaped in the table, but the narrative is not escaped at all.
  • The token and permissions are scoped well, so this is limited to content: links, images, or prose contradicting the mechanical verdict.
  • Confidence 70 · architectural · Fix: restrict input to issues from maintainers, or drop the body excerpts. Strip links and images from the narrative before publishing.
• new — Commit subjects are written to the wiki unescaped — xtask/src/release_readiness.rs:419, :422. Squash subjects are contributor-written PR titles, while issue titles already go through escape_markdown. Confidence 70 · logic · Fix: escape e.subject too.
• new — "👍 N" counts every reaction type — xtask/src/release_readiness.rs:321, :446. It sums users.totalCount over all reactionGroups, including 👎 and confused. Confidence 85 · logic · Fix: filter to THUMBS_UP, or label the number "reactions".
• standing (round 1) — The known-issue count is silently capped at 100 — xtask/src/release_readiness.rs:299-300, :432-435. With --limit 100, the header shows "(100 open)" once there are more than 100 open bugs. Confidence 70 · logic · Fix: paginate, or print "100+" when the cap is hit.
• new — ! counts as a breaking change for any commit type — xtask/src/release_readiness.rs:216, :236. Unscoped chore!:, docs!:, ci!: and test!: subjects land in the end-user "Breaking Changes" list. The scope filter does not catch them. Confidence 70 · logic · Fix: count as breaking only for user-facing types.
• standing (round 1) — A PR suffix without a leading space is printed twice — xtask/src/release_readiness.rs:229-233. For fix: x(#12), pr = 12 is extracted, but only " (#12)" is stripped, so it renders as x(#12) (#12). Confidence 70 · mechanical · Fix: strip the exact span that extract_pr_number matched.
• standing (round 1) — The baseline tag pattern also matches prerelease tags — .github/workflows/nightly.yml (since_tag=$(git describe --tags --abbrev=0 --match 'v*')). The repo has v0.2.0-rc.1, v0.1.0-preview.1 and similar tags. Once an RC is tagged on main, "since the last release" becomes "since the RC". Confidence 70 · logic · Fix: add --exclude 'v*-*'.
• new — The new page and its secret are not documented — docs/ci-hardware-testing.md (the publish-e2e-wiki paragraph). The doc covers the E2E wiki page and its one-time setup. Nothing under docs/ mentions the Release-Readiness page, the COPILOT_PAT prerequisite, or cargo xtask release-readiness. Confidence 70 · mechanical · Fix: extend that paragraph.

Decisions for the author

• Name collision with the existing release gate — tradeoff
  • scripts/release_readiness.py (documented in docs/release-trust.md and docs/testing.md, and referenced in nightly.yml's env comments) is the signing/trust release gate.
  • cargo xtask release-readiness is a nightly summary with its own GO/NO-GO.
  • Sharing the name is convenient. The cost is that readers may take one for the other, especially since both say "release readiness".
• The Build row shows ✅ on a no-op night — non-blocking-improvement
  • On a skip night, nightly succeeds without building and windows-nightly is skipped.
  • The row "Build (Linux + Windows nightly) ✅" reads as "built and passed". "Not built (no new commits)" would be more accurate.
  • Accepting skipped for nightly_result does nothing, because that job has no if:.

Positive signals

• The verdict, counts and PR numbers are computed without the LLM, and the narrative job is continue-on-error with an optional artifact download. A broken LLM step cannot change the table.
• The new contract test runs the real bash guard from nightly.yml against real render_table output instead of a pinned string. Removing exit 0 from the guard made it fail.
• The LLM response reaches the shell only through env: and printf '%s\n', never inline ${{ }}. GITHUB_OUTPUT uses a random heredoc delimiter.
• Each job has narrow permissions (contents: read / issues: read), and both checkouts use persist-credentials: false. Every other new action pin was verified via gh api to match its # vX.Y.Z comment.
• A failing git log or gh issue list aborts the run instead of producing an empty list.

Deployment notes

• Requires a COPILOT_PAT repo secret holding a Copilot-licensed PAT. Without it the narrative job fails (non-blocking) and the page has no narrative.
• Requires at least one non-prerelease v* tag reachable from main. Without one, git describe fails and the page is never published.
• Uses the existing repo wiki, which must already be initialized (same prerequisite as publish-e2e-wiki).

What this covered

• Read every changed file completely against 21be0034…367acd35: xtask/src/release_readiness.rs, xtask/src/main.rs, xtask/src/workflow_contract.rs, .github/workflows/nightly.yml and .github/prompts/release-readiness-narrative.txt. Also read the jobs these depend on in nightly.yml, the consolidated-report generator in crates/e2e-report, the bug issue template, and the docs that mention the nightly wiki.
• Two read-only workers covered the code passes (xtask module; workflow + contract test). The reviewer ran the agent-instruction, history and code-comment passes and the design questions itself, so those are not independent.
• CI at this head: every hosted check passed, including commit signatures/DCO. Three self-hosted GPU lanes (MI300X, Strix Halo Ubuntu nightly and release) were still queued or running. That is missing evidence, not a defect.
• Did not run:
  • prior-changes pass — no access to earlier review threads, by design.
  • workflow end-to-end and actionlint — not runnable here.
  • ROCMAI-568 — no tracker access.
• Not checked: whether Copilot CLI 1.0.97 accepts the claude-sonnet-5.5 slug.
• Not reconciled against the PR's discussion or earlier reviews. Standing objections were settled separately, as noted above.

[juhovainio]

Addressed the blocking findings from @rominf's review (5522757577):

1. E2E lane results not gating the verdict — added a repeatable --e2e-lane-result NAME=RESULT flag threaded from nightly.yml (one per GPU/WSL lane job's own needs.<job>.result) through to assemble_facts. A lane that dies before uploading its report.json (preflight failure, timeout, runner loss) now fails the verdict instead of silently reading as zero E2E failures.
2. Table header hidden on unreadable E2E evidence — render_table no longer suppresses the header in that case; unreadable evidence is itself a NO-GO signal and should be visible, not hidden. Updated workflow_contract.rs's wiki-publish-guard test to match (only a truly missing/empty table.md stops the publish now).
3. Markdown injection via commit subjects — commit subjects are now escaped before being interpolated into the rendered table.
4. actions/ai-inference@...# v3 looking like a moving major-tag pin — verified via the GitHub API that this SHA is exactly what the v3 tag (a genuine, non-prerelease release) points to, and that main has since moved past it. actions/ai-inference tags its releases as bare majors rather than vX.Y.Z; left the SHA pin as-is and added a comment explaining why it's a frozen release, not an AGENTS.md §6 violation.
5. Only THUMBS_UP reactions should count toward issue ranking — fixed, plus bumped the gh issue list limit from 100 to 1000 so older issues aren't silently dropped from consideration.

Also fixed the two non-blocking findings: escape_markdown now doubles a literal backslash, and categorize() no longer treats a trailing ! as breaking for commit kinds other than feat/fix, nor double-appends the PR suffix.

On the two "decisions for the author" items, left as-is deliberately:

• The naming overlap with scripts/release_readiness.py's own git describe calls is a pre-existing convention in this codebase, not something introduced here — didn't want to rename either in this PR's scope.
• Skipping a dedicated nightly-Build row distinct from the Linux/Windows build gates is intentional: the verdict already treats either build failing as NO-GO via nightly_result/windows_result, so a separate row would be redundant with the existing gates.

cargo fmt, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace are all clean (one tar-launch test is flaky under full-parallel runs, confirmed passing in isolation — unrelated to this change).

Commits: 68d7cee, f389e2f, 7cfce6a.

Addressed all blocking and non-blocking findings in 68d7cee, f389e2f, 7cfce6a (see PR comment for details). Re-requesting review.

[rominf]

🔴 Automated review · pr-review-watcher · 1cb356d

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.

Round 3 — full review again: the round-2 head (367acd3) no longer exists after the force-push. Each finding is marked new or standing. Round 2's change request was dismissed by the author. This change request restates every objection that still holds.

Round-2 objections settled this round:

• E2E gate gives GO when lanes upload no report: still holds. It was re-found blind (first item under Blocking). The new --e2e-lane-result gate does not close it. Commit 40f5d62's message also says "a lane that dies before producing a report still fails the verdict", and that sentence would land on main with the squash.
• New gate checks have no test that fails when they break: still holds. It was re-found blind with a new set of surviving mutations (fourth and third items under Blocking).
• NO-GO never published: partly resolved. The blind review reported only a related non-blocking item. When asked afterwards (anchored evidence, not a blind finding), the reviewer confirmed:
  • An empty or unreadable E2E report now publishes a NO-GO page. That part is resolved.
  • Any failure of release-readiness-nightly still publishes nothing, and the wiki keeps the last page, which may say GO. The causes are a missing report artifact or file, git describe, gh issue list, or the xtask build. This part is restated as the last item under Blocking.
• Untrusted issue text reaches the public wiki: partly resolved.
  • The narrative half still holds and was re-found blind (second item under Blocking).
  • The title half was put to the reviewer afterwards (anchored). The backslash bypass is fixed: escape_markdown now doubles \, and a test catches removing that. Commit subjects now go through the same escaping.
  • Bare URLs in titles and subjects still pass through unescaped and would likely be autolinked. This was not rendered to confirm. It is folded into the narrative item rather than gated on separately.
• # v3 pin comment (not gating): still holds. It was re-found as non-blocking below.
• Copilot CLI's floating detect-libc ^2.1.2 dependency in the COPILOT_PAT step (not gating): not re-checked this round.

CI at this head (from the driver, compared against main's own runs):

• E2E tests fails with the same dashboard-quit and full-model-name scenarios on main's latest CI run (6186f15), so it does not gate.
• E2E tests (Strix Halo, Windows, current) failed on a 404 downloading an external llama.cpp release. The same lane also failed on main at c7a0d65. This PR does not touch that path.
• The consolidated report failed downstream of the lanes, and MI300X was cancelled.
• The method lists these under Blocking. This change request does not rest on them; re-running after a rebase should settle them.

Review — needs work

Full review of the whole change.
The description names Jira ROCMAI-568, which could not be retrieved from here (no tracker tool). The description's account of the change is out of date in places (see Blocking).

Blocking

• standing — The new per-lane E2E gate can never fire for a failed lane, and it leaves out the W7900 lane — .github/workflows/nightly.yml:1701-1710, .github/workflows/nightly.yml:1747-1752, xtask/src/release_readiness.rs:134-144, xtask/src/main.rs (--e2e-lane-result help), docs/ci-hardware-testing.md:199-203

  • Why it never fires: every lane job sets continue-on-error: true, so needs.<lane>.result reads success even when the lane failed. Nightly run 39103436218 shows this:
    • Both Strix Halo Windows lane jobs concluded failure.
    • In the same run, e2e-report-nightly's annotation says: lane e2e-gpu-nightly-strix-windows ran (needs result: success) but uploaded no e2e-gpu-strix-windows-release-report.
    • Run 39554529729 shows all 7 lanes reporting "success" in NEEDS even though three of them failed.
  • Consequence: a lane that dies before uploading drops out of the **Total** row, and the verdict can still read GO. The code comment, the --e2e-lane-result help and the docs sentence all say this case fails the gate. The code governs here: those statements describe a gate that does not work.
  • W7900: separately, e2e-gpu-nightly-w7900 is in e2e-report-nightly's needs and LANE_REPORTS, but not in the new job's needs or in its lane list.
  • The signal that does work: needs.e2e-report-nightly.result. That job's "Check every lane that ran uploaded its report" step exists for exactly this case. Release readiness already lists the job in needs but never gates on its result.
  • Confidence 100 · logic · Fix: gate on needs.e2e-report-nightly.result, or reuse its artifact-list check. Drop the --e2e-lane-result plumbing or correct what it claims. Add a contract test that ties any lane list to LANE_REPORTS.

• standing (narrative half) — The link-stripping sed does not stop what its comment says it stops, and its input is attacker-controllable — .github/workflows/nightly.yml (publish step, sed -E 's/!?\[([^]]*)\]\([^)]*\)/\1/g')

  • The comment says the narrative "can't smuggle a redirect or tracking pixel onto the public wiki page".
  • I ran the sed. These pass through unchanged:
    • <img src="https://example.com/p.png">
    • reference-style [a][1] with [1]: https://example.com/x
    • bare URLs and autolinks
  • Nested [outer [inner](https://a.example)](https://b.example) comes out as a live link: outer [inner](https://b.example).
  • Who can inject: .github/ISSUE_TEMPLATE/bug_report.md sets labels: bug, so any GitHub user's issue title plus a 240-character body excerpt goes into the LLM prompt. The LLM output is then published to the public wiki.
  • Confidence 100 · logic (security) · Fix: render the narrative as plain text (escape it, or put it in a fenced block), or allowlist its output. At minimum, correct the comment so it does not claim protection that is not there.

• new — collect_known_issues_counts_only_thumbs_up_reactions never calls production logic — xtask/src/release_readiness.rs:816-828

  • The test re-implements filter(THUMBS_UP).map(..).sum() inline. The only thing it shares with production is the serde shape.
  • Replacing the production filter with "count all reactions", or reversing the sort, still passes. Both mutations were run.
  • Confidence 100 · mechanical · Fix: extract the RawIssue → KnownIssue mapping and sort into a pure function, and test that function.

• standing — Several branches the change introduces are not exercised by any test. Each mutation below was run alone against cargo test -p xtask release_readiness (24 tests) and survived:

  • The verdict headline. ## Release Readiness — ✅ GO vs ❌ NO-GO is never asserted; swapping them passes. (release_readiness.rs:406-413)
  • PR-number rendering. Dropping the (#N) suffix from rendered entries passes. (:452-459)
  • Treating skipped as passing for the nightly job and for lanes. Only the Windows twin is tested. (:123, :141)
  • The n > 0 boundary. Changing it to n > 1 passes. (:131)
  • escape_markdown's newline collapsing and its </> escaping. Removing either passes. (:394, :396)
  • The ends_with(')') guard in split_trailing_pr_reference. Removing it passes, and it is not equivalent: the len()-1 slice can then panic on a subject ending in a multibyte character. (:269)
  • Confidence 100 · mechanical · Fix: add an assertion for each.

• new — The PR description misstates the change

  • It says "the table header is omitted when E2E evidence is unreadable, so the guard can actually trip". The code does the opposite. render_table always emits the header (release_readiness.rs:416-427), and the new contract test asserts that unreadable evidence reaches the publish step.
  • It does not mention the per-lane --e2e-lane-result gate added in 40f5d62, which is the main gating change in the later commits.
  • It says the job mirrors the E2E wiki job's guards. publish-e2e-wiki also skips when e2e-report-nightly fails; this job does not.
  • Confidence 90 · Fix: update the description to match the code. Narrow it; don't add work to fit it.

• new — does not gate (see the CI note above) — Required CI checks are failing at 1cb356d

  • E2E tests (CI): dash-esc-menu-quit-row-confirms-when-serving regressed (TUI quit timeout).
  • E2E tests (Strix Halo, Windows, current): 404/403 errors downloading external llama.cpp and TheRock artifacts.
  • E2E consolidated report (self-hosted) is failing, and E2E tests (MI300X, current) was cancelled.
  • These failures are in code paths this PR does not touch, so they may be shared with main. I did not compare against main.
  • Confidence 100 · Fix: re-run or rebase once main is green.

• standing (round 2, narrowed), unverified blind this round — When release-readiness-nightly fails, nothing is published, so the wiki keeps the previous verdict — .github/workflows/nightly.yml (publish-release-readiness-wiki's needs.release-readiness-nightly.result == 'success' condition), xtask/src/release_readiness.rs:89-90, :332-337

  • This round's blind review rated a related item non-blocking at 75 (the second item under Non-blocking). The rest comes from the follow-up question and is anchored evidence.
  • The job fails outright, rather than producing a NO-GO page, when:
    • the E2E report artifact or file is missing (the ? at :89);
    • git describe fails;
    • gh issue list fails (:332-337), which runs before the E2E report is read, so a flaky issue listing alone suppresses the page;
    • the xtask build fails.
  • In each case the page keeps whatever the last successful night published, which may be GO, and nothing on the page marks it stale. For a verdict page that fails in the wrong direction.
  • Fix: publish a NO-GO / "evidence unavailable" page when the job cannot reach a verdict. A fallback publish step on failure() would do it, as would a staleness line naming the run that produced the page.

Non-blocking

• new — The docs paragraph is inaccurate beyond the lane sentence — docs/ci-hardware-testing.md:199-211
  • It opens "On main", but release-readiness-nightly and release-readiness-narrative have no ref guard. They run on any branch dispatch and use the COPILOT_PAT secret there.
  • It says "same … guards as publish-e2e-wiki", which is not true (see the description finding).
  • Confidence 85.
• new — A missing E2E report publishes nothing, and the wiki keeps the previous verdict, possibly GO — xtask/src/release_readiness.rs:89-90
  • A missing artifact or file fails the job via read_to_string(...)?. Only a report that is present but unparseable becomes NO-GO.
  • The comment at :416-420 ("a NO-GO page must still publish") holds only for that second case.
  • Confidence 75.
• new — The status table hides which gate failed — xtask/src/release_readiness.rs:421-441
  • failing_gates is never rendered in table.md.
  • When a lane is cancelled, the row reads | E2E | 0 failure(s) across all platforms | ❌ |: the evidence says zero failures while the status is red.
  • Confidence 85.
• standing — The actions/ai-inference pin comment is # v3, not the # vX.Y.Z form AGENTS.md §6 requires — .github/workflows/nightly.yml (Draft narrative step)
  • The SHA does match the lightweight v3 tag, and no v3.x.y tag exists, so this is a reasoned exception.
  • But the comment's statement that the action "tags its releases as bare majors" is wrong: v2.x.y and v1.x.y semver tags exist.
  • Confidence 85.
• new — extract_pr_number is a #[cfg(test)] wrapper that only forwards to split_trailing_pr_reference — xtask/src/release_readiness.rs:281-284. The tests could call the real function directly. Confidence 80.
• new — The --since-tag help text is out of step with the workflow — xtask/src/main.rs. The help cites --match 'v*', but the workflow also passes --exclude 'v*-*'. Confidence 85.

Decisions for the author

• LLM-drafted prose from public issue text, published to a public wiki — tradeoff. The prose makes the page more readable. The cost is a narrative that any issue filer can steer, which the prompt cannot fully prevent. The alternative is the mechanical table alone, or a narrative restricted to commit data. Confirm that this exposure is deliberate.
• Both wiki publish jobs push to the same wiki under different concurrency groups, with no retry — tradeoff. They seldom overlap in practice (seconds-long jobs, minutes apart). Overlap needs runner queueing, and the loser then fails non-fast-forward. A shared group such as nightly-wiki removes the race.

Positive signals

• Keeping the verdict and counts fully mechanical (assemble_facts), with the LLM only drafting prose, cleanly separates what is trusted from what is not.
• The new contract test runs the real publish guard, extracted from nightly.yml, against render_table's real output rather than a pinned string. Both guard mutations were caught.
• The facts are inlined through a with: expression with a random delimiter rather than through a shell, so there is no shell injection from issue titles.

Deployment notes

• The COPILOT_PAT repo secret must exist (a Copilot-licensed PAT), or the narrative job fails and the page shows its placeholder line.
• The wiki must already be initialised, the same prerequisite as publish-e2e-wiki.
• The @github/copilot@1.0.97 install and the claude-sonnet-5.5 model slug are pinned in run: and with:, so Dependabot will not bump them.
• A non-prerelease v* tag must be reachable from main, or the job fails before publishing. v0.2.0 satisfies this today.

What this covered

All 6 changed files were read completely, along with the existing nightly.yml lane jobs, e2e-report-nightly and publish-e2e-wiki, crates/e2e-report's consolidated markdown format, and the issue templates. Range: prw-base (6186f15)…1cb356de.

Ran two independent read-only workers: an xtask code pass with 27 single-branch mutations plus 2 workflow-guard mutations, and a workflow/docs/policy pass. I re-checked their key claims myself against run annotations and by running the sed.

• cargo test -p xtask release_readiness (24 tests) and cargo clippy -p xtask --all-targets -D warnings pass, as the worker ran them.
• The design questions (Step 7) were judged inline by me, not by independent workers.

Did not run:

• Prior-changes pass: would need other PRs' review threads.
• The new jobs end to end: no run at this head has exercised them.
• Copilot model slug and CLI version: not checked.
• Ticket: ROCMAI-568 is not retrievable here.

The PR's discussion was not reconciled here. Standing objections were settled separately, as noted above.

[juhovainio]

Addressed the latest review (id 5600395338) at 2ada5764. This review had no inline comments, so replying here at the top level.

Blocking

• Per-lane E2E gate never fires; W7900 missing — fixed. release-readiness-nightly now gates on needs.e2e-report-nightly.result (--e2e-report-job-result) instead of per-lane needs.<lane>.result, since every lane is continue-on-error: true and its own result can never read anything but success. e2e-report-nightly already fails when any lane in LANE_REPORTS (which includes W7900) ran but didn't upload a report, so gating on that job's result covers every lane, including future ones, for free. Dropped the --e2e-lane-result plumbing entirely rather than leaving dead flags around. Pinned by a new contract test (release_readiness_nightly_gates_on_the_e2e_report_job_not_individual_lanes) that asserts needs contains e2e-report-nightly and none of the 7 individual lane jobs.
• Link-stripping sed doesn't stop what its comment claims, and is attacker-controllable — fixed by switching from stripping to escaping: sed -e 's/\[/\\[/g' -e 's/\]/\\]/g' -e 's/</\&lt;/g' -e 's/>/\&gt;/g'. Ran this against the exact three bypass examples from the review (<img src=...>, reference-style [a][1], nested [outer [inner](url)](url)) — all render as inert escaped text now. Pinned by publish_release_readiness_wiki_escapes_markdown_link_and_html_syntax, which extracts the real sed line from the workflow and runs it via bash -c against those same three inputs. One accepted residual gap, called out in the PR description: GitHub's bare-URL autolinking isn't something sed-level escaping can prevent.
• collect_known_issues_counts_only_thumbs_up_reactions never calls production logic — fixed. Extracted the RawIssue → KnownIssue mapping and sort into a pure build_known_issues function; the test now calls it directly instead of reimplementing the filter/sort inline.
• Untested branches, including a real panic risk — fixed. Added assertions for: the GO/NO-GO headline, PR-number-suffix rendering, skipped-as-passing tested separately for the nightly build, the Windows build, and the E2E report job, the n > 0 vs n > 1 boundary, and escape_markdown's newline-collapsing/angle-bracket-escaping. Also added the one you specifically flagged as a real panic risk: split_trailing_pr_reference_does_not_panic_on_multibyte_text_trailing_a_mid_sentence_reference, confirming the ends_with(')') guard is load-bearing against a len()-1 mid-character slice panic.
• PR description misstates the change — rewritten. The header-omission claim was inverted (it's now described correctly: the header always renders, and the guard only trips on a table missing the header entirely). The per-lane gate is no longer mentioned since it no longer exists. The "mirrors publish-e2e-wiki's guards" line now explicitly says what's shared (branch/repo gate, concurrency group, degenerate-output guard) versus what's deliberately different (this job still runs and publishes an "evidence unavailable" notice when release-readiness-nightly fails outright, instead of skipping like publish-e2e-wiki does).
• Required CI checks failing at 1cb356d — unrelated to this diff; same dashboard-quit/download-infra flakes reproduce on main's own runs. Left unchanged, called out explicitly in the PR description.
• release-readiness-nightly failing outright publishes nothing, wiki keeps a stale (possibly GO) verdict — fixed. publish-release-readiness-wiki's guard loosened from == 'success' to != 'skipped', so it still runs when release-readiness-nightly fails. The artifact-download step is continue-on-error: true (the failed job may not have uploaded anything), and the push script now branches three ways: table missing → publish an explicit "EVIDENCE UNAVAILABLE" notice with run/commit links; table present but missing its header → warn and leave the wiki page unchanged; well-formed table → publish normally. Pinned by release_readiness_wiki_publish_guard_handles_missing_degenerate_and_well_formed_tables, which runs the real extracted guard against all three constructed inputs.

Non-blocking

• Docs paragraph inaccurate beyond the lane sentence — fixed, docs/ci-hardware-testing.md no longer claims main-only scope or guard-parity with publish-e2e-wiki.
• Missing (not just unparseable) E2E report publishes nothing — left unchanged, judgment call: a missing report file still fails the job via read_to_string(...)?, but the "evidence unavailable" fallback above already covers this at the publish step, so the wiki itself never goes stale or shows a wrong verdict silently — only the job's own logs say why the page didn't update.
• Status table hides which gate failed — fixed. render_table now lists the failing_gates reason strings beneath the table (e.g. E2E (lane report check: cancelled)), so a cancelled lane no longer renders as a contradictory "0 failure(s)" next to a red ❌.
• # v3 pin comment — fixed, corrected to note the action tags full semver releases alongside the floating major, not bare majors only.
• extract_pr_number test-only wrapper — left as-is; purely a test-ergonomics nit, not gating behavior.
• --since-tag help text out of step with --exclude 'v*-*' — fixed, help text now mentions both flags.

All fixes verified via cargo test -p xtask (349 tests), cargo clippy -p xtask --all-targets --all-features -- -D warnings, and cargo fmt -p xtask --check, all passing at 2ada5764.

Addressed in 2ada576 (see summary comment). All blocking findings fixed and pinned by new tests; non-blocking items fixed except two explicit judgment calls called out in the PR description.

[rominf]

🔴 Automated review · pr-review-watcher · 2ada576

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.

Round 4 — full review of the whole change at 2ada576 (not narrowed: some round-3 objections, such as the commit messages and missing tests, cannot be scoped to a file). Each finding is marked new or standing. Round 3's change request was dismissed by the author. This change request restates every objection that still holds.

Round-3 objections settled this round:

• Per-lane E2E gate never fires; W7900 missing. The blind review did not re-find it in this form. When asked afterwards (anchored evidence, not a blind finding), the reviewer confirmed:
  • Code: resolved. --e2e-lane-result is gone. The job now gates on e2e-report-nightly, whose LANE_REPORTS includes W7900. The blind review raised a separate new gap (only the release leg of each lane is checked, fourth item under Blocking).
  • Commit messages: still holds. It is restated as the last item under Blocking.
• Link-stripping sed lets links through; narrative built from outsiders' issue text: still holds. It was re-found blind in a new form: the escaping sed that replaced it never doubles \ (second item under Blocking).
• Thumbs-up test never calls production code: resolved. The blind review did not re-find it. Asked afterwards (anchored), the reviewer re-ran both named mutations against build_known_issues, and each now fails the test.
• Branches that survive mutation: still holds. It was re-found blind with a new set of surviving mutations (seventh item under Blocking). The round-3 specific set (headline, (#N), skipped, n > 0, newline/angle escaping, ends_with(')')) is now covered.
• Failed release-readiness-nightly publishes nothing, wiki keeps the old (maybe GO) page: resolved. The blind review did not re-find it. Asked afterwards (anchored), the reviewer confirmed that each listed cause now reaches the EVIDENCE UNAVAILABLE page. Its residue (no test pins the new != 'skipped' / !cancelled() conditions) is already part of the seventh item under Blocking.
• PR description misstates the change: still holds, in new places. It is folded into the first, second and fourth items under Blocking. On its own a description mismatch does not gate, because the squash body is built from commit messages.

CI at this head (from the driver, compared against main's latest CI run at 32834b2):

• windows-build-and-test fails on this PR's own new test, and passes on main. This gates (first item under Blocking).
• Test (affected crates) fails in preflight_runs_the_real_reclaim_assert_clear, a pre-existing test this PR does not touch. It passes on main, and the branch predates the main commit that addresses it, so a rebase should settle it. Does not gate.
• E2E tests fails with the same dash-gen-tps-expiry-boundary / dash-esc-menu-quit-row-confirms-when-serving regressions as main's own run. Does not gate.
• The actions/ai-inference pin item (sixth under Blocking) was rated non-blocking last round. Nothing new makes it a defect beyond what was known, so this change request does not rest on it.

Review — needs work

Full review of the whole change.
Ticket ROCMAI-568 is named but could not be retrieved from here, because no tracker tool is available. The description was compared against the diff; Blocking items 1, 2 and 4 cover where it overstates.

Blocking

• new — A test this PR adds fails on the required Windows lane, and the description says the CI failures are unrelated — xtask/src/workflow_contract.rs:3557 (publish_release_readiness_wiki_escapes_markdown_link_and_html_syntax)

  • The test runs Command::new("bash") but has no #[cfg(unix)]. Its sibling release_readiness_wiki_publish_guard_handles_missing_degenerate_and_well_formed_tables (:3431) and the other bash-driven tests in the file have that guard.
  • On windows-build-and-test (job 119771938583), bash resolves to the WSL launcher, which has no distro installed. The run ended: panicked at xtask\src\workflow_contract.rs:3602:13: sed command failed … "Windows Subsystem for Linux has no installed distributions" and 340 passed; 1 failed.
  • The description says "The CI check failures showing on this PR are pre-existing flakes unrelated to this diff". For this check, that is wrong.
  • Confidence 100 · mechanical · Fix: add #[cfg(unix)] like the sibling test, and correct the description.

• standing (narrative escaping, re-found blind in a new form) — The wiki escaping of the LLM narrative can be bypassed with a backslash, and the description claims a backslash step that is not there — .github/workflows/nightly.yml (the sed … release-readiness/narrative.md line in publish-release-readiness-wiki)

  • The actual command is sed -e 's/\[/\\[/g' -e 's/\]/\\]/g' -e 's/</\&lt;/g' -e 's/>/\&gt;/g'. It never doubles \.
  • The input \[click\](https://evil.example) comes out as \\[click\\](https://evil.example). GitHub's markdown renderer (gh api markdown) turns that into a live <a href="https://evil.example">, and !\[img\](url) behaves the same way.
  • The narrative is LLM output drafted from untrusted issue and commit text, so a prompt injection can reach this.
  • The description says the sed "doubles \ and escapes [, ], <, >". The code governs here: the doubling is not in the command.
  • Confidence 100 · logic · Fix: add -e 's/\\/\\\\/g' as the first expression, and add this input to the escape test.

• new — A lane report that is empty or unparseable still gives GO — xtask/src/release_readiness.rs:381-387 (total_e2e_failures), :129-133

  • The verdict reads only the Fail cell of the **Total** row and never reads any platform's Status.
  • When a report has no platform.json, crates/e2e-report uses its legacy counts. For a report.json of [] or not json, it renders | … | 0 | 0 | 0 | 0 | n/a | EMPTY |. The lane did upload an artifact, so the lane-report check passes too.
  • Verified with a temporary test through load_platform_reports → consolidated_summary_markdown → total_e2e_failures → assemble_facts. Both inputs gave Some(0) and Verdict::Go.
  • Also, total_e2e_failures_reads_the_totals_row uses a hand-copied table, although the doc comment says parsing the rendered row "keeps this in lockstep" with the renderer. A column reorder in consolidated.rs:1083 would feed the wrong column into the gate, and no test would notice.
  • Confidence 100 · logic · Fix: fail on any platform row whose status is not PASS. Better: have e2e-report emit a machine-readable summary and gate on that, and test it against real renderer output.

• new — The lane-report gate covers only the release leg of each lane, but the code, docs and description say it covers every lane — release_readiness.rs:134-143, xtask/src/main.rs (--e2e-report-job-result help), docs/ci-hardware-testing.md:199-204, nightly.yml:1588-1596

  • Every E2E lane runs channel: [release, nightly]. LANE_REPORTS lists only *-release-report, and the job's own comment says the release leg "stands for the whole lane".
  • So a nightly-channel leg that dies before uploading drops out of the Total row, and e2e-report-nightly still succeeds, giving GO.
  • That weakness was already in e2e-report-nightly. This PR makes its result release-gate evidence, and states "a lane that dies before uploading a report still fails the gate" in the description, the docs and the help text.
  • Confidence 85 · logic · Fix: check both channel legs in LANE_REPORTS, or narrow the three claims to match.

• new — The stale-evidence notice describes content that is not on the page — nightly.yml:1895

  • The notice ends "Any verdict below this line predates this run and does not reflect it."
  • The job then runs cp "/tmp/${page}" "wiki/${page}", which replaces the whole page. Nothing is below the line, and the previous verdict is deleted rather than kept.
  • Confidence 85 · mechanical · Fix: reword it (e.g. "No verdict was computed for this run"), or append the previous page body if keeping it is the intent.

• standing (rated non-blocking last round; this change request does not rest on it) — The actions/ai-inference pin has a # v3 comment, not the required # vX.Y.Z, and the comment explaining it is wrong for v3 — nightly.yml (the Draft narrative step)

  • AGENTS.md §6: "pin … to a full commit SHA with a trailing # vX.Y.Z comment, never a moving tag".
  • gh api repos/actions/ai-inference/git/matching-refs/tags/v lists v2.1.1 and earlier, plus only the floating v3 → 2c43c91a…. There is no v3.x.y tag.
  • The workflow comment and the description say the action tags full semver releases alongside the major alias. That does not hold for the line pinned here.
  • Confidence 85 · mechanical · Fix: pin a semver-tagged release (e.g. v2.1.1, a7805884…) if its interface fits, or record a documented exception and correct the comment.

• standing (untested branches, re-found blind with a new set) — Much of the new workflow gating has no test that fails when it breaks

  • Each of these mutations, made alone, left cargo test -p xtask workflow_contract green (70 passed):
    • removing if: ${{ !cancelled() }} from release-readiness-nightly. A failed e2e-report-nightly then skips the readiness job and the publish job, so the wiki keeps a stale GO. release_readiness_nightly_gates_on_the_e2e_report_job_not_individual_lanes still passes, so its name claims more than it checks.
    • changing the publish job's != 'skipped' to == 'success', or dropping its !cancelled()
    • dropping the refs/heads/main guard
    • feeding --windows-result from needs.nightly.result
    • inlining the narrative as ${{ }} in run: (expression injection)
    • replacing the random GITHUB_OUTPUT delimiter with a fixed EOF
    • dropping persist-credentials: false
  • In release_readiness.rs, these survived cargo test -p xtask release_readiness (36 passed):
    • taking the scope from head instead of head_no_bang (:237-239), so fix(ci)!: leaks into Breaking Changes
    • dropping .trim() in is_internal_only_scope (:215), so fix(core, ci) is misclassified
    • removing *, _ and the backtick, or '\r', from escape_markdown (:395-397)
  • With the production change reverted, every new test fails, because the jobs and functions it calls no longer exist.
  • Confidence 100 · mechanical · Fix: assert these conditions, and add the two categorisation cases (bang with an internal scope; a space after the comma).

• standing (round 3, commit-message half), settled by putting it to the reviewer: anchored evidence, not a blind finding — Commit messages that would form the squash body still describe behaviour this PR no longer has

  • This repository squash-merges, and the squash body is built from the commit messages, so these land on main. No later message in the range corrects them:
    • fc76d6a and 40f5d62 describe the per-lane --e2e-lane-result gate, including "a lane that dies before producing a report still fails the verdict". 1cb356d says the gate uses "each GPU/WSL lane's own result". e20b631 removed that gate, but its message does not say so, and no message says the gate moved to e2e-report-nightly.
    • 40f5d62 says links and images are stripped from the narrative. The code now escapes brackets and angle brackets instead, and no message says so.
    • fc76d6a says extract_pr_number "is kept test-only". The function no longer exists.
    • On the actions/ai-inference tags, 40f5d62 says "bare-major tags" and e20b631 says "full semver releases". Neither holds for the pinned v3 line.
  • I checked the e20b631 message myself and confirmed it has no correction.
  • Fix: reword the commits (rebase), or have the squash body rewritten to describe the final behaviour before merge.

Non-blocking

• new — The LLM narrative can render headings and tables, so it can print a fake verdict — nightly.yml (the ## Narrative section)
  • Only [ ] < > are escaped. A prompt-injected narrative could render ## Release Readiness — ✅ GO or a fake | Area | Evidence | Status | table under a real NO-GO. That undercuts the guarantee that the verdict stays mechanical.
  • Confidence 70 · logic · Fix: put the narrative in a blockquote or <details> labelled as LLM-drafted, or escape a leading #, | and > on each line.
• standing (was a tradeoff last round) — The two wiki publish jobs can race on the same wiki repo — publish-e2e-wiki and publish-release-readiness-wiki
  • They use separate concurrency groups, and each does a depth-1 clone, a commit and a git push. If the pushes overlap, the second is rejected as non-fast-forward and its job goes red.
  • The description treats this as a retry question, but sharing one concurrency group (e.g. wiki-push) would prevent it outright.
  • Confidence 70 · logic.
• new — A broken consolidated-report upload shows "EVIDENCE UNAVAILABLE" rather than NO-GO — the release-readiness-nightly "Download consolidated nightly report" step has no continue-on-error. If e2e-report-nightly dies before writing consolidated/, the page shows the notice, although the failed report job is itself a known NO-GO. Confidence 70 · logic.

Decisions for the author

• The name release-readiness is already in use — non-blocking-improvement. scripts/release_readiness.py is the release trust gate (AGENTS.md §8, docs/release-trust.md, the nightly.yml env comments). The new cargo xtask release-readiness / release_readiness.rs is an unrelated nightly summary. A distinct name (e.g. readiness-report) would stop contributors and agents confusing the two.
• A skipped build counts as a pass — tradeoff. skipped is accepted for nightly, windows-nightly and e2e-report-nightly. That is safe today, because skip=true only happens when there are no commits since the last published nightly, and that nightly exists only if both builds succeeded. The cost is that the Build row shows ✅ on nights with no build at all.
• Dropped scopes include deps — tradeoff. fix(deps): … commits, such as a security bump to a shipped dependency, can affect end users but are left out of Bug Fixes.

Positive signals

• assemble_facts is a pure function, so the GO/NO-GO rules are unit-tested at their boundaries (e.g. exactly one failure).
• A missing Total row and an empty bundle both fail safe to NO-GO, and git/gh non-zero exits are bail!ed rather than swallowed.
• The narrative reaches the shell through env: NARRATIVE rather than ${{ }} in run:. The facts are inlined with a random 16-hex GITHUB_OUTPUT delimiter.
• At this SHA, ai-inference passes no --allow-tool, so an injection through the facts can only affect the text output.
• The guard test runs the workflow's real shell block against real render_table output rather than a pinned string, which closes the hand-copied-guard gap from the sibling wiki job.

Deployment notes

• Needs a Copilot-licensed PAT in the COPILOT_PAT repository secret. Without it, the narrative job fails (it is allowed to) and the page publishes without prose.
• Publishes a new Release-Readiness wiki page on main, and needs the wiki already initialised (it is a prerequisite shared with publish-e2e-wiki).

What this covered

• Read: every changed file in full, 32834b29…2ada5764 (prw-base...HEAD). That includes xtask/src/release_readiness.rs, the xtask/src/main.rs command, the three new nightly.yml jobs, the new workflow_contract.rs tests, the prompt file and the docs paragraph. Also read: the e2e-report-nightly, publish-e2e-wiki, lane and trigger sections of nightly.yml, and the renderer in crates/e2e-report/src/consolidated.rs.
• Passes: code passes in two coupled groups (Rust logic; workflow plus contract tests) as independent read-only workers. Agent-instruction adherence, history, code-comment and design passes ran in the driver's context because of the 2-worker cap, so they were not independent. The prior-changes pass read the earlier automated reviews on #590, the sibling wiki-publish PR.
• CI, as data:
  • windows-build-and-test fails on this PR's own test (Blocking item 1).
  • Test (affected crates) fails in preflight_runs_the_real_reclaim_assert_clear (coreutils: unknown program 'llama-server'). The branch does not include 5dfa9a8 (#712, which addresses multicall coreutils and is on main).
  • E2E tests fails on two dash-* scenarios and on missing resolved model lines.
• Not done: ROCMAI-568 was not retrievable here. Not reconciled against this PR's review discussion.

[juhovainio]

Addressed in c333065 (round 3 of feedback):

Blocking — fixed:

• LANE_REPORTS in e2e-selfhosted.yml's consolidated-report-check step: e2e-gpu-strix-ubuntu now checks both its release and nightly artifact legs (comma-separated), so a run that uploads only one of the two no longer silently passes the "all expected artifacts present" gate.
• Added 10 new contract tests closing the "standing (untested branches)" mutation-coverage gap:
  • xtask/src/release_readiness.rs: categorize_drops_a_breaking_commit_scoped_entirely_to_internal_tooling, categorize_trims_whitespace_around_scope_names, escape_markdown_escapes_emphasis_and_code_markers, escape_markdown_collapses_a_bare_carriage_return.
  • xtask/src/workflow_contract.rs: assertions on --windows-result sourcing from needs.windows-nightly.result (not the Linux build's own result), plus new tests release_readiness_nightly_runs_even_if_an_upstream_job_failed, publish_release_readiness_wiki_guard_is_exact, release_readiness_jobs_checkout_without_persisting_credentials, release_readiness_narrative_delimiter_is_random_not_fixed, release_readiness_narrative_reaches_the_shell_only_through_env.

Non-blocking — fixed this round (not just acknowledged):

• actions/ai-inference was pinned to 2c43c91 tagged # v3, but v3 is a floating major alias with no semver release yet (confirmed via the GitHub API — only v1.x/v2.x have real tags). Re-pinned to v2.1.1's real commit SHA (a780588) with an exact # v2.1.1 comment per AGENTS.md's full-semver-pin policy. v2.1.1's default provider differs from v3's (github-models vs copilot), so added an explicit provider: copilot to keep behavior unchanged.
• Fixed a clippy::unnested_or_patterns lint in total_e2e_failures surfaced by --all-targets, unrelated to functional behavior.

Judgment call — deliberately left as-is:

• The commit-message/content mismatch you flagged (confidence 100) can't be fixed without rewriting already-pushed commit history, which I'm avoiding mid-review. It's best corrected in the squash-merge commit message or PR description at merge time rather than as a history rewrite now.

Other non-blocking items (LLM narrative fake-heading/table risk, two wiki-publish jobs racing, the "EVIDENCE UNAVAILABLE" download-failure edge case, and the three "decisions for the author" items — scripts/release_readiness.py name collision, skipped-build-counts-as-pass, dropped deps scope) are left unchanged as previously discussed/acknowledged tradeoffs.

Build/test/lint: cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace all pass.

Addressed in c333065

[rominf]

🔴 Automated review · pr-review-watcher · c333065

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.

Round 5 — full review of the whole change at c333065 (not narrowed: the commit-message and missing-test objections cannot be scoped to a file). Each finding is marked new or standing. Round 4's change request was dismissed by the author. This change request restates every objection that still holds, and answers the author's round-4 summary comment.

Round-4 objections settled this round. None was re-found blind. Each was then put to the reviewer (anchored evidence, not a blind finding):

• Windows-lane failure of the new bash test: resolved. The test now has #[cfg(unix)], and windows-build-and-test passes at this head (350 passed, 0 failed).
• Backslash bypass of the narrative sed: resolved. The line now starts with -e 's/\\/\\\\/g'. The reviewer ran it on the three bypass inputs and rendered the result with gh api markdown; no attacker link or image survives. Bare-URL autolinks remain, as the workflow comment says.
• Empty or unparseable lane report gives GO: half resolved. An EMPTY platform row now counts as a failure. The other half still holds: the parser is tested only against hand-written tables, never against e2e-report's real output. That is folded into the first item under Blocking, whose fix already asks for exactly that test.
• Only the release leg of each lane is checked: resolved. All 7 channel-matrixed lanes now list both legs in LANE_REPORTS, and the help text, code comment and docs match. One stale comment remains (first item under Non-blocking). The PR description does not mention the change (fourth item under Blocking).
• Stale-evidence notice refers to a verdict below the line: resolved. It now ends "No verdict was computed for this run."
• # v3 pin: resolved. The driver checked that it is now actions/ai-inference@a7805884… # v2.1.1.
• Untested workflow and gate branches and commit messages describing removed behaviour both still hold, and both were re-found blind (second, third and fifth items under Blocking). On the commit messages, the author proposes fixing them at merge time. Until the history or the squash body is actually reworded, the false claims land on main, so this still gates.

CI at this head, compared by the driver against main (latest CI run at ea1d871):

• Test (affected crates) fails in preflight_runs_the_real_reclaim_assert_clear. This PR's code does not cause it: the branch predates 5dfa9a8 (#712), which fixes it, and main passes this job. A rebase settles it. This was rated non-gating last round, so this change request does not rest on it (sixth item under Blocking).
• E2E tests fails on the same dash-* quit scenarios as main's own run. Does not gate.
• E2E tests (Strix Halo, Windows, current) failed to download the TheRock tarball on the runner (infrastructure). E2E consolidated report (self-hosted) fails because of that lane, which is correct. Neither gates.

Review — needs work

Full review of the whole change.
Implements ROCMAI-568 (nightly release-readiness wiki page). The ticket and the description agree on the feature. The description leaves out the LANE_REPORTS change, and the commit messages describe behaviour the code no longer has (both under Blocking).

Blocking

• new — The E2E failure count is inflated about 90-fold, so the verdict can never be GO and the wiki prints a false failure count. — xtask/src/release_readiness.rs:399-417 (total_e2e_failures); its tests at xtask/src/release_readiness.rs:658-682

  • The "silently empty platform" pass checks every | line in the document, not only the summary table.
  • consolidated_summary_markdown writes more tables after the summary: the expectation grid and command coverage. Their rows (| Scenario | …, | automations-01 … | ✅ |) have a non-numeric cell 5 and a non-PASS cell 8, so each row counts as one failure.
  • I ran the parser logic on the real support-matrix.md from nightly run 39554529729. The **Total** row has Fail = 7, and the function adds 620 phantom failures, for 627.
  • The parser tests only feed hand-written 4-line summary snippets, so they never see the producer's real output. The same blind spot would also miss a column reorder, which could silently read 0 failures and give a false GO.
  • Confidence 100 · logic · Fix: scan only the rows between the | Platform header and the | **Total** row. Add a test that runs e2e-report's real consolidated_summary_markdown (with sidecars, so the grid renders) and parses its output.

• standing (untested branches, re-found blind with a new set) — The gate logic has no tests that can fail: a cancelled or timed-out build reads as GO with every test green. — xtask/src/release_readiness.rs:123-128, 175-192, 311-339

  • I replaced both build gates with == "failure". All 46 release_readiness tests still pass, so cancelled or timed_out on nightly/windows-nightly would yield GO.
  • A worker's mutation run also found that making git log / gh issue list failures return empty lists survives. run, collect_commit_subjects and collect_known_issues have no tests.
  • Confidence 100 · logic · Fix: add cancelled and timed_out cases for both build gates, and failure-path tests for the two subprocess collectors (behind a seam if needed).

• standing (untested branches, re-found blind with a new set) — The workflow contract tests leave load-bearing wiring unpinned, and one test claims more than it checks. — .github/workflows/nightly.yml:1736, :1867, :1874; xtask/src/workflow_contract.rs:3760-3770

  • I removed continue-on-error from both publish-job downloads and also hard-coded --nightly-result "success". All 75 workflow_contract tests still pass.
  • Without continue-on-error, download-artifact fails whenever there is no narrative, which is every night until COPILOT_PAT exists. That fails the publish job and makes the "evidence unavailable" path unreachable.
  • The worker also found that these single mutations survive:
    • dropping release-readiness-narrative from the publish job's needs (the publish job then races the narrative);
    • dropping fetch-depth: 0 (git describe then fails every night);
    • renaming the page so it overwrites E2E-Support-Matrix.md.
  • release_readiness_narrative_reaches_the_shell_only_through_env only checks that the safe lines are present (job.contains(...)). A second step that inlines ${{ steps.draft.outputs.response }} into run: would still pass, so the "only" in its name is a claim the test cannot fail on.
  • Confidence 100 · logic · Fix: pin these items in the contract tests. Make the env test also assert that steps.draft.outputs.response appears nowhere in the job except the NARRATIVE: env line (and the if:).

• new — The description does not mention the LANE_REPORTS change, which changes when a per-PR check fails. — .github/workflows/nightly.yml:1575-1619, .github/workflows/e2e-selfhosted.yml:1691-1734, xtask/src/workflow_contract.rs (LANE_REPORTS tests)

  • Both workflows now require the nightly leg's artifact as well as the release leg's for every channel-matrixed lane. In e2e-selfhosted.yml that is the per-PR "E2E consolidated report (self-hosted)" check.
  • None of the Summary or "Changes since…" sections mentions it, so a reader of the description would not know a PR-gating check got stricter.
  • Confidence 85 · architectural · Fix: describe it in the PR body, or split it into its own PR as was done with #682.

• standing (round 3, re-found blind) — Commit messages that will land in the squash body describe behaviour the code no longer has. — commits 40f5d62, fc76d6a, ef489f5, 1cb356d

  • They claim:
    • a repeatable --e2e-lane-result flag (git grep finds none at HEAD);
    • release-readiness-nightly needing "the 6 GPU/WSL E2E lane jobs" (it needs e2e-report-nightly);
    • extract_pr_number "kept test-only" (it is gone);
    • "Strip markdown links/images" (replaced by escaping);
    • a bare-v3 pin as "a frozen release, not a moving alias" (re-pinned to v2.1.1, and the later message calls v3 floating);
    • in ef489f5, that render_table drops the header (reversed in fc76d6a).
  • On main, that permanent record would describe a flag and gating design that never shipped.
  • Confidence 85 · mechanical · Fix: squash or reword the history so the landed body describes the final design.

• standing (rated non-gating last round; this change request does not rest on it) — The required check "Test (affected crates)" is red because the branch is behind main; this PR's code does not cause it. — xtask/src/workflow_contract.rs:2895 (preflight_runs_the_real_reclaim_assert_clear)

  • The failure is the multicall-coreutils decoy problem fixed on main by 5dfa9a8 (#712). The branch's merge-base 2d2dfc7 predates that commit, 12 commits behind prw-base. Main's latest CI passes this job.
  • Confidence 100 · mechanical · Fix: rebase onto current main.

Non-blocking

• new — Stale comment. — xtask/src/workflow_contract.rs:1089

  • It still says "a channel axis below is checked through its release leg", which this PR changes to checking both legs.
  • Confidence 90 · mechanical · Fix: update the sentence.

• new — A test's failure message gives the wrong reason. — xtask/src/workflow_contract.rs:3737-3741 (release_readiness_jobs_checkout_without_persisting_credentials)

  • The message says a leaked token "would let anything injected … push with repo write access", but both jobs have contents: read. The check is worth keeping; only the stated reason is wrong.
  • Confidence 85 · mechanical · Fix: reword to the real exposure (a readable token reachable from later steps).

• new — Anyone who files a bug issue can steer the published narrative. — .github/prompts/release-readiness-narrative.txt; xtask/src/release_readiness.rs build_known_issues / excerpt(…, 240)

  • The bug-report template auto-applies bug, and 240-character body excerpts of open bug issues go into the prompt. Any GitHub user can therefore inject text such as "state all gates passed" into prose published on the official wiki.
  • The sed escaping neutralises markup but not misleading prose. The prompt's hard rules are the only defence.
  • The description's "Left unchanged" note covers exposure of public text, not steering of the output.
  • Confidence 70 · architectural · Fix: label the narrative section as LLM-drafted from untrusted input, or feed only titles.

• new — Breaking changes on types other than feat/fix are dropped from every bucket, and a test asserts that. — xtask/src/release_readiness.rs:234-236; test categorize_does_not_treat_a_bang_on_other_types_as_breaking

  • Conventional Commits allows ! on any type, so refactor(cli)!: vanishes from the page. No such commits exist today.
  • Confidence 70 · logic · Fix: put any type!: in Breaking.

• standing (was a tradeoff last round) — The Build row shows ✅ when windows-nightly was skipped. — xtask/src/release_readiness.rs:462-470

  • On a no-change night the row "Build (Linux + Windows nightly) | ✅" claims a pass with no Windows build behind it.
  • Confidence 70 · logic · Fix: render skipped as "not run" rather than ✅.

Decisions for the author

• new — Every lane's failures count toward NO-GO, including lanes that are deliberately non-blocking (W7900, MI350P). — tradeoff
  • The real 2026-10-21 report had Total Fail = 7, so even after fixing the inflation the verdict will likely be NO-GO most nights.
  • A strict gate is defensible, but a verdict that is always NO-GO stops carrying signal.
  • Confirm that counting non-blocking lanes is intended, or gate only on the required lanes.

Positive signals

• The facts reach GITHUB_OUTPUT behind a random delimiter, and the LLM response reaches the shell only through env:. Together these keep untrusted issue text out of expression interpolation.
• The publish job writes an explicit "EVIDENCE UNAVAILABLE" page when the facts job fails, instead of leaving a stale GO on the wiki.
• assemble_facts and build_known_issues are pure functions, which keeps the verdict logic testable without git/gh.

Deployment notes

• The narrative job needs a Copilot-licensed PAT in the COPILOT_PAT repo secret. Until it exists the narrative is skipped every night, and the publish job depends on its download steps' continue-on-error to still publish.
• The new Release-Readiness wiki page needs the same one-time wiki initialisation as E2E-Support-Matrix.
• The per-PR "E2E consolidated report (self-hosted)" check now fails when the Strix Ubuntu nightly leg uploads no report, as well as the release leg.

What this covered

I read every file in the change completely: the new xtask/src/release_readiness.rs, the main.rs wiring, the changed workflow_contract.rs tests, the new and changed jobs in nightly.yml and e2e-selfhosted.yml, the prompt and the docs hunk. I also read the e2e-report producer of support-matrix.md and the jobs these depend on or mirror. Reviewed range: 2d2dfc7…c333065a.

Two read-only workers ran in parallel: Rust logic, and workflows plus contract tests. I verified their headline findings myself:

• the 627 count, by re-running the parser logic on the downloaded artifact;
• the build-gate mutation: 46/46 tests pass;
• the workflow mutations: 75/75 tests pass.

The agent-instruction check (AGENTS.md: leak scan clean, action pins, docs sync), the history pass, CI attribution and the intent check (PR body, ROCMAI-568 by key) were run by me, the driver, not by an independent worker.

CI attribution for the failing checks:

• "E2E tests" (mock): the same two dash scenarios fail on main.
• "E2E tests (Strix Halo, Windows)": runner infrastructure (HTTP 403 on the TheRock tarball, GPU preflight).
• "E2E consolidated report (self-hosted)": fails correctly, downstream of the Windows lane.

Did not run:

• prior-changes: review comments on earlier PRs touching these files were not consulted, per this run's no-comments constraint.
• routing/behavioral for the LLM prompt: no API access.

Not reconciled against any discussion here. A worker left the scratch directory .rr-probe/ (the downloaded artifact) in the checkout.

Addressed in 78ae588 (see summary comment).

[rominf]

🔴 Automated review · pr-review-watcher · 78ae588

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.

Round 6. This is a full review of the whole change at 78ae588. It was not narrowed, because the commit-message objection cannot be scoped to a file. Each finding is marked new or standing. The author dismissed round 5's change request.

This change request rests on one item only: the standing commit-message objection, the last item under Blocking. The two new items under Blocking are real, but they do not gate:

• Red checks. Test (affected crates) and hosted E2E tests are fixed on main (#712, #713/#719), and this branch predates those fixes, so a rebase clears them. E2E tests (MI300X, current) also fails on main's own latest completed runs (39948978595, 39947062610). The Strix Windows lane failed downloading the external tarball, which is an infrastructure fault, and the consolidated report fails downstream of that lane, as it should.
• Untested delimiter newline. I checked its consequence. If the line is removed, only the narrative job fails, and that job is continue-on-error, so the page publishes without a narrative. The code is correct today, so this missing guard is below the gating floor.

The round-5 objections were settled as follows. None was re-found by the blind review. Each was then put to the reviewer one at a time, so the evidence below is anchored, not blind:

• E2E failure count inflated ~90×: resolved. total_e2e_failures now scans only the rows between | Platform and | **Total**. The reviewer re-widened the slice to the end of the document. total_e2e_failures_ignores_the_expectation_grid_and_command_coverage_tables, which builds its input with the real consolidated_summary_markdown, then failed with Some(2) instead of Some(0). One gap remains in that test: it writes no platform.json sidecar, so the expectation-grid table never renders and its rows are never parsed. The same slice excludes those rows, so there is no live defect. The 627-versus-7 figure was not re-derived.

• A cancelled or timed-out build reads as GO with all tests green: resolved. The reviewer changed both build gates to == "failure", and 4 of 58 tests then failed (the cancelled and timed-out cases for each build). Making either collector return an empty list on failure is now caught too, by collect_commit_subjects_fails_on_a_bogus_since_tag and collect_known_issues_in_fails_outside_a_github_repo. Still open: run() and the main.rs argument mapping have no test. That is carried as the non-blocking wiring item below.

• Workflow contract wiring that survived mutation: resolved. The reviewer applied each round-5 mutation on its own. Each failed exactly one of the 79 workflow_contract tests:

  • dropping continue-on-error on either publish download;
  • hard-coding --nightly-result;
  • dropping release-readiness-narrative from needs;
  • dropping fetch-depth: 0;
  • renaming the page to E2E-Support-Matrix.md;
  • adding a step that inlines steps.draft.outputs.response into run:.

  Observation, not tested: most of these are contains-style checks, so a matching line inside a YAML comment could satisfy some of them.

• The description omitted the LANE_REPORTS change: resolved. It now has its own paragraph.

• Commit messages that describe removed behaviour: still holds. I checked this myself; it is restated as the last item under Blocking.

Review — needs work

Full review of the whole change.
Jira ROCMAI-568 is named but could not be retrieved here (no tracker tool). The description matches what the diff does, except for the two non-blocking inaccuracies listed below.

Blocking

• new (does not gate; see banner) — The head commit has five red checks, all caused outside this diff, and all fixed on main or infrastructure-related — Test (affected crates), E2E tests, E2E tests (MI300X, current), E2E tests (Strix Halo, Windows, current), E2E consolidated report (self-hosted)
  • Test (affected crates) fails only workflow_contract::tests::preflight_runs_the_real_reclaim_assert_clear (coreutils: unknown program 'llama-server'). That is the multicall-coreutils decoy problem fixed on main by 5dfa9a8 (#712).
  • Hosted E2E tests fails on dash quit timeouts and short-name serve steps. #713 and #719 on main address the quit timeouts, and main's latest E2E tests (run 39950566642) is green.
  • MI300X fails on a vLLM "auto" tool choice 400.
  • Strix Windows fails because the external TheRock tarball download failed six times. The self-hosted consolidated check then correctly reports that lane's missing report.
  • The branch is 15 commits behind main and merges cleanly (git merge-tree). On the merged tree, cargo test -p xtask --bins ran 375 tests and all passed, including the reclaim test and all 58 release-readiness tests. cargo clippy -p xtask --all-targets -- -D warnings is clean.
  • Confidence 85 (the red checks come from the rollup; the attribution comes from the logs and main's runs) · mechanical · Fix: rebase onto main and re-run until green.
• new (does not gate; see banner) — The newline written before the GITHUB_OUTPUT delimiter is load-bearing, and no test exercises it — .github/workflows/nightly.yml:1799-1801, xtask/src/release_readiness.rs (run → write_parented)
  • facts.json is written with serde_json::to_string_pretty and has no trailing newline. Without the bare echo, the delimiter lands on the } line and the step output is never terminated. That is exactly the bug commit 45d3cd5 fixed in this PR.
  • Verification: I deleted that echo and re-ran cargo test -p xtask --bins release_readiness. All 58 tests still passed, including release_readiness_narrative_delimiter_is_random_not_fixed.
  • Confidence 100 · mechanical · Fix: extend the contract test to run the extracted step against a facts.json with no trailing newline and parse the output file. Alternatively, end the file with \n in run and assert that.
• standing (round 3 onwards; this is what gates) — The commit messages that will form the squash body describe behaviour the code no longer has. — commits 40f5d624, fc76d6a8, ef489f5d, 1cb356de
  • The history is unchanged at this head. The four commits still claim:
    • a repeatable --e2e-lane-result flag (git grep at HEAD finds none);
    • that release-readiness-nightly needs "the 6 GPU/WSL E2E lane jobs" (it needs e2e-report-nightly);
    • that extract_pr_number is "kept test-only" (it no longer exists);
    • "Strip markdown links/images" (replaced by escaping);
    • that the bare-v3 pin is "a frozen release, not a moving alias" (it was re-pinned to v2.1.1);
    • that render_table drops the header (reversed in fc76d6a8).
  • The description says these "will be reworded in the squash". That plan does not work here:
    • This repository's squash setting is squash_merge_commit_message: COMMIT_MESSAGES, so the squash body is built from these commit messages.
    • rocm-cli merges through the merge queue, which builds that commit itself and offers no step where the body can be edited.
    • Unless the history is rewritten before the PR is queued, these claims land on main as the permanent record.
  • Confidence 85 · mechanical · Fix: before queueing, squash or reword the branch history (for example, one commit whose message describes the final design), then force-push.

Non-blocking

• new — The "EVIDENCE UNAVAILABLE" notice can be false, and when it is, it overwrites a valid page — .github/workflows/nightly.yml (publish-release-readiness-wiki, the ! -f table.md branch)
  • The table download is continue-on-error: true, so a transient download failure after release-readiness-nightly succeeded also takes this branch.
  • The page then says "release-readiness-nightly failed before it could compute a verdict", which is not what happened. That breaks AGENTS.md §3's rule that a printed message must match the behaviour.
  • Confidence 70 · logic · Fix: pass needs.release-readiness-nightly.result through env: and derive the wording from it.
• new — The test for collect_known_issues_in passes without the branch it targets — xtask/src/release_readiness.rs (test collect_known_issues_in_fails_outside_a_github_repo)
  • Per the review worker's mutation run, the test still passes with the !output.status.success() check removed, because the JSON-parse error text also contains gh issue list. The code still fails closed either way.
  • Confidence 100 (worker run, not repeated by me) · mechanical · Fix: assert on "gh issue list failed".
• new — Nothing tests the main.rs → release_readiness::run wiring, and its five result strings are untyped — xtask/src/main.rs:396-412
  • Swapping windows_result and e2e_report_job_result would compile and keep every test green.
  • A typo such as succes would read as a failed gate rather than as an error.
  • Confidence 70 · architectural · Fix: use a clap ValueEnum JobResult (or an inputs struct) and test assemble_facts through it.
• new — skipped counts as passing for e2e-report-nightly, which is never legitimately skipped — xtask/src/release_readiness.rs:147
  • That job is if: always(). For the build gates, "skipped" has a real meaning (a no-op night); for this gate it has none, and a test pins the lenient behaviour.
  • Today the separate artifact download would fail first, which hides this.
  • Confidence 70 · logic · Fix: accept only success for this gate.
• new — The "" status arm in total_e2e_failures is unreachable — xtask/src/release_readiness.rs:437
  • The producer only emits PASS, FAIL or EMPTY for platform rows. Removing the arm leaves all tests green.
  • Confidence 70 · mechanical · Fix: drop the arm, or add the fixture that needs it.
• new — A **Total** line before the header panics instead of returning None — xtask/src/release_readiness.rs:424
  • When total_idx is less than header_idx, lines[header_idx + 1..total_idx] panics. That contradicts the function's contract that a broken report returns None. The outcome still fails closed.
  • Confidence 70 · mechanical · Fix: search for Total only after header_idx.
• new — Stale or incorrect comments — xtask/src/release_readiness.rs:63-66, .github/workflows/nightly.yml (ai-inference pin comment), docs/ci-hardware-testing.md:204
  • The failing_gates doc omits the E2E (lane report check: …) gate.
  • "per AGENTS.md's full-semver-tag policy" is wrong: AGENTS.md §6 requires a full commit SHA plus a # vX.Y.Z comment. The pin itself is correct. I checked a7805884… against v2.1.1; provider and system-prompt-file exist there, and the default provider is github-models.
  • "Unlike the wiki-publish jobs below" is wrong: publish-e2e-wiki is described above that sentence.
  • Confidence 85 · mechanical.
• new — Cosmetic branches that no test exercises — xtask/src/release_readiness.rs
  • The tie-break on issue number in build_known_issues, the exact-240 boundary in excerpt, and the nightly_result == "skipped" half of build_skipped all survive single-branch mutation.
  • Confidence 85 · mechanical.
• new — Breaking-change commits of internal types reach the user-facing Breaking list — xtask/src/release_readiness.rs:240-246
  • Only scopes are filtered, so ci!:, build!: and chore!: land under "Breaking Changes".
  • Confidence 70 · logic · Fix: if that matches the intent, also drop internal types on the ! path.
• new — The sdk_version axis is now handled differently from the channel axis next to it — .github/workflows/e2e-selfhosted.yml (around line 1088), xtask/src/workflow_contract.rs (uploaded_report_artifact)
  • The new reasoning ("either leg can die independently, so check both") applies equally to the -n-1/-n-2 legs, which are still represented by current. That is pre-existing, but the two are now stated side by side.
  • Confidence 70 · architectural · Follow-up.
• new — The description contradicts itself on escaping — PR body
  • The second-round section says issue-title escaping "covers text fed into the LLM prompt". It does not: escape_markdown runs only in render_table, and facts.json carries the raw titles.
  • The commit messages "address second/third/fourth round of review feedback" convey nothing either. Squash-merge collapses them, and the description says the squash body will be reworded.

Decisions for the author

• The E2E gate fails on any failure in any lane and either channel — tradeoff
  • The ROCm nightly-channel legs test upstream SDK nightlies. With this PR's LANE_REPORTS change, a nightly leg that dies (for example, on a TheRock nightly download) now fails e2e-report-nightly and so yields NO-GO.
  • It also fails the per-PR E2E consolidated report (self-hosted) check when the Strix Ubuntu nightly report is missing.
  • For: stricter evidence, and no lane vanishes silently. Against: the verdict can sit at NO-GO for upstream or infrastructure reasons, which weakens its signal. The description already flags the non-blocking-lanes half of this.
• The name collides with the existing release gate — non-blocking-improvement
  • scripts/release_readiness.py is already "release readiness" (dist-asset and signature verification, cited in AGENTS.md §8 and docs/testing.md).
  • A different name, such as nightly-verdict, would keep "run release readiness" unambiguous.
• COPILOT_PAT versus the built-in token — tradeoff
  • A review worker cited a GitHub changelog (2026-07-02) saying that Copilot CLI accepts the built-in token with copilot-requests: write when org policy allows it. That would remove a long-lived PAT from a step that reads untrusted text.
  • I did not verify this. If it is wrong, the PAT is required, as the comment and description state.
• Narrative steering through bug-labeled issue text — answered-by-intent
  • PR description: "Narrative-steering via bug-labeled issue titles/excerpts … is a real exposure, but fixing it … is a product decision … left for a follow-up."
  • Otherwise this would have been non-blocking. ai-inference v2.1.1 passes no --allow-tool, so the risk is steered prose rather than tool use. Backticks and bare URLs in the narrative are not escaped.
  • Confirm the follow-up is still planned before COPILOT_PAT is set.
• A missing support-matrix.md aborts the job instead of producing a NO-GO table — answered-by-intent
  • PR description: "A genuinely missing E2E report file … still fails the release-readiness-nightly job via read_to_string(...)? … the wiki-publish job's 'evidence unavailable' fallback … already covers this case."
  • Otherwise this would have been non-blocking, because the render_table comment implies that unreadable evidence still publishes a table.

Positive signals

• The gate reads e2e-report-nightly's own result, not the lane results, which continue-on-error always reports as success. Lanes added to LANE_REPORTS later are covered automatically.
• The verdict is computed only in the pure assemble_facts. The LLM output reaches the shell only through env:, behind a random heredoc delimiter. The sed escaping doubles backslashes first, so a pre-escaped \[ cannot slip a live link through.
• The grid-slicing regression test builds its input with the real e2e_report::consolidated_summary_markdown rather than a hand-written snippet, so it tracks format drift in the producer.
• The publish job's != 'skipped' condition means an outright upstream failure replaces a stale GO on the wiki instead of leaving it in place.

Deployment notes

• The narrative step needs a Copilot-licensed PAT in the COPILOT_PAT repo secret. Without it the job fails, continue-on-error hides the failure, and the page publishes without a narrative.
• This creates a new Release-Readiness wiki page. It relies on the wiki already existing, which the E2E matrix page already requires.
• Every PR's E2E consolidated report (self-hosted) check now also requires the Strix Ubuntu nightly leg's report. The nightly e2e-report-nightly job requires both legs for all seven lanes.

What this covered

• Read: every file in main...HEAD (2d2dfc7…78ae5883), completely, split into two coupled groups, each read by an independent worker:

  • group 1: release_readiness.rs, main.rs and the prompt;
  • group 2: both workflows, workflow_contract.rs and the docs.

  Also read: the e2e-report producer (consolidated.rs), the source of the pinned ai-inference action, and the history of LANE_REPORTS.

• Ran:

  • 58 release-readiness tests plus 78 workflow_contract tests at HEAD, with single-branch mutations;
  • the full xtask suite (375 tests) and xtask clippy, on a local merge with main;
  • a mutation check confirming the delimiter gap.

• Did not run:

  • the design-review step as independent workers; I ran it myself because of the session's two-worker cap;
  • the prior-changes pass (by instruction, no earlier review comments were read);
  • cargo xtask coverage against the xtask floor;
  • actionlint (not installed).

• The test release_readiness_wiki_publish_guard_… was judged by reading only, because it writes to /tmp.

• No prompt-injection content was found in the checkout.

• This report was not reconciled against the PR's discussion.

[juhovainio]

Addressed in 78ae5883 (round 4 of feedback, review 5625115103 at c333065a). Detail in the PR description's "Changes since the third round of review" section:

Blocking — fixed:

• E2E failure count inflated ~90x — total_e2e_failures now scans only the rows between the | Platform header and the | **Total** row, instead of every |-line in the document (it was also counting rows from the expectation-grid and command-coverage tables e2e-report emits after the summary). Added a regression test built against the real e2e_report::consolidated_summary_markdown output, not a hand-written snippet.
• Cancelled/timed-out builds read as GO — both build gates (nightly, windows-nightly) now treat cancelled/timed_out as failures. Added failure-path tests for collect_commit_subjects and collect_known_issues (behind a current_dir seam), closing the untested-subprocess-collector gap.
• Untested workflow-contract wiring — pinned every mutation the review's worker found survived: fetch-depth: 0 on the nightly checkout, the publish job's needs on both upstream jobs, both continue-on-error: true artifact downloads, the distinct wiki page names, and the dynamic --nightly-result read. The narrative env-only test now fails if any other line in the job references steps.draft.outputs.response.
• Description omitted the LANE_REPORTS change — now has its own paragraph (last bullet of the first "Changes since..." section).

Non-blocking — fixed this round:

• categorize now puts any type!: (not just feat!/fix!) in the Breaking bucket, matching Conventional Commits' actual grammar.
• Build row renders "not run" instead of ✅ when windows-nightly was skipped.
• Fixed the stale comment and misleading assertion message in workflow_contract.rs.

Left unchanged (judgment call, not an oversight):

• Commit messages describing removed behavior (--e2e-lane-result, "6 GPU/WSL E2E lane jobs", link-stripping, etc.) are left as-is; fixing them means rewriting already-pushed history, which this pass doesn't force-push to do.
• Test (affected crates) failing on the reclaim test is the pre-existing multicall-coreutils decoy issue fixed on main by fix(ci): keep the GPU reclaim decoys alive on multicall coreutils #712; a rebase (not a code change here) settles it, and this was already rated non-gating.

Note on 78ae5883's own commit message: it undersells what the commit actually contains (it names only 3 small items). The content above reflects what the diff actually contains, cross-checked against this review, which evaluated this exact commit.

[juhovainio]

Addressed in 0c5f1fed (round 5 of feedback, review 5628333214 at 78ae5883). Detail in the PR description's "Changes since the fourth round of review" section:

Blocking (non-gating per the review's own banner):

• Untested delimiter newline — fixed. facts.json now always ends with a trailing newline, so the $GITHUB_OUTPUT heredoc delimiter can't land on the same line as the JSON's closing brace even if the echo guard is ever dropped.
• Red CI checks — unrelated to this diff (multicall-coreutils decoy fixed on main by fix(ci): keep the GPU reclaim decoys alive on multicall coreutils #712, dash-quit timeouts fixed by test(e2e): stop quit_and_wait deadlocking on a slow quit prompt #713/test(e2e): don't count harness stalls against the quit timeout #719, Strix Windows tarball-download infra fault). Not something to fix here; a rebase clears them, as the review itself says this item doesn't gate.

Blocking (this is what gates) — left unchanged, judgment call not an oversight:

• Commit-message history-rewrite objection — still holds. The 4 commits (40f5d624, fc76d6a8, ef489f5d, 1cb356de) still describe removed behavior. Fixing this means squashing/rewording already-pushed branch history and force-pushing, which this pass doesn't do. Given this repo's COMMIT_MESSAGES squash setting and the merge queue's lack of an edit step, this needs to be an actual history rewrite before the PR is queued, not something deferred to "squash time" as earlier rounds assumed — flagging that explicitly rather than silently leaving it.
• Internal-type breaking commits (ci!:, chore!:, build!:) reaching the Breaking list — left as the reviewer explicitly flagged this as intent-dependent ("if that matches the intent, also drop internal types on the ! path"). No such commits exist in this branch today, so it doesn't affect the current page's output either way; a design call on the final policy, not guessed at here.

Non-blocking — fixed:

• e2e-report-nightly's result no longer accepts skipped as a passing gate (that job is if: always() with no legitimate no-op-night skip).
• total_e2e_failures no longer panics on a **Total** row preceding the | Platform header.
• Dropped the unreachable "" status arm (the producer only ever emits PASS, FAIL, or EMPTY).
• collect_known_issues_in_fails_outside_a_github_repo now asserts the specific gh issue list failure message instead of a substring both failure paths happen to share.
• Fixed 3 stale comments (failing_gates doc, ai-inference pin comment, ci-hardware-testing.md's "below" reference).
• Added tests for the 3 previously-untested branches this review's worker found survive mutation: the known-issue reaction tie-break, excerpt's exact-character-limit boundary, and build_skipped's skipped half for both build gates.
• The description's self-contradiction on escaping scope (claiming issue-title escaping covers the LLM prompt, when it only covers the rendered wiki table) is corrected in the current description text.

Non-blocking — left unchanged (deliberate deferrals, not oversights):

• EVIDENCE UNAVAILABLE false positive on a transient table-download failure — real; narrowing it needs threading needs.release-readiness-nightly.result through env:, left for a follow-up.
• main.rs → run wiring untested, result strings untyped — real; a ValueEnum JobResult would be the fix, left for a follow-up as a larger refactor than this pass's scope.
• sdk_version/channel axis asymmetry — pre-existing, just now stated side by side; left as the review's own fix label says ("Follow-up").

All of the above is also now in the PR description under "Changes since the fourth round of review."

Addressed in 0c5f1fe (see PR description's "Changes since the fourth round of review" section and inline reply comment). The two remaining blocking items (commit-message history-rewrite objection; internal-type breaking-commit classification) are explained as deliberate judgment calls in that reply, not guessed-at code changes.

[rominf]

🔴 Automated review · pr-review-watcher · 0c5f1fe

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.

Round 7. This is a full review of the whole change at 0c5f1fe. It was not narrowed, because the commit-message objection cannot be scoped to a file. Each finding is marked new or standing. Round 6's change request had been dismissed; its objection is treated as standing here.

This change request rests on one item only: the standing commit-message objection, the first item under Blocking. The blind review re-found it on its own, and I checked it directly: 40f5d624, fc76d6a8, ef489f5d and 1cb356de are still on the branch unchanged, and git grep finds no e2e-lane-result at the head. Your reply agrees it still holds.

The other three items under Blocking are real, but they do not gate:

• Narrative can forge a GO heading. Round 6 recorded this exposure (bug-labelled issue text steering the narrative) as answered-by-intent, and the description still defers it to a follow-up. This round adds a demonstration of how far that steering reaches, but it is the same exposure, and it only goes live once COPILOT_PAT is set. It is not escalated to a gate. I strongly recommend closing it before that secret is configured.
• Untested gate branches and artifact hand-offs. The code is correct today. Each item is a missing guard against a future regression, which is below the gating floor. Round 6 applied the same reasoning to the delimiter newline.
• Red checks. E2E tests (MI300X, current) is red on main's own recent runs. Strix Windows died on an HTTP 403 from the external tarball host, which is an infrastructure fault, and the consolidated report failed downstream of that, as it should. The Test (affected crates) reclaim test passed in the same run's Coverage job and on main, so treat it as a flake to re-run. As the item below explains, it is not a stale base.

Review — needs work

Full review of the whole change.
Implements ROCMAI-568 ("Nightly release-readiness summary (wiki page)") as the ticket describes it. One ticket claim is undercut, under Blocking: "the LLM ... never decides pass/fail".

Blocking

• standing (round 3 onwards; this is what gates) — The squash body will put claims the code no longer makes onto main — commits fc76d6a, 40f5d62, ef489f5, 1cb356d, e20b631
  The repo builds the squash body from these commit messages. They describe things that are not in the code:

  • fc76d6a and 40f5d62 say a repeatable --e2e-lane-result NAME=RESULT flag was added and that release-readiness-nightly "needs: the 6 GPU/WSL E2E lane jobs". Neither exists at the head commit: git log -S'e2e-lane-result' shows e20b631 removing it. e20b631's own message never says it reversed that gating design.
  • ef489f5 says render_table drops the table header when evidence is unreadable. A later commit restored the header.
  • 1cb356d says the docs gate on "each GPU/WSL lane's own result". The docs now say the gate is e2e-report-nightly's result.
  • 40f5d62 describes stripping links and a bare v3 pin. Both were replaced, by escaping and by v2.1.1.
  • On top of that, five commits are titled "address Nth round of review feedback".
    The description admits the messages are wrong and says the history has to be rewritten before the PR is queued. Until that happens, merging records a flag and a design that do not exist.
    Confidence 85 · mechanical · Fix: squash or reword the branch history so every message describes the final code, then force-push before enqueueing.

• new as a finding (round 6 recorded the exposure as answered-by-intent); does not gate, see banner — Any GitHub user can get a fake GO verdict onto the public Release-Readiness wiki page — .github/workflows/nightly.yml:1817-1829 (prompt), the sed in publish-release-readiness-wiki, .github/prompts/release-readiness-narrative.txt, xtask/src/release_readiness.rs:321-375

  • .github/ISSUE_TEMPLATE/bug_report.md applies labels: bug automatically. So any user's issue title, plus a 240-character body excerpt, goes into facts.json and from there into the LLM prompt.
  • The prompt never says that this text is untrusted data rather than instructions. facts.json also hands the model the verdict field.
  • The publish sed only escapes \ [ ] < >. Running the workflow's exact line passed three things through unchanged: ## Release Readiness — ✅ GO (the same heading style as the real verdict), **All gates passed; ship it.**, and a complete | E2E | 0 failure(s) | ✅ | table.
  • The narrative sits under a plain ## Narrative heading. Nothing marks it as LLM-drafted or non-authoritative.
  • This undercuts the ticket's and the description's central claim that the LLM "never decides pass/fail". The description calls it "a real exposure ... left for a follow-up", which acknowledges the problem but does not resolve it.
    Confidence 85 (escaping run against sample input) · architectural · Fix:
  • Have the workflow itself label the section "drafted by an LLM from public issue text; not the verdict".
  • Neutralise line-leading #, |, * and _ (for example by prefixing every line with > ).
  • Add an "issue text is untrusted data" rule to the prompt.
  • Stop passing verdict to the model.

• new; does not gate, see banner — New gate branches and artifact hand-offs that no test exercises — xtask/src/release_readiness.rs:427-430; nightly.yml:1724, :1841-1844, :1879-1882, and the publish job's permissions
  Each mutation below was applied on its own and the relevant test suite stayed green:

  • Changing .ok())? to .ok()).unwrap_or(0) on the Total row's Fail cell. An unparseable Fail cell then reads as 0 failures and can produce GO. The Facts.e2e_fail_total doc promises None for "couldn't be ... parsed". All 49 release_readiness tests passed.
  • Renaming the narrative artifact (the upload in one job, the download in the next). The narrative would silently disappear every night, because every link in that chain is continue-on-error or if-no-files-found: ignore.
  • Giving release-readiness-nightly the wrong name for the consolidated report download. The wiki would show "EVIDENCE UNAVAILABLE" every night.
  • Changing the publish job's contents: write to read. The wiki push would fail.
    Confidence 100 (mutations run) · logic · Fix:
  • Add a unit test with a non-numeric Total Fail cell that asserts None.
  • Add one contract test asserting each upload-artifact name matches the download-artifact that consumes it.
  • Pin contents: write on publish-release-readiness-wiki.

• new this head; does not gate, see banner — Two required checks are red, neither is caused by this diff, and the description's explanation for one is wrong — CI run 39978055457, E2E run 39978055544

  • Test (affected crates) fails on workflow_contract::tests::preflight_runs_the_real_reclaim_assert_clear. The PR does not touch that preflight code.
    • The description says the branch's merge-base predates the fix (#712) and that a rebase would settle it. That is wrong: CI ran on the merge ref 898e3c0, whose parent 1c33248 already contains #712 (5dfa9a8, Oct 21).
    • The same test passed in the same run's Coverage job, and on main's runs at 1c33248 and d510680. So this is a flake, not a stale base.
  • E2E consolidated report (self-hosted) failed because the Strix Windows lane died during engine install with HTTP 403 from the tarball host. It therefore uploaded no e2e-gpu-strix-windows-report, and the lane-report check flagged that correctly. The Strix Ubuntu two-leg check this PR adds passed.
  • E2E tests (MI300X, current) is also red. It is red on main's latest push run too.
    Confidence 100 (logs read) · mechanical · Fix: re-run both checks until green, and correct the description's root-cause claim for the preflight test.

Non-blocking

• new — publish-e2e-wiki now skips more often, and it skips silently — nightly.yml:1590-1597, :1640

  • The LANE_REPORTS check now also requires each lane's nightly-leg artifact.
  • So any of the 7 nightly legs dying before upload now fails e2e-report-nightly. publish-e2e-wiki requires that job to succeed, so it skips, and the E2E matrix page goes stale with no notice. That is the same stale-page problem this PR fixes for the Release-Readiness page.
  • docs/ci-hardware-testing.md:195 lists that job's skip conditions and does not mention this one.
  • Before this change, a missing nightly-leg artifact never failed that job.
    Confidence 70 · architectural · Fix: gate publish-e2e-wiki on the table being present (as the new job does), or document the skip.

• new — A lane can drop out of the E2E total and still allow a GO — xtask/src/e2e_report.rs:81-113

  • discover() silently skips any artifact directory that has no report.json.
  • The LANE_REPORTS check only tests that the artifact name exists.
  • So a lane that uploads other files but no report.json contributes no row, nothing fails, and the verdict can read GO.
  • The code is pre-existing, but this change makes it decide a release verdict.
    Confidence 70 (argued by reading, not observed) · logic · Fix: emit an EMPTY row for an artifact directory that has no report.json.

• new — The contract helper hardcodes the two channel names — xtask/src/workflow_contract.rs:1122-1133
uploaded_report_artifact substitutes the literals release and nightly instead of reading the lane's channel: list. If a lane adds a third channel, that leg goes unchecked and the test stays green.
  Confidence 70 · mechanical · Fix: derive the legs from the matrix.

• new — The narrative prompt's input grows without bound — release_readiness.rs:329-345, nightly.yml:1817-1829

  • Known issues are fetched with --limit 1000, and every one goes into both the public raw appendix and the prompt.
  • The pinned ai-inference passes system prompt plus facts to copilot -p as a single argv string. Linux caps a single argument at 128 KiB.
  • Today's input is about 25 issues and 109 commits, roughly 20–30 KB, and it grows as time passes since the last tag. Past the cap the narrative silently drops.
    Confidence 70 · logic · Fix: cap the issues and commits handed to the prompt, and the appendix.

• new — collect_known_issues_in_fails_outside_a_github_repo depends on the machine it runs on — release_readiness.rs:1272-1280

  • Without gh installed, the error is the .context("failed to run \gh issue list` ...") text, not the asserted "gh issue list` failed", so the test fails.
  • With GH_REPO set, it makes a live API call.
    Confidence 70 · mechanical · Fix: gate the test on gh being present, and clear GH_REPO for the child process.

• new — Two comments overstate what the lane check covers — xtask/src/main.rs (--e2e-report-job-result doc: "already fails if any E2E lane that ran didn't upload"), release_readiness.rs:141-152 ("every lane ... for free")
  The workflow's own comment on that check is the one that governs, because it describes what the step actually does: a re-run can pass on an earlier attempt's artifact, so "this can miss a lane".
  Confidence 70 · mechanical · Fix: add the re-run caveat to both comments.

Decisions for the author

• new — The verdict will read NO-GO almost every night — tradeoff
  • Each of the last 6 scheduled nightlies had a Total Fail of 6–12: MI300X/MI350P fail 1–2 every night, and Strix Windows 2–7.
  • e2e-report-nightly failed in 5 of the last 12 scheduled runs.
  • XPASS counts as Fail, and a dead nightly-channel leg now also forces NO-GO.
  • Failing closed is defensible. But a verdict that is constant carries little signal unless persistent known failures are separated from new regressions. Confirm that this is the intended release bar.
• new — About 26% of real commit subjects never reach the buckets — non-blocking-improvement
  • Over v0.2.0..main, 28 of 109 squash subjects are ticket-prefixed (ROCMAI-NNN:, EAI-NNNN:) or otherwise non-conventional (vllm:). categorize() drops them all.
  • That loses at least 5 user-facing changes: #698, #648 (gfx90c detection), #424, #646, #535.
  • Meanwhile fix(release) (#550, release tooling) is shown as a user-facing fix.
  • Better: recognise TICKET-N: type(scope):, add release to INTERNAL_SCOPES, or say on the page that only conventional-commit subjects are counted.
• new — The two wiki-push jobs can race on the same wiki repo — answered-by-intent
  Both do clone --depth 1, commit, and push, in separate concurrency groups and with no retry. The description says: "publish-e2e-wiki and publish-release-readiness-wiki use separate concurrency groups ... a retry/backoff story for wiki pushes would be a repo-wide improvement." This would otherwise have been a non-blocking finding (confidence 70). Confirm you accept that an overlap fails the later push.

Positive signals

• The LLM response reaches the shell only through env: NARRATIVE.
• The $GITHUB_OUTPUT heredoc uses a random delimiter.
• The sed doubles backslashes before it escapes brackets.
• The escaping contract tests run the workflow's real sed line, not a copy of it.
• The GO/NO-GO gating is well pinned: 24 of the 28 mutations of assemble_facts, total_e2e_failures, categorize and escape_markdown were caught.
• The new action pins are verified: actions/ai-inference v2.1.1 → a7805884, and actions/setup-node v7.1.0 → 949feb24.
• Checking both release and nightly legs is load-bearing. The latest nightly (40279551041) has no e2e-gpu-mi350p-nightly-report, yet passed under the old release-leg-only check.

Deployment notes

• A new repo secret, COPILOT_PAT (a Copilot-licensed PAT), is required. Without it the narrative fails quietly every night, and the table still publishes.
• The per-PR check "E2E consolidated report (self-hosted)" now also fails when the Strix Ubuntu nightly leg uploads no report.
• release-readiness-nightly and release-readiness-narrative run on a workflow_dispatch from any branch, so every dispatch spends Copilot quota. Publishing stays limited to main.

What this covered

• What was read: every changed file, completely, plus the code around it:
  • nightly.yml's nightly, windows-nightly, all 7 nightly lanes, e2e-report-nightly and publish-e2e-wiki jobs;
  • the lane matrices in e2e-selfhosted.yml;
  • xtask/src/e2e_report.rs and crates/e2e-report's consolidated summary producer.
• Commits: 2d2dfc78…0c5f1fed. That is the merge-base with prw-base (1c33248c); the branch is 17 commits behind it, and git merge-tree against prw-base is clean.
• What I ran:
  • cargo test -p xtask release_readiness: 61 passed, including all 49 release_readiness::tests.
  • cargo test -p xtask workflow_contract: 79 passed.
  • cargo clippy -p xtask --all-targets -D warnings: clean.
  • 28 mutations of the Rust logic and 13 of the workflows.
  • The leak scan: clean.
  • CI logs, compared against main's runs of the same jobs.
• Fan-out: three independent workers — the Rust logic, the workflows with their contract tests, and the design questions. I ran the agent-instruction (AGENTS.md), history and CI passes myself.
• Did not run:
  • The prior-changes pass, because reading earlier reviews is out of bounds for this run.
  • Live checks of the claude-sonnet-5.5 model slug and Copilot CLI 1.0.97 behaviour.
• Disclosures:
  • One contract test writes and then deletes a file under /tmp by design. One run of it happened before it was excluded from later runs.
  • Scratch logs (rr-*.txt, wc-log.txt) were left in the disposable checkout.
• Not reconciled against any PR discussion here.

[juhovainio]

Addressed in f3ec4869 and e993de2a (round 7, review 5632364164 at 0c5f1fed).

Blocking, gating: left unchanged, needs the author.

• Commit-message history. Still holds. fc76d6a8, 40f5d624, ef489f5d and 1cb356de still describe removed behavior. Fixing it means squashing or rewording and force-pushing the branch. This pass only pushes new commits on top, so it does not do that. It has to happen before the PR is queued.

Blocking, non-gating: fixed (f3ec4869).

• Narrative can forge a GO heading. The wiki page now labels the section "Narrative (drafted by an LLM from public issue and commit text; not the verdict)" and block-quotes every narrative line, so line-leading #, |, * and _ cannot form a top-level heading, table or bold line. The model no longer receives verdict (jq 'del(.verdict)'), and the prompt says issue and commit text is untrusted data. New contract tests run the real sed line against a forged heading, table and bold line, and pin the jq step. Capping the issues handed to the prompt is not done; see below.
• Untested hand-offs. Added tests for an unparseable Total Fail cell (None), matching artifact names across the upload/download chain (narrative, facts/table, consolidated report), and contents: write on publish-release-readiness-wiki.
• Red checks. Not code in this diff. The reclaim test and the MI300X lane are flakes or red on main, and Strix Windows hit a tarball host 403. I did not touch them, and a re-run is what will clear them. The description's claim that a rebase fixes the preflight test is wrong, as the review notes, and I have not edited the description.

Non-blocking, fixed.

• publish-e2e-wiki silent skip is now documented in docs/ci-hardware-testing.md.
• Re-run caveat added to the --e2e-report-job-result doc and the lane-check comment in release_readiness.rs.
• collect_known_issues_in_fails_outside_a_github_repo skips when gh is missing or GH_REPO is set. e993de2a also clears the hook-exported GIT_DIR/GIT_WORK_TREE for the gh child. Under the pre-push hook, gh resolved the repo from GIT_DIR instead of the temp dir and returned live issues, which is what failed the test there.

Left unchanged on purpose.

• e2e_report::discover emitting an EMPTY row for an artifact directory without report.json. It touches the E2E report producer's shared rendering. Left for a follow-up.
• Deriving the channel legs from the lane matrix in uploaded_report_artifact. Left for a follow-up.
• Capping issues and commits in the prompt and appendix. It needs a decision on the cap and on what the appendix shows. Left for a follow-up.
• The three "Decisions for the author" (NO-GO almost every night, ticket-prefixed commit subjects, wiki push race) are the author's calls and are untouched.

[juhovainio]

The standing commit-message objection from review 5632364164 is addressed: the 19 branch commits are squashed into one, b204a686, whose message describes the final code (no --e2e-lane-result flag, no per-lane needs, the e2e-report-nightly gate, escaping and the v2.1.1 pin as they are now). The tree is byte-identical to the previous tip e993de2a, so no code changed in this push. Force-pushed with --force-with-lease. The PR description still describes the history rewrite as pending and the preflight-test root cause as a stale base; I left it unedited for the author.

Addressed in b204a68 (history squashed into one commit with an accurate message; non-gating items fixed in the same stack)

[rominf]

🔴 Automated review · pr-review-watcher · b204a68

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.

Round 8, a full review of b204a686. The previous review point, 0c5f1fed, is no longer in the branch history, so the review could not be narrowed. Each finding below is marked new. Round 7's change request was dismissed after the squash.

The round 7 objection is settled. It said commits 40f5d624, fc76d6a8, ef489f5d, 1cb356de and e20b631 describe code that no longer exists. The blind review did not re-find it. I then put the objection to the reviewer directly; that check was handed the objection, so it is anchored rather than blind. Both point the same way:

• The branch is now one commit.
• None of the stale claims, such as --e2e-lane-result, the per-lane needs, link stripping or a bare v3 pin, appear in its message.
• The message describes the final gating, escaping and pin correctly.

This change request rests on one item only: the first item under Blocking. It is a new false claim in the squashed commit message. Rewording one paragraph clears it.

The other three Blocking items do not gate:

• Each one is a test that stays green under a mutation. The workflow behaves correctly today, so each is a missing guard against a future regression. That is below the gating floor, and round 7 treated its untested hand-offs and its hard-coded-channels item the same way. Fixing them is still recommended.
• The red Test (affected crates) check is not caused by this diff. CI checked out b204a686 itself, which does not contain #712, and the test passes on main. A rebase onto current main should clear it.
• The round 7 comment and the PR description call that check a flake. This run's checkout shows the stale base is the real cause.

Review — needs work

Full review of the whole change.
Implements ROCMAI-568 ("Nightly release-readiness summary (wiki page)"), and covers what that ticket describes. The PR names no GitHub issue to close.

Blocking

• new — this is what gates — The commit message gets the effect of the LANE_REPORTS change wrong, and that text becomes the squash body on main — commit b204a68 message, paragraph starting "e2e-selfhosted.yml:"

  • What it says: the e2e-selfhosted.yml change makes a missing leg "fail e2e-report-nightly", and "as a result, publish-e2e-wiki also skips".
  • What the code does: neither of those jobs exists in e2e-selfhosted.yml. Its check lives in the per-PR e2e-report job (e2e-selfhosted.yml:1632). e2e-report-nightly and publish-e2e-wiki are in nightly.yml (lines 1514 and 1633).
  • Where the effect really comes from: the nightly.yml LANE_REPORTS rewrite, which now requires both legs for all 7 nightly lanes. The commit message never mentions that change; its "Workflow (.github/workflows/nightly.yml)" bullets cover only the three new jobs.
  • What it leaves out: the actual effect of the e2e-selfhosted.yml change, which is that the per-PR self-hosted report check now also fails when the Strix Ubuntu nightly leg is missing.
  • The code is the side that governs here; the message describes jobs that do not exist in the file it names.
  • Confidence 85 · mechanical · Fix: reword the commit message. Attribute the e2e-report-nightly / publish-e2e-wiki effect to the nightly.yml LANE_REPORTS change, and give the e2e-selfhosted.yml change its real per-PR effect.

• new; does not gate, see banner — The contract test hard-codes the release/nightly legs instead of reading them from each lane's matrix — xtask/src/workflow_contract.rs:1122-1133 (uploaded_report_artifact), and the "Derived from the lanes" comment at :1146-1149

  • Run, Strix Ubuntu: I changed the lane's matrix in e2e-selfhosted.yml:434 to channel: [release]. cargo test -p xtask workflow_contract still passed (83 of 83). In that state the real check would demand a nightly artifact that can never exist, and fail on every run.
  • Run by the worker, rad3: adding a third channel to the nightly rad3 lane also stayed green, so that leg would go unchecked.
  • This is a check whose comment claims a derivation it cannot fail on.
  • Confidence 100 · architectural · Fix: read the job's channel: list and emit one artifact name per value.

• new; does not gate, see banner — The behavioural test of the per-leg loop never tests the release leg missing on its own, and its "skip a middle lane" case no longer removes the skipped lane's artifacts — xtask/src/workflow_contract.rs:1405-1433, :1372-1378

  • Run, release leg: I changed the loop to for artifact in "${wanted[@]: -1}" in both nightly.yml and e2e-selfhosted.yml, so it checks only the last (nightly) leg. All 83 tests stayed green. The release leg, which used to be the only leg checked, could therefore silently stop being checked. Cause: one_leg_missing only ever removes the second leg from split_once(','), and the victim lane has only one artifact.
  • Traced, not run, middle lane: lanes[1] is now Strix Ubuntu, whose value is comma-joined. The filter a != middle_artifact compares single names against that joined string, so it removes nothing. The case therefore no longer models a skipped lane that uploaded nothing.
  • Confidence 100 · mechanical · Fix:
    • run the one-leg-missing case once for each leg;
    • filter with !middle_artifact.split(',').any(|m| m == a).

• new; does not gate, see banner — Nothing checks that release-readiness-nightly lists the build jobs it reads results from — .github/workflows/nightly.yml:1705-1709, and the release_readiness_nightly_gates_on_the_e2e_report_job_not_individual_lanes test in xtask/src/workflow_contract.rs

  • Run: I deleted - windows-nightly from that job's needs. All 83 tests stayed green, because the test only matches the literal --windows-result "${{ needs.windows-nightly.result }}" text.
  • Effect: with the need gone, that expression evaluates to an empty string. Every night would then read NO-GO with "windows build ()". The job is also no longer ordered after the Windows build through its own needs.
  • Confidence 100 · logic · Fix: assert that needs contains both nightly and windows-nightly.

Non-blocking

• new — The red Test (affected crates) check was not caused by this diff.

  • What failed: only workflow_contract::tests::preflight_runs_the_real_reclaim_assert_clear, with the error coreutils: unknown program 'llama-server'. This PR leaves that test untouched.
  • Main is green: main's CI run at the base tip 63ecab2d passed the same test.
  • Where it was fixed: 5dfa9a8 (#712, "keep the GPU reclaim decoys alive on multicall coreutils") is on main. This branch's merge-base 2d2dfc7 is 18 commits behind main and predates it.
  • Fix: rebase onto current main.
  • Confidence 85 · mechanical.

• new — The trailing-newline comment in run() gives a reason that cannot apply — xtask/src/release_readiness.rs:103-107

  • The comment says the newline keeps the $GITHUB_OUTPUT heredoc delimiter on its own line "even if that step's own echo guard is ever dropped".
  • That step never copies facts.json directly. It runs jq 'del(.verdict)' …, and jq always writes its own trailing newline, so the file's newline has no effect on the heredoc.
  • The PR description repeats the same claim.
  • Confidence 85 · mechanical · Fix: drop or reword the comment. The newline itself does no harm.

• new — A contract-test name claims more than the narrative quoting delivers — publish_release_readiness_wiki_quotes_the_narrative_so_it_cannot_forge_a_verdict in xtask/src/workflow_contract.rs; the sed comment in the nightly.yml "Push page to the wiki" step

  • Prefixing each line with > still lets headings, tables and bold text render, just inside a blockquote. So > ## Release Readiness — ✅ GO still renders as a heading.
  • The YAML comment's word "top-level" is accurate, so the comment is the side to keep and the test name should be narrowed to match it.
  • I reasoned this from GFM rules; nothing was rendered.
  • Confidence 70 · mechanical · Fix: rename the test to say "top-level", or also escape a leading # and every | so the stronger claim becomes true.

• new — The narrative prompt reaches the Copilot CLI as a single command-line argument, so it has a size ceiling — nightly.yml "Draft narrative" step (prompt: ${{ steps.facts.outputs.json }}); collect_known_issues_in (--limit 1000)

  • ai-inference v2.1.1 passes the prompt with -p <prompt> through spawn.
  • Linux limits a single argument to 128 KiB. At about 400 bytes per issue, the facts JSON passes that at roughly 280 open bugs; there are about 25 today.
  • Past that point the narrative fails every night without anyone noticing, because the job is continue-on-error.
  • Confidence 70 · logic · Fix: cap the number of issues sent to the model, or pass them through a file.

• new — run() and the dispatch in main.rs have no test — xtask/src/main.rs:398-414, xtask/src/release_readiness.rs:79-112

  • The seven parameters are passed by position as plain &str/&Path.
  • Swapping windows_result and e2e_report_job_result would compile and leave all tests green.
  • Confidence 70 · logic · Fix: put the inputs in a struct with named fields (or a ValueEnum for job results), and add one test that drives run with a fixture report.

Decisions for the author

• new — The GO/NO-GO verdict ignores publish-nightly, which runs the repo's real release gate — non-blocking-improvement

  • publish-nightly runs scripts/release_readiness.py --require-signatures --require-exact-assets.
  • A night where both builds succeed but that exact-asset or signature check fails still reads GO.
  • Gating on that job's result would make the page match its name. Treating skipped as acceptable only on a no-op night would follow the same rule windows-nightly already uses.

• new — Requiring both legs on every lane widens what can block the E2E wiki page and the verdict — tradeoff

  • For: a missing nightly leg artifact on any one lane is now caught, so no leg silently drops out of the report.
  • Against: that one missing artifact now suppresses the whole E2E support-matrix page, including every other lane's results. It also forces NO-GO, even when the lane is one that is still being proven out (W7900, MI350P). This is documented; confirm it is intended.

• new — Two different tools share the name "release-readiness" — tradeoff

  • cargo xtask release-readiness and the existing scripts/release_readiness.py gate do different things.
  • ROCMAI-568 addresses this deliberately: "Not to be confused with ROCMAI-443 … a different tool with a similarly-named command, not a conflict or rename."
  • Readers of AGENTS.md §8 and docs/testing.md may still mix the two up.

Positive signals

• Untrusted text never reaches a run: script through a ${{ }} expression. The narrative goes through env:, the facts through a random heredoc delimiter, and the only interpolated values are needs.*.result strings.
• The new action pins check out: ai-inference@a780588 is the v2.1.1 commit and setup-node@949feb2 is the v7.1.0 commit. They carry # vX.Y.Z comments, as AGENTS.md requires.
• total_e2e_failures is tested against the real output of e2e_report::consolidated_summary_markdown, not a hand-written snippet. It fails closed on a missing Total row, an unparseable Total row, and an EMPTY platform.
• Every artifact name listed in LANE_REPORTS matches what that lane's upload step produces in both workflows. No listed artifact is impossible to produce today.

Deployment notes

• The narrative needs a COPILOT_PAT repo secret holding a Copilot-licensed PAT. Without it the narrative fails every night, quietly, because the job is continue-on-error. The table still publishes.
• The repository wiki must already exist. This is the same one-time setup publish-e2e-wiki needs.
• The per-PR self-hosted "E2E consolidated report" check now also fails when the Strix Ubuntu nightly leg uploads no report.

What this covered

• What was read: every file in the change, completely (7 files, prw-base...HEAD, commits 63ecab2d…b204a686). That includes all of xtask/src/release_readiness.rs, the workflow_contract.rs diff with the helpers it uses, every lane's matrix and upload steps in both workflows, and the e2e-report producer in crates/e2e-report/src/consolidated.rs.
• How it was split: two independent workers.
  • One covered the release-readiness module, its CLI wiring and the three new nightly jobs.
  • The other covered the workflow contract tests, the LANE_REPORTS changes and the docs.
  • The driver re-ran the four test-strength mutations itself (83 tests ran each time); all four stayed green.
• Other checks:
  • Read the CI rollup for the head commit and compared it with main's runs.
  • Sign-off check: exit 0, 1 commit checked, nothing missing.
  • Ran the AGENTS.md §2 leak scan over the diff: no hits.
  • Retrieved ROCMAI-568 by its identifier, without comments.
• Did not run: the prior-changes pass, because no earlier reviews of these files were consulted. The history pass was limited to git log on nightly.yml.
• Not reconciled against this PR's discussion.

[juhovainio]

Addressed in 5c732f80 (round 8, review 5651935137 at b204a686). The branch is still one commit, reworded and rebased onto current main (63ecab2d), force-pushed with --force-with-lease.

Blocking, gating: fixed.

• The commit message now attributes the e2e-report-nightly / publish-e2e-wiki effect (and the NO-GO verdict) to the nightly.yml LANE_REPORTS change, which requires both legs for all 7 lanes. It gives the e2e-selfhosted.yml change its real effect: the per-PR e2e-report check also fails when the Strix Ubuntu nightly leg uploads no report.

Blocking, non-gating: fixed.

• uploaded_report_artifact now reads each lane's channel: [..] list instead of hard-coding release/nightly. Setting the Strix Ubuntu matrix to [release] now fails every_lane_that_runs_is_checked_for_its_report_artifact.
• The one-leg-missing case runs once per leg, and the skip-a-middle-lane filters match comma-joined artifact names.
• release_readiness_nightly_gates_on_the_e2e_report_job_not_individual_lanes asserts nightly and windows-nightly are in needs. I re-ran the deletion mutation and it now fails.

Non-blocking, fixed.

• The red Test (affected crates) check: rebased onto main, which has fix(ci): keep the GPU reclaim decoys alive on multicall coreutils #712.
• The trailing-newline comment in release_readiness.rs no longer claims a heredoc effect.
• The narrative-quoting test is renamed ..._cannot_forge_a_top_level_verdict.
• The narrative prompt copy is capped to the top 100 reaction-ranked issues (jq 'del(.verdict) | .known_issues |= .[:100]'), keeping it far under the 128 KiB argument limit. The contract test pins the new filter. The raw appendix is unchanged.

Left unchanged on purpose.

• run() parameters as a struct with a test: a signature refactor, left for a follow-up.
• The three "Decisions for the author" (publish-nightly gating, both-legs-block-the-page, the shared "release-readiness" name) are the author's calls and are untouched.
• The PR description still repeats the stale heredoc claim; I did not edit it.

fmt, cargo test -p xtask (383) and clippy --workspace --all-targets pass.

Addressed in 5c732f8

[nowycondro]

Automated high-recall review pass (8 finder angles: line-by-line diff scan, removed-behavior audit, cross-file tracing, reuse, simplification, efficiency, altitude, CLAUDE.md conventions). The cross-file tracer, removed-behavior auditor, reuse, and conventions angles turned up nothing solid after verification — this PR's prior review rounds already closed most of the obvious cross-file/behavioral gaps. 8 findings survived verification, posted inline below, ranked roughly most-to-least severe.

[nowycondro]

Narrative sanitization is weaker than the mechanical table's escaping on the same page.

This sed chain neutralizes \, [, ], <, > and blockquotes every line, but never escapes #, *, `, or | — unlike escape_markdown() in release_readiness.rs, which is applied to issue titles/commit subjects on this same generated page and additionally escapes those characters.

The comment above claims quoting every line "can't forge a top-level verdict heading, status table, or bold 'all gates passed' line," but GitHub renders > # Heading as a blockquote containing heading-styled (large/bold) text — just indented, not neutralized. If the model ever reflects attacker-controlled text from an issue title/commit subject (despite the system prompt instructing it not to), a line like # Release Readiness — ✅ GO or **All gates passed** would still render with heading/bold styling inside the blockquote, which a skimming reader could mistake for corroborating (or contradicting) the real verdict above it.

Suggest reusing escape_markdown's character set (or exposing it as a small CLI/xtask subcommand this sed step could pipe through) so both trust boundaries on this page share one tested escaping implementation instead of two that can silently drift.

[nowycondro]

is_internal_only_scope is case-sensitive, so a differently-cased scope bypasses the internal-tooling filter.

INTERNAL_SCOPES (line 206) is all lowercase, and this function compares s.trim() against it via exact &str equality — no case-folding. A commit subject like fix(CI): retry flaky job or fix(Nightly): ... would not be recognized as internal-only ("CI" != "ci"), so it survives categorize()'s filter and leaks into the public "Bug Fixes" bucket (and the narrative LLM's input), even though it's exactly the kind of CI/nightly-internal commit this filter exists to drop.

I didn't find a commitlint/commit-msg hook in this repo enforcing lowercase Conventional-Commits scopes, so this is reachable from any contributor who happens to capitalize a scope. Consider scope.split(',').all(|s| INTERNAL_SCOPES.contains(&s.trim().to_ascii_lowercase().as_str())) (or normalizing once before the .all()).

[nowycondro]

.known_issues |= .[:100] slices the narrative's input after build_known_issues has already sorted it by 👍-reaction count (descending). Once the repo has more than 100 open bug-labeled issues, a brand-new, zero-reaction-but-genuinely-blocking issue (e.g. a crash filed yesterday) sorts below the cap and is cut before the LLM ever sees it — even though the system prompt explicitly tells the model to judge relevance from title/body "not its reaction count (reaction count is not a reliable signal of real-world impact)". The cap silently contradicts that instruction once the issue count grows past it. (The unfiltered table.md list is unaffected — only the curated narrative input.) A simple mitigation: dedupe the 128 KiB concern from relevance by sending titles-only for issues beyond some smaller N, instead of dropping them outright.

[nowycondro]

total_e2e_failures recovers structured per-platform pass/fail data by re-parsing e2e_report::consolidated_summary_markdown's already-rendered markdown — scanning for the literal substrings "| Platform"/"| **Total**" and then indexing columns positionally (.nth(5) for Fail, .get(8) for Status at line ~434). That coupling is purely string/offset-based: if e2e_report's table format is ever reordered or gains a column (e.g. an Xfail-reason column inserted before Status), nothing here would fail to compile — the offsets would just silently point at the wrong cells, and because this still returns Some(n) rather than None, it wouldn't even trip the "report unreadable" failing gate this function was designed to prefer over a wrong answer. Since e2e_report already has the typed pass/fail/skip/xfail counts moments before rendering them to markdown, exposing a small typed summary (or at least named column-index constants shared with the renderer) would remove this cross-crate drift risk.

[nowycondro]

collect_commit_subjects (shells out to local git log) and collect_known_issues (a network round-trip via gh issue list --json ... --limit 1000) are independent of each other — only categorize(&subjects) depends on the git call — but they run strictly sequentially here. Every nightly run pays git_log_latency + gh_api_latency back-to-back instead of max(...). Since this runs in a billed CI job, spawning both subprocess calls concurrently (e.g. std::thread::spawn + join) and only serializing on categorize afterward would shave real wall-clock time off every nightly run.

[nowycondro]

npm install -g @github/copilot@1.0.97 reinstalls the same pinned version from the registry on every nightly run, with no cache. Since the version is pinned and only changes when this line is edited, caching the global npm prefix (or ~/.npm) keyed on the pinned version string via actions/cache would let unchanged-version runs restore the installed CLI instead of re-downloading/reinstalling it every night.

[nowycondro]

assemble_facts's three job-result gates (nightly_result, windows_result, e2e_report_job_result, all plain &str) are each matched against a different hand-picked accepted-value set ("success" | "skipped" for the two build gates, "success"-only for the E2E report job) rather than going through one shared typed gate helper (e.g. a JobResult enum with a fn passes(&self, allow_skipped: bool) -> bool). This is adjacent to the already-acknowledged "untyped result strings" follow-up in the PR description, but specifically: a future 4th gate added here has nothing stopping a copy-paste of the wrong accepted-value set (e.g. accepting "skipped" for a gate that, like e2e_report_job_result, has no legitimate no-op night) — the compiler can't catch it since the whole check is string literal equality.

[nowycondro]

This downloads the entire e2e-consolidated-report-nightly artifact (both support-matrix.md and the rendered index.html) solely to read support-matrix.md a few lines below — index.html is never referenced by anything in this job. Since e2e-report-nightly already produces both files separately before bundling them into one artifact, uploading support-matrix.md as its own smaller artifact (or exposing it via a job output, if it stays under the size cap) would let this job skip pulling down the HTML report it never opens.

[nowycondro]

🤖 Automated review (Claude Code) · 5c732f8

This supplements the inline comments and review I just posted (#655 (review)) with a single consolidated summary in the required format.

What I read: the full diff of PR #655 at commit 5c732f8 (.github/workflows/nightly.yml release-readiness job, xtask/src/release_readiness.rs, and related wiring in main.rs). I spot-checked CLI-flag wiring between main.rs/release_readiness.rs/nightly.yml and the job-dependency graph. What I could not verify: no Rust toolchain was available in this sandbox, so none of this was compiled or executed — all findings are from static reading of the diff. I did not re-run the nightly workflow or exercise the actual GitHub API calls it makes.

Findings (most severe first):

1. Non-blocking (confidence: high) — .github/workflows/nightly.yml:1943: the sed-based markdown sanitization applied to the LLM narrative doesn't escape #, *, `, or | the way the Rust escape_markdown() used elsewhere on the same page does. If the narrative ever reflects attacker-influenced issue/commit text, a line like # Release Readiness — ✅ GO still renders as a styled heading inside the blockquote (GitHub renders > # Heading as indented-but-styled), undermining the comment's claim that quoting prevents a forged verdict line.

2. Non-blocking (confidence: medium-high) — xtask/src/release_readiness.rs:220: is_internal_only_scope compares commit scopes case-sensitively against an all-lowercase INTERNAL_SCOPES list. A commit like fix(CI): retry flaky job won't match "ci" and will leak into the public "Bug Fixes" bucket. No commitlint/hook enforcing lowercase scopes was found in the repo.

3. Non-blocking (confidence: medium) — .github/workflows/nightly.yml:1803: known_issues is sorted by reaction count and capped at 100 before reaching the narrative prompt, despite the step's own comment acknowledging reaction count isn't a reliable relevance signal. Once the repo has >100 open bug-labeled issues, a new zero-reaction but genuinely blocking issue can be silently excluded.

4. Non-blocking (confidence: medium) — xtask/src/release_readiness.rs:413: total_e2e_failures recovers structured E2E data by re-parsing already-rendered markdown via hardcoded column indices (.nth(5), .get(8)) rather than typed data. If the table format is reordered or gains a column, offsets silently point at the wrong cells and the function still returns Some(n) instead of None, bypassing the "report unreadable" failing gate it was designed to prefer over a wrong answer.

5. Non-blocking / efficiency (confidence: medium) — xtask/src/release_readiness.rs:88: collect_commit_subjects (git log) and collect_known_issues (gh issue list network call) run strictly sequentially in run() despite being independent, adding avoidable wall-clock time to a billed CI job.

6. Non-blocking / design (confidence: low-medium) — .github/workflows/nightly.yml:1727: the three job-result gates in assemble_facts use ad-hoc string matching with a different accepted-value set per gate rather than a shared typed helper, making it easy for a future 4th gate to copy-paste the wrong accepted-value set.

7. Non-blocking / efficiency (confidence: medium) — .github/workflows/nightly.yml:1786: npm install -g @github/copilot@1.0.97 reinstalls the same pinned version from scratch every nightly run with no caching.

8. Non-blocking / efficiency (confidence: medium) — .github/workflows/nightly.yml:1721: release-readiness-nightly downloads the full e2e-consolidated-report-nightly artifact (including index.html) solely to read support-matrix.md; index.html is never referenced by this job.

No findings here rise to blocking in my judgment — they're hardening/robustness/efficiency items rather than correctness bugs that break the feature as described. The merge decision remains with a human reviewer.

🤖 Generated with Claude Code

[nowycondro]

🤖 Automated review (Claude Code) · 5c732f8

I read the current head of the release-readiness xtask (xtask/src/release_readiness.rs) and the corresponding nightly workflow wiring (.github/workflows/nightly.yml), independent of the long existing human review thread, looking for anything still wrong rather than re-litigating settled points.

Likely bugs (worth a look before merge):

• xtask/src/release_readiness.rs:206-223 — is_internal_only_scope compares commit scopes case-sensitively against the all-lowercase INTERNAL_SCOPES list (ci, nightly, xtask, …), and the scope string is taken verbatim from the raw subject with no lowercasing anywhere in categorize(). A commit like fix(CI): retry flaky job or fix(Nightly): ... won't match and will leak into the public "Bug Fixes" bucket on the wiki page and into the LLM narrative's input, defeating the stated purpose of the filter. High confidence — verified by reading both the list and the call site.
• xtask/src/release_readiness.rs:166,496-501 — build_skipped = nightly_result == "skipped" || windows_result == "skipped" feeds a single combined "Build (Linux + Windows nightly)" status row, so if either leg is skipped while the other ran and passed, the published row reads "not run" — which reads as "no build evidence at all" when e.g. Linux nightly actually ran green. High confidence.
• .github/workflows/nightly.yml:1803 — jq 'del(.verdict) | .known_issues |= .[:100]' truncates to the first 100 after build_known_issues (release_readiness.rs:380) has already sorted by 👍-reaction count descending. Once a repo has >100 open bug-labeled issues, a newly filed but genuinely blocking issue with few reactions is silently dropped from what the narrative step ever sees — directly contradicting that same step's system prompt, which tells it to judge relevance from title/body "not its reaction count." Medium-high confidence.
• xtask/src/release_readiness.rs:413-434 (total_e2e_failures) — re-parses the e2e crate's already-rendered markdown table by scanning for literal substrings and indexing cells positionally (.nth(5), .get(8)). The surrounding comments show the author already hit one breakage this way; another column reorder upstream would silently misread counts instead of falling back to the "report unreadable" failing-gate path this function exists to protect. Medium confidence (not broken today, but fragile coupling with no compile-time link between the two crates).

Non-blocking:

• .github/workflows/nightly.yml:1943 — the blockquote-escaping sed chain for the LLM narrative only escapes \, [, ], <, >, unlike the stricter escape_markdown() (release_readiness.rs:457, which also escapes */`/_) used elsewhere on the same page. Exploitability depends on the narrative ever echoing attacker-influenced text despite the system prompt forbidding it, so this is defense-in-depth rather than a demonstrated hole, but worth aligning with the stricter escaper for consistency.
• collect_commit_subjects (local git log) and collect_known_issues (gh issue list network call) in run() execute strictly sequentially despite having no data dependency on each other — minor nightly-runtime cost, not correctness.
• .github/workflows/nightly.yml:1786 reinstalls the same pinned @github/copilot@1.0.97 via npm on every run with no cache; .github/workflows/nightly.yml:1721 downloads the full e2e-consolidated-report-nightly artifact (including index.html) solely to read support-matrix.md. Both are minor CI-efficiency nits.

Could not verify: no Rust toolchain or CI runner available in this sandbox, so none of this was compile-checked or executed — findings are from static reading of the diff/source only.

🤖 Generated with Claude Code

[rominf]

🔴 Automated review · pr-review-watcher · 5c732f8

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.

Round 9, a full review of 5c732f80. The previous review point, b204a686, is no longer in the branch history after the rebase, so the review could not be narrowed. Every finding below is new this round.

Nothing in this round gates, so no change request is filed. Round 8's change request (5651935137) has already been dismissed.

The round 8 objection is settled. It said the squashed commit message credited the e2e-report-nightly / publish-e2e-wiki effect to e2e-selfhosted.yml and never mentioned the nightly.yml LANE_REPORTS rewrite. The blind review did not re-find it. I then put the objection to the reviewer directly; that check was handed the objection, so it is anchored rather than blind. I also read the current message and nightly.yml's LANE_REPORTS block myself. All three point the same way:

• The message now credits that effect, and the NO-GO verdict, to the nightly.yml LANE_REPORTS change. That block lists both legs for all 7 lanes.
• The e2e-selfhosted.yml paragraph now gives only that file's real effect: the per-PR check fails when the Strix Ubuntu nightly leg uploads no report.

The two items under Blocking do not gate. Each is a test that stays green under a mutation, while the code it guards behaves correctly today. That is a missing guard against a future regression, which is below the gating floor. Round 8 treated its three test-strength items the same way. Fixing them is still recommended.

The red E2E tests (MI300X, current) check does not gate. The same job also fails on main's latest e2e-selfhosted runs. E2E tests (W7900, current) is still queued, so that lane's evidence is missing rather than failing.

Review — needs work

Full review of the whole change.
Implements ROCMAI-568 (the nightly release-readiness wiki page) in full. The PR also tightens the LANE_REPORTS check in both nightly.yml and e2e-selfhosted.yml. The ticket does not cover that, but the commit message and the description both disclose it.

Blocking

• new; does not gate, see banner — Two verdict-rendering branches in release_readiness.rs can be broken without any test failing — xtask/src/release_readiness.rs:447, xtask/src/release_readiness.rs:497
  • The non-PASS check at :447. In total_e2e_failures, the platform-row check !matches!(status, Some("PASS") | None) && fail == 0 exists to count a FAIL row whose Fail cell reads 0 as a failure. The legacy known-bugs path in crates/e2e-report/src/consolidated.rs can produce that row: an XPASS gives status FAIL while raw stats.failed is 0. I mutated the check to matches!(status, Some("EMPTY")) && fail == 0, which drops that case and turns it into GO.
  • The Build row at :497. I mutated g.contains("build") to g.contains("nightly build"). A failed Windows build then renders the Build row ✅ under a NO-GO headline.
  • With both mutations applied, cargo test -p xtask release_readiness::tests passed 50 of 50. Only the EMPTY status and the Linux build failure are ever exercised.
  • Confidence 100 · logic · Fix: add a test with a FAIL row whose Fail cell is 0 (assert NO-GO), and a render test where only windows_result fails (assert the Build row shows ❌).
• new; does not gate, see banner — The workflow hand-offs for the new jobs are not pinned, and when one breaks the run stays green — .github/workflows/nightly.yml:1737, :1833, :1842, :1763, :1910
  • A review worker ran these single mutations; each left all 87 workflow_contract tests passing:
    • the narrative upload path: changed to narrative/narrative.md;
    • "Save narrative" writing to ./narrative.md instead;
    • --e2e-report consolidated/support-matrix.md changed to consolidated/index.md;
    • continue-on-error: true removed from release-readiness-narrative;
    • the degenerate-table header check grep -q '^| Area | Evidence | Status |' replaced with [ ! -s table.md ].
  • Why this matters: the narrative job is continue-on-error. A path drift would therefore leave the wiki permanently on the "Narrative generation did not complete" fallback, or permanently on EVIDENCE UNAVAILABLE, with nothing going red.
  • release_readiness_artifact_hand_offs_use_matching_names compares artifact names only. The guard test's only degenerate input is "", so it cannot tell "no header" apart from "empty".
  • The docs claim the narrative is "allowed to fail without blocking the table", and no test checks that.
  • Confidence 100 · logic · Fix: assert that upload and download paths agree with the paths written and read. Assert the --e2e-report file name against what e2e-report-nightly writes. Pin continue-on-error on the narrative job. Add a header-less, non-empty table case to the guard test.

Non-blocking

• new — A revert of a breaking commit is listed as a new breaking change, and reverting a feature or fix leaves it listed — xtask/src/release_readiness.rs:240-260
  • Revert "feat!: drop flag (#12)" (#13) has the head Revert "feat!. That ends in !, so it goes into breaking. A worker's probe test confirmed it.
  • Revert "feat: x" matches no bucket, so the reverted feature still shows under New Features.
  • Any non-conventional head ending in !, such as Wow!:, is also read as breaking.
  • Confidence 85 · logic · Fix: skip or specially handle Revert " subjects, and only treat a head as breaking when it matches the conventional type(scope)! form.
• new — skipped is accepted as passing for the Linux nightly build, but that job cannot be skipped — xtask/src/release_readiness.rs:127
  • The nightly job has no needs: and no if:, so nothing legitimate produces skipped.
  • The comment's justification ("windows-nightly's own skip flag") only applies to the Windows gate.
  • Accepting it for Linux is an unneeded fail-open path, and assemble_facts_treats_a_skipped_nightly_build_as_passing pins it in place.
  • Confidence 70 · logic · Fix: accept only success for nightly_result.
• new — No test pins least-privilege permissions on the new jobs — .github/workflows/nightly.yml:1700, :1760, :1851
  • The narrative job holds a PAT and reads untrusted issue text. It currently runs with contents: read.
  • A worker broadened each job's permissions (contents: write/issues: write/actions: write), and every contract test still passed.
  • publish_release_readiness_wiki_can_push_to_the_wiki only checks that contents: write is present. The commit message's "contract tests covering … permissions" overstates what is pinned.
  • Confidence 85 · logic · Fix: assert each job's exact permissions: block.
• new — The block-quote test can pass when sed produces no output — xtask/src/workflow_contract.rs, publish_release_readiness_wiki_quotes_the_narrative_so_it_cannot_forge_a_top_level_verdict
  • The test never checks the exit status and never checks that any line came out.
  • Its sibling escaping test does check both, so the pair is covered today.
  • Confidence 70 · logic · Fix: assert success and a non-empty output.
• new — Two unrelated "release readiness" tools now share a name, and the docs do not tell them apart — docs/ci-hardware-testing.md:202, .github/workflows/nightly.yml:13
  • scripts/release_readiness.py is the signature/packaging gate that AGENTS.md §8, docs/testing.md and docs/release-trust.md refer to.
  • nightly.yml:13 says "every readiness step below runs --require-signatures". That is not true of the new jobs.
  • The ticket's naming note acknowledges the two tools, but nothing in the repo does.
  • Confidence 70 · mechanical · Fix: add a sentence to the new docs paragraph that distinguishes the two, and narrow the nightly.yml:13 comment.
• new — The new docs paragraph cites nightly.yml as a bare backtick path — docs/ci-hardware-testing.md:202
  • AGENTS.md §5 asks for new citations of repo files to be relative links. The older prose around it uses the same pattern.
  • Confidence 70 · mechanical · Fix: [nightly.yml](../.github/workflows/nightly.yml).

Decisions for the author

• new — run() and main.rs argument wiring is untested and untyped — answered-by-intent
  • A worker found that these mutations survive the full suite:
    • swapping nightly_result and windows_result in main.rs or in run();
    • replacing total_e2e_failures(&e2e_text) with Some(0) in run().
  • Would otherwise be blocking under "behaviour introduced is tested".
  • The PR description says: "main.rs → release_readiness::run's argument wiring and its 5 result strings remain untested and untyped … a ValueEnum JobResult would be the fix, left for a follow-up."
  • Confirm that the follow-up is still planned. Splitting a read-report-and-assemble function out of run() would cover the Some(0) case without that refactor.
• new — One PR carries two logical changes — tradeoff
  • The LANE_REPORTS both-legs change in nightly.yml, e2e-selfhosted.yml and its contract test stands on its own. It changes when the per-PR consolidated-report check and publish-e2e-wiki fail. AGENTS.md §11 asks for one logical change per PR.
  • On the other side, the verdict's E2E gate depends on that check being complete, and the commit message documents both changes.
  • The change itself is correct: all 7 nightly lanes upload both legs, and the per-PR sdk_version_matrix always includes current.
• new — The two wiki publishers use separate concurrency groups — tradeoff
  • publish-e2e-wiki and publish-release-readiness-wiki push to the same wiki repo with no retry. A single-job re-run can therefore fail on a non-fast-forward push.
  • Normal ordering makes overlap unlikely. A shared group would remove the possibility, at the cost of serialising the two pages.

Positive signals

• The narrative escaping holds against everything tried. A worker rendered adversarial input through GitHub's markdown API: inline, image, reference-style, nested and footnote links, raw <img>, autolinks and pre-escaped brackets all render as text, and each line stays inside the block quote.
• LLM output reaches the shell only through env:, and the $GITHUB_OUTPUT heredoc delimiter is random. ${{ }} in run: only ever carries needs.*.result enum values.
• The verdict fails closed on failure, cancelled, timed_out, empty, unknown and unreadable inputs. Gating on e2e-report-nightly's own result, rather than on lane results masked by continue-on-error, is the right signal.

Deployment notes

• A COPILOT_PAT repo secret (a Copilot-licensed PAT) is needed before the narrative will draft. Without it, the narrative step fails silently under continue-on-error, and the page publishes only the table plus the fallback line.
• From the first nightly run after merge, any lane whose nightly leg dies before uploading turns e2e-report-nightly red. That also stops the E2E-Support-Matrix wiki page from updating.

What this covered

• What was read: all 7 changed files, completely, between prw-base f6c8a8f and HEAD 5c732f8. Also read:
  • the report producer in crates/e2e-report/src/consolidated.rs (status_text/ok);
  • every nightly lane's matrix and upload steps;
  • the existing publish-e2e-wiki job.
• How the passes ran: two independent read-only workers, one on the xtask module plus main.rs, one on the workflows, contract tests, prompt and docs. The workers ran mutation passes over each new test, about 29 and 34 mutations respectively. The two release_readiness survivors were re-run by the reviewer.
• Source-reading passes: agent-instruction adherence and history (git log on the touched workflows) ran in the reviewing context. The prior-changes pass did not run, because earlier review comments are deliberately out of scope for the review.
• CI: every check is green except E2E tests (MI300X, current), which also fails on main's latest e2e-selfhosted runs and so is not attributed to this PR. E2E tests (W7900, current) is still queued.
• Sign-off check: check_signoff.py reported 1 commit checked, none missing (exit 0).
• Description and ticket: the description was read, and ROCMAI-568 was retrieved by its ID.
• What was not verified: the claude-sonnet-5.5 Copilot model slug, and whether v* release tags exist upstream (the checkout has none).
• Concurrent-edit caveat: both workers mutated the shared checkout at the same time. The affected runs were re-run with narrower filters, and the workflow files were confirmed to match HEAD afterwards.
• The review was not reconciled against any PR discussion; the standing-objection check in the banner happened after it.

[nowycondro]

🔴 Automated review · pr-review-watcher · 5c732f8

Review

Full review of the whole change.
Implements Jira ROCMAI-568 ("Nightly release-readiness summary (wiki page)"); ticket status is "Backlog" but the PR's own scope matches the ticket's summary exactly, including its disambiguation from the unrelated scripts/release_readiness.py --require-signatures script.

Blocking

• E2E tests (MI300X, current) is red on the current head — PR-level status check, commit 5c732f80.
  gh pr view 655 --json statusCheckRollup shows this check as FAILURE while every other check (including E2E tests (W7900, current), E2E consolidated report (self-hosted), Test (affected crates), etc.) is green. The PR description asserts this is "a pre-existing flake unrelated to this diff," but this review could not independently verify that: the environment's network egress proxy returns 403 on repos/.../actions/jobs/* and actions/runs endpoints, so the job log and a same-job comparison against main's recent runs were not reachable from here. Per this review's standing rule, a failing check is reported as blocking until confirmed otherwise, not assumed benign from the description's say-so.
  Confidence 85 (the check's red state is directly observed; only the "unrelated to this diff" characterization is unverified) · logic · Fix: before merging, confirm (e.g. by re-running the job, or pointing at a concurrent failure on main for the same lane) that this is genuinely pre-existing and not a regression from this change; if confirmed, this is a non-issue.

Non-blocking

None

Decisions for the author

• Two jobs now push to the same wiki git remote with no shared lock — tradeoff/answered-by-intent, .github/workflows/nightly.yml:1633-1692 (publish-e2e-wiki) and :1850-1964 (publish-release-readiness-wiki).
  Each job independently clones https://.../${GITHUB_REPOSITORY}.wiki.git (shallow, depth 1), writes a different page, commits, and git push origin HEAD under set -euo pipefail with no retry. They use separate concurrency.group values (publish-e2e-wiki vs publish-release-readiness-wiki) and nothing else serializes them, so if both run close together in the same nightly run, the second push can hit a non-fast-forward rejection and fail outright. Before this PR there was only one job pushing to the wiki, so this is a new race, not a pre-existing one. The PR description already names this precisely: "publish-e2e-wiki and publish-release-readiness-wiki use separate concurrency groups (each only serializes against itself) and neither retries a transient wiki-clone failure; a retry/backoff story for wiki pushes would be a repo-wide improvement, not something specific to this PR's new job." That is a real decision to leave it, not an oversight, so it's recorded here rather than under Blocking — would otherwise have been a blocking architectural finding (confidence 75: mechanism verified by reading both job definitions, real-world trigger odds depend on scheduling timing this review can't observe). The failure mode is self-healing (the next night's run succeeds cleanly) but produces a spurious red run; confirm that's an acceptable ongoing cost.

• release-readiness-nightly/release-readiness-narrative have no branch/repo guard; only publish-release-readiness-wiki does — tradeoff, .github/workflows/nightly.yml:1699-1849.
  Verified directly: release-readiness-nightly's if: is only ${{ !cancelled() }} and release-readiness-narrative's is only ${{ !cancelled() && needs.release-readiness-nightly.result == 'success' }} — neither checks github.repository/github.ref. Only publish-release-readiness-wiki (the job that actually writes anywhere) carries github.repository == 'ROCm/rocm-cli' && github.ref == 'refs/heads/main'. So a manual workflow_dispatch of nightly.yml from any branch computes facts and (if COPILOT_PAT is available there) drafts a narrative, without ever being able to publish it — burning CI minutes and a Copilot-CLI call but never reaching the wiki. Reasonable as designed (nothing public leaks beyond normal Actions-run visibility), but confirm the minute/API cost on a stray dispatch is acceptable rather than tightening the first two jobs' guards to match the third.

• Internal-commit-type filtering only looks at scope, not type, so a breaking commit of an internal type is never dropped — tradeoff/answered-by-intent, xtask/src/release_readiness.rs:220-269 (is_internal_only_scope, categorize).
  Traced independently: INTERNAL_SCOPES (ci, nightly, xtask, build, e2e, etc.) is checked only against a commit's parenthesized (scope), never against its bare Conventional-Commit type before the colon. A commit like ci!: rework the dispatch matrix (no scope, type ci, marked breaking) is unconditionally pushed into buckets.breaking regardless of its internal type — confirmed by categorize_treats_a_bang_on_any_type_as_breaking's own test, which exercises this with docs!: and asserts it lands in breaking. The PR description already flags this exact case: "Whether internal-type breaking commits (ci!:, chore!:, build!:) should be dropped from the Breaking bucket is left as the reviewer explicitly flagged it as intent-dependent; no such commits exist in this branch today, so it doesn't affect the current page's output either way." No live commit triggers it today, so this doesn't change this page's output — confirm the intended behavior for when one eventually does.

• Stringly-typed job-result arguments could be silently swapped by a future refactor — non-blocking-improvement, xtask/src/main.rs:398-414 → xtask/src/release_readiness.rs:79-170.
  nightly_result, windows_result, and e2e_report_job_result are three same-typed String parameters threaded positionally through run → assemble_facts. Correctly wired and test-pinned today (verified end-to-end against nightly.yml's --nightly-result/--windows-result/--e2e-report-job-result flags), but nothing at the type level would catch a future reordering. A small enum JobResult { Success, Skipped, Other(String) } would make a swap a compile error instead of a silent behavior change. The PR description already defers this as a larger main.rs argument-surface refactor than this PR's scope — reasonable to leave for a follow-up.

• Whether non-blocking hardware lanes (W7900, MI350P) should count toward the NO-GO verdict — tradeoff.
  Not something this review can decide unilaterally; the gate currently keys only on nightly, windows-nightly, and e2e-report-nightly's own result (which in turn depends on every lane in LANE_REPORTS having uploaded a report, blocking or not). Confirm this matches the intended release gate.

Positive signals

• assemble_facts's gates fail closed on every axis checked: an unparseable/missing E2E total reads None → NO-GO rather than defaulting to zero failures, and any nightly_result/windows_result value other than the literal "success"/"skipped" is treated as a failing gate rather than silently passing — confirmed directly in xtask/src/release_readiness.rs:124-159 and its paired tests.
• Defense-in-depth around the LLM narrative step: the mechanical verdict is stripped from the JSON the model sees (nightly.yml's jq 'del(.verdict)'), the model's output is then escaped (not just link-stripped) before reaching the wiki with a dedicated bypass-via-nesting test, and the rendered page explicitly labels the section as "drafted by an LLM from public issue and commit text; not the verdict" — three independent layers rather than relying on prompt wording alone.
• The new workflow_contract.rs tests extract and execute the real shell snippets from the YAML under bash (e.g. the narrative-escaping sed command, the env-only-reaches-the-shell check) rather than asserting against a hand-copied string, so they catch a real drift between the workflow file and what the test believes it does.

Deployment notes

• release-readiness-narrative needs a COPILOT_PAT secret (a Copilot-licensed PAT) configured in the repo; without it the step fails but the pipeline tolerates this (the table still publishes, with "narrative generation did not complete this run").
• publish-release-readiness-wiki requires the repo's wiki to already have at least one page created via the web UI (GitHub does not create the wiki's git repo otherwise) — the job fails with an explicit message if not, it does not silently no-op.
• First real nightly dispatch after merge is the only way to confirm release-readiness-nightly/release-readiness-narrative/publish-release-readiness-wiki behave end-to-end in production; the PR description notes a local dry run and one successful manual workflow_dispatch (run 38396499964) but flags a follow-up dispatch to confirm the fail-safe paths specifically as still outstanding.

What this covered

Every file in the diff, read completely: xtask/src/release_readiness.rs (new, full file), xtask/src/main.rs (changed regions plus dispatch context), .github/prompts/release-readiness-narrative.txt (full), .github/workflows/nightly.yml and .github/workflows/e2e-selfhosted.yml (full files), every new/changed test function and helper in xtask/src/workflow_contract.rs, and docs/ci-hardware-testing.md (changed regions), via two independent fan-out workers grouped by coupling (Rust core logic + prompt; CI workflows + their contract tests), plus direct spot-verification of assemble_facts, total_e2e_failures, categorize, and the wiki-publish job bodies against the real files, cross-checked against crates/e2e-report/src/consolidated.rs for the E2E status-value assumptions. Base/head reviewed: 63ecab2d26ffb569ed857a0e7b0ab26a12bd5853…5c732f80f217c5b36a2f2d8a6a4c358ed64829bb, diffed locally via git merge-base/git diff against a local clone (GitHub's PR-diff API and Actions-log/run-listing endpoints both returned 403 through this environment's network proxy and were not used).

Did not run: cargo test/cargo clippy (no cargo toolchain in the worker environment) — all Rust findings are from static tracing and cross-referencing tests to production code, not execution; a GitHub Actions job-log/history comparison for the failing MI300X check (blocked by the proxy, see Blocking above); a dedicated "prior changes" pass (this PR's own description documents many rounds of a different automated reviewer's feedback, which — per this method's blind-review rule — was deliberately not shown to either fan-out worker and was read by this top-level pass only at the point the rubric calls for reading the change's stated intent, after the code-level findings above were already settled).

Methodology note: this PR already carries a distinct prior automated review (nowycondro, a "high-recall" pass) and a long series of reviews from a different account (rominf, tagged pr-review-watcher) including one already at this exact head (5c732f80). This review is independent of both — it did not read their conclusions, only (per the dedup check this task required) their author/body-prefix metadata to confirm no nowycondro review already covered this head under this review's own tag.

[nowycondro]

🔴 Automated review · pr-review-watcher · 0343a9f

Review — needs work

Full review of the whole change (single commit 0343a9f on top of merge-base ea6dd11).
Implements Jira ROCMAI-568 ("Nightly release-readiness summary wiki page") in full; the PR description's scope matches the ticket's summary closely, including one disclosed, reasoned deviation from the ticket's "mirrors publish-e2e-wiki's guards" framing — publish-release-readiness-wiki also runs (with an explicit "EVIDENCE UNAVAILABLE" notice) when release-readiness-nightly fails outright, not only on success, so the wiki page never goes stale without saying so.

Blocking

• prek (lint / hygiene) CI check is failing on this PR's current head commit — .github/workflows (CI), job prek (lint / hygiene), run 40666987693
  GitHub's status-check rollup for commit 0343a9f5 reports this job as FAILURE. I could not retrieve the job's logs or annotations in this review environment (the GitHub API returned 403/proxy errors for check-runs/.../annotations and job-log endpoints) to determine root cause. Manual spot-checks of every file this diff touches — trailing whitespace, final-newline, CRLF, YAML validity — found nothing that would trip the hygiene hooks this job runs (trailing-whitespace, end-of-file-fixer, check-yaml, mixed-line-ending, ruff-check/ruff-format, shellcheck, cargo-fmt --check, per .pre-commit-config.yaml), which makes an unrelated/pre-existing flake plausible — but that is not established from here, only suggested. The PR description's blanket claim ("CI check failures... are pre-existing flakes unrelated to this diff") is an assertion, not evidence, and does not name this specific job; the one CI failure the description does explain by name ("Test (affected crates)... multicall-coreutils decoy issue") is a different check that currently shows SKIPPED, not this one.
  Confidence 100 (that the check is failing, per GitHub's own rollup) · mechanical/CI · Fix: open the actual job log and either fix the real hygiene violation, or confirm and document that this specific job's failure is an unrelated, pre-existing flake.

• Build-row priority behavior is introduced by this change and has no test for the one case where it actually matters — xtask/src/release_readiness.rs:496-503 (the if ... contains("build") { "❌" } else if build_skipped { "not run" } ... chain), tests at xtask/src/release_readiness.rs:1037-1053 and :1056-1070
  The new render_table logic must decide, per build row, whether a failing build gate (❌) outranks a skipped sibling build (not run) when both are true at once (e.g. nightly_result="failure", windows_result="skipped"). The current if/else if order gets this right today, but no test sets both conditions simultaneously — the two existing tests each isolate one condition alone. Swapping the branch order (checking build_skipped first) would still pass both existing tests. This is new behavior this PR introduces, cheaply testable, and the PR's own later rounds explicitly closed equivalent gaps elsewhere (e.g. "Added tests for... build_skipped's skipped half for both the nightly and Windows build gates") without covering this specific combination.
  Confidence 85 (traced directly against the two cited tests and the branch order) · logic/test-coverage · Fix: add a test with nightly_result="failure", windows_result="skipped" (or the reverse) asserting the row still renders ❌, not not run.

Non-blocking

• Three new Action pins in nightly.yml are older than every other call site of the same actions in this file — .github/workflows/nightly.yml:1725 (actions-rust-lang/setup-rust-toolchain@166cdcf... # v1.17.0, vs. @ecabd13d... # v2.0.0 used at 7 other sites in this file); :1728,1782,1871,1883 (actions/download-artifact@3e5f45b2... # v8.0.1, vs. @9000827c... # v8.0.2 used elsewhere); :1751,1845 (actions/upload-artifact@043fb46d... # v7.0.1, vs. @cf430e03... # v7.0.2 used at 8+ other sites)
  All three are still full-SHA pins per this repo's own convention, just stale ones — likely authored against an earlier branch state before the repo-wide Dependabot action bump (#688) landed. No confirmed behavioral break: the workflow's own top-level comment notes RUSTFLAGS: -D warnings is set at the workflow level specifically so warnings-as-errors reaches jobs regardless of setup-rust-toolchain version, so the version drift is a consistency/maintenance issue rather than a demonstrated regression.
  Confidence 90 · mechanical · Fix: bump all three to the SHAs already used elsewhere in this file (Dependabot will otherwise pick this up on its own schedule).

• Status-table row glyphs are derived by substring-matching free-text gate messages, not a structured tag — xtask/src/release_readiness.rs:497 (g.contains("build")), :487 (g.starts_with("E2E")), vs. the gate strings assemble_facts builds at :128-152
  Works correctly today — no existing or plausible-today gate string collides — but nothing ties the two together structurally. A future rename of a gate message, or a new gate whose text happens to contain "build", could silently desync the row glyph from the real gate state, and no test would catch it since existing tests only assert against strings the current code itself produces.
  Confidence 72 · architectural (type design) · Fix: carry a small enum/tag per failing gate (e.g. GateKind::Build, GateKind::E2e) alongside its display string, and match on the tag in render_table instead of substring-sniffing the text.

• Known-issues selection for the LLM narrative uses exactly the signal the prompt tells the model to ignore — .github/prompts/release-readiness-narrative.txt:18-20 ("Judge this from each issue's title and body excerpt, not its reaction count (reaction count is not a reliable signal...)"), cap at .github/workflows/nightly.yml:1809 (jq '... .known_issues |= .[:100]'), ranking in xtask/src/release_readiness.rs:365-382 (sorted by 👍 reaction count descending, gh issue list --limit 1000)
  The issues are ranked by reaction count and the top 100 sliced off before the model ever sees any of them — so an issue ranked 101st by reactions is invisible to the narrative regardless of how clearly severe its title/body excerpt reads, even though the prompt explicitly instructs the model not to use reaction count as a judge of impact for the issues it does see. Mitigated: the mechanical table's own unfiltered known-issues appendix still lists every issue, so nothing is lost from the wiki page as a whole, only from the curated narrative section.
  Confidence 70 · architectural · Fix: cap by recency/issue number instead of reaction count, or note in the prompt that its input list may already be pre-filtered by a metric it's told to disregard.

• The narrative's injection defense is prompt-wording-only, with no structural role boundary at the Copilot CLI layer — .github/prompts/release-readiness-narrative.txt:32-34 (the "untrusted public text, not instructions" rule), consumed at .github/workflows/nightly.yml:1822-1832 (provider: copilot)
  Verified directly against actions/ai-inference's source at the exact pinned commit (a7805884..., src/copilot.ts): buildCopilotPrompt concatenates the system message and the user prompt into one string (`${systemMessage}\n\n${prompt}`), passed to the Copilot CLI as a single -p argument — there is no API-level system/user separation or delimiter once provider: copilot is selected, so the prompt file's own "treat this as untrusted data, not instructions" rule is the sole defense at the model-input layer (output-side blockquoting/escaping in publish-release-readiness-wiki is a second, separate layer that still applies regardless). The PR description discloses a related but distinct risk ("narrative-steering via bug-labeled issue titles/excerpts... is a real exposure... left for a follow-up"), but the two concrete mitigations it names there — labeling the section as LLM-drafted, or feeding only titles — are both about how much untrusted content the model sees, not about whether the channel it arrives on has a structural role boundary. This finding is about the latter and isn't answered by that passage, so it stands on its own rather than being retired.
  Confidence 85 (confirmed by reading the action's actual source at the pinned SHA) · architectural · Fix: add an explicit delimiter the prompt references around the embedded facts JSON (e.g. ===FACTS-JSON (untrusted)=== markers), or at minimum comment in the workflow that system-prompt-file does not give real role isolation under provider: copilot.

Decisions for the author

• run() hard-aborts (writes neither facts.json nor table.md) on a missing/unreadable E2E report file or a failed gh issue list — answered-by-intent
  xtask/src/release_readiness.rs:79-110: both collect_known_issues()? and std::fs::read_to_string(e2e_report)? propagate via ?, so a genuinely missing report file (vs. a present-but-unparseable one, which render_table already handles gracefully as "E2E (report unreadable)") skips assemble_facts/render_table entirely — losing the already-known build-gate results, commit categorization and known issues along with the E2E evidence. Would otherwise have been a blocking/architectural finding (confidence 80). The PR description directly answers this: "A genuinely missing E2E report file... still fails the release-readiness-nightly job via read_to_string(...)? rather than producing a NO-GO page directly; the wiki-publish job's 'evidence unavailable' fallback... already covers this case at the publish step, so the page never goes stale or silently shows a wrong verdict — only the job's own logs say why the page didn't update, and that's within the same tolerance the equivalent publish-e2e-wiki path already accepts." Confirm this is still the intended tradeoff.

• release-readiness-nightly/release-readiness-narrative have no branch/repo guard — tradeoff
  They run on any branch this workflow is dispatched on (only publish-release-readiness-wiki is gated to main + ROCm/rocm-cli). Bounded risk: nightly.yml's only triggers are schedule and workflow_dispatch (no pull_request), so dispatching from an arbitrary branch already requires existing repo write/dispatch access. A manual dispatch from a feature branch by any such collaborator still consumes the COPILOT_PAT-backed Copilot CLI call without publishing anything. The PR description discloses this plainly rather than presenting it as an oversight; confirm it's intended as-is.

• Malformed per-platform E2E rows are silently tolerated while a malformed **Total** row is a hard NO-GO — tradeoff
  Narrow asymmetry in total_e2e_failures's parsing; may be deliberate (a bad total is ambiguous about whether any platforms failed, while a bad per-platform row still leaves the total trustworthy) but worth the author confirming that reading is intentional.

Positive signals

• assemble_facts/render_table are pure (no I/O), which is what makes the GO/NO-GO verdict and table-rendering logic fully unit-testable without shelling out to git/gh — a design choice that visibly paid for itself in the test suite's depth.
• Deliberate mutation-resistant tests at exactly the high-risk edges: the n > 0 vs n > 1 failure-count boundary, multibyte-safe slicing in split_trailing_pr_reference, **Total**-row-before-header ordering, and total_e2e_failures's scoping — tested against the real e2e_report::consolidated_summary_markdown producer rather than a hand-rolled fixture.
• workflow_contract.rs's tests extract and execute the real bash/sed snippets from the actual YAML under bash -c (confirmed by hand-tracing several, including the markdown-escaping pipeline and the three-way wiki-publish guard), not copies that could silently drift from production.
• Layered, well-reasoned defense for the narrative's untrusted content: backslash-doubling runs before bracket-escaping (correctly defeats a self-escaping bypass attempt), and every line is blockquoted (s/^/> /), which neutralizes forged headings/tables even from markup the escaping step doesn't target.
• Single, clean, DCO-signed commit whose message accurately describes the final code with no stale claims — a commit-message-accuracy objection raised in an earlier round of this PR's own review history appears to have been properly resolved by this head commit.

Deployment notes

• Requires a COPILOT_PAT repository secret (a Copilot-licensed PAT; a bare GITHUB_TOKEN is not accepted by the Copilot CLI actions/ai-inference shells out to). Its absence degrades gracefully: release-readiness-narrative fails via continue-on-error: true and the mechanical table still publishes without a narrative section.
• First run needs the repo wiki to already have at least one page saved via the web UI (GitHub does not create the wiki git repo otherwise) — the same prerequisite publish-e2e-wiki already has, now shared by publish-release-readiness-wiki.

What this covered

Read the full diff (ea6dd112e435d34e432b36ae7c8b2b88d57594ae...0343a9f5a6a5337cc23da84377298c9677f5f97a, 7 files, +2379/-36) and every changed file completely, via four independent blind fan-out passes grouped by coupling (xtask/src/release_readiness.rs + xtask/src/main.rs; .github/workflows/nightly.yml + .github/workflows/e2e-selfhosted.yml; xtask/src/workflow_contract.rs; .github/prompts/release-readiness-narrative.txt + docs/ci-hardware-testing.md), plus my own direct verification of every candidate finding against the source (including cloning actions/ai-inference at its pinned commit to confirm the role-collapse claim, and grepping every Action-pin call site across the touched workflow files) and the existing publish-e2e-wiki job for pattern comparison.

Did not run: cargo test/clippy/fmt — no Rust toolchain is available in this review environment and crates.io/rustup are network-blocked here, so Rust-level findings are scored on traced logic and cross-referenced tests rather than an actual run. History/prior-changes passes were light: xtask/src/release_readiness.rs is a net-new file (no history to check); nightly.yml/e2e-selfhosted.yml's changes were compared against the existing publish-e2e-wiki job they mirror; a full trawl of past review-thread comments on these files was not performed. CI status was read from gh pr view --json statusCheckRollup (taken at face value); the one failing check's own logs/annotations could not be fetched in this sandbox (API access blocked) — noted as a Blocking finding rather than assumed.

Not reconciled against this PR's own review discussion/threads — this report was produced blind to them, per method, and reads the PR description only for Step 9 (intent-vs-code reconciliation), never as a source of findings.

[nowycondro]

🔴 Automated review · pr-review-watcher · e639a98

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, via three independent blind fan-out passes grouped by coupling: (1) xtask/src/release_readiness.rs + xtask/src/main.rs; (2) xtask/src/workflow_contract.rs + .github/workflows/e2e-selfhosted.yml; (3) .github/workflows/nightly.yml + .github/prompts/release-readiness-narrative.txt + docs/ci-hardware-testing.md.
Implements Jira ROCMAI-568 ("Nightly release-readiness summary (wiki page)") in full; the ticket's summary matches the PR's scope exactly, including its disambiguation from the unrelated pre-existing scripts/release_readiness.py --require-signatures gate. Ticket status is "Backlog" but the PR description's own account of the shipped behavior matches the code.

Blocking

None.

Non-blocking

• Three new GitHub Actions pins in nightly.yml are older than every other call site of the same actions in this file — .github/workflows/nightly.yml:1725 (actions-rust-lang/setup-rust-toolchain@166cdcf... # v1.17.0, vs. @ecabd13d... # v2.0.0 used at 8 other sites in this file), :1728,1782,1871,1883 (actions/download-artifact@3e5f45b2... # v8.0.1, vs. @9000827c... # v8.0.2 used elsewhere), :1751,1845 (actions/upload-artifact@043fb46d... # v7.0.1, vs. @cf430e03... # v7.0.2 used at 8+ other sites).
  Confirmed directly: grepped every call site of all three actions in the PR-head file, and confirmed the base commit uses only the newer SHAs throughout — this PR introduces the older ones fresh, only in its new jobs. All three are still full-commit-SHA pins with a trailing version comment, so this satisfies this repo's own pinning rule; it's an unexplained version regression, not a security gap.
  Confidence 90 · mechanical · Fix: align the three new pins to the SHAs already used elsewhere in this file, or note why they intentionally differ.

Decisions for the author

• Two jobs now push to the same wiki git remote with no shared lock — answered-by-intent, .github/workflows/nightly.yml:1639-1698 (publish-e2e-wiki, pre-existing) and :1856-1970 (publish-release-readiness-wiki, new).
  Each job independently clones https://.../${GITHUB_REPOSITORY}.wiki.git (shallow), writes a different page, commits, and does a plain git push origin HEAD (lines 1698 / 1970) with no retry. They carry separate concurrency.group values (publish-e2e-wiki at line 1648 vs. publish-release-readiness-wiki at line 1867) and no needs: ordering between them, so if both run close together in the same nightly run the second push gets a non-fast-forward rejection and the job fails outright. Before this PR there was exactly one job touching the wiki repo, so this specific cross-job race could not previously occur — it is introduced by this PR. Would otherwise be a blocking architectural finding (confidence 85: verified directly from both job bodies, not merely argued). The PR description already names the exact mechanism and accepts it: "publish-e2e-wiki and publish-release-readiness-wiki use separate concurrency groups (each only serializes against itself) and neither retries a transient wiki-clone failure; a retry/backoff story for wiki pushes would be a repo-wide improvement, not something specific to this PR's new job." The failure mode is self-healing (the next night's run succeeds cleanly) but produces a spurious red run each time it triggers — confirm that ongoing cost is still acceptable, since a shared concurrency.group across both jobs would remove it cheaply.
• release-readiness-nightly/release-readiness-narrative have no branch/repo guard — tradeoff, .github/workflows/nightly.yml:1699-1849. Verified: their if: conditions check only !cancelled() (and the narrative job additionally checks the facts job's result), while only publish-release-readiness-wiki checks github.repository == 'ROCm/rocm-cli' && github.ref == 'refs/heads/main'. Already disclosed in docs/ci-hardware-testing.md and the PR description. Low risk (workflow_dispatch already requires repo write access), but a manual dispatch from any branch computes facts and drafts a narrative (burning COPILOT_PAT/CI minutes) without ever being able to publish. Confirm that cost is acceptable as-is.
• E2E evidence is derived by re-parsing the rendered support-matrix table (splitting on |, indexing columns 5/8) rather than consuming crates/e2e-report's structured types directly — tradeoff, xtask/src/release_readiness.rs. Fragile to column reordering in that crate; the code's own comment explains the choice (stays in lockstep with that crate's known-bugs/xfail handling), and one regression test exercises the real producer output rather than a hand-written fixture. Worth confirming the coupling is an accepted ongoing cost.
• collect_known_issues_in hard-caps at --limit 1000 open bug-labeled issues with no pagination — tradeoff, low likelihood today, confirm it's accepted rather than an oversight.

Positive signals

• assemble_facts's gates fail closed on every axis: an unparseable/missing E2E total reads as None → NO-GO rather than defaulting to zero failures, and any job-result value other than the literal "success"/"skipped" is treated as failing rather than silently passing.
• Defense-in-depth around the LLM narrative step: the mechanical verdict is stripped from the JSON before the model sees it (jq 'del(.verdict)', covered by a dedicated test), the model's output is then escaped — not just link-stripped — before reaching the wiki (with a bypass-via-nesting regression test), secrets/outputs are passed via env: rather than interpolated into run: shell strings, and the $GITHUB_OUTPUT heredoc uses a per-run random delimiter specifically because the JSON carries untrusted issue titles.
• workflow_contract.rs's new tests extract and execute the real shell snippets from the actual YAML (e.g. the LANE_REPORTS per-leg check, the narrative-escaping sed pipeline) rather than asserting against a hand-copied string, and the per-leg test actually withholds an artifact and asserts the real script fails naming only that leg — this is checking production behavior, not a parallel reimplementation that could pass even if the real logic broke.
• Deliberate mutation-resistant tests at the high-risk edges: multibyte-safe slicing in split_trailing_pr_reference, the exact n > 0 vs n > 1 failure-count boundary, and categorize's internal-only-scope detection reading off head_no_bang rather than head (confirmed by hand that reverting to head would leak an internal-only breaking commit into the Breaking bucket).
• Pure logic (assemble_facts, categorize, render_table, total_e2e_failures) is cleanly separated from I/O, making the GO/NO-GO verdict fully unit-testable without shelling out to git/gh.

Deployment notes

• release-readiness-narrative requires a COPILOT_PAT repository secret (a Copilot-licensed PAT; a bare GITHUB_TOKEN is not accepted by the Copilot CLI actions/ai-inference shells out to). Its absence degrades gracefully — the mechanical table still publishes without a narrative section.
• publish-release-readiness-wiki requires the repo wiki to already have at least one page saved via the web UI (GitHub does not create the wiki's git repo otherwise) — the same pre-existing prerequisite publish-e2e-wiki has, now shared by a second page, and the job fails with an explicit message rather than a silent no-op if unmet.
• The PR's own test plan already flags that a follow-up live nightly dispatch is still needed to confirm the new fail-safe/guard paths end-to-end in production (local/dry-run verification is in place).

What this covered

Every file in the diff ea6dd112e435d34e432b36ae7c8b2b88d57594ae...e639a98250f961201871eced46bd8137922574a0 (7 files, +2379/-36), read completely by three independent blind fan-out workers grouped by coupling, each also reading the surrounding context needed to judge its files (call sites, the real e2e-report producer, the pre-existing publish-e2e-wiki job for comparison). I independently re-verified the wiki-push-race and action-pin findings directly against the PR-head YAML before scoring them, and checked live CI (gh/GitHub check-runs API): all required checks are green on this exact commit; two individual Strix-Ubuntu E2E matrix legs show cancelled but the consolidated "E2E consolidated report (self-hosted)" gate that depends on them still reports success, consistent with this PR's own tested per-lane-artifact-presence logic rather than a regression.

Did not run: cargo test/clippy/fmt myself or via the fan-out workers (no Rust toolchain available in these environments) — Rust-level findings and verifications are from direct tracing against the source and existing tests, not execution. No dedicated git log/blame history pass was run (release_readiness.rs is a net-new file; the workflow/doc diffs were judged against their current form and the pre-existing publish-e2e-wiki pattern they mirror, not their own history). This PR carries an extensive prior review history on this same branch (including earlier rounds from this same automated account); per this method's blind-review rule, none of that was shown to the fan-out workers or used to reach any finding above — it was read only afterward, to check for an existing review at this exact head (none found) and to evaluate the stated-intent reconciliation in "Decisions for the author."