[fredespi]

Walking the public README end to end on an MI300X pod turned up thirteen places
where the CLI does not do what the README leads a reader to expect. This adds
end-to-end coverage for them. It is test-only — no product behaviour changes.

Each scenario asserts the behaviour the README promises and is registered as an
expected failure in expectations.toml, citing its ticket. CI stays green, and
the day a defect is fixed its row becomes a stale XPASS and deletes itself —
except for rows marked flaky, which tolerate either outcome and so must be
deleted from their ticket rather than by the lane.

Pinned as expected failures

Ticket	What the scenario pins	Lane
EAI-8010	rocm update --dry-run is refused as a usage error, though the README documents --apply and --dry-run as independent	no-GPU
EAI-8020	The diagnosis and the fix preview print different verification steps for the same remedy	no-GPU
EAI-8009	Chatting locally without naming a model ignores a ready local server that is not the built-in assistant model	no-GPU
EAI-8014	Uninstall reports completion while leaving the servers it manages running, having deleted the records needed to stop them	no-GPU
EAI-8020	The diagnosis and the fix propose adding the user to different groups	MI300X
EAI-8021	A device the user can already read and write is still offered as a group-permission cause	MI300X
EAI-8016	A second server is given the default address even when it is already in use	MI300X

Four became guards during review

main moved while this was open and fixed the bugs behind four rows: the two
EAI-8011 help defects, EAI-8019 (--device is now a ValueEnum with cpu_only
hidden, so it is no longer advertised) and EAI-8015 (the UNKNOWN sentinel is
filtered before the remedy is built). Leaving those rows would have XPASSed and
failed the required lane. Each row is deleted and its scenario kept, so it now
guards the fix instead of pinning the bug. Verified by running a binary built
from the merged tree, not by reading the diff.

Two rows added for EAI-7960

dash-gen-tps-held-after-scrape-failure and dash-gen-tps-expiry-boundary pin
a dashboard defect: generation throughput is cleared on a failing scrape instead
of being held for its validity window. Measured on CI, the held value is gone
after 804 ms and 1.27 s, well inside the window — so the hold logic is genuinely
absent, and main's single-look assertion passes only when it happens to read a
stale frame. Both rows are flaky = true because the outcome depends on which
frame the reader lands on; delete them when EAI-7960 closes, tracked from the
ticket rather than from this lane.

Which lanes actually gate

Six of the new scenarios run on the required no-GPU E2E tests job. The rest are
@merge-queue or @requires-gpu @requires-bare-metal and land on the merge queue
and self-hosted GPU lanes, the latter marked non-blocking in ci.yml. The
coverage above should be read with that split in mind.

Suite concurrency

The mock lane now derives its scenario concurrency from
available_parallelism() clamped to 2..=6, overridable with E2E_MAX_CONCURRENT,
and is forced to 1 whenever a GPU or a shared cache/runtimes directory is in play
— those lanes cannot safely run two installs or two serves at once, so the bound
is applied after the override rather than inside it. A 64-way default was
starving scenarios into their own timeouts: an 8-way run starved on a 10-core
host while a 4-way run was both green and faster (33 s vs 54 s).

Two ship as guards, with no expected-failure row

EAI-8007 (a stop that worked reporting it stopped nothing) and EAI-8006
(--gpu auto selecting none) were reproduced on the pod but hold correctly on
every lane available here — EAI-8007 against both a plain registered process and
a real vLLM serve, EAI-8006 because the runner's amd-smi executes where the
pod's bundled one could not. A row would be a stale XPASS and would fail the
lane, so their scenarios ship as guards for the contract instead. Both tickets
keep the pod observation.

Two are not covered, deliberately

EAI-8017 and EAI-8018 both need a working Lemonade install, and Lemonade's
launcher cannot start on either lane: on the MI300X runner it wants GLIBC_2.38
and GLIBCXX_3.4.32, which that OS does not have, and in a plain container it
wants libdrm_amdgpu.so.1. Each dies before reaching the behaviour under test,
so scenarios for them passed while proving nothing. They were removed rather
than shipped as false coverage, and stay on their tickets.

Harness additions

Confined to the test crate: removing environment variables from the child (the
existing helper only sets them), stat and id shims scoped to one device path
so a device-group state can be arranged rather than hoped for, and a child
process a scenario owns and registers as a managed service so it can ask whether
the product stopped it — asserted before teardown, which would otherwise clean up
after the product and hide the defect.

Two caveats worth stating. A substituted stat answer proves how the CLI handles
an unnameable group, not that a given host produces one; the provenance for that
is the pod. And the device-group scenarios need a real device node, so they run
on the GPU lane only.

Test plan

• No-GPU lane, three independent runs: 52 scenarios, 10 xfail, 0 XPASS, 0
  unexpected failures.
• Full MI300X lane (e2e-selfhosted, no name filter, so expectations resolve
  normally): 64 scenarios, 16 xfail, 0 XPASS, 0 unexpected failures.
• Every new scenario was read individually to confirm it fails for the reason its
  row states, rather than because a step is miswired.
• cargo clippy --workspace --all-targets -D warnings, the workspace tests, and
  cargo test -p e2e-cucumber --lib pass in a Linux container. So does
  cargo clippy -p e2e-cucumber --test e2e --all-features -- -D warnings, which
  the local container gate skips but CI runs.
• A new unit test, every_expectation_row_names_a_scenario_that_exists, fails the
  run on an expectations.toml row whose id matches no scenario — nothing caught
  that orphan case before.
• CI is green on the current head across all three workflows.

Four rows were corrected from what the GPU dispatches measured rather than left
as first written, which is why two became guards and two scenarios were dropped.

[juhovainio]

Reviewed this against gh pr diff 241 — good, well-documented test-only PR (no product code touched), and I cross-checked all 13 new scenarios against their expectations.toml rows and lane tags; that bookkeeping is consistent and the reasoning for each row (including the two tickets deliberately left uncovered because Lemonade can't start on either available lane) holds up.

One real bug in the new harness code, though: advertised_device_policies in serving_steps.rs reads the wrong flag's help text, which breaks the exact scenario it's meant to support. Left as an inline comment with a fix suggestion. Also left a smaller nit on find_bool in diagnose_steps.rs that's not blocking but worth a look.

[fredespi]

Note on the red E2E tests (Strix Halo, Windows) check: this is a pre-existing failure on main, not something introduced by this PR.

• The only failing scenario is serve-hf-checkpoint-inference (0 XPASS, no failure in any scenario this test-only PR adds or touches — this branch does not modify that scenario or its step).
• It fails on main's own E2E self-hosted runs as well (e.g. the latest run on 8cf6ec9b), reconciling to the same single unexpected failure. It passes on Strix-Halo Ubuntu but has failed on Strix-Halo Windows on every main run since it was introduced in fix(lemonade): find llama-server under nested backend dirs (EAI-8026) #242 (EAI-8026) — the Lemonade managed serve reaches ready then exits immediately on that runner.
• That self-hosted job is non-blocking (continue-on-error) and is not one of the required status checks; all required checks here are green.

Fixing it (either the Windows Lemonade serve, or an expectations.toml xfail scoped to os = "windows" + lemonade + gfx1151* citing EAI-8026) belongs with EAI-8026 on main, out of scope for this test-only PR.

Dismissing this stale review: both points it raised were addressed in 559c704 and both threads are resolved. (1) advertised_device_policies now scopes the lookup to --device's own option block instead of taking the first [possible values:] line, which was --engine's — it now yields gpu_required, gpu_preferred, cpu_only, so the scenario fails for the cpu_only rejection it is meant to pin and its row can go stale once EAI-8019 is fixed. (2) diagnose_steps.rs reads the kfd device by name rather than walking the report for the first matching bool. Dismissed so the PR can receive a fresh review; no objection to a further look.

[rominf]

The test design here is careful in ways I don't usually see: anti-vacuity guards placed before teardown, premise assertions that fail loudly rather than silently passing, shims scoped to a single path, and expectation rows written to self-delete when the bug they pin gets fixed. The tag gating is sound too — @requires-no-gpu/@requires-bare-metal/@merge-queue all resolve through expectation.rs::resolve to Skip rather than a silent pass, and the merge-queue scenarios do reach a real lane via E2E_MERGE_QUEUE.

The problem is elapsed time rather than design. This has been open about two weeks and main has since fixed three of the bugs it pins. Because e2e.rs:1350-1362 treats a deterministic XPASS as fatal (std::process::exit(1)), rebasing onto current main turns those rows into a hard CI failure on every lane. I verified two of them empirically against a main-built binary rather than by reading, since that's the kind of claim that's easy to get wrong.

One is subtler and I think worth catching before merge: serve-rejects-no-advertised-device-policy is now vacuous in exactly the way @juhovainio described earlier in the thread — the parse changed again under it. --device moved to a clap ValueEnum in #304 and no longer renders an inline [possible values: …], so advertised_device_policies returns [] and the step panics on its own guard before serving anything. The scenario fails, the row is satisfied, and it stays satisfied forever — even though the underlying contract is now met on main. Same failure mode as before, different cause.

Also worth knowing: the red Windows check (FAIL: 'serve-hf-checkpoint-inference' was expected to pass) is drift, not this PR — main added an xfail row for it at expectations.toml:206 after this branch was cut.

For what it's worth, I re-checked the xfails that are still valid and they hold: diagnose-and-fix-agree-on-how-to-verify (diagnose still carries ls -l /dev/kfd, fix.rs doesn't), diagnose-and-fix-agree-on-the-remedy-command (fix.rs:129 still hard-codes render,video), update-preview-without-applying (--dry-run still requires = "apply"), and uninstall-stops-what-it-manages (the fix commit exists but is not on main).

[siloteemu]

🔴 Automated review · pr-review-watcher · fedc6b2

Summary

