[fredespi]

Why

The latest nightly E2E support matrix was only readable by opening a run's artifacts. Publishing it to the repo wiki makes it visible without a manual step (ROCMAI-488).

What

• nightly.yml: e2e-report-nightly also saves the markdown matrix to consolidated/support-matrix.md.
• New publish-e2e-wiki job overwrites the E2E-Support-Matrix wiki page nightly (run link, commit, date in header). ROCm/rocm-cli only.
• Contract test in xtask/src/workflow_contract.rs.

Prerequisite

The wiki git repo does not exist until its first page is saved in the web UI; until then the job fails with an explicit error saying so.

Test plan

• cargo test -p xtask contract test
• Linux gate on acer: only the known architecture_doc container artifact fails
• After the wiki is initialised, dispatch Nightly and confirm the page updates

[jussielo-amd]

🔴 Automated review · pr-review-watcher · 27946f6

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

Review — needs work

Full review of the whole change (PR #590, ROCm/rocm-cli, base 58c03df6…head 27946f6d).
Implements the nightly-wiki-publishing scope of ROCMAI-488 ("Publish the latest nightly E2E support matrix on a rocm-cli wiki page"), not a claim to close it — the PR body references the ticket parenthetically only, matching ROCMAI-488's current In Progress status.

Blocking

• Required CI check "Commit signatures + sign-off" is failing on both commits — no location in the diff itself; this is a repo-policy gate (AGENTS.md §2/§8: "a blocking CI gate (commit-signatures) re-checks every PR with GitHub 'Verified' status").
  Confirmed live: gh api .../check-runs shows Commit signatures + sign-off = failure (completed 2026-10-07T13:08:15Z). The job log (cargo xtask verify-commits --require-verified) reports both edf201f7 and 27946f6d with -> signature is not GitHub-"Verified". DCO sign-off itself is present (both commits carry Signed-off-by: fredespi-agent-app[bot] ...), so the only outstanding issue is that the commits aren't cryptographically signed/registered as GitHub-Verified.
  Confidence 100 · architectural (blocking required gate) · Fix: enable commit signing for the authoring identity (SSH or GPG) and re-sign the two commits (git rebase --signoff --exec 'git commit --amend --no-edit -S' <base> or equivalent), then force-push and re-run the check.

Non-blocking

• A transient git clone failure is indistinguishable from "wiki not initialized." .github/workflows/nightly.yml, publish-e2e-wiki step "Push page to the wiki": if ! git clone --depth 1 "$wiki" wiki; then echo "::error::Cannot clone the wiki. Enable the wiki ..."; exit 1; fi treats any clone failure (including a transient network blip) as "the wiki was never initialized," printing instructions to create a wiki page that may not actually be the problem.
  Confidence 70 · logic · Fix: optionally distinguish "repository not found" from other clone errors (e.g. capture stderr and grep for the not-found message), or just note in the error text that a non-404 failure may be transient.

Decisions for the author

• Manual one-time wiki bootstrap vs. a PAT that can self-initialize the wiki — tradeoff
  The second commit's comment states "git init cannot create the remote wiki." Community precedent confirms the default GITHUB_TOKEN generally lacks permission to push an uninitialized .wiki.git, but a fine-grained PAT with wiki/contents-write can create it on first push. The chosen design (default token, explicit failure, manual one-time UI step) avoids managing an extra secret at the cost of a one-time manual step and failing-nightly noise until that step happens. That's a reasonable, more secure default — confirm it's the deliberate choice rather than something to revisit with a PAT later.

Positive signals

• The fix commit (27946f6d) correctly replaces a subtly-broken git init -b master wiki; git remote add origin fallback — which, per GitHub's documented wiki semantics, still fails at git push for an uninitialized wiki, just more confusingly — with an explicit, actionable ::error:: and exit 1. Verified this reasoning against external confirmation of GitHub's wiki-repo-creation behavior.
• The new regression test (xtask/src/workflow_contract.rs::nightly_publishes_the_consolidated_matrix_to_the_wiki, specifically !publish.contains("git init")) precisely targets the bug just fixed. Traced: the assertion would fail against the pre-fix commit's own code, which literally contains git init -b master wiki, and passes after the fix — confirmed by running it (cargo test -p xtask workflow_contract::tests::nightly_publishes_the_consolidated_matrix_to_the_wiki on the PR head in an isolated worktree: ok).
• Correct, repo-consistent housekeeping: job-level permissions: contents: write scoped only to the job that needs it, the github.repository == 'ROCm/rocm-cli' fork guard, and actions/download-artifact pinned to a full commit SHA with a trailing # v8.0.1 comment matching the existing convention.

Deployment notes

After merge, every nightly run will fail at publish-e2e-wiki (and likely trigger GitHub's default scheduled-workflow-failure email) until a maintainer manually visits the repo's Wiki tab and saves an initial page — the PR's own test-plan item for this ("After the wiki is initialised, dispatch Nightly and confirm the page updates") is still unchecked. Worth doing that bootstrap step immediately at/after merge to avoid repeated failure noise.

What this covered

Read completely: .github/workflows/nightly.yml (the full e2e-report-nightly and new publish-e2e-wiki job blocks, plus the file header/permissions/triggers) and xtask/src/workflow_contract.rs (the new test plus the job_block/job_scalar/read_workflow helpers it depends on), against 58c03df6…27946f6d. Also read both individual commits (edf201f7, 27946f6d) separately to isolate the bug-fix commit from the feature commit. Ran the new contract test against the PR head in an isolated git worktree (cargo test -p xtask workflow_contract::tests::nightly_publishes_the_consolidated_matrix_to_the_wiki → ok). Queried live PR/check status (gh pr view, gh api .../check-runs, job logs) directly rather than trusting an earlier snapshot. Retrieved ROCMAI-488 via Jira. Ran the AGENTS.md internal-keyword leak scan over the full diff (clean).

Did not run: the full cargo test --workspace --all-targets / cargo clippy --workspace gate — deferred to CI's own already-SUCCESS results for clippy, Test (affected crates), build-and-test, etc. History and prior-changes passes found no directly relevant prior PR touching wiki-publishing in this repo. Not reconciled against any PR discussion/thread — none was read for this review.

[fredespi]

#590 (review)

Re-signed all commits under my own identity; all three now read Verified on GitHub (head 06245c5). Clone-failure error text now says a non-first-run failure may be transient and to re-run. On the wiki bootstrap: the manual one-time first-page step with the default token and no PAT is deliberate and acceptable.

[jussielo-amd]

Full review of the whole change (.github/workflows/nightly.yml, xtask/src/workflow_contract.rs). Implements ROCMAI-488 (checked against the ticket directly) — scope, the "no history" overwrite design, and acceptance criteria all match.

Blocking

None

Non-blocking

None

Decisions for the author

• publish-e2e-wiki is gated on needs.e2e-report-nightly.result == 'success' and github.repository == 'ROCm/rocm-cli', but not on github.event_name == 'schedule' — tradeoff
  nightly.yml allows workflow_dispatch with no ref restriction (GitHub always permits picking an arbitrary branch there, independent of this file). Anyone with write access could manually dispatch Nightly from a feature branch and have this job overwrite the shared E2E-Support-Matrix wiki page with that branch's results. The page header does attribute the actual run/commit, so it's traceable, and dispatching already requires the same write access that could edit the wiki directly — so this is a design question, not a vulnerability. Worth confirming it's intended, or adding && github.event_name == 'schedule' if not.

Positive signals

• The "cannot clone the wiki" path gives an explicit ::error:: message distinguishing the first-run prerequisite (wiki never initialized) from a transient clone failure, matching what the PR description calls out.
• The new contract test (nightly_publishes_the_consolidated_matrix_to_the_wiki) asserts the real coupling points — the report job keeps the markdown as a file, the wiki job needs it, downloads the right artifact, targets .wiki.git, and never uses git init. Verified by running it directly against this PR's branch in a disposable worktree — passes.
• concurrency: group: publish-e2e-wiki, cancel-in-progress: false correctly serializes overlapping nightly runs against the single mutable wiki page.
• Action SHA-pin reuses the same pin already used elsewhere in the file, consistent with repo convention.

What this covered

Read both changed files in full plus surrounding context (the e2e-report-nightly job this extends, top-level permissions/on block, and xtask's job_block/job_scalar helpers). Verified cargo xtask e2e-report's stdout contract against xtask/src/e2e_report.rs to confirm the new > consolidated/support-matrix.md redirect still captures what previously went to $GITHUB_STEP_SUMMARY. Built and ran the new contract test against the PR branch in a disposable worktree — passes. Ran the repo's leak scan over the diff and PR body — clean. Confirmed all 26 CI checks green via gh pr view --json statusCheckRollup. Retrieved ROCMAI-488 from Jira directly and compared its scope/acceptance criteria against the implementation.

Did not run: history/prior-changes passes found no earlier related commits or review threads (first concurrency: block and first wiki-related job in this file). Not reconciled against any discussion on the PR — none was read as part of this review.

[rominf]

🔴 Automated review · pr-review-watcher · 06245c5

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

Review — needs work

Full review of the whole change.
Implements ROCMAI-488 (publish the latest nightly matrix to a wiki page with run link, commit and date). The description matches the diff and states the one-time wiki prerequisite.

Blocking

• [new] The contract test leaves most of the new job's branches unguarded, and one assertion message claims a check it cannot fail on — xtask/src/workflow_contract.rs:1707-1730, against .github/workflows/nightly.yml:1637-1693
  Reverted verdict: the test fails if the whole change is reverted, because job_block(&nightly, "publish-e2e-wiki") panics.
  Single-branch verdict: the test stays green if any one of these is broken on its own:

  • git push origin HEAD is deleted. The job then publishes nothing.
  • needs.e2e-report-nightly.result == 'success' or the repository guard is dropped from the if:.
  • The job's permission is lowered to permissions: contents: read. The push then fails at run time.
  • The if ! git clone …; then echo ::error::…; exit 1; fi guard is replaced by git clone … || true.

  The last assertion's message says "a missing wiki must fail with the prerequisite". All it checks is !publish.contains("git init"), so it cannot fail when the fail-with-prerequisite branch is removed. That makes the gap look covered when it is not.
  The first assertion has a similar weakness. It is satisfied by any occurrence of the string consolidated/support-matrix.md in the job. It does not show that stdout is redirected into that file.
  Confidence 85 (traced by reading; not run because of the disk constraint) · logic
  Fix:

  • Assert on the job's if: line with job_scalar(publish, "if").
  • Assert on contents: write.
  • Assert on the git push line.
  • Assert that an exit 1 follows the clone guard.
  • Assert on the > consolidated/support-matrix.md redirect, not just the path.
  • Reword the git init message to say only what that assertion checks.

Non-blocking

• [new] An empty, partial or cancelled nightly replaces the last good page — .github/workflows/nightly.yml:1644, xtask/src/e2e_report.rs:23-28, crates/e2e-report/src/consolidated.rs:965-967

  • When no report.json is found, e2e-report only warns on stderr and exits 0. It prints "No per-platform report.json files were found to consolidate.", so e2e-report-nightly succeeds.
  • The publish job checks nothing beyond that result, so it overwrites the wiki page with the empty matrix.
  • A cancelled run behaves the same way, because always() stays true after cancellation. The job then publishes whatever subset of lanes finished, and nothing on the page says rows are missing.
  • The ticket's acceptance is that the page "matches the latest nightly consolidated report". A run where no lane reported is not what that wording is after.

  Confidence 70 · logic
  Fix:

  • Use !cancelled() rather than always() in the publish job's if:. always() or !cancelled() is still needed there, because otherwise a failed upstream lane makes the job skip.
  • Skip publishing when the report consolidated zero platforms, for example by having xtask exit non-zero or write a marker file.

• [new] A workflow_dispatch run on a non-default branch publishes under the "latest nightly" title — .github/workflows/nightly.yml:6, :1644, :1671
  The guard checks github.repository but not the ref. The comment at :34-37 treats "un-overridden" dispatch as the normal case and does not rule out the other. The page header shows the commit but not the branch.
  Confidence 70 · logic
  Fix: add github.ref == 'refs/heads/main' to the if:.

• [new] The docs do not mention the wiki page or its one-time setup — docs/ci-hardware-testing.md:146-154, tests/e2e-cucumber/README.md:178-196
  Both still describe the nightly consolidation's output as the HTML report and step summary. The only place the prerequisite (enable the wiki and save its first page in the web UI) is written down is the job's ::error:: string. AGENTS.md §5 asks for docs to change in the same PR.
  Confidence 70 · mechanical
  Fix: add a sentence to docs/ci-hardware-testing.md naming the E2E-Support-Matrix page, the job that writes it, and the setup step.

• [new] Pre-existing: the matrix text is written for a PR audience, and it now appears on a standalone public page — crates/e2e-report/src/consolidated.rs:1018-1037
  The markdown that gets published says Mock "gates the PR" and that a failure "does NOT block the PR from merging". That was already in the nightly step summary. On a wiki page aimed at users, as the ticket intends, it reads as out of place.
  Confidence 70 · mechanical
  Fix: optionally emit a nightly-specific preamble, or leave it as is.

Decisions for the author

• Overwrite the page vs. preserve the last complete matrix — tradeoff
  The ticket specifies "overwritten each night; no history", and the header records which run produced each page. Always publishing makes a broken night visible. Publishing only complete runs keeps the user-facing page useful. The first non-blocking finding is the narrow version of this choice; the broader choice is yours.

Positive signals

• Run, commit and repository values reach the script through env: rather than ${{ }} inside run:, so the script has no expression-injection surface.
• contents: write is granted to this job alone, which matches the narrowing deliberately done on e2e-report-nightly.
• When the wiki is missing, the job fails loudly with the reason. It does not try a git init that cannot create a remote wiki.
• The new actions/download-artifact pin 3e5f45b2… matches upstream tag v8.0.1, a lightweight tag resolving straight to that commit.

Deployment notes

• The wiki git repo does not exist yet. gh api repos/ROCm/rocm-cli reports has_wiki: true, but git ls-remote https://github.com/ROCm/rocm-cli.wiki.git returns "Repository not found". Every nightly will go red on this job until someone saves the first wiki page in the web UI.
• Nothing in this change or its test plan has shown that GITHUB_TOKEN with contents: write can push to <repo>.wiki.git. The PR's own test plan leaves "dispatch Nightly and confirm the page updates" unchecked. Do that before relying on the page.
• On a public repo, the wiki accepts edits from any GitHub user unless "Restrict editing to collaborators only" is set. The page's "do not edit by hand" notice enforces nothing. Consider turning that setting on.

What this covered

Both changed files were read in full. Around them I read:

• the workflow-level triggers and permissions in nightly.yml;
• the e2e-report-nightly job and the nightly lanes it depends on;
• the test helpers read_workflow, job_block and job_scalar;
• xtask/src/e2e_report.rs::run() and consolidated_summary_markdown;
• the docs that describe the nightly consolidation.

The diff reviewed was prw-base 28f9ff7 (merge-base b863f77) … 06245c5.

How the review was split: two independent workers ran, one on the code pass (both files coupled) and one on the source-reading passes (agent-instruction adherence, history, prior-PR review comments on #221/#460/#512/#406, code comments). The design questions were looked at in the driver context, so they are not an independent pass. The commits are signed, carry DCO sign-off, and use a personal author email. AGENTS.md requires an employer identity only "when required", and that could not be established from here, so it is not a finding.

Verification was static only, because the host disk is critically low. No cargo build, test or clippy ran, and the test-branch verdicts above come from reading the code, not from mutation runs. I read CI from statusCheckRollup for 06245c5 and every check there passed. That rollup does not include the nightly workflow, so the new job has never run in CI.

I read ROCMAI-488 by its key, without comments. I did not reconcile this report against the PR's discussion or any earlier reviews.

[fredespi]

#590 (review)

Fixed in a95a82b. Contract test now asserts the if: (!cancelled(), upstream success, repo, main ref), contents: write, the stdout redirect, git push, the clone-failure exit 1 and the empty-report guard; I confirmed it fails against the previous workflow. The job now skips cancelled runs, non-main refs and runs that consolidated no platforms, and docs/ci-hardware-testing.md documents the page and its one-time wiki setup. Left the PR-oriented preamble in the matrix text as is (pre-existing, shared with the step summary).

[rominf]

🔴 Automated review · pr-review-watcher · a95a82b

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

Review — needs work

Full review of the whole change.
Implements ROCMAI-488 in full ("copy that run's results into a ROCm/rocm-cli wiki page … run link, commit and date … overwritten each night"). The one acceptance step left is the unchecked live dispatch, which needs the wiki to be set up first.

Blocking

• [new] No test checks that the empty-report guard stops the publish, although the test's assertion message says it does — xtask/src/workflow_contract.rs:1759-1763, .github/workflows/nightly.yml (the grep -q 'No per-platform report.json files were found' / exit 0 guard in publish-e2e-wiki), crates/e2e-report/src/consolidated.rs:966
  The assertion says "an empty consolidation must leave the last good page in place". It only checks that two strings appear somewhere in the job text.

  • I replaced the guard's exit 0 with true, so an empty matrix would be published. cargo test -p xtask nightly_publishes_the_consolidated still passed.
  • A worker inverted the condition to if ! grep -q …. The test still passed.
  • The guard's grep string is also copied by hand from the generator's output (_No per-platform report.json files were found to consolidate._). No test links the two. The generator's own test (consolidated.rs:1901) only pins the shorter prefix No per-platform report.json files. So rewording the generator would silently turn the guard off, and an empty matrix would replace the page.
  • AGENTS.md §3 says a test that only pins the wording "certifies the string, not the truth of it".

  Confidence 100 · logic · Fix: feed consolidated_summary_markdown(&[]) to the extracted guard (or to a shared constant both sides use) and assert the publish path is not reached. At minimum, tie the sentinel to the generator's actual empty output.

Non-blocking

• [new] The PR body names a machine ("Linux gate on acer") — PR description, Test plan
  AGENTS.md §2 forbids internal hostnames on upstream surfaces, and PR bodies are included. If "acer" is a host name, replace it with something neutral like "local Linux gate". If it is just a laptop brand, ignore this.
  Confidence 70 · mechanical · Fix: reword the test-plan line.

Decisions for the author

• [new] A partial nightly overwrites a complete page — tradeoff
  • The guard only skips runs where zero platforms were consolidated. The lanes are continue-on-error, and discover() silently leaves out any platform without a report.json.
  • So on a night when, say, five of six runners are offline, the public page is replaced by a one-platform matrix. The header (run, commit, date) does not say that platforms are missing.
  • For: the page always reflects the latest run, as the ticket asks. Against: a runner outage looks like a platform dropping out of support.
  • Options: an expected-platform check, or a "platforms reported: N" line in the header.

Positive signals

• The if: uses !cancelled() with result == 'success', not always(), and the contract test rejects always(). A cancelled run cannot publish.
• permissions: contents: write is scoped to the publish job, and the consolidation job stays contents: read.
• The git clone failure path fails loudly with the actual prerequisite and is not silently ignored. The test also blocks a git init fallback that could never create the remote wiki.

Deployment notes

• One-time setup before the job can succeed: enable the repo wiki and save its first page in the web UI. Until then every nightly publish-e2e-wiki run fails. The docs and the error message both say this.
• I found no evidence about whether the org's wiki edit restriction ("collaborators only") blocks pushes made with GITHUB_TOKEN. Third-party wiki actions do default to github.token with contents: write. The unchecked dispatch item in the test plan is what will settle it.

Standing objection from the previous round (06245c5) — settled, no longer blocking

"The contract test leaves most of the new job's branches unguarded" was not re-found by this round's blind review. It was then put to the reviewer, with the author's reply, as a separate check (anchored evidence, not a blind finding). Mutating nightly.yml one item at a time: removing git push, dropping the upstream-success or repository clause from if:, lowering to contents: read, and replacing the clone guard with || true each now fail the test; the git init message now says only what it checks; the redirect is matched exactly. That objection is withdrawn. The one residue — exit 1 fi is matched anywhere in the job rather than after the clone — is minor and not blocking. The other non-blocking points from that round (cancelled runs, non-main dispatch, docs) are addressed in this head.

What this covered

I read all three changed files completely (a95a82b against base 2499351), plus:

• the whole e2e-report-nightly job and the nightly skip wiring
• xtask/src/e2e_report.rs
• the markdown generator in crates/e2e-report/src/consolidated.rs
• the per-PR twin in e2e-selfhosted.yml, which correctly does not publish

Verification:

• cargo test -p xtask workflow_contract passed: 41 tests.
• Mutation checks on the workflow were run by a worker and by me.
• CI for a95a82b is all green (statusCheckRollup). No GPU lanes ran for this commit, and none apply to it.
• I read ticket ROCMAI-488 by its key (summary, description and status only).

Did not run or ran partly:

• Prior changes: I did not read review comments on earlier PRs that touched these files.
• Agent-instruction adherence was folded into the single code-pass worker, so it was not an independent pass.
• History: only a brief git log on nightly.yml.
• The change is small, so the fan-out was two workers: one code pass, and one for the design questions.

I did not reconcile this report against the PR's discussion.

[fredespi]

#590 (review)

Fixed in 7f2caca. New test wiki_publish_guard_stops_on_the_generators_empty_output extracts the guard from the job and runs it in bash against consolidated_summary_markdown(&[]), so rewording the generator or breaking the guard is caught by behavior. Mutation-checked: exit 0 -> true, an inverted grep, and a reworded sentinel each fail it. Not changed: the partial-night overwrite (kept as the ticket specifies, latest run always published; the header names the run) and the PR body wording, which I cannot edit under this handoff.

[rominf]

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

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.
Implements ROCMAI-488 (publish the latest nightly matrix to a wiki page with run link, commit and date). The ticket's acceptance check, a live page update, is still an unchecked item in the PR's test plan.

Blocking

None

Non-blocking

• [standing — raised last round as a tradeoff] A partial nightly silently overwrites the public page and drops the missing platforms — .github/workflows/nightly.yml (publish-e2e-wiki if: and the empty-report guard), xtask/src/e2e_report.rs:75-108
  • All six lanes are continue-on-error: true and e2e-report-nightly is if: always(). So needs.e2e-report-nightly.result == 'success' says nothing about how many lanes reported.
  • discover() lists only the report.json files that exist. A lane that died before writing one, for example in preflight or on a timeout, produces no row and no "missing" marker. A comment already in nightly.yml describes this for the WSL upload: the platform "would silently vanish from the report".
  • The new guard catches only the zero-platform case. A run with 1 of 6 platforms therefore replaces the last good page, and the Total row sums only the platforms that are present.
  • This is an old weakness, but this change makes it worse. Before, it affected one run's step summary. Now it reaches the durable page that ROCMAI-488 says is for users to "see what works on which hardware". On that page a vanished row reads as "not supported".
  • Confidence 70 · architectural · Fix: before publishing, compare the discovered platforms against the expected lane set. Either skip the publish, or add a "not reported this run: …" line.
• [new] Nothing tests that the nightly step summary is still written — .github/workflows/nightly.yml cat consolidated/support-matrix.md >> "$GITHUB_STEP_SUMMARY"
  • This change split the one-line redirect into two lines.
  • Deleting the cat line left all 273 cargo test -p xtask tests green. The new contract test pins only the redirect into support-matrix.md.
  • Confidence 100 · mechanical · Fix: in nightly_publishes_the_consolidated_matrix_to_the_wiki, also assert that the matrix file is appended to $GITHUB_STEP_SUMMARY.
• [new] The guard test hand-rolls its temp dir — xtask/src/workflow_contract.rs (wiki_publish_guard_stops_on_the_generators_empty_output, the run closure)
  • It uses std::env::temp_dir().join("wiki-guard-{pid}-{len}"). The directory leaks whenever an assert! or unwrap panics before remove_dir_all.
  • Sibling tests in the same file use tempfile::tempdir(), and tempfile is already a dependency in xtask/Cargo.toml.
  • Confidence 70 · mechanical · Fix: use tempfile::tempdir().
• [standing] The PR body names a machine — PR description: "Linux gate on acer"
  • AGENTS.md §2 forbids internal hostnames on upstream surfaces, including PR bodies.
  • Confidence 70 · mechanical · Fix: say "local Linux gate" instead.

Decisions for the author

• The nightly stays red until the wiki is initialised — tradeoff
  • The job has no continue-on-error, and a failed clone runs exit 1.
  • For: the prerequisite cannot go unnoticed, and the error message is explicit.
  • Against: every nightly fails until someone creates the first page in the web UI.
• The page gets a new commit every night — tradeoff
  • The header carries the run number and a minute-resolution date, so content always differs. The git diff --cached --quiet "already up to date" branch is in practice reachable only by a re-run in the same minute.
  • That suits "overwritten each night", but the wiki history grows by one commit per night even when the matrix did not change.
• PR-oriented prose lands on a user-facing page — non-blocking-improvement
  • consolidated_summary_markdown (crates/e2e-report/src/consolidated.rs:1018-1039) explains Mock as gating the PR and the GPU lanes as not blocking merges.
  • The nightly has no Mock lane, and wiki readers are not reviewing PRs.
  • The text is accurate but aimed at the wrong reader.

Positive signals

• wiki_publish_guard_stops_on_the_generators_empty_output runs the workflow's real guard, extracted from the YAML, against the generator's real empty output. Rewording either side breaks it by behaviour, not by a string pin.
• The write permission is confined to the new job. e2e-report-nightly keeps its deliberately narrowed contents: read.
• Publishing is gated to ROCm/rocm-cli on refs/heads/main and excludes cancelled runs, so a branch dispatch or a fork cannot overwrite the page.

Deployment notes

• One-time setup: enable the repository wiki and save its first page in the web UI. Until then every nightly run is red at publish-e2e-wiki.
• The push uses the default github.token with contents: write. Widely used wiki actions document this as sufficient, but it is unverified here until the first live run, which is the unchecked test-plan item.

Standing objection from the previous round (a95a82b) — settled, no longer blocking

"No test checks that the empty-report guard stops the publish" was not re-found by this round's blind review. It was then put to the reviewer, with the author's reply, as a separate check (anchored evidence, not a blind finding), and re-run by the driver:

• Guard exit 0 → true: wiki_publish_guard_stops_on_the_generators_empty_output now FAILS (reproduced by the driver independently).
• Inverted if ! grep -q …: FAILS.
• Rewording the generator's empty-output sentinel in consolidated.rs: FAILS. The guard and the generator are now linked by behaviour, not by a hand-copied string.
• Guard extraction cannot silently find nothing: both its start and its closing fi are .expect()ed.

That objection is withdrawn. One residue, not blocking: the test runs the extracted guard in isolation, so moving the guard below git push origin HEAD leaves both tests green. The guard currently sits before the clone and the push. An assertion that the guard's line precedes git clone/git push in the job would close it.

What this covered

• Read: the full diff and all three changed files (24993519…7f2caca4), the e2e-report-nightly and publish-e2e-wiki jobs, the new tests and the helpers they call, the docs paragraph, the generator (xtask/src/e2e_report.rs, consolidated_summary_markdown), and the lanes' continue-on-error/needs settings.
• Ran: cargo test -p xtask (273 passed); mutations on the step-summary line and on the guard, as above.
• Test verdicts: both new tests fail with the production change reverted, because the job and the redirect would be absent. The if:, permissions and clone-failure branches are pinned by strings; the step-summary line is not pinned, as reported above.
• CI: every check for head 7f2caca reported success.
• Did not run: the main code pass ran in the reviewing context rather than as an independent worker (the agent-instruction/history and design passes were independent workers); the prior-changes pass over earlier PRs' review comments did not run.
• Discussion: findings were reached without the PR's discussion; only the settled objection above was reconciled with the author's reply.

Both objections are resolved at 7f2caca: the new guard test runs the real guard against the generator's empty output and fails when the guard is broken or the sentinel is reworded (verified by mutation). See review 5455665440.

[fredespi]

#590 (review)

Fixed in 8e04662: the contract test now asserts the matrix is still appended to $GITHUB_STEP_SUMMARY (checked: deleting that cat line fails it), and the guard test uses tempfile::tempdir(). Not changed: the partial-night overwrite (a platform-count/expected-lane check is a design change beyond this ticket; the page always reflects the latest run, as specified), and the PR-body hostname wording, which I cannot edit under this handoff.

[rominf]

🔴 Automated review · pr-review-watcher · 8e04662

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. Each finding is marked new this round or standing from an earlier one. The two items under Blocking are published as a comment, not a change request. The first is the same fact the round at 7f2caca rated a tradeoff ("the page gets a new commit every night"), and nightly.yml is unchanged since then. The second understates the change rather than overclaiming, so merging leaves behind only a terser squash message. The two non-blocking test findings from 7f2caca (the step-summary cat line, and the hand-rolled temp dir) are addressed by 8e04662.

Review — needs work

Full review of the whole change.
This publishes the per-platform summary table from ROCMAI-488, but not the "scenario x platform grid, with versions" that the ticket also scopes. The markdown generator (crates/e2e-report/src/consolidated.rs:964-1016) emits only the platform table, so the page will not "match the latest nightly consolidated report" as the ticket's acceptance line asks. The PR does not claim to close the ticket.

Blocking

• [standing — raised at 7f2caca as a tradeoff] The "Wiki page already up to date." branch can never fire — .github/workflows/nightly.yml:1670-1698
  The page header is written into the file being compared, and it always contains ${GITHUB_RUN_NUMBER} and $(date -u +'%Y-%m-%d %H:%M UTC'). The run number is unique per run. A re-run keeps the run number, but it runs in a later minute. So git diff --cached --quiet cannot succeed, and every nightly commits to the wiki even when the matrix has not changed. The branch is dead code, and it misleads a reader into thinking unchanged matrices are skipped.
  Confidence 90 · logic · Fix: compare only the matrix body against the existing page (e.g. strip the header before diffing), or drop the branch and accept a commit per night.

• [new] The description leaves out behaviour the change adds — PR description vs .github/workflows/nightly.yml:1633-1645, docs/ci-hardware-testing.md:156-162, xtask/src/workflow_contract.rs:1775-1819
  The "What" section mentions only the repo restriction ("ROCm/rocm-cli only") and "Contract test". The change also does the following, none of which the description mentions:

  • restricts publishing to refs/heads/main
  • skips cancelled runs and runs that consolidated no platforms, leaving the last page in place
  • adds a second, behavioural test that runs the guard under bash
  • adds a docs paragraph to docs/ci-hardware-testing.md

  A maintainer deciding whether "empty run overwrites the page" was handled reads the description and finds nothing.
  Confidence 85 · mechanical · Fix: add these to the description. The code does not need to change.

Non-blocking

• [new] The empty-report guard fails open, and its test skips the CLI path the workflow runs — .github/workflows/nightly.yml:1665-1668, xtask/src/workflow_contract.rs:1775-1819, xtask/src/e2e_report.rs:19-50
  The guard blocks publishing only when the output contains one exact sentence. Any other degenerate output passes it and gets published as a header-only page. That includes a zero-byte file and a reworded empty message.
  The test's doc comment claims it runs against "the generator's real empty output", but it calls the library consolidated_summary_markdown(&[]) and never calls xtask::e2e_report::run. A mutation added an early return Ok(()) in run()'s inputs.is_empty() branch, which makes support-matrix.md empty. Both new tests still passed. Today run() prints the right sentence: the real CLI was run against an empty and a missing --artifacts-dir, and the guard matched both.
  Confidence 70 · logic · Fix: turn the guard around so it publishes only when the file contains the matrix's table header (| Platform | OS |), or drive the test through run().
• [standing] The PR description names a machine — PR description, "Linux gate on acer"
  AGENTS.md §2 forbids internal hostnames on upstream surfaces, PR bodies included. If "acer" is a host name, it is one.
  Confidence 70 · mechanical · Fix: reword to "local Linux gate".

Test verdicts:

• nightly_publishes_the_consolidated_matrix_to_the_wiki
  • Reverted: fails. Restoring the base >> "$GITHUB_STEP_SUMMARY" form fails it, and so does removing the cat line.
  • Single branch broken: it checks if ! git clone … and exit 1 fi as separate substrings, so nothing ties the exit 1 to the clone failure. That holds today only because exit 1 appears once in the job.
• wiki_publish_guard_stops_on_the_generators_empty_output
  • Reverted: fails. Changing the guard's exit 0 to true fails it, and so does pointing the guard at the wrong file.
  • Single branch broken: run()'s empty-input path stays green when broken (above).

Decisions for the author

• [new] Wiki page versus an orphan branch — tradeoff
  • Wiki: more discoverable. But it needs a manual first page before the git repo exists, which is checked live: git ls-remote https://github.com/ROCm/rocm-cli.wiki.git returns "Repository not found".
  • Orphan branch: the repo already uses this pattern, peaceiris/actions-gh-pages publishing to a media branch in .github/workflows/demo-gifs.yml. It bootstraps itself, renders on github.com, and avoids the red-until-set-up window.
• [standing] A partial nightly replaces a complete page with no marker — non-blocking-improvement
  The lanes are continue-on-error and e2e-report-nightly is if: always(). A lane that is offline or uploads no report just drops its row from a page headed "latest nightly", and the guard only catches the case where zero platforms were found. A header line listing lanes whose needs.*.result was not success would make the gap visible.

Positive signals

• The guard is tested by running the workflow's own extracted shell under bash against the generator's output, not by pinning both strings separately. That catches a reword on either side.
• The if: uses !cancelled() and the contract test explicitly rejects always(). This matters because the report job runs even on cancelled workflows.

Deployment notes

• The wiki git repo for ROCm/rocm-cli does not exist yet (checked live). Until someone saves a first page in the web UI, publish-e2e-wiki fails every scheduled nightly with exit 1. The PR description and docs/ci-hardware-testing.md both state this. Create the page before merge, or expect red nightlies until then.
• The new job does not widen permissions. nightly.yml already grants contents: write at the top level, and the job's script interpolates no artifact content into the shell.

What this covered

Every changed file in full: .github/workflows/nightly.yml, docs/ci-hardware-testing.md, xtask/src/workflow_contract.rs. Also the test helpers they use, xtask/src/e2e_report.rs, and the markdown generator in crates/e2e-report/src/consolidated.rs. Diffed efcd480…8e04662d.

What ran:

• Fan-out: a coupled code pass; the agent-instruction, history and code-comment passes; and a design-question pass, all as independent workers.
• Locally:
  • cargo test -p xtask: 283 passed.
  • cargo clippy -p xtask --all-targets -- -D warnings: clean.
  • Mutation runs on the guard, the cat line and run().
• CI on 8e04662: all reported checks green. The change is CI-only, so the new job itself has not run anywhere.

Did not run: prior-changes, because no earlier review comments were consulted (by design). Not reconciled against the PR's discussion.

[fredespi]

#590 (review)

Fixed in 5d11b50. The dead "already up to date" branch is removed (every night commits, as the ticket specifies). The guard now fails closed: it publishes only when the matrix table header is present, so a zero-byte or reworded output leaves the last page in place; the behavioural test covers the empty file and the generator output, mutation-checked (exit 0 -> true, inverted condition, wrong file all fail). Not done under this handoff: the PR description edits (hostname, listing the main-only/cancelled/empty-run guards and docs) which the author-side owner will make. Kept: wiki page over an orphan branch (the ticket asks for a wiki page) and the partial-night behaviour.