[jussielo-amd]

Summary

serve-01 (@id:serve-short-name-expansion) and serve-02 (@id:serve-short-name-consistent-across-engines) fail on every self-hosted lane and sit under all-hosts expected-fail rows for EAI-7219 (alias resolution). Their failure message said "no 'resolved model' in output" and nothing else, because the steps ran rocm serve qwen2.5 --engine ... and kept stdout only: stderr and the exit code were thrown away. This change keeps them and puts the engine, the exit code and the first stderr line in the failure message, so a run that stopped on a refusal reads as a refusal. It does not change which scenarios run, the rows' scope, or any CLI behaviour.

What the lane logs show

In the Strix Halo Ubuntu run of #866 the lemonade half of serve-02 printed resolved model: Qwen/Qwen2.5-1.5B-Instruct, so the alias is expanded there. The vLLM run's stdout was an empty string, in serve-01 and in serve-02 alike. rocm serve prints serve plan and the resolved model: line only after a chain of checks has passed: the OS gate that rules an engine out (vLLM on native Windows), the no-usable-GPU bail, the Docker-aware engine gate, the "no active ROCm runtime is configured" bail, and the engine's model-resolution call, in the order serve_cmd.rs runs them. A run that stops at any of them has an empty stdout and its reason on stderr. A vLLM run that reached the plan but left the alias unexpanded would have printed resolved model: qwen2.5, and the scenario would then fail on "expected a fully qualified model name", which is not what those logs show.

Those logs had no stderr, so they could not say where the run stopped. The first lane run with this change (run 41811714290, Strix Halo Ubuntu release and nightly, and MI300X) does: on all three, vLLM stopped with no active ROCm runtime is configured; run rocm runtimes list and rocm runtimes activate <runtime_key>, or pass --runtime-id/--env-id, before any alias handling. So on those lanes the two rows are red because the scenarios run with no active runtime, not because of the alias bug. On native Windows the stop is the up-front refusal of vLLM, a designed refusal and also not the alias bug.

Change

• tests/e2e-cucumber/src/serve_plan.rs (new): ServeRun holds one run's engine, stdout, stderr and exit code. resolved_model moves here from the steps file, and missing_plan_message says whether the run stopped before it printed a plan (exit code and first stderr line) or exited 0 without one, and renders both streams through the shared section helper so an empty one is marked (empty). It lives in the library target so its logic gets unit tests; the cucumber binary has harness = false, so plain #[test] functions there never run.
• serving_steps.rs: the two When steps build ServeRuns (new serve_runs field on the world), and the two Then steps use them. The Then steps still fail loudly on a missing line rather than comparing two empty strings.
• expectations.toml: the comment and the two reason strings no longer say "serve output does not surface the expanded name", which the lemonade output contradicts. The reasons say a vLLM serve of a short name either stops before printing a plan or prints the alias unexpanded, so they stay true whichever the lane shows, and the comment records what the first lane run with stderr showed. The rows keep their all-hosts scope; narrowing them is the follow-up below.

Tests

Five unit tests in serve_plan.rs: a printed plan names its model; a refusal is reported with its exit code and first stderr line; an exit 0 without a plan is not called a refusal; a silent failure says stderr was empty and marks the empty stdout; blank and indented stderr lines are skipped and trimmed. To check they protect the behaviour, I made the exit code always read as 0 and two of them failed (the refusal and the silent-failure test), made the first-line selection take the first line unfiltered and the refusal test failed, and removed the trim and the blank-line test failed; I restored each. cargo test -p e2e-cucumber --lib (197 passed), cargo clippy -p e2e-cucumber --all-targets -- -D warnings and cargo fmt --check are clean.

I also ran serve-01 and serve-02 through the cucumber harness on a GPU-less WSL2 host. Both fail as before, and the message now reads `rocm serve --engine vllm` stopped before it printed a serve plan (exit 1): Error: no usable AMD GPU detected; ... and the same for lemonade in serve-02. That run used a -n name filter, and its reconciliation line reported the two as unexpected failures although the world dump showed expect_xfail: true. I did not run an unmodified checkout to compare; the reconciliation code and the rows' scope are not touched by this change, and docs/testing.md warns that -n replaces the harness's own filtering.

