test(e2e): pin the contracts a README walkthrough expects (EAI-8024)

[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.

[fredespi]

All blocking findings from @siloteemu's review are addressed, and @rominf's points are covered by the same work. CI is green on de41663b across CI, CodeQL and E2E self-hosted, including the required E2E tests lane.

Blocking

Stale rows / branch behind main. Merged current main in and re-measured against a binary built from the merged tree rather than reading the diff. Four rows were stale, not three: both EAI-8011 help rows, EAI-8019, and EAI-8015. Each row is deleted and its scenario kept, so it now guards the fix. On EAI-8019 you and @rominf reached the same conclusion by different routes — the flag became a ValueEnum with cpu_only marked hide = true, so it is no longer advertised and the scenario's premise dissolves; the helper was also fixed to read both clap shapes, which is what your clap 4.6 reproduction confirmed.

run_rocm_without_env dead code. Deleted. git grep over the merged tree returns nothing.

model_serving.feature comment contradicting the TOML. Amended. The comment now says the guard is unconditional except on the lemonade-default Linux GPU lane, where EAI-7423 pre-empts it, and points at the row's own note.

Non-blocking

• Verbatim sentence matching in assert_no_policy_refused — kept, with the reasoning written down. A name-plus-nonzero-rc rule would call every run on the no-GPU lane a rejection, because the ordinary host-level refusal names the policy too. Clap's own invalid value wording is now recognised as a third form.
• Duplicate Scenario: 11 in examine.feature — fixed by main's examine-NN renumbering, absorbed in the merge.
• Which lanes gate — now stated in the PR description.
• Weak device-group premise — the step now prefers a group a device node plausibly belongs to (kvm, input, disk, …) and only falls back to /etc/group order on an unusual host.
• Nothing lints for an orphan expectations.toml row — added every_expectation_row_names_a_scenario_that_exists, which fails the run on a row whose id matches no scenario.
• A harness guard failing the scenario rather than the run — not changed. It is a real gap and I think it wants a harness-level fix rather than one wired into a single step; the immediate instance is fixed and the guard's message now says plainly that it is a read failure, not a verdict.

Two things this PR gained that were not in the original review

Two rows added for EAI-7960: the dashboard clears generation throughput on a failing scrape instead of holding it for the validity window. Sampling on CI put the held value 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 reads a stale frame. Both rows are flaky = true and must be deleted from the ticket, not by the lane.

Suite concurrency is now derived from available_parallelism() clamped to 2..=6 and forced to 1 on GPU or shared-cache lanes. The previous 64-way default was starving scenarios into their own timeouts; a 4-way run was both green and faster than an 8-way one (33 s vs 54 s) on a 10-core host.

Ready for re-review.

[siloteemu]

🔴 Automated review · pr-review-watcher · a8803fe

Summary

Test-only PR adding e2e cucumber scenarios (chat, diagnose, examine, model-serving, update) that pin CLI contracts a README walkthrough expects, plus the xfail rows for the ones the product does not yet honour — Approve. Verified: read the cited production source directly and confirmed every load-bearing claim the new comments make — --dry-run really is declared #[arg(long, requires = "apply")] (apps/rocm/src/main.rs:308), select_runtime_update_source really bails on an empty manifest list (main.rs:15939), runtime_update_plan really calls resolve_latest_for_manifest (therock.rs:952/982) so a populated registry needs network, and " managed runtimes: none" is the literal the product emits (therock.rs:801); the feature-naming enforcement test was run and its 4 checks pass (the one permitted cheap check). CI for this head: 15 success, 1 skipped, 2 pending, no failures. Blocking: 0 · Non-blocking: 5.

Prior objection (a guard that could not fail for the defect its comment named, because the scenario runs against a machine with nothing installed): FIXED. a8803fe removes the overclaim: the step is rescoped to the readback path (re-runs rocm update, asserts rc 0 and the empty-registry line), and the comment now states plainly which contract it does not prove and why (it needs a managed runtime, which drags in the network the mock lane lacks). That residual assertion can fail — an update that stopped answering or changed its empty-registry wording after a preview would break it — so it is no longer vacuous, merely narrower. The companion fix in OwnedProcess::is_running (tests/e2e.rs:136) is also sound: the Err arm previously answered "not running", i.e. "the product did its job", for a wait the harness could not perform, which failed open on exactly the defect serve-21 exists to catch; it now panics, and the justification checks out — std::process::Child memoises the reaped status so a repeated try_wait returns Ok(Some(_)) rather than erroring, EINTR is retried inside std, the sole caller is serving_steps.rs:1136 (a Then step, not a Drop path), and that step already guards against an empty owned_processes vacuous pass.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• tests/e2e-cucumber/features/update.feature:30 — And the machine still manages no runtimes reads, in the feature file, as exactly the non-mutation guard the step's Rust comment says it cannot be; the disclaimer is invisible to anyone reading only the Gherkin. This is the specific misreading the last round tripped on, and a competent reader will trip on it again — one Gherkin comment line above the step ("this pins the readback path, not non-mutation; see the step comment") would close it for good.
• tests/e2e-cucumber/expectations.toml:255-257 — the rationale still warns that the row XPASSes where the runner's user is already in the conventional groups, but given_usable_device (diagnose_steps.rs:897) now shims id as well as stat, making the row deterministic; the stale prose invites a future reader to add a spurious flaky = true.
• tests/e2e-cucumber/README.md:94-101 — the new E2E_MAX_CONCURRENT override (tests/e2e.rs:1437) is absent from the env-var table, so the documented bisecting workflow is undiscoverable; @merge-queue, used by three new scenarios, is likewise missing from the tag table.
• tests/e2e-cucumber/features/chat.feature:96 — the comment credits "Scenario 7" with naming the model, but chat-07 auto-detects; chat-08 is the one passing --model.
• tests/e2e-cucumber/tests/e2e/serving_steps.rs:1448-1451 — the "not available" disjunct is the engine's message for when amd-smi cannot count devices, which should not co-occur with the scenario's own @requires-gpu premise; the "out of range" branch is what actually fires, so either drop the disjunct or reframe the comment as a defensive fallback. (Related: engines_steps.rs:111-127's new polling loop, unlike the existing wait_for_screen, has no early-exit/reader-panic check, so a crashed shell burns the full 30 s before a generic failure.)

All three blocking findings are fixed at de41663, so withdrawing this change request. Verified rather than taken on trust: the branch is now level with main and four stale rows were deleted with their scenarios kept as real guards (you found one more than I did); the dead helper is gone repo-wide; and the feature comment now matches the expectations file including the lane scope. The new concurrency work also checks out — deriving serialization from the hazards rather than the GPU probe alone, and placing the force-to-1 outside the override so neither the override nor the clamp floor can raise a serialized lane, is the correct ordering. Non-blocking notes are in the review comment; the one worth acting on is that the two new flaky dashboard rows gate nothing on the required lane, so the measured evidence for a non-flaky row should live on the ticket rather than only in a code comment.

[fredespi]

Thanks — and thank you for re-verifying the three prior findings against the head rather than taking the response at face value. Two of the five non-blocking points were actionable and are now fixed in fac85e06; the other three I'm answering rather than changing, with reasons.

Fixed

OwnedProcess::is_running() failing open (e2e.rs:136-148). You were right, and I think the exposure is slightly worse than "small": the sole consumer asserts the product stopped the process, so answering false on an error answers "the product did its job" — a wait the harness could not perform reads as a pass for the exact defect serve-21 exists to catch. Rather than narrowing to ECHILD I removed the arm: Child caches the exit status once it has reaped, so a repeated call returns Ok(Some(_)) rather than erroring, which means the "already reaped by an earlier call" case the comment described cannot actually reach that arm. There is no benign case left to absorb, so it now panics with the pid and the error.

update-02 non-discriminating once its row is deleted (update_steps.rs). Agreed, and this is the more valuable of the two because it is a future vacuity — it would have started failing to catch anything on the day the bug was fixed, which is the hardest kind to notice. Added And the preview has not installed anything, which reads back through rocm update and asserts managed runtimes: none. Keyed on the line update-01 already pins, and host-invariant because the scenario runs against an isolated empty ROCM_CLI_DATA_DIR — nothing is managed before the preview, so any runtime named after it was installed by the preview. It cannot affect the row today: the usage error fails the preceding step first, which I confirmed in the run below.

Answered, not changed

The two flaky EAI-7960 rows gate nothing. Correct, and I agree a non-flaky row is the right end state once the observation is deterministic. Your point that a flaky row cannot advertise its own removal is the reason it should not live only in a code comment — it is now in the PR description as well as the ticket.

Rows scoped os = "windows" / therock_family = "gfx*" have no stale-XPASS signal on a gating lane. True, and it is a property of which lanes are required rather than of these rows; it applies equally to the rows already on main. Worth a separate issue against the lane policy rather than something this PR can fix.

serialize failing open if has_amd_gpu misdetects AND neither shared-dir var is set. Accepted as a real if narrow hole. I'd rather not add a third signal speculatively — on the GPU lanes CI sets the shared-dir vars, so the disjunction has a second independent source of truth there.

Verification

Linux container, on the pushed tree: cargo clippy -p e2e-cucumber --test e2e --all-features -- -D warnings clean, cargo test -p e2e-cucumber --lib 114 passed, and the full mock suite reconciles to 6 xfail, 0 stale XPASS, 2 flaky XPASS (the EAI-7960 pair, tolerated). update-02 still fails on the usage error with --apply demanded, confirming the new step is unreachable while the row stands.

Two mock-lane failures in that run — artifact-prefetch-failed-marker-leaves-no-temp and engine-shell-marks-the-prompt — are container-environment gaps, not regressions; both are green on CI at this head.

[siloteemu]

🔴 Automated review · pr-review-watcher · fac85e0

Summary

Test-only PR pinning the README-walkthrough contracts as cucumber scenarios and expectation rows. This round adds two remediation commits on the baseline I already looked at. Fix 1 holds up under adversarial checking — the sole caller is confirmed, there is no Drop path into it, EINTR is retried inside std and the status is cached after reap, so no benign error arm remains; cucumber's per-step catch_unwind plus intact Drop cleanup make panicking the right call here. The engine-shell poll change is sound and its assertion unchanged. The three items you answered without changing are all reasonably answered. Fix 2 is correctly isolated and dormant, but it is not discriminating for the defect its comment names — details below. Blocking: 1 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

• tests/e2e-cucumber/tests/e2e/update_steps.rs:100-126 (step) with tests/e2e-cucumber/features/update.feature:30 — the new guard cannot fail for the defect its comment claims to catch. The comment says it would catch "a binary that accepted the preview and then performed the update". On this scenario's fixture that outcome is unreachable regardless of whether the product is correct: rocm update --apply routes through apply_runtime_update → select_runtime_update_source (apps/rocm/src/main.rs:15824, 15939-15942), which bails with "no managed runtimes are registered" on an empty registry — update upgrades an existing managed runtime, it never installs a first one. update-02 has no Given, never calls use_shared_runtimes(), so it always runs against an empty data/runtimes. "managed runtimes: none" is therefore guaranteed by the fixture, not by the product's dry-run honesty. And if the fix being anticipated simply drops requires = "apply" from dry_run (apps/rocm/src/main.rs:308-309), --dry-run alone falls into the report-only else branch (main.rs:1806-1889) and cannot install either.

  So round 1's finding — a guard that stops discriminating once the bug it pins is fixed — is answered here with a second guard that never discriminated at all, and a durable comment asserting otherwise will mislead the next maintainer. Either:

  • (a) keep the assertion, but replace the comment's claim with what it actually guards (the readback path stays clean on an empty machine) and note that the real preview-does-not-mutate contract needs a managed-runtime fixture; or
  • (b) move the guard onto a scenario that opts into a managed runtime, where a performed update is observable.

  (a) is comment-only and closes the misleading claim at zero risk. I would take that and leave (b) for whenever a managed-runtime fixture exists.

Non-blocking

• tests/e2e-cucumber/tests/e2e.rs:145-152 — fix 1's comment explains why false was wrong but not why panic! is safe. One clause — never reached from Drop, and cucumber catches a step panic per scenario — would stop the next reader re-litigating abort-during-unwind. That was my first concern too, and it took a while to discharge.
• tests/e2e-cucumber/expectations.toml:365-380 — the dash-08/09 note prescribes sampling as the durable fix without recording that sampling was tried this round, measured the hold vanishing 0.8 s / 1.3 s into a 6 s window, and was reverted in 88f46e3 as out of scope. A reader of the file alone loses that evidence; one sentence pointing at the finding would carry it.
• tests/e2e-cucumber/tests/e2e.rs:1426-1427 — accepted. It now applies the 1 after the E2E_MAX_CONCURRENT override, so the override can no longer raise a serialized lane. Fail-open would need a GPU lane whose probe reads false and neither shared dir exported; the documented exclusion of shared_uv_cache_dir is consistent, since uv is content-addressed and self-locking.
• tests/e2e-cucumber/tests/e2e/engines_steps.rs:111-127 — the hand-rolled deadline loop matches four sibling loops in this tree, so it is convention rather than a novel pattern. A wait_for_screen_where(predicate, timeout) on TuiSession would DRY all five if the pattern grows again.

Verification notes: one cheap targeted check was run (cargo fmt --check -p e2e-cucumber, clean); per policy the full and e2e suites were not re-run locally, since CI covers hardware this environment does not have. CI at this head was 23 success, 1 skipped, 1 pending, 0 failures at fetch time — still in flight, so not yet evidence of success, and no outcome is attributed to any named job.

On the round-1 item you pushed back on: your reading was right and stronger than mine. A helper reporting "not running" when the check itself errored means a failure-to-observe reads as "the product did its job", for exactly the defect the scenario exists to catch. Removing the arm rather than narrowing it is the correct call.

[siloteemu]

🔴 Automated review · pr-review-watcher · fac85e0

Summary

Test-only PR pinning the README-walkthrough contracts as cucumber scenarios and expectation rows. This round adds two remediation commits on the baseline I already looked at. Fix 1 holds up under adversarial checking — the sole caller is confirmed, there is no Drop path into it, EINTR is retried inside std and the status is cached after reap, so no benign error arm remains; cucumber's per-step catch_unwind plus intact Drop cleanup make panicking the right call here. The engine-shell poll change is sound and its assertion unchanged. The three items you answered without changing are all reasonably answered. Fix 2 is correctly isolated and dormant, but it is not discriminating for the defect its comment names — details below. Blocking: 1 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

• tests/e2e-cucumber/tests/e2e/update_steps.rs:100-126 (step) with tests/e2e-cucumber/features/update.feature:30 — the new guard cannot fail for the defect its comment claims to catch. The comment says it would catch "a binary that accepted the preview and then performed the update". On this scenario's fixture that outcome is unreachable regardless of whether the product is correct: rocm update --apply routes through apply_runtime_update → select_runtime_update_source (apps/rocm/src/main.rs:15824, 15939-15942), which bails with "no managed runtimes are registered" on an empty registry — update upgrades an existing managed runtime, it never installs a first one. update-02 has no Given, never calls use_shared_runtimes(), so it always runs against an empty data/runtimes. "managed runtimes: none" is therefore guaranteed by the fixture, not by the product's dry-run honesty. And if the fix being anticipated simply drops requires = "apply" from dry_run (apps/rocm/src/main.rs:308-309), --dry-run alone falls into the report-only else branch (main.rs:1806-1889) and cannot install either.

  So round 1's finding — a guard that stops discriminating once the bug it pins is fixed — is answered here with a second guard that never discriminated at all, and a durable comment asserting otherwise will mislead the next maintainer. Either:

  • (a) keep the assertion, but replace the comment's claim with what it actually guards (the readback path stays clean on an empty machine) and note that the real preview-does-not-mutate contract needs a managed-runtime fixture; or
  • (b) move the guard onto a scenario that opts into a managed runtime, where a performed update is observable.

  (a) is comment-only and closes the misleading claim at zero risk. I would take that and leave (b) for whenever a managed-runtime fixture exists.

Non-blocking

• tests/e2e-cucumber/tests/e2e.rs:145-152 — fix 1's comment explains why false was wrong but not why panic! is safe. One clause — never reached from Drop, and cucumber catches a step panic per scenario — would stop the next reader re-litigating abort-during-unwind. That was my first concern too, and it took a while to discharge.
• tests/e2e-cucumber/expectations.toml:365-380 — the dash-08/09 note prescribes sampling as the durable fix without recording that sampling was tried this round, measured the hold vanishing 0.8 s / 1.3 s into a 6 s window, and was reverted in 88f46e3 as out of scope. A reader of the file alone loses that evidence; one sentence pointing at the finding would carry it.
• tests/e2e-cucumber/tests/e2e.rs:1426-1427 — accepted. It now applies the 1 after the E2E_MAX_CONCURRENT override, so the override can no longer raise a serialized lane. Fail-open would need a GPU lane whose probe reads false and neither shared dir exported; the documented exclusion of shared_uv_cache_dir is consistent, since uv is content-addressed and self-locking.
• tests/e2e-cucumber/tests/e2e/engines_steps.rs:111-127 — the hand-rolled deadline loop matches four sibling loops in this tree, so it is convention rather than a novel pattern. A wait_for_screen_where(predicate, timeout) on TuiSession would DRY all five if the pattern grows again.

Verification notes: one cheap targeted check was run (cargo fmt --check -p e2e-cucumber, clean); per policy the full and e2e suites were not re-run locally, since CI covers hardware this environment does not have. CI at this head was 23 success, 1 skipped, 1 pending, 0 failures at fetch time — still in flight, so not yet evidence of success, and no outcome is attributed to any named job.

On the round-1 item you pushed back on: your reading was right and stronger than mine. A helper reporting "not running" when the check itself errored means a failure-to-observe reads as "the product did its job", for exactly the defect the scenario exists to catch. Removing the arm rather than narrowing it is the correct call.

Duplicate — this review was submitted twice by our tooling. The identical review below it is the live one; nothing here is withdrawn.

[fredespi]

Blocking finding accepted — and it's the right catch. I verified it rather than taking it on trust: select_runtime_update_source bails with "no managed runtimes are registered" on an empty registry (apps/rocm/src/main.rs:15939-15942), so update upgrades a managed runtime and never installs a first one. On update-02's fixture "manages none" is guaranteed by the fixture, not by the product's honesty. Answering a non-discriminating guard with another one, and shipping a comment that claims otherwise, is worse than the original problem. Fixed in a8803fe9.

I took (a), with one addition you didn't ask for: the Gherkin line overclaimed too. And the preview has not installed anything reads, to anyone skimming the feature file, as exactly the contract you showed is unprovable here. It is now And the machine still manages no runtimes, which is literally what the step asserts. The comment states what it does hold — the readback path stays clean, update still answers and reports the same machine update-01 pins — and says plainly that this is not the non-mutation contract.

On why (b) is not taken, with a sharper reason than "no fixture exists": a managed runtime would make the scenario network-dependent. With a manifest present, render_update_report calls runtime_update_plan → resolve_latest_for_manifest per runtime (apps/rocm/src/therock.rs:810, :957), which reaches the TheRock index. The empty-registry branch at therock.rs:800 is precisely why this scenario needs no network on the mock lane. That is recorded in the comment and tracked on EAI-8010, so whoever adds the fixture knows the cost up front.

Non-blocking

• panic! safety not explained (e2e.rs:145-152) — added. The clause names both discharges: no Drop path reaches it (the serve-21 step is the only caller), and cucumber catches a step panic per scenario, so the failure is reported and Drop still runs its kill/wait.
• Sampling evidence missing from the dash note — respectfully, it is already there: expectations.toml:360-364 records that sampling was tried this round, that it measured the value gone 0.80 s and 1.27 s into a window meant to be at least 6 s, and that it was reverted as out of scope. The one thing genuinely absent was the commit, so the note now cites 88f46e3 and points at the reverted diff as the starting point.
• wait_for_screen_where to DRY five deadline loops — agreed as a direction, declined for this PR. You note it is convention rather than a novel pattern; adding a TuiSession API in a test-only change that touches one of the five would leave the other four unconverted.
• Concurrency ordering — noted, and thanks for the shared_uv_cache_dir reasoning; content-addressed and self-locking is exactly why it is excluded.

Verification

Linux container on the pushed tree: cargo fmt --check -p e2e-cucumber clean, cargo clippy -p e2e-cucumber --test e2e --all-features -- -D warnings clean, cargo test -p e2e-cucumber --lib 114 passed, and the full mock suite reconciles to 6 xfail, 0 stale XPASS, 2 flaky XPASS (the EAI-7960 pair) — unchanged from before the rename, confirming the renamed step still binds. update-02 still fails first on the usage error, so the readback step remains unreachable while the row stands.

Two failures in that run, artifact-prefetch-failed-marker-leaves-no-temp and engine-shell-marks-the-prompt, are container-environment gaps that also fail on clean main here; both are green on CI.

One process note: this review arrived twice, as 5165544083 and 5165558250 with identical bodies. Addressed once.

Withdrawing this change request: the objection is resolved at a8803fe. The step that could not fail for the defect its comment named has been rescoped to the readback path it can actually pin, and the comment now states plainly which contract it does not prove and why. The companion is_running fix is sound as well. Full round posted as a comment. You were right to push back on the wording; the underlying objection held and you addressed it directly.