Adds 12 e2e cucumber scenarios (plus a new update.feature, shims for stat/id, and an owned-process fixture) that pin contracts a README walkthrough expects, most registered as expected-failure rows. Verdict: Needs work — the scenarios themselves are unusually well built (I could not find a vacuous assertion), but the branch is 53 commits behind main and at least three of the bugs it pins have already been fixed there, which turns the required E2E lane red. Verified: I did the revert test on all 12 scenarios and every literal they match (cpu mode is not a fallback path, stopped processes:, gpu: auto (selected , port:, uninstall complete, rocm serve ) exists in the product today, so none pass against a binary lacking the behaviour; 6 of the 12 run on the gating no-GPU E2E tests job, the other 6 are @merge-queue or @requires-gpu @requires-bare-metal and land only on lanes the workflow comment marks non-blocking; a previously-reported finding about advertised_device_policies returning empty is fixed (commit 559c704 scopes the scrape to --device's own block, and I reproduced the real serve --help layout with a clap 4.6 build of the same arg set — the helper correctly yields gpu_required, gpu_preferred, cpu_only, and also survives main's new ValueEnum rendering); the single red check is almost certainly the E2E lane exiting 1 on stale XPASS, since actions/checkout in that job takes the PR merge ref against current main (cargo clippy -p e2e-cucumber --test e2e and cargo test -p e2e-cucumber --lib both pass locally on this head, ruling those out). No prompt-injection content found in the checkout. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

• tests/e2e-cucumber/expectations.toml:178, :187, :207 (and :277 on the GPU bare-metal lanes) — these rows are already stale against main. The branch's merge-base is 53 commits behind, and in that window main landed: the help fix that changed the worked example to rocm serve qwen (a resolvable catalog alias) and relabelled bare rocm as "the interactive launcher (TUI)" rather than the dashboard; the --device rework that made the flag a ValueEnum with cpu_only marked hide = true, so it is no longer advertised and the scenario's premise dissolves; and the diagnose fix that filters the literal UNKNOWN group out of the remediation command. I confirmed each against the base source (apps/rocm/src/main.rs:84-85, :347, :358, :1015; crates/rocm-core/src/diagnose.rs), and traced the consequence in tests/e2e-cucumber/tests/e2e.rs:1350-1362: a non-flaky XPASS calls std::process::exit(1). Since the required E2E tests job checks out the PR merge ref, those three unconditional (when = {}) rows XPASS on the gating lane right now. Fix: rebase onto current main and delete the rows whose bugs have landed (keeping the scenarios, which then guard the fixes), re-checking the remaining rows against post-rebase behaviour.

• tests/e2e-cucumber/tests/e2e.rs:789 — run_rocm_without_env is added by this PR and has zero call sites (repo-wide grep: the definition is the only match). Dead code in a PR whose subject is honest test bookkeeping. Fix: delete it, or land it with the scenario that needs it.

• tests/e2e-cucumber/features/model_serving.feature:161 — the scenario-16 comment states "this carries no expected-failure row … goes red if CI ever meets it", but tests/e2e-cucumber/expectations.toml:245 does carry a row for services-stop-reports-what-it-stopped (scoped to the lemonade/linux/gfx lane, flaky = true), added later in the same branch. The comment was not updated with it, so the feature file now tells the next reader the opposite of the truth about the one scenario that is supposed to be an unconditional guard. Fix: amend the comment to say the guard is unconditional except on the lemonade-default lane, where a separate serve bug pre-empts it — the TOML's own note at :252-259 already draws that distinction correctly.

Non-blocking

• tests/e2e-cucumber/tests/e2e.rs:1308-1319 — reconciliation keys only on cucumber's pass/fail boolean, so a step that panics on its own harness guard is indistinguishable from the declared bug reproducing; serving_steps.rs:790 (assert!(!policies.is_empty(), …)) behind an unconditional row is the live instance. The current fix is correct, but a stale-help regression there would hide forever — consider having the guard fail the run rather than the scenario.
• tests/e2e-cucumber/tests/e2e/serving_steps.rs:817-819 — assert_no_policy_refused matches two full product sentences verbatim; a harmless rewording ("CPU execution is not a fallback") silently turns the scenario green with the defect intact. Matching on the policy name plus a nonzero rc would be more durable.
• tests/e2e-cucumber/features/examine.feature:26 and :128 — two scenarios are both labelled Scenario: 11, and 11/12 are inserted ahead of 3. Harmless (the harness keys on @id:, which I confirmed unique) but confusing to read.
• Six of the twelve new scenarios (@merge-queue ×3, @requires-gpu @requires-bare-metal ×3) never run on the required job — the merge queue and self-hosted GPU lanes are where they land, and ci.yml:653-655 calls the latter non-blocking. Worth stating in the PR description so the coverage claim is not read as gating.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:824-827 — the "recognised non-default group" premise takes the first entry in /etc/group (likely root/daemon), not a plausibly device-owning group; the assertion still holds, but the premise is weaker than the comment claims. Also: nothing lints for an expectations.toml row whose id no longer matches any scenario, so a future orphan row would sit inert and unreported.

[rominf]

🔴 Automated review · pr-review-watcher · dd5fce0

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 note (dd5fce0, after the rebase): this change request replaces the automation's earlier one at bd8a44c and carries the complete set. Of that round's three blockers, two were re-found blind this round and are marked standing below (the absence-wait drain in tui_driver.rs; diagnose-42's missing @requires-bare-metal). The third — scenario-index collisions with main — is cleared: this round's review did not re-find it, and putting it to the reviewer afterwards confirmed the head merges cleanly with the current base and that feature_naming (7) and the crate's lib tests (190, incl. scenario_indexes_are_unique_across_the_suite and every_expectation_row_names_a_scenario_that_exists) pass on that merge result. Everything else is new. The gate rests on the three logic findings (examine-29, the absence-wait drain, diagnose-42); the bare-path citations, the Lint (PowerShell) failure (a PowerShell Gallery install error at the module-install step; the job passes on main — a re-run should clear it) and the description mismatch are reported but would not block on their own. The "E2E self-hosted" workflow is still pending with no jobs started at this head, so no GPU/Windows/WSL lane has measured the new scenarios — missing evidence, not a finding. The hand-written change requests on this PR are not this automation's and are untouched.

Review — needs work

Full review of the whole change.
Implements part of EAI-8024 (the tracker now files it as ROCMAI-108). Eight scenarios get expected-failure rows, five ship as guards, and EAI-8017/8018 are deliberately left uncovered. The EAI-8010 coverage the description lists is not in this change; it is main's existing update-04.

Blocking

• (new) examine-29 now fails on the only lane it runs on: its When writes a slot its Thens no longer read — tests/e2e-cucumber/tests/e2e/examine_steps.rs (user_inspects_both_ways, assert_examine_reports_driver_version_and_date, assert_examine_driver_age_follows_the_date, user_inspects_for_scripting_first)

  • user_inspects_both_ways used to store the human examine report in cli_stderr. It now stores it in cli_other_output.
  • Commit 97615d6 moved examine-15's reader to the new slot. The two driver Thens used by examine-29 (@requires-os:windows @requires-gpu) were not moved. They still do world.cli_stderr.as_ref().expect("no readable inspection"), so on the Windows GPU lane they panic before asserting anything.
  • At the merge-base c586618 the When wrote cli_stderr and those Thens read it, so this change is what breaks it. No expectations.toml row covers it.
  • The same half-migration remains in user_inspects_for_scripting_first, which still stores the human report via world.cli_stderr = Some(human). That contradicts the new cli_other_output doc in e2e.rs.
  • Confidence 85 · logic · Fix: point both driver Thens, plus user_inspects_for_scripting_first and its reader, at cli_other_output.

• (standing since bd8a44c, re-found blind) A crashed dashboard now satisfies every absence wait — tests/e2e-cucumber/tests/e2e/tui_driver.rs (wait_for_screen_where, wait_until_gone), tests/e2e-cucumber/src/diagnostic_wording.rs (the_drain_is_still_reached_with_the_callers_clause)

  • When the child has exited, wait_for_screen_where now calls drain_final_frame_where(Some(describe), &mut is_ready) and returns Ok(()) if the predicate holds on the dead process's last frame. prw-base has no drain here.
  • wait_until_gone passes |screen| !screen.contains(marker), which any final frame without the marker satisfies. A dashboard that crashed or quit therefore passes every wait_until_gone and cleared-cell wait. That includes the steps whose whole point is that a key did not quit, such as the decline-quit-prompt step in dash_steps.rs.
  • The doc comments still state the opposite. wait_until_gone says "A child that exits first is a failure rather than a success". wait_for_screen_where says "Here that recovery is given up on purpose" and that liveness is checked "before the predicate".
  • The new guard in diagnostic_wording.rs asserts that this exact drain call is present, so it holds the regression in place. AGENTS.md §3 warns about this: "a pin over a false claim holds the claim in place."
  • Traced by reading; not reproduced under a pty.
  • Confidence 85 · logic · Fix: drain only on the positive path (wait_for_screen, or a flag only that path sets), then realign the docs and the guard.

• (standing since bd8a44c, re-found blind) diagnose-42 runs on WSL2, where the check it compares is never produced, and the xfail row hides that — tests/e2e-cucumber/features/diagnose.feature (@id:diagnose-and-fix-agree-on-how-to-verify), tests/e2e-cucumber/expectations.toml (its when = {} row)

  • The scenario's only gate is @requires-os:linux. WSL2 reports os_family linux, so it resolves to run on e2e-wsl.
  • In crates/rocm-core/src/diagnose.rs, check_4_render_group is registered for &["linux"] only, and platform_family returns "wsl" there. fix-4-render-group is never emitted, so section_for fails and the step panics. The when = {} row records that as the EAI-8020 bug reproducing.
  • The row's own comment says the bug "holds wherever the catalog runs at all", which is false on WSL2. That lane reports a bug it cannot reach and can never go stale.
  • Confidence 85 · logic · Fix: add @requires-bare-metal, as diagnose-43/44/45 have.

• (new) New markdown citations are bare paths, against AGENTS.md §5 — tests/e2e-cucumber/README.md (the new tier paragraph, the @merge-queue row, and the cite-a-symbol paragraph)

  • The added text cites .github/workflows/e2e-selfhosted.yml, docs/ci-hardware-testing.md and apps/rocm/src/main.rs in backticks.
  • §5 requires a citation of a repo file "as a link relative to the citing file". The citation is not in any exempt location: not .github/, not root README/CONTRIBUTING, not docs/rocm-docs. This README already uses relative links elsewhere, e.g. [src/bin/](src/bin/).
  • Confidence 85 · mechanical · Fix: make them relative links. The illustrative main.rs:16427 can stay bare.

• (new) A required check is failing at this head — Lint (PowerShell)

  • The status rollup for dd5fce02 shows it FAILURE, and it is in main's required-status-check list.
  • It failed at the "Install PSScriptAnalyzer" step with a PowerShell Gallery "No match was found" error. This change touches no PowerShell, and the same check passes on main's latest commit, so it looks like infrastructure. It is still red.
  • Confidence 100 · mechanical · Fix: re-run it; escalate if PowerShell Gallery keeps failing.

• (new) The description misstates what the change contains — PR description

  • It claims work that is not in the diff:
    • an EAI-8010 update --dry-run expected-failure row (no such row or scenario here; main's update-04 already covers it);
    • a section adding two EAI-7960 rows (deleted in d6825bb);
    • a "Suite concurrency" section (available_parallelism / E2E_MAX_CONCURRENT appear nowhere in the diff; reverted in ea4fd91);
    • "CI is green on the current head across all three workflows", which is false: one required check is red and the self-hosted workflow has started no jobs.
  • It contradicts the diff: it says EAI-8006 "ship[s] as [a] guard, with no expected-failure row", but serve-auto-gpu-selection-names-a-device carries an EAI-8006 Windows row and its scenario says "Expected to FAIL on Windows".
  • It omits real changes:
    • the tui_driver.rs wait/drain refactor (the source of the second finding);
    • the new diagnostic_wording.rs guard module;
    • the moved examine report slot;
    • the non-blocking-rationale rewording across ci.yml, e2e-selfhosted.yml, docs/ci-hardware-testing.md and crates/e2e-report;
    • the new xtask date-agreement test;
    • the widened ascii_output_contract directory skip.
  • Confidence 85 · architectural · Fix: rewrite the description to match the diff. Narrowing it is the remedy, not adding the missing work.

Non-blocking

• (new) A merge left a duplicated fragment in a module doc — tests/e2e-cucumber/tests/e2e/update_steps.rs (module //! block)

  • //! binary (EAI-8072). Mock lane. now follows the update-07/08 paragraph mid-sentence.
  • Confidence 85 · mechanical · Fix: delete the stray line.

• (new) chat-10's comment points at the wrong scenario — tests/e2e-cucumber/features/chat.feature (chat-10 comment)

  • "Scenario 7 names the model and works", but the CLI one-shot that passes the model is chat-08. chat-07 talks to the endpoint over HTTP.
  • Confidence 85 · mechanical · Fix: say chat-08.

• (new) One of three sibling "proven out" comments was left behind — .github/workflows/e2e-selfhosted.yml (e2e-gpu-w7900)

  • This change rewrote the strix-ubuntu and strix-windows comments so that non-blocking is "for the reason in this file's header".
  • The w7900 job still says "Non-blocking while this hardware is proven out, as the MI350P lane is". That mixes the continue-on-error reason back into "non-blocking", which the new header calls "not the reason any of them is non-blocking". It also cites an MI350P lane this file does not define.
  • The text was there before this change.
  • Confidence 85 · mechanical · Fix: reword it to the continue-on-error sense only.

• (new) The guard module's own doc gives a false reason — tests/e2e-cucumber/src/diagnostic_wording.rs (body_of doc)

  • It says wait_until_gone "quotes its own bare marker in messages of its own". wait_until_gone emits no message; its {marker:?} only builds the describe label.
  • Confidence 70 · mechanical · Fix: correct the stated reason.

• (new) The "harness read failure, not a verdict" panics are scored as the verdict — tests/e2e-cucumber/tests/e2e/diagnose_steps.rs (assert_verification_agrees, assert_remedy_command_agrees)

  • Under when = {} xfail rows, any panic counts as the bug reproducing. A missing section or unparsable groups therefore reads as EAI-8020; the WSL2 finding above is a live case.
  • Confidence 70 · logic · Fix: make precondition failures distinguishable from the measured assertion, or soften the messages.

• (new) serve-26's help parser is half-exercised — tests/e2e-cucumber/tests/e2e/serving_steps.rs (advertised_device_policies)

  • Only the block Possible values: rendering runs today; the inline [possible values: …] branch is never reached.
  • The block scan stops only at a line starting --, so a following -x, --foo option would not end it. The empty-list guard does catch a total misread.
  • Confidence 70 · logic · Fix: a small unit test with both clap renderings.

• (new) groups_in_command parses forms neither command prints — tests/e2e-cucumber/tests/e2e/diagnose_steps.rs (groups_in_command)

  • Both diagnose and fix print -a -G x,video. The --groups= and bundled -aG branches are untested.
  • Confidence 70 · logic · Fix: drop them (the empty-result guard already fails loudly on any unrecognised form) or unit-test them.

• (new) The ASCII-contract skip is wider than its stated reason — xtask/src/ascii_output_contract.rs (directory walk)

  • name.starts_with("target") skips a directory of that prefix at any depth, so a future src/targets/ would silently drop out of the scan.
  • The justification, "target-container/ is the container gate's build dir", matches nothing in the tree.
  • Confidence 70 · mechanical · Fix: name == "target" || name.starts_with("target-"), or apply it at the repo root only, and cite what creates that directory.

• (new) Two copies of the feature-file scan are kept identical only by comments — tests/e2e-cucumber/src/expectation.rs (feature_files_in, under cfg(test)), tests/e2e-cucumber/tests/feature_naming.rs (feature_files_in)

  • Both comments say the copies must not diverge; nothing enforces it.
  • Confidence 70 · architectural · Fix: expose one pub fn from the lib and call it from both places.

Decisions for the author

• The real-serve scenarios are telemetry, not gates — tradeoff

  • serve-27, serve-29 and serve-30 are @merge-queue. E2E_MERGE_QUEUE is set only on the self-hosted lanes in merge_group (not e2e-gpu), and none of those lanes is required today. Their pinned contracts can regress without blocking anything.
  • The README states this honestly, which keeps per-PR cost down. Confirm that telemetry-only coverage is the intended standing for these contracts.

• A lot of guard code now pins prose rather than behaviour — non-blocking-improvement

  • diagnostic_wording.rs (400 lines over a test driver's doc comments), the "Expected to FAIL" prose binding, and the date-agreement xtask test all guard wording.
  • Mutation runs show they do catch drift. But the second blocking finding shows a wording guard pinning a behavioural regression in place.
  • Consider whether these guards, and the CI-rationale rewording, belong in a separate change from the README contracts. That would also bring this PR back to one logical change (AGENTS.md §11).

Positive signals

• serve-28 checks whether the owned process is still running (OwnedProcess::is_running) before teardown, so the harness cannot clean up after a product failure and turn it green. Its uninstall stays host-safe: --keep-binaries and --keep-cache, under the scenario's own isolated root.
• compare_diagnosis_with_fix gives the stat/id shims to both diagnose and fix. The comparison will not silently become shimmed-versus-real once fix starts reading the device.
• given_usable_device establishes its premise through examine --json before relying on it.

Deployment notes

None. Test, CI-comment and xtask-test changes only; no product code changes.

What this covered

Read: every file in git diff prw-base...HEAD, across two coupled-group workers, plus the product code the new steps drive:

• diagnose.rs / fix.rs (check_4, platform_family, CHECKERS)
• examine.rs probes
• the serve, uninstall and chat paths
• rocmd service stop

The diff runs from merge-base c586618 to head dd5fce02. prw-base a9e9aa2 merges cleanly with the head (git merge-tree).

Run, in the checkout's own target/:

• cargo test -p e2e-cucumber --lib (190 passed)
• --test feature_naming (7 passed)
• cargo test -p e2e-cucumber --test e2e --no-run (built)
• cargo clippy -p e2e-cucumber --test e2e -- -D warnings (clean)
• the xtask date and ascii tests (28 passed)
• cargo test -p e2e-report (66 passed)
• nine mutation checks, all of which went red: five on diagnostic_wording.rs, two on the expectation.rs scan, one deleted expectations.toml row, one re-dated docs/ci-hardware-testing.md

Sign-off check: 54 commits, none missing a trailer.

Did not run:

• no e2e scenario was executed;
• no Windows-target cargo check;
• the GPU, Windows and WSL lanes did not run at this head, so every GPU-lane xfail row is unmeasured on this commit;
• prior-changes pass: earlier review comments are deliberately out of reach here;
• the design questions were investigated in the driving context, not by independent workers (fan-out budget of two).

Ticket: EAI-8024 retrieved by identifier (title, body, status only).

Not reconciled against any PR discussion here.

Requested changes addressed: commits were pushed after this review and all review threads are resolved. Re-review requested.

[tomastola]

Thanks for the quick round. Re-read at 7ce66d5b:

• wait_until_gone: looks right. Draining the last frame on exit now only happens for the positive wait_for_screen form, so an absence wait can no longer be satisfied by a dead process's empty screen. The diagnostic_wording.rs guard follows the move to wait_loop.
• diagnose-42: looks right. @requires-bare-metal keeps it off WSL2, and the row's comment now says so.

Two findings that still hold at this head:

1. serve-28's row pins documented behaviour as a bug. README.md:1202-1203 says, under what uninstall warns about: "Managed service records exist. uninstall doesn't stop their background processes." serve-28 asserts the opposite, and the serve-uninstall-stops-what-it-manages row in expectations.toml calls the warning "the behaviour being disputed". That contradicts the file's own rule that a row means a known bug, and the section header that each scenario asserts "the behaviour the README leads a reader to expect". Either the product and README change first, with the scenario landing alongside, or this one is framed as a proposed contract outside the README-walkthrough set, with its row comment and the header adjusted.

2. serve-27 never checks that anything stopped. the CLI reports that it stopped a process (tests/e2e-cucumber/tests/e2e/serving_steps.rs:1301-1322) asserts rc == 0 and stopped processes: N with N ≥ 1, and nothing else. A services stop that signals the pid but leaves the server answering would pass, even though the panic text says "the server was running and is now stopped". Suggest also asserting that the endpoint no longer answers or that the recorded pid is gone, the way serve-28 checks OwnedProcess::is_running.

Non-blocking:

• Orphan-row test. every_expectation_row_names_a_scenario_that_exists (tests/e2e-cucumber/src/expectation.rs:1926) is correct, and a bogus row does turn it red. But its tag-lines-only rule is untested: switching it to scan whole lines stays green today, because every commented @id: in model_serving.feature also exists as a real tag. Extracting the scan into something like scenario_ids_in(text) and giving it a fixture with a commented-out id would pin the property the comment describes.
• README citations. The header above the README-contract rows says they "were all filed from one pass through README.md". I couldn't find README text stating the contracts behind diagnose-42/44/45 (that diagnose and fix agree on the remedy and its verification, and that a usable device isn't offered as a permission cause) or serve-30 (a second server avoids an address in use). The README's diagnose/fix section only says rocm fix takes the id diagnose reports. Either the header could say these are contracts implied by the walkthrough, or the README could state them.
• tests/e2e-cucumber/tests/e2e.rs:1544-1551. The concurrency comment narrates this branch's history ("tried on this branch and reverted … failed here at 2..=6"). A reader of main won't have that context. The rationale for 1 vs 64 fits better in the code, and the history fits better in the PR or commit message.

[nowycondro]

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

Review — needs work

Full review of the whole change: the diff between merge-base c586618 and head 7ce66d5b across all 25 files (test-only; 12 new Gherkin scenarios plus matching expectations.toml rows, a new diagnostic_wording.rs self-consistency guard, an e2e.rs harness extension, and comment-only CI/docs rewording). This PR has already been through several rounds of review (an automated pass and a human pass by tomastola at this exact head); most previously-reported blocking issues (the absence-wait drain in tui_driver.rs, diagnose-42's missing @requires-bare-metal, bare-path README citations, a stale PowerShell check) are confirmed fixed at this head. Three issues remain open.

Blocking

• The PR description still misstates what the diff contains — PR description vs. tests/e2e-cucumber/expectations.toml, tests/e2e-cucumber/tests/e2e.rs
  Independently reconfirmed at this head: the description's "Pinned as expected failures" table lists an EAI-8010 row, but no such row exists in expectations.toml (grep finds nothing). The "Two rows added for EAI-7960" section describes dash-gen-tps-held-after-scrape-failure/dash-gen-tps-expiry-boundary rows that are not in the file. The "Suite concurrency" section describes an available_parallelism()/E2E_MAX_CONCURRENT scheme that is not in e2e.rs — the actual comment there (around the max_concurrent block) says this was "tried on this branch and reverted". And "Two ship as guards, with no expected-failure row" names EAI-8006 as row-less, but expectations.toml carries a bug = "EAI-8006" row (os = "windows") for serve-auto-gpu-selection-names-a-device.
  Confidence 85 · architectural · Fix: rewrite the description to match the current diff rather than an earlier draft of it.

• serve-28 pins the README's own documented behaviour as a bug — tests/e2e-cucumber/features/model_serving.feature:384, tests/e2e-cucumber/expectations.toml:225-232, README.md:1201-1203
README.md states plainly, under what uninstall warns about: "Managed service records exist. uninstall doesn't stop their background processes." The EAI-8014 row and serve-28 scenario assert the opposite as an expected failure, and the row's own comment calls the warning "the behaviour being disputed" — but the README isn't disputing it, it documents it. This contradicts the PR's own framing (each row pins a gap between what the README promises and what the product does); here the README already promises the behaviour the scenario pins as a bug.
  Confidence 85 · logic · Fix: either land the product/README change together with this scenario, or reframe this as a proposed contract outside the README-walkthrough set (adjust the row's reasoning and the section header's "the behaviour the README leads a reader to expect" claim accordingly).

• serve-27 never checks that the server actually stopped — tests/e2e-cucumber/tests/e2e/serving_steps.rs:1302 (assert_stop_count_reported)
  The step only asserts rc == 0 and a stopped processes: N line with N >= 1. A services stop that signals the PID but leaves the process (or its listening endpoint) alive would still pass, despite the panic message's own claim ("the server was running and is now stopped"). serve-28's assert_owned_server_stopped shows the available pattern (OwnedProcess::is_running) for actually confirming this.
  Confidence 85 · logic · Fix: additionally assert the registered PID (or endpoint) is gone, not just the reported count.

Non-blocking

• tests/e2e-cucumber/tests/e2e/serving_steps.rs (advertised_device_policies): only the block Possible values: clap rendering is exercised by any scenario; the inline [possible values: …] branch has no test reaching it.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs (groups_in_command): the --groups= and bundled -aG parsing branches are unexercised by any scenario.
• tests/e2e-cucumber/src/expectation.rs (every_expectation_row_names_a_scenario_that_exists): correct, but its "tag lines only, not whole-file" scanning rule is itself untested — a regression there (e.g. picking up a commented-out @id:) would currently go unnoticed.

Decisions for the author

• Near-duplicate directory-scan logic in src/expectation.rs and tests/feature_naming.rs (feature_files_in) — tradeoff. The comments are explicit that this duplication is deliberate (independent guards that must "belong together" rather than share one helper), trading a small amount of duplication for two independently-failing checks. Reasonable as stated; flagging only so it's a confirmed choice rather than an oversight.

Positive signals

• tests/e2e-cucumber/src/diagnostic_wording.rs is a genuinely useful guard: it pins the TUI driver's wait/drain diagnostics against the doc comments describing them in both directions (code→doc and doc→code), which the file's own history shows has drifted silently before.
• The new stat/id shims (stat_shim_path, id_shim_path in tests/e2e-cucumber/tests/e2e.rs) are scoped tightly to the one invocation shape the product makes, so they can't quietly swallow unrelated commands.
• Consistent, careful separation throughout the new assertions between "harness could not measure this" and "the product failed the check" (e.g. assert_verification_agrees, assert_named_groups_exist), which avoids a harness read failure masquerading as a product defect.

Deployment notes

• Test-only change; no product (crates/, apps/) behaviour is touched.
• CI is green at this head: gh pr checks 241 shows every required check passing, including E2E tests and E2E consolidated report (both were pending/red in earlier rounds). The self-hosted GPU/Windows/WSL lanes in e2e-selfhosted.yml are non-blocking by design (not in branch protection's required list), so their results remain informational rather than a merge gate — consistent with this PR's own rewording of that rationale.
• No migration or compatibility concerns; this only adds scenarios, harness helpers, and xtask contract tests.

What this covered

Read the entire diff between c586618 (merge-base) and 7ce66d5b (head) across all 25 changed files in full, including the CI/docs comment-only changes, expectations.toml's new rows, the new diagnostic_wording.rs module, the expectation.rs/feature_naming.rs scan-and-guard tests, mock_server.rs, the e2e.rs harness additions (OwnedProcess, stat_shim_path/id_shim_path, stop_managed_services's env fix), every touched step-definition file, and the tui_driver.rs wait/drain refactor.

Did not run any tests, lint, or builds — this sandbox has no Rust toolchain or network access for cargo; relied on gh pr checks 241 for CI status, which is the live signal reported above.

Reconciled against this PR's existing review history: compared this head against the most recent automated round (against the prior head dd5fce02) and the most recent human round (tomastola, at this exact head). Confirmed by direct inspection — not by trusting either report — that the earlier round's blocking findings are fixed here, and independently verified tomastola's two still-open findings (serve-28, serve-27) against the README and the step code myself before including them above. The PR-description mismatch above was independently re-derived from the current diff rather than carried over from an older round's text.

---

(Correction note: an earlier version of this review, posted at this same commit, contained content from a different PR due to a sandbox file-collision bug on the review tooling's side. This text replaces it as the actual review of PR #241.)

Requested changes addressed: commits were pushed after this review and all review threads are resolved. Re-review requested.

[rominf]

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

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 note (7ce66d5): this round files no change request, and the automation's standing change requests on this PR (at bd8a44c3 and dd5fce02) are being withdrawn. The three logic objections they carried were standing rather than re-found: this round's blind full review did not re-find any of them, and each was then put to the reviewer after its report was frozen (anchored evidence, read not run), which found it no longer holds — examine-29's driver Thens and both "both ways" Whens now share cli_other_output; absence and cleared-cell waits go through wait_loop(.., drain_on_exit = false), which returns an error when the child has exited, so only the positive wait_for_screen drains; diagnose-42 carries @requires-bare-metal and its row comment now scopes the bug to bare metal. Of the earlier non-gating items: the README citations are now relative links, and Lint (PowerShell) is green at this head (every reported check is green). The description mismatch is standing (re-found blind below). The method rates three findings below as blocking; this account does not gate on them: the description mismatch is description-only (the squash builds its body from commit messages), and the other two — a wording guard that cannot detect the drain becoming unreachable, and a stale doc paragraph — leave behind no false green in what the suite reports (an unreachable drain would surface as a loud presence-wait failure), so they fall under the floor. All three are worth fixing. The "E2E self-hosted" workflow is still pending with no jobs started at this head, so no GPU/Windows/WSL lane has measured the new scenarios — missing evidence, not a finding. Every finding below is new unless marked standing. The hand-written change requests on this PR are not this automation's and are untouched.

Review — needs work

Full review of the whole change.
This covers most of EAI-8024 (EAI-8006, 8007, 8009, 8011, 8014, 8015, 8016, 8019, 8020, 8021). It does not cover EAI-8010. EAI-8017 and EAI-8018 are left out on purpose and recorded as uncovered.

Blocking

• (standing since dd5fce0, re-found blind) The PR description claims work the diff does not contain, and leaves out work it does — PR description; tests/e2e-cucumber/expectations.toml; tests/e2e-cucumber/tests/e2e.rs:1545-1552

  • Claimed but absent:
    • An EAI-8010 update --dry-run scenario and row. EAI-8010 appears nowhere in the diff.
    • "Two rows added for EAI-7960". Commit d6825bb deletes them, and expectations.toml has no EAI-7960 row.
    • A mock-lane concurrency clamp of 2..=6 with an E2E_MAX_CONCURRENT override. No such variable exists, and the comment at e2e.rs:1545 says the 64-way default was "tried on this branch and reverted".
    • EAI-8006 "ships as a guard, with no expected-failure row". The serve-auto-gpu-selection-names-a-device row exists, scoped os = "windows".
  • Present but not mentioned:
    • The tui_driver.rs wait-loop refactor and the new src/diagnostic_wording.rs.
    • The re-dated required-check observation in e2e-selfhosted.yml and docs/ci-hardware-testing.md, plus its new xtask test.
    • The target-* skip in xtask/src/ascii_output_contract.rs.
    • The reworded non-blocking text in crates/e2e-report.
    • The EAI-7423 row for serve-27.
  • Confirmed by grepping HEAD for each claimed identifier.
  • Confidence 100 · architectural · Fix: rewrite the description to match the diff. The work is done, so correct the description rather than add work.

• (new) A guard test can never fail on what its name says it checks — tests/e2e-cucumber/src/diagnostic_wording.rs, the_drain_is_still_reached_with_the_callers_clause

  • The test only checks that the source text contains drain_final_frame_where(Some(describe),. Whether that call is reachable depends on drain_on_exit.
  • Worker A set wait_for_screen's true to false, making the drain unreachable from every caller. All 7 diagnostic_wording tests stayed green, including this one.
  • Its failure message also claims wait_for_screen_where hands its clause to the drain. It does not: it calls wait_loop(.., false).
  • Confidence 100 · logic · Fix: assert the call-site flag the drain depends on (the true passed from wait_for_screen), or rename the test to what it actually checks.

• (new) wait_for_screen_where's doc still describes the driver as it was before the refactor — tests/e2e-cucumber/tests/e2e/tui_driver.rs:775-808

  • It says "Unlike wait_for_screen, the liveness checks run before the predicate", and that the positive form "can afford the opposite order — and drains … via drain_final_frame". Both forms now run the same wait_loop, liveness first. The positive form drains through drain_final_frame_where.
  • It also says describe reaches "four diagnostics", including the drain message. With drain_on_exit = false it reaches three.
  • The code governs: wait_loop's own doc gives the reason that absence waits must never drain.
  • This is the comment that diagnostic_wording.rs exists to keep accurate (AGENTS.md §5, keep behaviour claims in sync).
  • Confidence 85 · mechanical · Fix: rewrite the paragraph so the only difference it states is drain_on_exit, and list three diagnostics plus the drain message reached through wait_for_screen.

Non-blocking

• (new) wait_for_screen changed behaviour with no doc update and no test — tui_driver.rs:738-745

  • Previously a marker already on screen returned Ok even if the reader thread had panicked. Now the reader panic returns Err first.
  • Its doc still says a call whose marker is already present "returns immediately".
  • The new direction is the safer one.
  • Confidence 70 · logic · Fix: note the change in the doc.

• (new) Nothing tests the new orphan-row test's tag-line filter — tests/e2e-cucumber/src/expectation.rs:1937-1941

  • The comment calls ignoring a commented-out @id: "the one thing this test exists to catch".
  • Worker A deleted the filter and all 51 expectation::tests still passed.
  • Confidence 100 · logic · Fix: extract the scan into a helper and add a fixture test containing # @id:gone.

• (new) xfail rows absorb premise and harness-read failures — tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1685, :1692, :1806, :1817; reconciler at tests/e2e-cucumber/tests/e2e.rs:1713-1724

  • The new steps' panics say "harness read failure, not a verdict". The reconciler counts any failure of an ExpectXfail id as the expected failure.
  • So a broken premise in diagnose-42/44/45, chat-10, serve-28 or serve-30 is recorded as the bug reproducing.
  • Confidence 85 · logic · Fix: pin the expected failing assertion in the row or step, or report premise failures as unexpected.

• (new) serve-30's Then rejects one of the two outcomes its feature comment accepts — tests/e2e-cucumber/tests/e2e/serving_steps.rs:1426-1460; tests/e2e-cucumber/features/model_serving.feature:403-406

  • serve prints port: in its plan before launch.
  • A fix that refuses at bind time, after printing the plan, would land in the Some(SERVE_PORT) arm and stay red. That row would never go stale.
  • Confidence 70 · logic · Fix: in that arm, also accept a nonzero exit that names the port or says "in use".

• (new) The "e2e-mock" id is a contract between two files, held together only by prose — tests/e2e-cucumber/src/mock_server.rs:679-684, :718; tests/e2e-cucumber/tests/e2e.rs:867

  • Confidence 85 · mechanical · Fix: use one shared const in both places.

• (new) Prose that is already wrong or will mean nothing after a squash-merge — tests/e2e-cucumber/README.md:173-177; tests/e2e-cucumber/tests/e2e.rs:1545-1552

  • The README says main.rs "alone is over twenty thousand lines". wc -l gives 19,642.
  • The README also tells this branch's review history ("Most of the citations written during this suite's review had already drifted…").
  • e2e.rs says "tried on this branch and reverted … failed here".
  • Confidence 100 · mechanical · Fix: keep the rule and the reason; drop the count and the history.

• (new) Leftover MEASURE. planning markers — tests/e2e-cucumber/expectations.toml:235, :244, :280

  • The word is defined nowhere. Commit f06f417 says the dispatch "answered the MEASURE questions", but three rows still carry the marker.
  • Confidence 85 · mechanical · Fix: remove the markers, or say what each row's scope was measured on.

• (new) diagnose-44 runs a symptom its Gherkin never states — tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1629-1647; tests/e2e-cucumber/features/diagnose.feature:659-664

  • The Given "a user who hit a device-permission failure" is deliberately empty. The When hard-codes DEVICE_PERMISSION_SYMPTOM, and diagnose-44 does not include that Given.
  • Confidence 70 · mechanical · Fix: have the Given set the symptom on the World and both Whens read it, and add the Given to diagnose-44.

Decisions for the author

• diagnostic_wording.rs checks doc comments against source text — non-blocking-improvement

  • Its 400 lines grep the source to hold comments and format strings together. That certifies the strings, not the behaviour, which is what AGENTS.md §3 warns about.
  • The drain finding above is a case of it: every check stays green while the clause can never reach the drain.
  • A unit test that drives wait_loop's outcomes and asserts the messages would check what the doc claims.

• Three new GPU scenarios are telemetry rather than gates — tradeoff

  • serve-27, 29 and 30 are @merge-queue and run only on the non-blocking self-hosted lanes.
  • That saves per-PR GPU budget, but a regression there blocks nothing.
  • The description says this, so confirm it is the intended trade.

• The PR mixes several unrelated changes — non-blocking-improvement

  • Besides the contract scenarios, it carries a harness wait-loop refactor, a rewording and re-dating of the required-check observation across five files, and an xtask directory-skip change.
  • Splitting the last two out would make each one reviewable on its own.

Positive signals

• The uninstall scenario (serve-28) checks the product stopped its child before harness teardown runs, so teardown cannot clean up after a product failure and turn it green.
• Rows for bugs that no lane reproduces (EAI-8007, EAI-8017/8018) were left out instead of shipped as stale XPASS rows or vacuous coverage, and expectations.toml records why.

Deployment notes

None

What this covered

What was read: every changed file in 24e405a7…7ce66d5b (diff from merge-base c5866180), read completely across two coupled-group workers.

• Group A: harness src/, expectations.toml, e2e.rs, tui_driver.rs, feature_naming.rs, e2e-report, README.
• Group B: feature files, step files, workflows, xtask, docs.
• The product code the new steps assert against was traced too: diagnose.rs, fix.rs, examine.rs, serve_cmd.rs, uninstall.rs and the help text in main.rs.

What was run:

• cargo test -p e2e-cucumber --lib --test feature_naming: 190 + 7 passed.
• cargo test -p xtask on the date and ASCII tests: 28 passed.
• Mutations:
  • Turned the tests red: examine-30, examine-31, serve-26, and the xtask date test.
  • Stayed green: the drain-reachability guard and the orphan-row tag filter.
• Mock e2e for examine-30/31, serve-26/28 and chat-10 with the checkout's own binary: 3 passed; chat-10 and serve-28 failed for the bugs their rows state.
• CI: every reported check on the head is green. No self-hosted GPU lane reported for this head, so diagnose-43/44/45 and serve-27/29/30 were not run on the reviewed commit, and nothing here ran them.
• Sign-off: the check tool reported all 56 commits signed off.

Test verdicts:

• Guards examine-30/31 and serve-26 fail when the product contract is broken (shown by mutation).
• The xfail scenarios XPASS when their bug is fixed. A broken branch that would stay green in them is any premise failure, which gets recorded as the xfail.
• The new expectation-scan fixture tests pin their branches, except the tag-line filter.
• The pre-existing-behaviour resolver tests added here would also pass on the base. They add coverage; they do not test this change.

Did not run:

• The prior-changes pass: no earlier review comments on these files were read.
• clippy.
• The GPU scenarios.

Process deviation: one worker's first mock e2e run used the host-installed rocm from PATH, because the harness falls back to it when ROCM_CLI_BINARY is unset. The operator forbids that. The run was confined to the harness's isolated temp dirs, including serve-28's uninstall --yes --keep-binaries --keep-cache, whose plan listed only /tmp/rocm-e2e-* paths. It was discarded and repeated with the checkout's binary, and only the repeat counts as evidence above.

The PR discussion was not reconciled here.

Re-reviewed at 7ce66d5: the three logic objections (examine-29 report slot, absence waits satisfied by a dead dashboard, diagnose-42 on WSL2) no longer hold, the README citations are relative links, and every reported check is green. The description mismatch persists and stays reported as non-blocking. See the 7ce66d5 round.

[nowycondro]

🔴 Automated review · pr-review-watcher · 40d58dd

Review — needs work

Full review of the whole change (d59f596f...40d58ddf, 25 files, +2925/-139, test-only — no product code touched). This implements most, but not all, of the work ticket EAI-8024 (cited in the PR title) describes — that ticket lists 13 README-walkthrough defects in scope (EAI-8006..8021 inclusive), explicitly including EAI-8010; this PR's own diff does not actually deliver EAI-8010 despite the description listing it as pinned (see Blocking below).

Blocking

• The PR description makes several specific, checkable claims about this diff that the diff does not support — PR description (no single file/line; each sub-claim checked against the cited paths below).
  Four separate instances, all verified by reading the actual diff/repo state rather than trusting the prose:

  1. "Suite concurrency" section is fabricated. The description claims the mock lane's concurrency "now derives from available_parallelism() clamped to 2..=6, overridable with E2E_MAX_CONCURRENT, forced to 1 whenever a GPU or shared cache/runtimes directory is in play," with specific benchmark numbers (an 8-way run starving on a 10-core host vs. a 4-way run at 33s/54s). None of this exists: grep -rn "available_parallelism\|E2E_MAX_CONCURRENT" over the repo at this PR's head returns zero matches, and tests/e2e-cucumber/tests/e2e.rs:1553-1558's max_concurrent computation is byte-identical to main (still a hardcoded 1/64 split, no env override, no core-count derivation). A comment this PR does add at that exact spot says the clamped approach "was tried on this branch and reverted" — confirmed against the branch's own history (a commit titled "stop bounding mock-lane concurrency, which broke two of main's scenarios"). The described feature was attempted and abandoned, not shipped.
  2. "Two rows added for EAI-7960" is fabricated. tests/e2e-cucumber/features/dash.feature and tests/e2e-cucumber/tests/e2e/dash_steps.rs have zero diff in this PR (byte-identical to main), and no flaky = true (or any) row for dash-gen-tps-held-after-scrape-failure / dash-gen-tps-expiry-boundary exists in expectations.toml at head or base. The scenarios themselves (pre-existing, untouched) assert throughput correctly stays visible across a scrape failure — the opposite of the "gen throughput is cleared" defect the description says they pin.
  3. The EAI-8010 row in the "Pinned as expected failures" table does not exist. The string "EAI-8010" appears nowhere in the repository at this PR's head or base; tests/e2e-cucumber/features/update.feature (where a rocm update --dry-run scenario would live) is byte-identical to main; no corresponding expectations.toml row exists. This isn't a stray unrelated ticket reference — EAI-8010 is one of the 13 defects EAI-8024 explicitly scopes this PR to cover, so the gap is a real shortfall against both the ticket and the PR's own claimed accomplishments, not just a wording slip.
  4. The EAI-8006 "ships as a guard... with no expected-failure row" claim is also wrong, in the other direction: expectations.toml has a real row for serve-auto-gpu-selection-names-a-device (when = { os = "windows" }, bug = "EAI-8006"), so it is not an unconditional guard the way EAI-8007's (correctly, no-row) entry is.

  Confidence 100 (each point directly verified: grep/diff against the repo at both the PR's base and head, plus the branch's own commit history for point 1). Fix: rewrite the "Suite concurrency" and "Two rows added for EAI-7960" sections and the "Pinned as expected failures" table to match what the diff actually contains — either land EAI-8010's scenario or drop it from the table, and correct the EAI-8006 characterization.

Non-blocking

• Stale doc comment in tui_driver.rs after this PR's own refactor — tests/e2e-cucumber/tests/e2e/tui_driver.rs:788-798
  This PR merges wait_for_screen and wait_for_screen_where into one shared wait_loop (a sound, behavior-preserving consolidation — traced the drain_on_exit flag and confirmed each function keeps its pre-existing semantics). But the untouched paragraph just above wait_for_screen_where still says "Unlike wait_for_screen, the liveness checks run before the predicate, and deliberately so" — true at main, false now that both functions share one check order (panic → exit → predicate) via wait_loop. The same paragraph's link to [`drain_final_frame`](Self::drain_final_frame) is also now the wrong function; the exit-recovery path it describes calls drain_final_frame_where. No runtime effect, but it misdescribes ordering guarantees in TUI-synchronization code a future maintainer will reason from.
  Confidence 85 · mechanical · Fix: update or remove the "Unlike wait_for_screen..." paragraph and retarget the link to drain_final_frame_where.

Decisions for the author

• Test-plan counts not independently re-verified — tradeoff
  The description's "Test plan" section states specific scenario/xfail counts for the no-GPU and MI300X lanes (52/10 and 64/16). This review did not reproduce those runs. Given the description errors found above, it's worth the author re-confirming these numbers are current before merge rather than carried over from an earlier draft.
• chat-local-uses-the-running-server's assertion is slightly looser than the bug it's named for — non-blocking-improvement
  The scenario pins EAI-8009 by asserting the absence of the mock-reply success text rather than asserting the specific "nothing is running" failure wording the bug description names. This is a normal, acceptable xfail pattern (it does fail for the right underlying reason, traced against apps/rocm/src/providers.rs), but a reader matching the scenario against its row's reason text has to take the wording on faith rather than see it pinned. Author's call whether it's worth tightening.

Positive signals

• diagnose_steps.rs's labelled() helper deliberately returns None rather than Some("") for a blank verify-line specifically so two empty values don't compare equal and read as the diagnosis and fix preview "agreeing" when neither said anything — a real false-positive the comparison would otherwise have had.
• serving_steps.rs's assert_owned_server_stopped guards against a vacuous pass (asserts a process was actually registered before checking liveness) and deliberately checks the OS for the process's liveness before scenario teardown runs, so the harness's own cleanup can't be mistaken for the product having stopped it — exactly the ordering bug this scenario exists to catch, applied to the test itself.
• The device-group shim scenarios (e.g. diagnose-credits-a-usable-device) prove their own fixture precondition through the CLI's own examine --json output before relying on it, rather than assuming the stat/id substitution produced the intended state.

Deployment notes

None. Test-only change; no product code touched. The CI workflow edits are reworded comments/doc clarifications only (traced both ci.yml and e2e-selfhosted.yml hunks directly) — no required-vs-non-blocking job reclassification.

What this covered

Full diff d59f596f6e08297c207f008021eecb37defc3558...40d58ddf84ca92bfe5f8dbb373d58dfacf7137bc (25 files), split into 7 coupled-file groups. 4 groups were reviewed by independent fan-out workers: harness core (expectation.rs/diagnostic_wording.rs/lib.rs/mock_server.rs), xtask contracts (ascii_output_contract.rs/workflow_contract.rs/feature_naming.rs), examine/chat/engines step defs + features, and the e2e driver/orchestration group (e2e.rs/tui_driver.rs/update_steps.rs) — the last of these is where the concurrency fabrication above was independently re-confirmed by that worker. 3 groups — diagnose (diagnose.feature+diagnose_steps.rs), serving (model_serving.feature+serving_steps.rs), and CI/docs/expectations (ci.yml, e2e-selfhosted.yml, consolidated.rs, expectations.toml, both READMEs) — were read completely by me directly rather than by an independent worker, after repeated fan-out attempts for them failed with a concurrent-subagent-capacity error in this shared environment (not a capability failure: 4 of 7 spawns did succeed); flagging per the method's rule that a pass run by the reviewer itself is not an independent pass.

Did not run: a dedicated "prior changes" pass (mining historical PR review threads on these files) — no practical route to that from here; git log/git blame over the core harness and driver files and expectations.toml's recent main-branch history was read instead (history pass) and surfaced nothing concerning. Code-comment accuracy was folded into each code pass above rather than run separately.

CI: read via gh pr view --json statusCheckRollup — all 24 reported contexts (CI, CodeQL, readthedocs) are SUCCESS; no failing required check.

Step 9: read the PR description in full, and retrieved the ticket named in the title (EAI-8024) by identifier through the project's tracker. Only identifiers and scope already disclosed by this PR's own title/description are referenced above.

Not reconciled against any discussion on this PR — this review was conducted blind to any existing review thread, by design.

[siloteemu]

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

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 note (1cb79b8): this change request carries the complete set of objections that hold now; the automation's change request at bd8a44c3 was dismissed since. Of that round's five blockers: the description mismatch and the orphan-row half of the guards finding were re-found blind this round and are marked standing below. Two were not re-found, and each was then put to the reviewer after this report was frozen (an anchored check, not part of the blind review); both still hold in part and are appended at the end of Blocking as standing: the report-row metadata half of the guards finding, and the "any host" step comment from the diagnose WSL finding (its tag and toml comment are fixed). The remaining two — the head not merging / no CI, and an exited process satisfying negative screen waits — were not re-found blind, and the anchored check also found they no longer hold at this head (the head merges cleanly into the base and CI ran; absence waits now check liveness first and only the positive form drains). They are withdrawn.

Review — needs work

Full review of the whole change (prw-base...HEAD, 25 files). It is test, CI-comment and docs work only. The only product-facing artefacts are scenarios and expectation rows.
Ticket EAI-8024 is named in the title but could not be retrieved: no issue-tracker tool was available in this run. The description claims the change is test-only and "pins the contracts a README walkthrough expects"; the Step 9 mismatches are in Blocking.

Blocking

• (standing) The description claims work that is not in the diff, and omits work that is. — PR body

  • Claimed but absent:
    • An EAI-8010 update --dry-run row and scenario. EAI-8010 appears nowhere in the tree. update-04 is a pre-existing scenario, and prw-base already has 14 dry-run hits in update.feature.
    • "Two rows added for EAI-7960". prw-base has no such rows, and the branch deletes them (commit d6825bb). The dash-gen-tps-* rows are not in expectations.toml.
    • The "Suite concurrency" section (available_parallelism(), E2E_MAX_CONCURRENT). grep finds neither symbol, and e2e.rs adds no concurrency code. This is probably the ea4fd91 revert ("stop bounding mock-lane concurrency").
    • "EAI-8006 … ships as a guard, no expected-failure row". The diff adds an xfail row for it, serve-auto-gpu-selection-names-a-device with when = { os = "windows" }.
  • Present but unmentioned:
    • The ci.yml and e2e-selfhosted.yml header and comment rewrites, and the re-dating of the required-check observation from 2026-09-11 to 2026-09-15.
    • A change to the text printed into the consolidated report (crates/e2e-report/src/consolidated.rs).
    • A new xtask contract test.
    • The target-* skip in xtask/src/ascii_output_contract.rs.
    • The wait_for_screen / wait_loop refactor in tui_driver.rs. It changes behaviour: liveness is now checked before the marker, so a reader panic with the marker already on screen was Ok and is now Err.
    • The diagnostic_wording.rs guard.
    • The feature_naming.rs scan changes.
  • Also note that the "non-blocking in ci.yml" claim is stale: the self-hosted lanes live in e2e-selfhosted.yml.
  • Confidence 85 · architectural · Fix: rewrite the description to match the diff, or restore the missing work. The two ci.yml/e2e-selfhosted.yml and target-* items also fall under one-logical-change (AGENTS §11), so consider splitting them out.

• (new) A doc comment on wait_for_screen_where now contradicts the code this PR rewrote — tests/e2e-cucumber/tests/e2e/tui_driver.rs:788, :803-806

  • It says "Unlike wait_for_screen, the liveness checks run before the predicate", and that the positive form "can afford the opposite order … via drain_final_frame".
  • wait_for_screen (:742) now delegates to the same wait_loop, which checks reader panic and child exit first (:831-854). It drains via drain_final_frame_where only when drain_on_exit is set. The linked drain_final_frame is now the predicate-less variant.
  • The code governs. It is one shared loop, and the base's "predicate first" wait_for_screen was the only thing that made the sentence true.
  • Confidence 85 · mechanical · Fix: rewrite the paragraph to say liveness is checked first in both forms and only the positive form drains on exit. Also update the "returns immediately" caveat at :733, which now holds only while the child is alive and the reader has not panicked.

• (new) The new wording guard stays green when a diagnostic it claims to pin is reworded — tests/e2e-cucumber/src/diagnostic_wording.rs:121-153, :182-205

  • Three of the four clause templates are asserted with a file-wide code_text(source).contains(template). Each also appears verbatim in the sibling wait_for_popup_body_where:
    • panicked while waiting until (tui_driver.rs:833, :885)
    • timed out after … waiting until (:857, :911)
    • before {describe}. (:848, :893)
  • The count test counts {describe} placeholders, not wording.
  • Verified by a run: I changed before {describe}. to ahead of {describe}. at tui_driver.rs:848 (in wait_loop only). cargo test -p e2e-cucumber --lib diagnostic_wording stayed 7/7 green. The module header claims "every format! template it names is still in the code".
  • Test verdicts: it fails on base, so it does test the change (verdict 1). Rewording one template in wait_loop alone leaves it green (verdict 2).
  • Confidence 100 · logic · Fix: search body_of(source, "wait_loop") and body_of(source, "drain_panic_message") instead of the whole file. body_of already exists in the file.

• (standing) The orphan-row test comment claims a protection no input can fail on — tests/e2e-cucumber/src/expectation.rs:1998-2008 (every_expectation_row_names_a_scenario_that_exists)

  • The comment says tag lines only are read, so a #-commented @id: cannot vouch for an orphan row.
  • Every commented @id: in features/ is backtick-wrapped, so strip_prefix("@id:") rejects it anyway. I replaced !line.starts_with('@') with false and the test still passed, and no fixture exercises the guard.
  • Per the test-quality standard, a check whose comment claims a branch it cannot fail on ranks worse than no test: it stops the next reviewer from looking. It would also fail on base (declared_ids is new), so verdict 1 holds.
  • Confidence 100 for the mutation staying green · logic · Fix: extract fn tag_ids(text) -> BTreeSet<String>, unit-test it with a # @id:ghost line, and reuse it in tests/feature_naming.rs:174-196, which has a third copy of the same scan.

• (new) The README added by this PR states things the code contradicts — tests/e2e-cucumber/README.md:111-114, :159, :174

  • :174 says main.rs "is over twenty thousand lines". apps/rocm/src/main.rs is 17,891 lines. The paragraph exists to forbid unchecked numbers, and its sample citation looks like a real one.
  • :111 says "Three tags tier the suite at runtime", but @requires-docker / E2E_INCLUDE_DOCKER is a fourth (expectation.rs:465, e2e.rs:1498).
  • :159 says the self-hosted lanes set E2E_MERGE_QUEUE on merge_group. e2e-gpu (MI300X) does not; only the Strix Ubuntu, Strix Windows, WSL and W7900 lanes do (e2e-selfhosted.yml:475, :628, :976, :1567). expectation.rs:152-157 and workflow_contract.rs:4695 agree with the workflow, not the README.
  • Confidence 85 · mechanical · Fix: drop the line count, say "four tags" or reword, and say "every self-hosted lane except MI300X".

• (new) A citation added by this PR points at the wrong file — tests/e2e-cucumber/tests/e2e/serving_steps.rs:1486-1487

  • It says the serve plan println! is in apps/rocm/src/main.rs. It is at apps/rocm/src/serve_cmd.rs:840. The comment exists to stop a reader chasing the wrong symbol, so the wrong path undercuts it.
  • Confidence 85 · mechanical · Fix: name serve_cmd.rs.

• (standing — not re-found blind; settled by putting it to the reviewer, anchored) The report-row metadata test cannot fail on the label mapping it exists to check — tests/e2e-cucumber/src/expectation.rs:2429-2477 (the_report_row_carries_each_expectations_own_metadata)

  • The bd8a44c3 round's wording: "Making Expectation::label() return "pass" for ExpectXfail leaves the first test green. It compares xfail.expected with ….label(), which is label() compared with itself. The skip and pass rows never assert expected at all. … Fix: assert the literals "xfail", "skip" and "pass"."
  • Still holds at this head, checked by a run: the xfail assertion at :2444-2452 compares xfail.expected with Expectation::ExpectXfail{..}.label(); the skip and pass rows (:2460-2476) assert only bug, reason and flaky. Changing the ExpectXfail arm of label() (:374) from "xfail" to "pass" leaves cargo test -p e2e-cucumber --lib at 198 passed, 0 failed. The test is new in this PR.
  • Fix: assert the literals "xfail", "skip" and "pass".

• (standing — not re-found blind; settled by putting it to the reviewer, anchored) A step comment still says the device-permission symptom scores "on any host" — tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1657

  • The bd8a44c3 round's wording: "Two comments are false for the same reason: "makes the device-permission cause score on any host" (diagnose_steps.rs:1108) and "holds wherever the catalog runs at all" (the toml row)." That round's main point — the scenario running on WSL — is fixed: the scenario (now diagnose-48, diagnose.feature:735) carries @requires-bare-metal, and the toml row's comment was corrected.
  • The step comment part still holds: check_4_render_group is still registered for linux only and platform_family returns "wsl" first, so the cause does not score on WSL or Windows. The tag keeps the scenario off those hosts, so the comment is wrong rather than harmful.
  • Fix: "on any bare-metal Linux host".

Non-blocking

• (new) serve-29's When panics if the Given's relaunch rescued a stalled serve — serving_steps.rs:1276-1311 against :544-588. A relaunch mints a new service id (service_identity.rs:97-109) and the stopped first record stays on disk, so assert_eq!(recorded.len(), 1) fails. Read from the code, not run; it needs a GPU lane. Confidence 70 · logic · Fix: select the non-stopped record, or have the Given record the id it served.
• (new) Duplicate helpers in diagnose_steps.rs — section_for (:1696) against cause_block (:3409); labelled(_, DIAGNOSE_VERIFY_PREFIX) against diagnose_verify_line (:3419); labelled(preview, FIX_VERIFY_PREFIX) against printed_verify_line (:471); DEVICE_PERMISSION_FIX_ID (:1655) against COMMAND_FAILURE_FIX_ID (:58). The two block matchers differ (contains(fix_id) vs ({fix_id})), so they can drift. Confidence 85 · mechanical.
• (new) advertised_device_policies has an unexercised branch — serving_steps.rs:1135-1148 (inline [possible values: …]). --device is a ValueEnum rendered in block form, so the branch is unreachable today, and the block-boundary handling has no unit test. Mutation not run. Confidence 70 · mechanical.
• (new) proposed_group_command's Will run: strip is probably never reached — diagnose_steps.rs:1707-1721. find returns the first line containing usermod/-G, which for the preview is the catalog Commands: block, not the "about to run" line. The verdict is unaffected today. Read, not run. Confidence 70.
• (new) xfail rows cannot tell a measured divergence from a harness read failure — expectations.toml rows for diagnose-and-fix-agree-on-how-to-verify, -the-remedy-command and diagnose-credits-a-usable-device. The steps panic with "harness read failure", but expectation.rs treats any failure of an xfail scenario as the expected outcome. A product format drift stays "satisfied". Confidence 70 · architectural.
• (standing in part) Stale or misleading comments (all in scope):
  • diagnose.feature:3-6 still says every scenario is GPU-independent. diagnose-48 to -51 are @requires-bare-metal, and 49 to 51 are @requires-gpu too.
  • The "Expected to FAIL on a host whose …" comments for diagnose-50 and -51 understate the failure. The Given steps substitute the device answer, so both fail on every bare-metal GPU host.
  • diagnostic_wording.rs:48 says send_until's "only caller", but dash_steps.rs has at least four.
  • e2e.rs:711-713 says start_managed_service_process registers a "ready" service. The CLI demotes a ready record whose endpoint never answers to starting.
  • engines_steps.rs:96-111 narrates a two-step wait and a "local poll loop" that exist in neither base nor head.
  • examine_steps.rs:595-597 and :768-770 narrate how the old slot behaved.
  • e2e-selfhosted.yml:30-34 says consolidated.rs and the README "point at these rather than asserting it themselves". Both state the non-blocking fact (consolidated.rs:1263-1266, :1677-1678; README :117, :159), and only the date is deferred.
  • expectations.toml labels MI300X as gfx943, but the repo's fixtures treat gfx942 as correct.
  • Confidence 70 to 85 · mechanical.
• (standing) Review-process narration in repository text. "drifted by ~170 lines before this PR was through review" (serving_steps.rs:1231), "Two reviewers have now landed on that one" (:1491), "round-9 false positive" and "Four rows were deleted while this file was being written" (expectation.rs), "tried on this branch and reverted" (e2e.rs:1565), "several consecutive review rounds" (diagnostic_wording.rs), README "during this suite's review". Run numbers such as "run 196" in expectations.toml name no workflow. "Since fixed on main" and "this PR" wording will not mean anything after merge. Keep the reasons, drop the history. Confidence 70.
• (new) The target-* skip has no test — xtask/src/ascii_output_contract.rs:274-276. It is also a second logical change (see the first Blocking item). Confidence 70.
• (new) Vacuity guards invert the matrix's stated end state — expectation.rs around :2011-2016 and :2312 assert non-empty rows and claims, while expectations.toml says each row "deletes itself". Removing the last row would force deleting the guard. Confidence 70 · design.
• (new) README / examine scenario titles overstate their assertions. examine-31's Then accepts any owner/repo without resolving it, though the title says "resolve". examine-32 only asserts the word "dashboard" is absent from the default-command paragraph, though the title says it "tells the two apart". Confidence 40 to 70.

Decisions for the author

• Tradeoff: diagnose-49 to -51 are @requires-gpu only because stat_device needs a real device node. diagnose-40 and -42 already plant ROCM_CLI_TEST_HOST_ROOT, and stat_device reads through host_path. Planting dev/kfd there would let these run per PR instead of only on non-blocking GPU lanes. The trade is harness complexity against earlier detection.
• Tradeoff: a 400-line wording guard in diagnostic_wording.rs covers comment wording in one helper file. It is unrelated to the README-contract theme, and its prose-staleness gap is what the Blocking findings above show. Confirm it belongs in this PR.
• The "re-confirmed 2026-09-15" date is a reading no checkout can verify, and it is about six weeks old. The new xtask test, the_required_check_observation_is_dated_the_same_in_both_places, only checks that the two carriers agree. The workflow's suggested gh api …/branches/main/protection reads classic protection only, not rulesets. Someone with API access should re-read the setting.

Positive signals

• OwnedProcess::is_running (e2e.rs:234-263) panics on a try_wait error instead of reporting "stopped". Teardown therefore cannot turn the uninstall bug into a pass.
• The confirmed_dates test fails on a count of 0 or 2 as well as on a mismatch, and each branch (flattening, the re-confirmed match, the date shape) was shown by reading to fail when broken. Its message names which file to re-date.
• given_usable_device proves its premise through the CLI's own examine --json and shims stat and id narrowly. diagnose-51 also asserts exit 0 and not-out-of-scope, so an absent cause means "checked and cleared".
• claimed_failure_ids reports a claim that binds to nothing and has a fixture test per branch. The orphan-row test checks both sides for vacuity, and the feature scans refuse a nested directory instead of silently covering less.
• The known_model_names and serve_example_models helpers derive both sides of the examine-31 check from the CLI's own output, so the scenario does not hardcode which model the help should show.

Deployment notes

None. The change is test and CI-comment only, with no product change and no new dependency.

What this covered

• What was read. Every changed file in the diff prw-base...HEAD (merge-base 84f951e2, head 1cb79b88), read completely, plus the product code, callers, workflows and expectation rows they pin. The work was split across five read-only workers:
  • the expectation harness;
  • diagnose scenarios and the wording guard;
  • serving, chat and TUI driver;
  • examine, CI, docs and xtask;
  • the agent-instruction, history and code-comment passes.
• What I ran.
  • Against the head tree: cargo test -p e2e-cucumber --lib (198 passed), --test feature_naming (7 passed), and cargo test -p xtask -- workflow_contract ascii_output (100 passed).
  • Mutations: reworded the wait_loop diagnostic (the wording guard stayed green) and disabled the @ line guard in the orphan-row test (it stayed green). Both were reverted.
  • Not run: clippy, the full workspace tests and python scripts/smoke_local.py. The scenario verdicts for the xfail rows come from reading, not runs.
• Sign-off. check_signoff.py returned {"checked": 56, "missing": []} with exit 0 for prw-base..HEAD, so no finding. The repository requires sign-off (AGENTS §2).
• CI at this head. Run via gh pr view:
  • "E2E tests (W7900, current)" is still QUEUED.
  • Both "E2E tests (Strix Halo, Ubuntu, release/nightly, current)" lanes concluded CANCELLED. A cancelled or queued check is not a pass.
  • MI300X, Strix Halo Windows, Strix Halo WSL2, the required E2E tests, E2E consolidated report, clippy, build-and-test, windows-build-and-test, prek and the other listed checks are green.
  • The @requires-gpu and @merge-queue scenarios (diagnose-48 to -51, serve-29 to -32) have therefore not been seen on the Strix Ubuntu or W7900 lanes at this head.
• Did not run.
  • Prior-changes pass: it needs earlier review comments, which this review does not read.
  • The history pass judged merge fallout from the net prw-base...HEAD hunks, not each of the 70+ merge-commit resolutions.
  • EAI-8024 and the other EAI ids were not retrieved (no tracker tool).
  • The GitHub description was read for Step 9 only after the findings were settled. Not reconciled against any review discussion here.
• Leak scan. Hits were only "AMD GPU" product wording and bare EAI ids, both permitted. No emails, URLs or hostnames were added.
• The scratch build directory /home/teemu/.cache/pr-review-watcher/tmp/review-241-af3df3f7 was deleted. Free space stayed at 38G or more.

[tomastola]

Thanks for the round. Re-read at 54892b52.

Fixed, and checked by mutation (cargo test -p e2e-cucumber --lib passes 199/199 at head; each mutation below then turns exactly one test red):

• Wording guard scope. assert_both_sides now searches only wait_loop and drain_panic_message. Rewording before {describe}. in wait_loop alone now fails every_clause_diagnostic_is_both_emitted_and_documented, and so does rewording the , waiting until {wanted} drain fragment.
• Orphan-row tag filter. It is now tag_ids with a # @id:ghost test. Replacing the @ filter with if false now fails a_commented_out_id_does_not_declare_a_scenario. This covers my earlier non-blocking item.
• Report-row labels. The test now checks the literal labels "xfail", "skip" and "pass". Making label() return "pass" for ExpectXfail now fails the test.
• Docs and citations. The README now says four tags, says MI300X doesn't set E2E_MERGE_QUEUE, and no longer gives a line count. The serve_cmd.rs citation is corrected, and the "any bare-metal Linux host" comment is fixed.

Still blocking from my last review:

1. serve-30 (was serve-28) still pins documented behaviour as a bug. README.md:1243-1244 still says, under what uninstall warns about, "Managed service records exist. uninstall doesn't stop their background processes." The serve-uninstall-stops-what-it-manages row (expectations.toml:224-233) still calls that warning "the behaviour being disputed". The section header at :170-172 still says every row asserts what "the README leads a reader to expect". Same options as before:

   • land the product and README change alongside this scenario, or
   • present it as a proposed contract outside the README-walkthrough set, and adjust the row comment and the header to match.

2. serve-29 (was serve-27) still doesn't check that anything stopped. assert_stop_count_reported (serving_steps.rs:1377-1397) is unchanged. It asserts rc == 0 and stopped processes: N ≥ 1, while its panic message claims "the server was running and is now stopped". A services stop that signals the process but leaves it serving would still pass. Assert that the recorded process is gone, the way serve-30 uses OwnedProcess::is_running.

New at this head:

3. CI is red on two checks:
   • prek fails on cargo fmt: rustfmt wants the new assert_both_sides signature on one line (diagnostic_wording.rs:186-191). I reproduced it with cargo fmt --check.
   • Commit signatures + sign-off fails because merge commit 027af0a5 has no Signed-off-by trailer. Adding the trailer to that merge (or redoing the merge with -s) fixes it.

Non-blocking:

• tui_driver.rs:778-783. wait_for_screen_where's doc still says describe reaches "four diagnostics", including the drain message. The paragraph your fix rewrote now says only the positive form drains. wait_for_screen_where passes drain_on_exit = false, so it reaches three; the drain message is reached only through wait_for_screen.
• Two items from my last review are unchanged:
  • The header still says all rows "were all filed from one pass through README.md". The diagnose-48/50/51 and serve-32 contracts aren't stated in the README.
  • e2e.rs:1565-1572 still tells this branch's history ("tried on this branch and reverted … failed here at 2..=6").
• The PR description still claims the EAI-8010 row, the EAI-7960 rows and E2E_MAX_CONCURRENT. None of them are in the diff, and this is the description mismatch every automated round has blocked on.

Requested changes addressed: commits were pushed after this review and all review threads are resolved. Re-review requested.

[nowycondro]

🤖 Automated review (Claude Code) · 4002c3e

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, done as a single-context pass by deliberate instruction (the fan-out this method normally requires was explicitly overridden for this run; every file was still read in full rather than partially).
Implements EAI-8024: adds e2e coverage pinning 13 README-walkthrough defects as expected-failure rows, plus four scenarios whose underlying bugs were fixed while this PR was open (now guards) and two flaky rows for EAI-7960. Test-only, no product code touched — confirmed by the diff (.github/workflows/, docs/, tests/e2e-cucumber/, xtask/ only).

Blocking

• The PR description claims a concurrency-tuning change that is not in the diff — tests/e2e-cucumber/tests/e2e.rs:1568-1573 (description's "Suite concurrency" section).
  The description states: "The mock lane now derives its scenario concurrency from available_parallelism() clamped to 2..=6, overridable with E2E_MAX_CONCURRENT … A 64-way default was starving scenarios into their own timeouts: an 8-way run starved on a 10-core host while a 4-way run was both green and faster (33 s vs 54 s)." git grep -n "available_parallelism\|E2E_MAX_CONCURRENT\|2\.\.=6\|clamp(2" across the full repository at commit 4002c3e returns exactly one hit, and it is inside a newly added comment explaining why the mock lane does not get such a clamp ("The mock lane stays at 64 … Both pass at 64-way and fail at 2..=6"). The max_concurrent computation itself (if cap.has_amd_gpu || shared_cache_dir().is_some() || shared_runtimes_dir().is_some() { 1 } else { 64 }) is byte-identical to the base commit (df1548a) — only the comment was added. No available_parallelism, no clamp, no E2E_MAX_CONCURRENT override exists anywhere in this diff or in the resulting tree.
  Confidence 90 · logic · Fix: either the concurrency-tuning change was dropped before this diff was finalized (restore it or drop the paragraph), or the paragraph describes work from a different PR/commit that got pasted in here — either way the description needs to match what is actually shipped.

• The PR description claims a new "remove environment variables from the child" harness addition that is not in the diff, and also misdescribes the prior state — tests/e2e-cucumber/tests/e2e.rs (the run_rocm_with_env helper; description's "Harness additions" section).
  The description states: "Confined to the test crate: removing environment variables from the child (the existing helper only sets them) …" No such addition exists: run_rocm_with_env still takes only &[(&str, &str)] and only calls cmd.env(key, value) — unchanged in this diff. The "only sets them" framing is also inaccurate independent of this PR: .env_remove() already existed pre-PR in tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:1142 and tests/e2e-cucumber/tests/e2e/tui_driver.rs:293, neither touched by this diff.
  Confidence 90 · logic · Fix: drop this clause from the description, or add the capability it claims.

Everything else in the (unusually long and detailed) description was cross-checked against the diff and the product source it cites and held up: the six no-GPU-lane scenario count, the four "became guards" ids (confirmed absent from expectations.toml), the EAI-8020 diagnose/fix divergence (confirmed against crates/rocm-core/src/diagnose.rs:922 vs crates/rocm-core/src/fix.rs:365/2044), the EAI-8015 UNKNOWN-sentinel fallback (confirmed at crates/rocm-core/src/diagnose.rs:908-918), and the EAI-8019 ValueEnum/cpu_only fix (confirmed at apps/rocm/src/main.rs:1342-1352). That makes the two mismatches above stand out rather than reflect general carelessness, but per the review method a description that claims work not present in the diff blocks regardless.

Non-blocking

None.

Decisions for the author

None.

Positive signals

• tests/e2e-cucumber/src/diagnostic_wording.rs is a genuinely good anti-drift guard: it checks, in both directions, that the TUI driver's diagnostic format! templates and the doc comments describing them agree — including the message count stated in prose — which is exactly the kind of rot ordinary tests don't catch.
• The new every_expectation_row_names_a_scenario_that_exists / every_expected_to_fail_comment_still_has_a_row pair in tests/e2e-cucumber/src/expectation.rs closes a real gap (an orphaned xfail row, or a stale "Expected to FAIL" comment after its row is deleted) that nothing previously caught.
• Citing product code by symbol rather than line number in long-lived test comments (tests/e2e-cucumber/README.md's new rule, applied throughout diagnose_steps.rs/serving_steps.rs) is a sound convention given the PR's own account of citations drifting by hundreds of lines during review.

Deployment notes

None — test-only change; CI is green on the pinned head commit across all required checks (E2E tests, E2E consolidated report, clippy, build-and-test, etc.), confirmed via gh pr view --json statusCheckRollup against head SHA 4002c3e.

What this covered

Read the entire diff df1548a…4002c3e4 (25 files, ~3,726 diff lines) in full, including both new files (diagnostic_wording.rs) and every modified test/step/harness file, plus cross-referenced the scenarios' claims against the actual product source (crates/rocm-core/src/diagnose.rs, crates/rocm-core/src/fix.rs, apps/rocm/src/main.rs) and against expectations.toml's own rows. Verified CI status and head-SHA match via gh pr view.

This ran as a single-context pass by deliberate instruction (the task explicitly overrode this method's normal multi-worker fan-out for this review), so the step-7 design-question fan-out and the per-file-group code-pass fan-out described in the method did not run as independent workers; one reviewer read everything sequentially instead. No Rust toolchain was available in this environment, so none of the suite's own tests (including the new unit tests in expectation.rs/diagnostic_wording.rs) were compiled or executed — absence of the claimed concurrency/env-removal mechanisms was established by static reading and repository-wide git grep at the pinned commit, not by running anything. Ticket EAI-8024 and the individual EAI-80xx rows were deliberately not retrieved from Jira (out of scope per this review's instructions); the description's claims were checked for internal consistency against the diff and against the product source instead. Not reconciled against the PR's existing review discussion (65 comments) — this was a blind read of the code and description only.