No Gherkin scenario: this changes only what the test steps report when they fail; the existing serve-01 and serve-02 are the scenarios, and they run on the self-hosted Strix Halo lanes.

Lane results

On the first head, serve-01 and serve-02 failed as expected on the Linux GPU lanes, with the message above. The only unexpected failure on those lanes was @id:serve-stop-leaves-no-background-helper, which this change does not touch: the lemonade backend downloads on the runner failed with HTTP 404 (llama.cpp release asset) and 403 (the ROCm tarball), so the engine never came up.

Follow-up

Give serve-01 and serve-02 a managed runtime, or gate them on the engine (@requires-engine:vllm, as serve-05 does), then narrow the two EAI-7219 rows to the real bug.

[Copilot]

🟢 Approval recommended

No active review issues remain for the short-name serving diagnostic improvements.

No open issues 🎉

What changed in this PR

Improves failure diagnostics for the short-name serving scenarios so early command failures can be distinguished from missing model-name expansion. CLI behavior, scenario selection, and expected-failure scope remain unchanged.

Changes:

• Preserve each serve run’s engine, stdout, stderr, and exit code for assertion failures.
• Move plan parsing and diagnostic formatting into a library module with four unit tests.
• Revise expected-failure explanations to describe the observed missing vLLM plan without claiming a confirmed cause.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​serving_steps.rs	Capture complete serve results and use richer failure messages.
tests/​e2e-cucumber/​tests/​e2e.rs	Add and initialize scenario state for serve results.
tests/​e2e-cucumber/​src/​serve_plan.rs	Add result storage, plan parsing, diagnostics, and unit tests.
tests/​e2e-cucumber/​src/​lib.rs	Export the new serve_plan module.
tests/​e2e-cucumber/​expectations.toml	Clarify comments and failure reasons while retaining all-hosts scope.

_{Review effort: Balanced}

[juhovainio]

Looks good. Moving the stderr/exit-code reporting into the lib target means its unit tests actually run (the cucumber binary has harness = false), and the Then steps still fail loudly when the expected line is missing. No blocking issues from a read of the diff; CI on the GPU lanes was still running when I looked, so that is not covered by this approval.

A few non-blocking notes:

• The xfail reason in expectations.toml says vLLM "prints no serve plan". ROCMAI-351 reports vLLM printing resolved model: qwen2.5 (alias left unexpanded), and the comment above admits the cause isn't known yet. Worth wording the reason so it stays true if the next lane run shows the alias bug rather than a refusal (inline comment).
• The mutation check described in the PR covers the exit-code verdict. The first-line-of-stderr selection isn't mutation-checked, though the refusal test does exercise skipping a blank leading line.
• The rows keep all-hosts scope even though lemonade passes on the Strix lanes. The reasoning (need stderr evidence from the next lane run) makes sense; please make sure there's a follow-up to narrow the scope once that evidence is in.

[juhovainio]

ROCMAI-351 (EAI-7219) reports vLLM printing resolved model: qwen2.5 (alias unexpanded), whereas this reason asserts it "prints no serve plan". Since the cause is still unknown, consider wording the reason to cover both so it doesn't become a false claim if the next lane run shows the alias bug.

[jussielo-amd]

Reworded. Both reason strings now say a vLLM serve of a short name either stops before printing a serve plan or prints the alias unexpanded, so they stay true whichever shape a lane shows, and the comment above them says EAI-7219 as reported is the unexpanded shape, which the lane logs have not shown. The first lane run with stderr (run 41811714290: Strix Halo Ubuntu release and nightly, and MI300X) shows vLLM stopping at no active ROCm runtime is configured, before any alias handling, so on those lanes the rows are red for a missing runtime and not for the alias bug; the comment records that as well (e97223b and 0a3fcd6).

On your other notes: the first-line-of-stderr selection is now mutation-checked, since taking the first line unfiltered turns the refusal test red, and I added a test for blank and indented stderr lines that fails when the trim is removed. The follow-up to narrow the rows is written into the PR body: give serve-01 and serve-02 a managed runtime or gate them on the engine, then narrow the two rows to the real bug.

[nowycondro]

