test(e2e): pin the contracts a README walkthrough expects (EAI-8024) by fredespi · Pull Request #241 · ROCm/rocm-cli

fredespi · 2026-08-12T19:09:49Z

Why

Walking the public README end to end on an MI300X pod turned up places where the CLI does not do what a reader is led to expect (EAI-8024 tracks the set). This adds end-to-end coverage for them. It is test-only: no product code changes.

Each scenario asserts the expected behaviour. Where the product does not do that yet, the scenario gets an expected-failure row in expectations.toml citing its ticket, so CI stays green and the row goes stale (a fatal XPASS) the day the bug is fixed.

Expected-failure rows

Ticket	Scenario	What it pins	Row scope
EAI-8009	chat-13	Chatting locally without naming a model ignores a ready local server that is not the built-in assistant model	everywhere
EAI-8014	serve-42	`uninstall` reports completion while the server it manages keeps running, and removes the records needed to stop it. A proposed contract: the README documents that `uninstall` warns it does not stop them	everywhere (Linux scenario)
EAI-8020	diagnose-49	`diagnose` and `fix` print different steps for verifying the same remedy	everywhere (bare-metal Linux scenario)
EAI-8020	diagnose-51	`diagnose` and `fix` propose adding the user to different groups	everywhere (bare-metal Linux GPU scenario)
EAI-8021	diagnose-52	A device the user can already read and write is still offered as a group-permission cause	everywhere (bare-metal Linux GPU scenario)
EAI-8006	serve-43	`--gpu auto` reports it selected no device on a machine that has one	`os = "windows"`
EAI-8016	serve-44	A second server is given the default address even when it is already in use	everywhere

serve-41 (stopping a server reports and actually achieves the stop) also carries a row, but for EAI-7423: on Strix Halo Linux its real serve is shut down right after reaching ready, the same flaky row its EAI-7423 siblings carry. Its own contract, EAI-8007, held on every lane measured.

diagnose-50..52 and serve-41, -43 and -44 need a real device or a real serve, so they run only on the self-hosted GPU lanes, which are not required checks. serve-41, -43 and -44 are also @merge-queue, so they do not run on per-PR CI at all.

serve-42 runs a real rocm uninstall --yes --keep-binaries --keep-cache, so it is tagged @lifecycle and runs only with the lifecycle set (Linux "Acceptance install lifecycle" CI step), not on every Linux lane. With --keep-binaries the uninstall plans no binary, PATH or shell-profile change. It installs no systemd unit, so there is none to remove. With --keep-cache it leaves the shared cache. What remains is the config and data directories, which isolate_env points at the scenario's own temporary root. The uv and Hugging Face caches are only reported, never removed. The scenario asserts the server stopped before it checks the completion message, so the row records the intended failure, not a wording change.

Guards, no row

These pin a contract that holds on current main, because the bug was fixed while this PR was open or never reproduced on a CI lane:

Each was written against the unfixed product and shipped with an expected-failure row in this PR's first commit. The row was dropped after the fix landed. No separate revert-and-observe-red check has been run against the fixed code.