🤖 Automated review (Claude Code) · c2d3af2

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.
Does not claim to close either ticket it discusses: EAI-7219 (= Jira ROCMAI-351, "vLLM engine does not resolve model aliases to full HuggingFace IDs," still In Progress) and ROCMAI-823 (vLLM native-Windows refusal, still In Progress) — both retrieved independently and consistent with the PR's own narrative. This change only improves test diagnostics for two existing xfail scenarios; it explicitly does not narrow their expected-failure scope or touch any CLI behavior, and the diff confirms that scope (only tests/e2e-cucumber/** files changed).

Blocking

• CI is red on the reviewed commit, from a scenario this diff does not touch — GitHub check runs E2E tests (Strix Halo, Ubuntu, release, current) (job 126512476385) and E2E tests (Strix Halo, Ubuntu, nightly, current) (job 126512476087), both on commit c2d3af2.
  Both job logs end with the harness's own reconciliation verdict: Reconciliation: 7 xfail (failed as expected), 0 XPASS (0 flaky, 0 stale), 1 unexpected failure(s). / FAIL: 'serve-stop-leaves-no-background-helper' was expected to pass on this host but FAILED — a regression. That scenario is unrelated to serve-01/serve-02 or any file this PR touches (no changes to serve-stop or background-helper handling). It looks like a pre-existing issue on this self-hosted lane rather than something this diff caused, but it is a failing status check on the exact commit under review, so it blocks a clean CI signal for this PR regardless of root cause. Worth confirming (e.g. by checking if this scenario is already flaky on main) before merge.
  Confidence 100 · logic · Fix: not something to fix in this diff — confirm the failure's cause on main/this lane and either re-run or file it separately if it's pre-existing flakiness.

Non-blocking

• New module doc comment lists the refusal-chain order backwards relative to the code it describes — tests/e2e-cucumber/src/serve_plan.rs:9-11, vs. apps/rocm/src/serve_cmd.rs:1261-1267.
  The new doc comment says the chain is "the platform and engine gate, the no-usable-GPU bail, the 'no active ROCm runtime' bail...". In serve_cmd.rs, the no-usable-GPU bail (bail!("{}", no_usable_gpu_refusal(&device_policy));, line 1262) actually runs before the platform/engine gate (ensure_engine_allowed_on_platform(&selected_engine, ...)?;, line 1267) — confirmed by serve_cmd.rs's own nearby comment, which describes "every refusal that needs no daemon (the AIM token and GPU pre-flight, the no-usable-GPU bail)" running ahead of the gate. The "no active ROCm runtime" bail and the model-resolution call are correctly ordered relative to each other. This is purely explanatory prose in a new file (it doesn't drive any assertion), so it doesn't affect correctness of the tests, but it will mislead a future reader trying to use this comment to reason about which refusal fired first.
  Confidence 85 · mechanical · Fix: swap the first two items in the doc comment's list to match serve_cmd.rs's actual order.

Decisions for the author

• Keeping both expectations.toml rows at all-hosts scope rather than narrowing them now — tradeoff.
  The lane logs already in hand show lemonade does print a resolved-model line on Strix Halo while vLLM's stdout is empty — one could argue that's enough to narrow the rows to "vLLM only." The PR's own reasoning (recorded in Jira ROCMAI-351's 2026-10-26 comment, independently retrieved) is that an empty vLLM stdout could still be caused by several different gates (no-GPU bail, missing runtime, the ROCMAI-823 Windows refusal) rather than conclusively the alias bug, so narrowing now could misattribute the row. Both choices are defensible; the PR explains its choice and the Jira history supports that the "why is stdout empty" question is still genuinely open. Confirm this is still the intended tradeoff rather than an oversight.
• engine: "vllm".to_owned() in user_serves_short_name duplicates the "--engine", "vllm" string literal passed to run_rocm — non-blocking-improvement (tests/e2e-cucumber/tests/e2e/serving_steps.rs, user_serves_short_name).
  If one is edited without the other, ServeRun.engine would silently disagree with the command actually run, and the diagnostic message would name the wrong engine. The loop-based user_serves_multiple_engines avoids this by reusing the same engine loop variable for both. Deriving the literal the same way here would remove a small drift risk; the current code is not wrong, just slightly more fragile than its sibling.

Positive signals

• missing_plan_message keeps the original no 'resolved model' in output: prefix from the code it replaces while appending the new engine/exit-code/stderr detail, so anything already grepping that exact string (tooling or habit) keeps working.
• The four new unit tests are genuine regression tests, not restated assertions: tracing a "treat rc as always 0" mutation through the match arms in missing_plan_message shows exactly the two tests the author says failed under that mutation (a_refusal_is_reported_as_a_refusal_with_its_stderr_line, a_silent_failure_says_stderr_was_empty) would indeed fail, and the other two would still pass — matching the PR's own description of the mutation check.
• Placing the new parsing/diagnostic logic in the library target (src/serve_plan.rs) rather than the harness = false cucumber binary is the right call for unit-testability, and matches the project's own serve_log.rs precedent and the e2e-cucumber README's documented module layout.

Deployment notes

None — test-only change; no CLI behavior, schema, or dependency changes.

What this covered

Every file in the diff, read in full, plus surrounding context: tests/e2e-cucumber/src/serve_log.rs (the structurally similar precedent module), the rest of serving_steps.rs (checked for other readers of cli_output/cli_outputs on these two scenarios — none found), apps/rocm/src/serve_cmd.rs (to verify the new module doc comment's ordering claim), the root AGENTS.md and tests/e2e-cucumber/README.md in full (agent-instruction adherence — no violations found), and the project's CI: all 32 check runs plus status on the head commit, with the two failing job logs read in full. Reviewed against 1a76a205a456…c2d3af26 (base…head); one fan-out worker separately confirmed the same 5-file diff against the true immediate parent commit, so the file scope is doubly confirmed.

Both ticket keys the PR discusses (EAI-7219/ROCMAI-351, ROCMAI-823) were retrieved directly from Jira and read in full (not just the description's framing of them).

Did not run: cargo test/clippy/fmt myself — relied on the PR's own description of a local run plus the green clippy and Test (affected crates)/build-and-test CI checks on this commit, all of which passed. Did not execute the e2e cucumber binary. The history/prior-review fan-out pass hit tool errors with search_pull_requests/search_issues in its environment and fell back to list_pull_requests plus direct commit/file history, so its citations of prior PRs (#69, #280, #182, #142) are moderate- rather than high-confidence precedent and were not used as standalone findings here, only as context.

Not reconciled against this PR's own review discussion: a prior bot review and a human reviewer's approval (with several non-blocking notes of their own, including one that independently touches the same xfail-wording tradeoff noted above) exist on this commit but were read only to check for a prior automated review at this exact SHA, never as input to this review's findings, per this method's blind-review rule.

🤖 Generated with Claude Code

[jussielo-amd]

Thanks for the review. Taking the points in turn.

CI: the two red Strix Halo Ubuntu jobs, and the MI300X one, each end with a single unexpected failure, serve-stop-leaves-no-background-helper, which this diff does not touch. In those logs the lemonade backend downloads failed on the runner (HTTP 404 for the llama.cpp release asset, HTTP 403 for the ROCm tarball), so the engine never came up. The other scenarios that need a served model failed the same way earlier in the run and are expected-fail rows. I have not checked whether it reproduces on main, and the lanes re-run on the new head.

Doc comment order: you were right that the order was off, but there are two platform gates rather than one. ensure_engine_allowed_on_os (vLLM on native Windows) runs before the no-usable-GPU bail, and the Docker-aware ensure_engine_allowed_on_platform runs after it. The module doc now lists the OS gate, the GPU bail, the Docker-aware gate, the runtime bail and model resolution in that order (e97223b).

Engine literal: user_serves_short_name now takes the engine from one variable for both the command and the ServeRun, as the loop in user_serves_multiple_engines does.

Scope of the rows: keeping the all-hosts scope is still deliberate, and the PR body now records the follow-up to narrow them. The first lane run with stderr shows vLLM stopping at no active ROCm runtime is configured on all three Linux GPU lanes, so the narrowing has real evidence behind it now, but it needs the scenarios to get a managed runtime or be gated first, which is a separate change.