examine-31: every model the help offers as an example resolves (EAI-8011, fixed in fix(help): recommend a resolvable model and label bare rocm as the launcher (EAI-8011) #296)
examine-32: the help tells bare rocm and rocm dash apart (EAI-8011, fixed in fix(help): recommend a resolvable model and label bare rocm as the launcher (EAI-8011) #296)
serve-40: every --device policy the help offers is accepted (EAI-8019, fixed in fix(serve): validate --device via clap possible values (exit 2) #304)
diagnose-50: no remedy names the group UNKNOWN (EAI-8015, fixed in EAI-8240: Fix diagnose emitting invalid UNKNOWN render-group remediation #302)

Not covered

EAI-8017 and EAI-8018 both need a working Lemonade install, which neither the MI300X runner nor a plain container can provide (missing GLIBC_2.38 / libdrm_amdgpu.so.1). Their scenarios passed without reaching the behaviour under test, so they were removed. EAI-8010 (update --dry-run) was fixed on main while this PR was open and is covered by main's own update scenarios.

Harness changes (test crate only)

stat and id shims scoped to one device path, so a scenario can set up a device-group state instead of depending on the host's. diagnose-50, -51 and -52 each check that examine --json sees the substituted state before relying on it.
Teardown, for every scenario: Drop for E2eWorld and stop_managed_services now pass the scenario's full isolate_env() to rocm services stop. Before, they rebuilt only the three directory variables, so teardown could talk to a different cache than the scenario it cleans up after.
OwnedProcess and start_managed_service_process: a child process the scenario owns and registers as a managed service. The product's stop is asserted with is_running before teardown, so teardown cannot clean up after a product failure and hide it.
cli_other_output for a second command's stdout, and the examine steps moved onto it.
expectation.rs checks: every row names a scenario that exists, every "Expected to FAIL" comment still has a row (with tests for how a comment binds to its tag: contiguous blocks, file headers, blank lines, a claim at end of file), and a commented-out @id: does not count. The feature scan refuses an empty or nested features/ directory. Also tested: a when key misspelled is rejected, a WSL-scoped row holds only there, and only GPU scenarios are gated on the engine starting. The report-row metadata test asserts the literal labels.
ServiceRecordOptions.service_id: a planted service record can carry its own id. The default e2e-mock is left alone by teardown. A scenario that registers a real process it owns passes its own id, so teardown stops it too.
feature_naming.rs refuses a nested features/ directory instead of silently skipping it.
diagnose_steps.rs: the comments on KNOWN_SYMPTOM and MUTATING_FIX_ID are rewritten to match the product. The old MUTATING_FIX_ID comment said fix-4-render-group exits early once the user is in the groups, which run_render_group never did. No behaviour change.
README: documents the four runtime tiering tags, @requires-no-gpu, @merge-queue, and two comment conventions (cite symbols, not line numbers; keep "Expected to FAIL" directly above its tag line).

The parsing helpers (groups_in_command, labelled, section_for, proposed_group_command, advertised_device_policies, serve_example_models) and the shim builders have no unit tests. They are exercised only by their scenarios, most of which run on the self-hosted GPU lanes.

Test plan

Linux container: cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings and --all-targets, cargo test -p e2e-cucumber --lib (206 pass), --test feature_naming (7 pass), cargo xtask e2e (mock lane: 268 scenarios, 4 xfail, 0 XPASS, 0 unexpected), and the lifecycle set with E2E_INCLUDE_LIFECYCLE=1 E2E_ONLY_LIFECYCLE=1 SHELL=/bin/bash (13 pass, serve-42 the 1 xfail, failing at "the server is no longer running"; its uninstall removed only the scenario's own config and data under /tmp/rocm-e2e-*).
Required CI checks on the head commit.
The Strix Halo Ubuntu self-hosted lanes have been cancelled at their 45-minute cap on recent runs here and on main. They are not required checks.

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 a scenario for each and registers it as an expected failure, so CI stays green and every row goes stale the day its defect is fixed. No product behaviour changes. Pinned on the no-GPU lane, so they gate every PR: EAI-8011 the help offers `rocm serve qwen2.5-7b-instruct`, which is neither a name the model listing knows nor an owner/repo reference EAI-8011 the help describes running `rocm` with no subcommand as opening the dashboard, which is what it says `rocm dash` does EAI-8010 `rocm update --dry-run` is refused as a usage error, though the README documents --apply and --dry-run as independent EAI-8019 `serve --help` advertises cpu_only and the command refuses it EAI-8020 the diagnosis and the fix preview print different verification steps for the same remedy EAI-8009 chatting locally with no --model ignores a ready local server that is not the built-in assistant model EAI-8014 uninstall reports completion while leaving the servers it manages running, having deleted the records needed to stop them Pinned on the GPU lane, scoped `when = {}` for a first measurement: EAI-8007 a stop that worked reports that it stopped no processes EAI-8015 a remedy names the group-lookup failure UNKNOWN as a group EAI-8020 the diagnosis and the fix propose different group commands EAI-8021 a device the user can already read and write is still offered as a group-permission cause EAI-8006 `--gpu auto` selects none on a machine that has a GPU EAI-8016 a second server is given an address already in use EAI-8018 Lemonade is advertised ready on the GPU but never answers EAI-8017 installing an engine fails when the session provides no scratch directory Harness additions, all small: removing environment variables from the child (not just setting them), a `stat` shim 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. EAI-8007 was first written against that child-process fixture on the no-GPU lane and XPASSed: a record with no start-time token takes the legacy best-effort path and is counted correctly. It now serves a real model instead, which is the path the pod exercised. Verified in a Linux container: ten new scenarios fail, each for the reason its row states, with no XPASS and no new unexpected failures. The one remaining failure there, engine-shell-marks-the-prompt, reproduces identically on unmodified main — the container runs as root, so the prompt ends in `#` where the assertion looks for `$`. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

The first app-dev-gpu dispatch answered the MEASURE questions the rows were carrying, and three of them came back differently from the plan. EAI-8007 loses its row. Neither fixture reproduces it: a plain process registered as a managed service is counted correctly (no-GPU lane), and so is a real vLLM serve (MI300X). A row would be a stale XPASS on both lanes and would fail them, so the scenario ships as a guard for the contract the pod violated instead. What was different about that host is still unknown. EAI-8021 needed a second substituted answer. The scenario made the device world-writable and expected the group-permission cause to stop being offered, but the GPU runner's user is already in the conventional groups, so the cause scored zero there for a reason that has nothing to do with device access and the scenario passed. It now substitutes the group answer too, so the state it describes — access works, membership absent — holds on any host. EAI-8006 was failing for the wrong reason: the serve was refused for having no active ROCm runtime, before it ever chose a device. It now takes the same managed-runtime precondition its sibling serve scenarios do. Two scenarios could not be measured at all, because `rocm engines install lemonade` fails on the GPU runner within two seconds: - EAI-8017 was passing vacuously — the install never got far enough to leave the engine without a scratch directory, and the assertion tolerated that. It now requires the install to have got under way before it will speak. - EAI-8018 asserted a claim the inventory never makes there, because Lemonade is not installed in the scenario's isolated directories. It now installs it first and reports the install output when the claim is absent. Both will say why on the next dispatch. Also scopes the diagnosis comparisons to the cause under test rather than the first one reported (which cause ranks highest is host-dependent), tightens the scratch-directory match so ordinary path output cannot satisfy it, and stops the uninstall assertion passing vacuously when no server was registered. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

The second dispatch settled the remaining MEASURE questions. EAI-8021 now fails for its stated reason once the group answer is substituted alongside the device one (score 45 with the device readable and writable), so its row stands. EAI-8006 loses its row and ships as a guard. `--gpu auto` reported selecting none on the pod, where the bundled amd-smi could not execute; the MI300X runner's amd-smi runs, so the CLI names a device and the scenario passes there. EAI-8017 and EAI-8018 lose their scenarios entirely. 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 the scenarios were passing vacuously — which reads as coverage while proving nothing. Better no scenario than that one; both stay on their tickets, reproduced by hand on the pod, until a lane can run Lemonade. Removing the Lemonade install from the engine-inventory scenario also stops the suite writing into the runner's shared pre-warm tree, which it had begun doing. That leaves nine of the thirteen pinned as expected failures, two shipping as guards for a contract our lanes uphold and the pod did not, and two not covered. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

#242 landed `Scenario: 14` in model_serving.feature while this branch was also using 14, and the two touch different parts of the file — so git merges them cleanly and the collision is invisible until someone tries to refer to "scenario 14" and finds two of them. Renumbers this branch's five additions to 15-19, after main's highest. No scenario, tag, step or expectation changes. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

The Strix Halo Windows lane finally reported, and it contradicts the reading this branch shipped with. `--gpu auto` really does report `selected none` on a machine that has a GPU — just not on the lanes measured first. MI300X gfx943 linux passes Strix Halo Ubuntu gfx1151 linux passes Strix Halo Windows gfx1151 windows fails: `gpu: auto (selected none)` Same GPU family on both Strix lanes, so the discriminator is the OS rather than the family — which is why this row uses `os = "windows"` and not the `therock_family` scoping `examine-both-forms-agree-on-gpu` uses. So the scenario stops being a bare guard and gets the expected-failure row it should have had, scoped to where the defect was actually observed. It still guards both Linux lanes, where the device is named. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

… bug `services-stop-reports-what-it-stopped` needs a real running server before it can ask what stopping one reports, and that Given serves on whatever engine the host defaults to. On a lemonade Linux host that is the serve which reaches ready and is shut down ~0.08s later (EAI-7423), so the PRECONDITION fails and the scenario never reaches the contract it exists to hold. Measured on Strix Halo Ubuntu: passed in run 196, failed in run 210 with "endpoint did not serve model Qwen3-0.6B after 2 attempt(s) of 300s" — the EAI-7423 signature its sibling rows already describe. Adds the row those siblings carry, scoped identically: os=linux because native Windows lemonade serve works and this passes there, gfx* because it needs a real GPU host, and flaky=true because which of these pass varies run to run and a deterministic-XPASS row would fail that lane on unrelated PRs. The row cites EAI-7423, not EAI-8007 — the stop-reporting contract itself still holds everywhere it has been observed, so it keeps no expected-failure row of its own. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

The Strix Halo lanes cap at 35 minutes, and on main alone the Ubuntu one already runs 32.3 of them. This branch adds three scenarios that each start a real server, costing about nine more minutes on that lane, so both Strix jobs being cancelled at the cap is arithmetic rather than bad luck. Tags them @merge-queue, the mechanism scenarios 6, 6b and 8 already use for exactly this: they drop off the PR fast path and run in the merge queue, where the budget for heavy serves lives and where all three self-hosted lanes still execute them before anything lands. Measured on Strix Ubuntu after the fail-fast row in the previous commit: 232s, 136s and 149s. Everything this branch pins that does NOT need a running server keeps running on every PR, on every lane, unchanged: the help contracts, update, device policies, the diagnose comparisons and shims, local chat, and uninstall. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

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.

The device-policy scenario scraped the FIRST '[possible values:' line out of 'serve --help', which is --engine's clap-generated list, not --device's. So it served '--device lemonade' and '--device vllm', which are refused as unsupported policies -- wording close enough to the cpu_only rejection that the assertion matched and the scenario looked like it was failing for the reason its row states. It was not, and it could never have gone stale: those two values stay invalid whatever happens to EAI-8019, so a fixed bug would still have read as unfixed. Scopes the lookup to --device's own block, stopping at the next option so a --device that advertises nothing cannot borrow a neighbour's list. Verified against the real 'serve --help': the parser now yields gpu_required, gpu_preferred, cpu_only. Introspecting clap's device arg directly would be better still, but this suite is black-box by design and cannot import the product crates; main.rs already guards that list from the inside. Also reads user_can_read/user_can_write from the kfd device by name rather than searching the whole report. Every device in it carries those fields, so the generic walk was answered by whichever sorted first rather than by the device the scenario shimmed. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

Resolve two additive conflicts: - diagnose.feature: keep both scenario sets, renumbering the README-walkthrough scenarios to 13-16 and updating the internal cross-reference. - expectations.toml: keep both appended row blocks. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi · 2026-08-20T09:35:19Z

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.

Brings the branch up to date with main and addresses the review findings that depend on it. Stale expectation rows deleted (their bugs are fixed on main); the scenarios stay as guards: - help-serve-example-names-a-resolvable-model, help-describes-the-default-command (#296 made the worked example 'rocm serve qwen' and relabelled bare 'rocm' the launcher) - serve-rejects-no-advertised-device-policy (#304 hid cpu_only from the advertised list) - diagnose-commands-name-a-real-group (#302 filters the UNKNOWN group lookup) Verified in a Linux container on two hosts: 0 XPASS, 0 stale. Other review fixes: - advertised_device_policies reads clap's block rendering as well as the inline one, so the scenario keeps measuring the contract instead of tripping its own guard - run_rocm_without_env deleted (no call sites) - the services-stop comment now matches its expectations.toml row - 'not the leading remedy' reworded to 'not offered as a cause', which is what the assertion checks - the non-default device group is chosen from plausible device groups rather than whichever /etc/group lists first - scenario numbers made unique and prefixed after main's renumbering - new unit test rejects an expectations.toml row whose scenario no longer exists Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

Missed by the --no-verify commit; caught by the prek lint job. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

@id

feature_naming.rs (added on main) requires every @id: to start with its feature's key so the id alone says which feature it belongs to. Four ids predating that convention are renamed, with their expectations.toml rows. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

The mock lane ran a flat 64 scenarios at once. Its scenarios are safe to run together, but several drive a TUI under a pseudo-terminal and assert on wall-clock windows: the dashboard held-value window is 6s, and at 64-way those steps were descheduled for seconds and missed the window they were timing. Measured at 4.7s, which reports a value that expired on schedule as one that was never held. This surfaced here because the branch adds scenarios: main passes dash-08/09 at 64-way, this branch failed them on three separate hosts, and they pass 10/10 in isolation. Bisected in a container: 8-way starves them even with 10 cores free, 4-way is green and faster (33s vs 54s, since starvation costs more than it buys). Bound to the core count, clamped to 2..=6, with E2E_MAX_CONCURRENT to override when bisecting a lane whose failures look like contention rather than behaviour. Verified twice on the merged tree: 6 xfail, 0 XPASS, dash-08/09 green, with dash_steps.rs unchanged. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

The held-value assertion took a single screenshot after the first failed scrape. Under suite load that look lands late, sees a value that expired exactly on schedule, and reports it as 'cleared immediately' — the harness losing its window, stated as a product regression. It now samples for as long as the window is open, so every in-window sample must show the value and one that does not is the regression. That is strictly stronger than one look and does not depend on the look being prompt. If a whole window passes with no sample at all, the observation is retried on a fresh window (up to three) rather than reported: nothing was measured, so there is nothing to say. The failure instant now comes from the mock's own counter moving, counted relative to its value before the switch so a retry is not satisfied by an earlier failure. Verified on the merged tree: green at the default bound, and the honest 'lost the window' message rather than a false regression claim when forced to 32-way. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

@id

…eview Scope the diagnostic wording guard to wait_loop and drain_panic_message so the sibling popup wait cannot vouch for a reworded template, extract and unit-test the @id tag scan, assert the report label literals, and correct the driver doc, README and citation text. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

…ed contract serve-29 now also asserts the stopped server stops answering. serve-30's row and the expectations header say it is a proposal the README does not make. Also rustfmt the wording guard and trim history narration from two comments. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

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

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

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.

juhovainio

I went through the whole diff and found two things that need fixing before this can merge, plus a few smaller points. Details are in the inline comments.

Blocking

The PR description doesn't match the diff. It describes an EAI-8010 update --dry-run scenario and expectations row, two EAI-7960 dash-gen-tps rows, and a concurrency change (available_parallelism() clamped to 2..=6, E2E_MAX_CONCURRENT). None of these are in the diff, and the e2e.rs comment actually says the mock lane stays at 64. The body also says EAI-8006 ships as a guard with no row, but the diff adds an os=windows row for it. update_steps.rs refers to "flag-handling steps below" that don't exist here. This looks like work lost in a rebase/merge conflict resolution, or a stale body. The PR is currently conflicting, so please rebase, then either restore the missing pieces or rewrite the body from the final diff. "CI is green on the current head" also isn't literally true: the two Strix Halo Ubuntu lanes (nightly, release) ended cancelled at ~45 min. They're non-blocking, but please say so.
Several unrelated changes aren't mentioned and aren't part of the README-contract work: the branch-protection wording rewrites (workflow, ci.yml, docs, consolidated.rs), the new 411-line diagnostic_wording.rs, the tui_driver.rs wait refactor (liveness now checked before the marker, new wording), the engine-prompt wait change, the xtask target-* skip and the new date-match contract test. Please split these into their own PR(s), or at minimum list them in the body so they get a deliberate review.

Non-blocking

The new guard scenarios (EAI-8007, Linux half of EAI-8006, and the others that main now satisfies) have no evidence that they go red when their target is broken. Could you break the target and confirm?
The EAI-8016 row is when = {} but its scenario is @merge-queue, so it doesn't run on the MI300X lane the body credits it to. Where was it measured? The same question applies to the EAI-8006 row.
The EAI-8014 scenario runs a real rocm uninstall on every Linux lane without @lifecycle, which the ticket asked for. Please confirm isolate_env isolates the install dir and HOME-scoped files.
Dropping EAI-8017/8018 is documented here. Worth recording on the ticket too.
Minor: a misplaced doc comment in expectation.rs, and a hard-coded dated branch-protection claim in the public docs that can't be verified from a checkout.

juhovainio · 2026-11-02T14:31:26Z

+# default port is used whether or not something already holds it, so a second
+# server is handed the address the first is on. The scenario holds the address
+# itself rather than starting a second real server. ---
+[["serve-second-server-gets-a-free-port"]]


This row is when={} but its scenario (serve-32) is @merge-queue, so it is skipped on the MI300X lane the body credits it to, and only non-blocking Strix merge-queue lanes run it. Was it measured anywhere? An unmeasured unscoped row risks an XPASS on lanes where the bug does not reproduce.

Fair: it is unmeasured in its current form. It was measured before it became @merge-queue: E2E self-hosted runs 192 (MI300X) and 196/210 (Strix Ubuntu and Windows) all failed it for the stated reason (the taken default address still assigned), with 0 XPASS. It went @merge-queue later to keep the Strix lane under its time cap, and since then only a merge_group run sets E2E_MERGE_QUEUE, so neither PR runs nor a workflow_dispatch can run it. The scenario holds the address itself rather than relying on a second real server, so it does not depend on which engine or GPU the lane has, and serve_cmd.rs on main has no port-availability change since. If it does go stale, the XPASS shows up on the non-blocking merge-queue lanes. The description (7768c4d) no longer credits it to the per-PR MI300X lane.

juhovainio · 2026-11-02T14:31:26Z

+  # with the server already there. Either outcome is fine — pick a free address,
+  # or say the usual one is busy — but silently reusing it is not.
+  # @merge-queue for the serve-cost reason on `@id:serve-services-stop-reports-what-it-stopped`.
+  @id:serve-second-server-gets-a-free-port @requires-gpu @merge-queue


serve-32 is @requires-gpu @merge-queue, so it does not run on the per-PR MI300X lane. The PR body lists it as an MI300X-lane row. Which lane actually measured it?

Answered in the expectations.toml thread for the same row: measured on MI300X and both Strix lanes before it became @merge-queue, unmeasured since, and the description no longer credits it to the per-PR MI300X lane.

Resolves conflicts with main (consolidated report taken from main, chat scenario renumbered to chat-13) and reverts the changes reviewers asked to split out: the TUI wait refactor and its wording guard, the engine-prompt wait, the branch-protection wording and its xtask date test, and the ascii-contract directory skip. Also moves a misplaced doc comment. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

juhovainio

Test-only and well scoped; the scenarios read as contracts and the xfail design is sound. Two things I would like answered before merge, plus smaller items.

To address

serve-30 runs a real rocm uninstall --yes on every Linux job, including the blocking mock lane. ROCMAI-108 planned this one behind @lifecycle on the GPU/bare-metal lane. --keep-cache is explained, but nothing in the diff shows the shared runtimes dir, HOME or shell rc are out of reach. See inline.
Unconditional xfail rows (when = {}) on chat-13, serve-30, diagnose-48/50/51 and serve-32 also absorb Given/precondition panics, so they stay green without proving the Then step is the one failing. Please record per scenario that the failure is at the intended step, and ideally add a positive control for the parser-heavy ones (diagnose-48/50).
The repo rule that a new test is shown to fail when its target is broken: the PR text does not say how the new step logic (serve-28, examine-31/32, diagnose-49 guards) was shown to go red, e.g. serve-28 against a pre-#304 binary.

Smaller items (inline where anchored)

is_running blocks an async worker with std::thread::sleep (up to 5s) while scenarios run concurrently on the mock lane; use tokio::time::sleep or spawn_blocking.
serve-31 has no flaky row on Linux: if serve fails before printing the plan (the EAI-7423 class), assert_auto_gpu_named panics. serve-29 carries a scoped row for this; please confirm serve-31 passes on the Linux lemonade lane or add one.
The "mock lane stays at 64" comment in main() is unrelated to these contracts; drop it or move it.
The 8014 contract is labelled "PROPOSED, not a README one"; make sure the direction is agreed before it is pinned.
The PR body links tracker URLs; please use bare ticket ids on this surface.
ROCMAI-108 still lists 8010/8017/8018 and a new update feature file; update it so it does not read as incomplete.

CI was still pending on E2E (mock), affected-crates tests, Windows and the GPU lanes when I looked.

juhovainio · 2026-11-02T15:33:21Z

+# warning is documented behaviour, so this stays a row only until the product and
+# README change together. No hardware needed. Remove when uninstall stops what it
+# manages first, or when the contract is dropped. ---
+[["serve-uninstall-stops-what-it-manages"]]


Unconditional rows (when = {}) turn any failure of this scenario, including a failing Given (the services list precondition), into the expected failure. Please show in the PR that the failure is at the final Then and that the premise steps pass.

From the mock lane at 3012275 (cargo xtask e2e: 5 xfail, 0 XPASS, 0 unexpected), each unconditional row's scenario fails at its final step and every premise step passes:

chat-13: Given a model is being served ✔, And the model is registered ✔, When one-shot chat without a model ✔, Then the CLI prints the assistant's reply ✘

diagnose-48: Given a device-permission failure ✔, When compare diagnosis with fix preview ✔, Then both give the same way to verify ✘

serve-30: Given a managed server is running ✔, When removing managed files ✔, Then removal reported complete ✔, And the server is no longer running ✘

diagnose-50/51 need a real device, and serve-32 needs a merge-queue run, so the mock lane cannot show them. I will add the step-level evidence for them from a GPU-lane run.

Stop the owned server only when the scenario has an isolated root, require the stopped endpoint to have answered first, require conflict wording for a port clash, match --device exactly, select service records by manifest name, and drop an unrelated comment. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

juhovainio

Requesting changes on one blocking item; the rest are non-blocking.

Blocking

The PR body says serve-34..37 and diagnose-49..51 run only on the self-hosted GPU lanes. serve-35 (@id:serve-uninstall-stops-what-it-manages @requires-os:linux) has no @requires-gpu/@merge-queue tag and uses a sleep child, so it runs a real rocm uninstall --yes on per-PR mock CI and every Linux lane. Please correct the body, and state which paths that uninstall can reach (HOME/shell-rc/systemd as well as config/data/cache) and that isolate_env redirects them. Alternatively gate it as the ticket suggested (@lifecycle).

Non-blocking

The chat-13 and serve-35 xfail rows pass on any failure of the scenario, not the intended one. serve-35 fails first on the literal "uninstall complete", so it may never reach the "server no longer running" assertion.
serve-35 pins a proposed contract that the README contradicts (README says uninstall warns it does not stop managed processes). Confirm a permanent xfail is intended until product and README change together.
The guards (examine-31/32, serve-33, diagnose-49) have no red-before-green evidence in the PR text (AGENTS.md section 3). Please say whether the revert-and-observe-red check was done.
serve-37 holds 127.0.0.1:SERVE_PORT in the cucumber process and relies on GPU lanes being serialized while ensure_serve_port_free uses fuser -k.
Minor: a stray blank line in e2e.rs main(), duplicated shim-writing code in stat_shim_path/id_shim_path, and comments that narrate review history ("round-9 ...") that will age badly.

CI / scope

All visible checks are green. The self-hosted GPU lanes are not in the check list, so serve-34/36/37 and diagnose-49..51 are unexercised here.
The change is test-only, as the ticket asks. It deviates from the ticket in three places: serve-35 isn't @lifecycle-gated, serve-34 is GPU/merge-queue rather than mock-lane, and there are 12 scenarios against roughly 14.

juhovainio · 2026-11-03T10:30:56Z

+# warning is documented behaviour, so this stays a row only until the product and
+# README change together. No hardware needed. Remove when uninstall stops what it
+# manages first, or when the contract is dropped. ---
+[["serve-uninstall-stops-what-it-manages"]]


This row pins a contract the README contradicts (README says uninstall warns it does not stop managed processes). Confirm a permanent xfail is intended until product and README change together.

Yes, it's deliberate: the row stays an expected failure until the product and the README change together. It is reported as a proposed contract (EAI-8014), not a README promise, in both the scenario comment and the PR description. Whether to adopt that contract is for the maintainers to decide on EAI-8014. If you'd rather this PR not carry a proposal, I can drop serve-38.

Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

Renumber this PR's diagnose scenarios to 49..52 after main's diagnose-48. serve-38 runs a real uninstall, so it now runs with the lifecycle set, and it asserts the server stopped before the completion message so its row records the intended failure. Also share the shim-writing code and drop a stray blank line and a review-history comment. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

juhovainio

Re-reviewed at dc5638b. One item to fix before merge, plus some non-blocking notes.

Needs fixing

The PR body's "Harness changes" list omits the teardown change: stop_managed_services and Drop for E2eWorld now pass the full isolate_env() to services stop, which affects every scenario. The rewritten KNOWN_SYMPTOM/MUTATING_FIX_ID comments in diagnose_steps.rs are also unmentioned. Please add both; the body should match the final diff.

Non-blocking

diagnose-50 and diagnose-51 never prove the stat shim answered, so a format drift in the matched argv would let diagnose-50 pass vacuously. diagnose-52 already checks its premise via examine --json; the same would work here.
The guards (examine-31/32, serve-36, diagnose-50) were reworked after their xfail rows were added and have not been shown red again (AGENTS.md section 3). The PR body acknowledges this.
serve-40 accepts "unavailable" as an address-taken phrase, so an unrelated refusal satisfies the check and then trips a fatal XPASS on a when = {} row.
The rows marked MEASURE in expectations.toml are when = {} and unmeasured at this head; the self-hosted run (41632552390) is still pending. diagnose-50/51/52 and serve-37/39/40 only run on those lanes, so they have no green run yet.
The new parsing helpers and shim builders (groups_in_command, labelled, section_for, proposed_group_command, advertised_device_policies, serve_example_models, the shim builders) run only on gated lanes and have no unit tests. The PR states the lane gap but not this one.
Scope vs EAI-8024 / ROCMAI-108: 8017, 8018 and 8010 are disclosed as not covered, and 8007 is covered only as the gated serve-37 rather than on the mock lane the ticket named. Worth reconciling the ticket with what landed.

juhovainio · 2026-11-03T12:47:17Z

+reason = "Letting the CLI choose a GPU reports that it selected none on a Windows machine that has one."
+serve_timeout_secs = 180
+
+# --- EAI-8016: MEASURE. The serve path has no address-availability check: the


Rows marked MEASURE (here, and the diagnose-51 and EAI-8021 ones) are when = {}; the ticket asks for them to be measured on the self-hosted lanes and narrowed before merge. Replace the MEASURE wording with what was measured once run 41632552390 finishes.

Not done yet, because nothing has been measured. Every E2E self-hosted run on this branch has been cancelled before reaching these scenarios (ROCm/rocm-cli runs #3755, #3769, #3777, #3885 and #3933), and #3977 on dc5638b had not started when this push superseded it. I'll replace the MEASURE wording with the measured lanes once a self-hosted run on the head completes, or narrow or drop any row it contradicts.

nowycondro

🤖 Automated review (Claude Code) · dc5638b

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

Review — no blocking findings

DEGRADED REVIEW (single-context, see What this covered)

Full review of the whole change (all 14 changed files, read completely).
Implements most of the local tracking ticket behind EAI-8024 (adding e2e coverage for a batch of README-walkthrough defects); the ticket covers 13 defect ids, and this PR's own description accounts for every one of them individually (a row, a dropped-row guard, or an explicit non-coverage explanation) — no gap in that accounting.

Blocking

None

Non-blocking

None

Decisions for the author

feature_files_in's subdirectory-rejection scan is duplicated between tests/feature_naming.rs and src/expectation.rs — tradeoff
Both copies are near-identical (same Path::extension() test, same non-empty assertion, same "has grown a subdirectory" panic), and the code comments on both sides already argue for keeping them separate ("the two guards belong together... a subdirectory that made one of them fail loudly while the other silently skipped it would be the worst of both"). A shared helper would remove the duplication but the two scopes (lib #[cfg(test)] module vs. a separate integration-test binary) aren't trivially unifiable without exposing this as a public lib function purely for test reuse. Worth a second look if a third copy ever appears, not before.

Positive signals

tests/e2e-cucumber/src/expectation.rs's new self-checks (every_expectation_row_names_a_scenario_that_exists, every_expected_to_fail_comment_still_has_a_row, the comment-binding scan in claimed_failure_ids) close a real drift hazard — stale expectations.toml rows and "Expected to FAIL" comments that no longer match a live row — and each edge case (blank-line binding, commented-out @id:, dangling claim at EOF) has its own targeted unit test.
serving_steps.rs's assert_owned_server_stopped (serve-38) correctly orders its liveness check before teardown and explicitly guards against the vacuous-pass case (!world.owned_processes.is_empty()); verified against the actual Gherkin step order in model_serving.feature (Then the server is no longer running precedes And the removal is reported as complete), not just the source-file order of the step functions, which would have been misleading.
Repeated care across serving_steps.rs and diagnose_steps.rs to avoid the "loose contains()" failure mode this repo's own guidance calls out — e.g. assert_new_server_avoids_taken_address requires both a nonzero exit code and specific conflict wording rather than either alone, and groups_in_command/proposed_group_command parse structured values instead of substring-matching a whole line.

Deployment notes

None (test-only change; no product code, no migration, no new runtime dependency).

What this covered

DEGRADED REVIEW. Fan-out to independent reviewer workers (the review-code skill's required mode for a change this size) was attempted but could not run: every Agent spawn failed with a global concurrent-subagent-capacity error across multiple retries (other sessions on this host were saturating the shared worker pool), and the tool's own response said not to retry further. Per the skill's fallback rule, this is reported as a single-context, degraded pass rather than presented as a full fan-out review. Recommend re-running with fan-out available for independent coverage of the passes a single context is more likely to miss (cross-file root-cause consolidation, the design-review pass run as genuinely independent workers).

Despite the degraded mode, every one of the 14 changed files was read in full at the diff level (9875940ebc7d1dbf9db0706e01d29842e8ec1d27...dc5638b75aed153258db8106d032fce988206d33) with wide surrounding context, including: README.md, expectations.toml, src/expectation.rs, src/mock_server.rs, tests/e2e.rs, tests/feature_naming.rs, tests/e2e/{chat,diagnose,examine,serving}_steps.rs, and features/{chat,diagnose,examine,model_serving}.feature. Cross-checked: expectations.toml rows against the PR's own table (ticket ids, scopes, and the three dropped-since-fixed guard rows all verified present/absent as claimed); scenario numbering for gaps/duplicates across all four touched feature files (none found); the cli_other_output migration in examine_steps.rs (complete — no stale reads left pointed at cli_stderr for data that moved); the serve-38 assertion ordering against the actual Gherkin (not the Rust source order); AGENTS.md for applicable rules (test-coverage, subprocess/kill-on-drop — satisfied via OwnedProcess's own Drop, though this harness uses std::process, not tokio::process, so the letter of the kill_on_drop rule doesn't directly apply — and the symbol-not-line-number citation convention, which the new code and README text both follow).

Did not run: a fan-out multi-pass review (see above); cargo test/cargo clippy locally (not re-derived, since CI already reports every required check green at this exact head commit — clippy, E2E tests, build-and-test, Windows lanes, coverage, commit-signatures, etc. — 25/25 success or skipped, 0 failing); the GPU/self-hosted lanes that exercise several of the new scenarios (diagnose-50..52, serve-37/39/40, serve-38) are not required per-PR checks by this repo's own CI design (documented in the touched README) and were not independently re-run — the PR's own test-plan section reports a manual local run of the lifecycle set.

History/prior-changes passes: git log on the two most heavily-touched files (expectation.rs, feature_naming.rs) showed an active, conventional commit history; nothing in it contradicted what this diff does. This review did not have access to this PR's own prior review-thread content (by design — per the review method, past review bodies are excluded from a blind pass). The one unavoidable exposure was the get_reviews metadata call required for the mandatory duplicate-review claim-check: only login/state/commit_id/submitted_at fields were read for that check, never any review body text, and none of it fed into the findings above.

Ticket EAI-8024 was retrieved by its identifier (resolves to the org's internal key for the same ticket) for the Step 9 intent check only, after findings were settled; its content is summarized on the intent line above and was not used to derive any finding.

🤖 Generated with Claude Code

…rding Renumber this PR's serve scenarios to 40..44 after main's serve-36..39. diagnose-50/51 now confirm through `examine --json` that the CLI sees the substituted device group, their comments say the premise is substituted, the port-conflict check no longer accepts "unavailable", and the device-policy doc comment is back on its function. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

juhovainio

Approving. No blocking findings; a few non-blocking items for follow-up.

CI is green on the current head. The leak scan is clean. The shim-based scenarios prove their premise via examine --json before relying on it, and serve-42 asserts the child is gone before teardown.

Non-blocking:

Unmeasured xfail rows. The EAI-8016, EAI-8020 and EAI-8021 rows are headed "MEASURE" with no run cited and are scoped when = {}. serve-43/44 are @merge-queue, so a lane that does not reproduce becomes an XPASS that fails the merge queue. Please cite a measured run per row or scope the rows.
Untested parsing helpers. The helpers in diagnose_steps.rs, serving_steps.rs and examine_steps.rs have no unit tests, and under an xfail row a parser break reads as the expected failure.
Guards not shown red. examine-31/32, serve-40 and diagnose-50 have not been revert-and-observe checked; the PR body says so.
PR body omissions. It does not mention ServiceRecordOptions.service_id or three of the new expectation.rs tests.
Scope. The teardown env change (affects every scenario), the cli_other_output migration and the generic guard tests could be split out.
EAI-8007 coverage. It has no blocking-lane scenario, because serve-41 is @requires-gpu @merge-queue. EAI-8010, 8017 and 8018 are dropped, with reasons given in the body.

juhovainio · 2026-11-03T14:51:25Z

+  # @merge-queue because it serves a real model and the Strix Halo lanes have
+  # little headroom left on main alone, so three more real serves follow
+  # serve-06/07/08 onto the merge-queue path where the budget for them lives.
+  @id:serve-services-stop-reports-what-it-stopped @requires-gpu @merge-queue


The expectations comment says EAI-8007 is covered by a plain-child fixture on the no-GPU lane, but this scenario is @requires-gpu @merge-queue and nothing else exercises the services stop count, so no blocking lane holds this contract. Consider a mock-lane scenario built on start_managed_service_process.

Corrected in d180cba: the expectations note now says the plain-child check was a one-off during development and that no blocking lane holds EAI-8007. A mock-lane scenario on start_managed_service_process is a good idea. I'd rather add it in a follow-up than grow this PR again.

juhovainio · 2026-11-03T14:51:25Z

+/// equal to another empty list. Both callers then pass while measuring nothing.
+/// The callers additionally reject an empty result, so a form not handled here
+/// fails loudly instead.
+fn groups_in_command(command: &str) -> Vec<String> {


groups_in_command (and labelled, section_for, proposed_group_command) feed xfail scenarios, so a parser regression is indistinguishable from the expected product failure. Pure unit tests for these helpers would close that gap (the module tests in expectation.rs show the pattern).

Agreed that's a gap. The PR description already states it. These helpers live in the harness = false e2e test binary, so unit-testing them means moving them into the lib crate. I'd do that as a follow-up rather than in this PR.

…symbol Replace the MEASURE markers on the EAI-8016, EAI-8020 and EAI-8021 rows with the lanes and runs that measured them, say that no blocking lane holds EAI-8007, and drop a source path the examine split may have moved. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

… EAI-7998 row Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi requested a review from a team as a code owner August 12, 2026 19:09

fredespi force-pushed the e2e-pin-the-contracts-for-the-13-readme branch 3 times, most recently from d94221b to 9fc2de0 Compare August 13, 2026 07:18

fredespi added 7 commits August 13, 2026 14:47

fredespi force-pushed the e2e-pin-the-contracts-for-the-13-readme branch from 9fc2de0 to 39afe38 Compare August 13, 2026 12:49

juhovainio previously requested changes Aug 13, 2026

View reviewed changes

Comment thread tests/e2e-cucumber/tests/e2e/serving_steps.rs Resolved

Comment thread tests/e2e-cucumber/tests/e2e/diagnose_steps.rs Resolved

fredespi requested a review from juhovainio August 13, 2026 14:56

fredespi force-pushed the e2e-pin-the-contracts-for-the-13-readme branch from d2c7ac6 to fedc6b2 Compare August 18, 2026 21:03

volen-silo mentioned this pull request Aug 24, 2026

fix(serve): pick a free port instead of colliding on the default #301

Closed

1 task done

rominf requested changes Sep 2, 2026

View reviewed changes

siloteemu previously requested changes Sep 9, 2026

View reviewed changes

fredespi added 5 commits September 9, 2026 22:12

style(e2e): rustfmt the device-group selection

5f27fd3

Missed by the --no-verify commit; caught by the prek lint job. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi added 2 commits October 30, 2026 12:31

fredespi force-pushed the e2e-pin-the-contracts-for-the-13-readme branch from 54892b5 to 090539f Compare October 30, 2026 11:32

fredespi requested a review from tomastola October 30, 2026 11:32

fredespi requested a review from a team October 30, 2026 12:06

test(e2e): use from_mins for the stop-check deadline (clippy)

4002c3e

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

nowycondro reviewed Oct 30, 2026

View reviewed changes

jussielo-amd mentioned this pull request Nov 2, 2026

ROCMAI-763: split the e2e-report HTML and markdown renderers out of consolidated/mod.rs #833

Merged

juhovainio requested changes Nov 2, 2026

View reviewed changes

fredespi requested a review from juhovainio November 2, 2026 15:23

juhovainio requested changes Nov 2, 2026

View reviewed changes

fredespi requested a review from juhovainio November 2, 2026 16:40

Merge origin/main; renumber this PR's serve scenarios to 33..37

26d16dc

Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

juhovainio requested changes Nov 3, 2026

View reviewed changes

Merge origin/main; renumber this PR's serve scenarios to 36..40

de40a17

Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi requested a review from juhovainio November 3, 2026 11:39

juhovainio requested changes Nov 3, 2026

View reviewed changes

nowycondro reviewed Nov 3, 2026

View reviewed changes

fredespi requested a review from juhovainio November 3, 2026 13:53

juhovainio approved these changes Nov 3, 2026

View reviewed changes

jussielo-amd mentioned this pull request Nov 4, 2026

ROCMAI-762: split rocm-core fix.rs, automation.rs, net.rs and openmpi.rs by domain #886

Merged

fredespi and others added 2 commits November 5, 2026 09:44

Merge origin/main; renumber serve scenarios to 42..46, drop the stale…

97f1082

… EAI-7998 row Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

style: end expectations.toml with a single newline

0703a1a

Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

Conversation

fredespi commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Expected-failure rows

Guards, no row

Not covered

Harness changes (test crate only)

Test plan

Uh oh!

juhovainio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fredespi commented Aug 20, 2026

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

nowycondro left a comment

Choose a reason for hiding this comment

Review — needs work

Blocking

Non-blocking

Decisions for the author

Positive signals

Deployment notes

What this covered

Uh oh!

juhovainio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juhovainio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredespi Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juhovainio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fredespi commented Aug 12, 2026 •

edited

Loading

fredespi Nov 2, 2026 •

edited

Loading