[r0x0r]

Important

Whoever squash-merges this: write the squash body by hand, from the final
diff — do not inherit the per-commit trail. This repo's squash setting is
COMMIT_MESSAGES, and about 16 of this branch's commits narrate work that is
no longer in the diff: the reuse pre-gate and its serve-26..30 scenarios
(moved to #703), the --gpu validation / auto-select work (already on main
via #251), and 956891f's claim that the fix-16 checker is "gated to Linux"
(reversed by 13bd32c). 1de61359's message also contains one sentence that
was false when written. The final diff is: the serve-summary OOM note, the
crates/rocm-core/src/vllm_oom.rs reader, the shell-quoting guard's move into
rocm-core, the e2e-oom-fault-injection feature and the release-feature
guard, serve-28, and the docs. History is not being rewritten for this
(it would need a force-push to a shared branch).

Note

Merged with main on 2026-11-02 and re-verified. main's Phase-6x/7x
modularization moved most of what this branch touches, so several conflict
hunks had an empty origin/main side; each was re-applied into the module
that now owns the code (see the merge commit message for the file-by-file
account). main took serve-26/serve-27 for the AIM lanes, so this
branch's OOM-launch scenario is now serve-28 — the feature file, the
step file, capability.rs and docs/testing.md all moved with it.
The sections below that still describe serve-29 and the reuse pre-gate
describe work that now lives in #703, not in this branch.

Summary

OOM guidance on a failed rocm serve (ROCMAI-419, ex EAI-8059). When a
managed vLLM launch fails to become ready and its own log carries a real
allocator OOM signature, the interactive deployment summary names the memory
knobs (--gpu-memory-utilization, --gpu <index>), says plainly that lowering
the reservation does not help a model that simply does not fit, and routes the
user's actual failing line into rocm diagnose --symptom '<line>'.

Note

The serve-behaviour half of this PR has been split out to #703.
Reusing an already-running managed service is now exempt from the no-GPU
refusal — a user-visible change outside ROCMAI-419's "diagnostics only" scope.
Both a human reviewer and pr-review-watcher asked for it to ship on its own
under AGENTS.md section 11, and 7bc22302 removes it from this branch. The
mechanics are unchanged; only the PR they live in moved. What this branch
loses with it is serve-25 (Reuse succeeds on a GPU-less host and the summary
carries no memory guidance), and it loses no coverage doing so: that
scenario's memory assertion was already vacuous, because the real reuse path
reports log_path: None and append_oom_serve_note is withheld on that ground
before the already_running clause is reached. The clause itself stays pinned
here by the unit test
append_oom_serve_note_ignores_an_already_running_services_log, which feeds it
an OOM-bearing log path directly.

Detection is deliberately narrow: each tail line is classified with the same
rule as the rocm diagnose vLLM-OOM checker (scored as vllm: <line> against
the shared match threshold), so a bare out of memory from a kernel OOM-killer
line, a dependency's log, or vLLM's generic EngineCore wrapper never gets
reported as memory exhaustion. One shared helper,
rocm_core::vllm_oom_diagnose_symptom, is both the OOM gate and the line
selector for the engine hint and the CLI note, so neither surface can apply a
rule the other does not. That is a shared rule, not a shared answer: the two
read the shared log at different points, and two measured consequences of that
are stated under Correction and Known gap below. An earlier version of this
paragraph claimed the two "cannot drift"; that claim was false and is withdrawn.

quotable_in_single_quotes moves out of engines/vllm/src/process.rs into the
existing crates/rocm-core/src/terminal.rs, so both surfaces share one copy of
the rule that decides whether the user's line can be put in a shell-quoted
argument. (strip_terminal_control_sequences and rendered_lines already lived
there on main; an earlier version of this paragraph described all three as
moving into a new file, which was wrong on both counts.)

Base is main. #251 merged on 2026-10-06 and is no longer a dependency.

Behavior coverage

User-observable behavior	Scenario (@id:)	Lane
A managed launch that OOMs and never becomes ready names the memory knobs	serve-oom-launch-memory-guidance (serve-28)	GitHub-hosted mock lane, gated @requires-oom-fault-injection

The four reuse-exemption scenarios this table used to list moved to
#703 with the exemption. serve-28 is this PR's only new scenario; it
sits behind main's own serve-25 (The recommended model list prints
ASCII-only text, from #683, merged after this branch forked).

@id:diagnose-vllm-oom-is-conditional and @id:serve-vllm-low-vram-oom-guidance
(serve-23) came from #251, now merged; this PR does not modify them.

The positive scenario cannot run a real launch on a GPU-less host, so the
mock-lane binary compiles in a test-only fault-injection hook (feature
e2e-oom-fault-injection, armed per child process by
ROCM_E2E_SIMULATE_OOM_LAUNCH) that fabricates exactly the failed-launch state.
The hook is absent from shipped binaries (release.yml never passes the
feature), so the scenario reports a skip on the self-hosted
(prebuilt-release) lanes rather than a silent green.

That hook proves rendering, not the launch path: it returns before the spawn,
the record claim and the readiness wait, so serve-28 shows that a failed
launch report renders the note, not that a real failed launch produces one.

Stated gap: the vLLM engine's own startup-failure text has no scenario

AGENTS.md section 3 asks that a change to command output be covered by a Gherkin
scenario, and that a gap be stated in the PR text with the lane named. Stating it:
no scenario covers the vLLM engine's startup-failure message, and none of
today's lanes would run one.

The text in question is what a user sees through rocm services logs <service-id>, --verbose or --foreground when a managed vLLM launch dies.
This PR changes it four ways (startup_log_context / oom_utilization_hint in
engines/vllm/src/process.rs):

• the header now reads Startup log, last 80 lines as the terminal drew them;
• the tail is echoed as rendered rows rather than raw bytes;
• the memory hint no longer fires for a bare HIP error: out of memory, a kernel
  OOM-killer line, or vLLM's generic EngineCore wrapper (the deliberate
  narrowing described above, which keeps the hint in step with rocm diagnose);
• the hint now fires on \r / ESC[2K repaint lines it previously missed.

Nothing under tests/e2e-cucumber/features/ reaches that code: grepping for
exited before becoming ready and Startup log finds no match, and serve-28's
log fixture is written by the CLI's own fault-injection hook, so it never calls
the engine.

The lane that would cover it is E2E tests (MI300X) in
e2e-selfhosted.yml — the only lane
whose GPU family makes vLLM the effective serve engine, and so the only one where
a real vLLM process can be started and made to fail. It does not cover it today,
and this PR does not add a scenario there, for two reasons worth stating rather
than burying: that lane runs the prebuilt release binary, which has the
fault-injection hook compiled out by design, so the failure would have to be a
genuine VRAM exhaustion; and provoking one reliably on a 192 GB MI300X means
either a deliberately oversized model download or a --gpu-memory-utilization
value tuned against whatever else is resident, which is GPU-expensive and racy
against the other scenarios sharing that card.

What does hold the changed text today: rocm-engine-vllm's unit tests
(the_echoed_tail_never_welds_two_rendered_rows_into_one_line,
the_hint_reads_the_raw_tail_and_displays_the_row_that_scored,
the_colourised_round_trip_quotes_the_row_the_terminal_drew,
oom_utilization_hint_stays_quiet_for_unrelated_failures) exercise
startup_log_context end to end over a real file. Per AGENTS.md section 3 that
proves the function, not the behaviour, so it is a gap and not a discharge.

Known gap: the note can go missing when the OOM line is early in the window

Reported by the automated review and reproduced here (PROBE engine_hinted=true cli=None). Both surfaces use the same tail budget
(rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES = 80), but not the same window.
The engine scans the last 80 raw lines and bails; the supervisor appends that
whole bail — a header, the echoed tail, the hint — to the same shared log; the
CLI then reads 80 lines of the now-longer file. Its window therefore always
reaches less far back into the original log than the engine's did, and when the
bail is itself longer than 80 rows it holds nothing the engine wrote directly.

Measured: a log with the OOM line followed by 77 further lines is diagnosed by
the engine and produces no serve note at all.

This is not fixed here, and the reason is that the obvious fix trades one defect
for another. Re-windowing the CLI to the 80 lines before the bail restores
agreement, but it brings the raw colourised line back into view, and the note
stops quoting the user's own rendered row — the regression
the_colourised_round_trip_quotes_the_row_the_terminal_drew exists to prevent.
The clean fix is structural: have the engine record its selected symptom as data
(in the service record or a sidecar) instead of having the CLI re-parse text the
engine already diagnosed and echoed. That is a larger change than this PR, and I
would rather raise it than guess at it — say the word if it should land here.

What changed instead: the gap is now pinned by
the_cli_window_can_miss_an_oom_the_engine_saw, the comment in
append_oom_serve_note that claimed the two surfaces "agree on what counts as
'the tail'" states the gap, and the README's "absence is not evidence" list names
this condition alongside the others.

Verification

Full gate green on Linux at the pushed head 7bc22302, all four lanes:
cargo test --workspace --all-targets --no-fail-fast (3805 run across 33 test
binaries; 0 real failures — the only red was
cli_progress::tests::animated_spinner_keeps_ticking_without_progress_calls, a
known timing flake under parallel load, which passes in isolation: 1 passed, exit
0), cargo clippy --workspace --all-targets -- -D warnings (exit 0), and the
separate cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings
(exit 0), which --all-targets skips because that harness is harness = false.
cargo fmt --all clean. cargo test -p e2e-cucumber --test feature_naming
(exit 0, 4 passed) pins the renumbering.

Merged main (f0d6ce3b) in 43032a37, and two silent semantic conflicts
only a real build caught — git took main's copy of each with no marker:

• engines/vllm/src/process.rs referenced MAX_TAIL_READ by bare name in its
  test module; main had dropped that import in favour of the fully-qualified
  path. Fixed in 860f906e.
• fix(output): replace non-ASCII punctuation with ASCII in CLI output text #683 landed a whole-tree ASCII-output contract after this branch forked, and
  the OOM note's em dash plus the one in rocm serve --help tripped
  every_literal_that_can_reach_a_redirectable_sink_is_ascii and
  every_help_text_in_the_command_tree_is_ascii. Fixed in 7bc22302.

Changes in the latest round (2026-10-15 review)

The round was COMMENTED — nothing gated. Every claim was checked against the
code before acting; all of them held.

• The gpu_preferred guard from 1de61359 is reverted (blocking, and the
  automation retracted its own earlier advice that produced it). parse_device_policy
  (main.rs) maps auto, gpu, gpu_required and gpu_preferred onto
  DevicePolicy::GpuRequired and rejects cpu_only with an error, so
  GpuRequired is the only policy serve() can hand
  managed_service_reuse_refusal — its only caller that supplies one. The
  !matches!(.., None | Some(CpuOnly)) predicate was therefore identical at
  runtime to the GpuRequired check it replaced, and three artefacts
  described a bypass that cannot happen: the production comment, the test
  comment, and 1de61359's own commit message (see the banner above).

  Reverted rather than kept as defence in depth. The broader match guards
  nothing a caller can produce, it contradicts README.md's "that is the only
  policy pair refused" — and the README governs, since GpuRequired is the only
  value the CLI can pass — and this repo does not carry branches for states that
  cannot occur. The term is back on Some(GpuRequired) with the exhaustiveness
  argument written out and pointed at
  parse_device_policy_defaults_to_gpu_required_without_cpu_fallback. The two
  unreachable-input assertions go with it, one of which was a verbatim duplicate
  of the CpuOnly case already below it (a separate non-blocking finding,
  resolved by the same revert). Falsified: dropping the device-policy branch
  makes reuse_is_refused_on_every_term_the_spawn_and_the_gpu_bail_depend_on
  fail (1 failed), restored green (1 passed).

• serve-29 closes the untested seam at serve()'s pre-gate call site
  (blocking). The claim was reproduced exactly: restoring the old
  !cpu_only && any_live_managed_service_for_model(..) call site leaves the
  whole rocm bin suite green — 1170 passed, 0 failed. The unit test pins
  reuse_pregate_opens but re-derives the host predicate in a local closure, and
  serve-25/27/28 all run on GPU-less hosts where the gate opens either way.

  A unit test cannot close this: the host GPU probe is real hardware with no
  injection point, so the hand-off is only observable end-to-end. serve-29
  (@id:serve-live-reuse-candidate-does-not-preempt-gpu-validation,
  @requires-gpu @requires-os:linux) plants a live vLLM record matching on
  liveness, engine and canonical model id — so every pre-gate term except
  host_has_no_usable_gpu is satisfied and that predicate alone holds the gate
  shut — then serves the same model with --gpu 99. It borrows serve-27's
  setup-venv fixture, and deliberately passes no --env-id, so
  recover_setup_runtime_registration is reachable: with the gate wrongly open
  the manifest is re-filed before the refusal.

  Falsified on a real GPU host (platform=mi300x os=linux gpu=true): green as
  written (1 scenario, 6 steps passed); with the call site mutated back, red at
  exactly the discriminating step — no runtime registry manifest was written
  (1 scenario failed, 5 of 6 steps passed, recover_setup_runtime_registration
  re-filed therock-release-gfx942-1.0.0.json). Restored, green again.

• docs/testing.md was stale (blocking). The quoted serve-25 name predated its
  retitle in 1de61359 — corrected, in the passage that exists to warn that -n
  matches the scenario name. "The pre-gate has three conditions" omitted
  host_has_no_usable_gpu and cpu_only; it now documents all five, splits them
  into the match terms and the admission terms, and records serve-29 with its
  lane.

Non-blocking items taken:

• reuse_pregate_opens's doc said the gate opens "exactly when the bail is in
  play". The bail also requires !scripted_backend_failure and
  !e2e_simulate_oom_launch(), so in test-hook builds the gate can open where
  the bail cannot fire. Qualified, with why that is deliberate and why it costs
  nothing in a shipped binary.
• README.md listed "a progress-bar repaint" among the cases where the engine hint
  and the serve note print different commands. Confirmed overstated against
  every_rendered_line_break_keeps_the_gate_in_step_with_the_scorer: the log is
  split on \r, so a bare-CR repaint yields a clean segment both surfaces quote
  identically. Narrowed to repaints carrying an escape (ESC[1A, ESC[2K).
• crates/rocm-core/src/terminal.rs claimed the rejected line "stays visible in
  the human-readable sentence beside the command (and in the log tail printed
  with it)". True of the vLLM engine hint, which renders the row and prints the
  tail above it; oom_memory_note is one sentence plus the command and shows
  neither. Split per surface, pointing at rocm services logs.
• docs/manual-testing.md's colour-codes bullet cited the README as plain text;
  now a relative link, as AGENTS.md §5 asks.
• tui_driver.rs named ROCM_E2E_SIMULATE_OOM_LAUNCH as a user of
  spawn_with_env. Confirmed stale: the switch goes onto world.command_env,
  which the shared spawn path already applies, and the only spawn_with_env
  caller is the fix-prompt step in diagnose_steps.rs.

Non-blocking items declined:

• Also requiring !matches!(gpu_selection, GpuSelection::Index(_)) in
  reuse_pregate_opens. The reasoning is sound — on a GPU-less host every
  --gpu <index> is out of range, so the gate can still install Lemonade and
  write the registry before refusing. But it is a behaviour change on a path no
  finding in this round calls wrong, it needs its own scenario to be worth
  having, and this PR is already carrying one out-of-ticket behaviour change
  (see Open question). Follow-up, not this round.
• Workflow comments vs xtask features (e2e-selfhosted.yml, nightly.yml).
  Still true, still unrelated to this PR's hunks; left in Known deferrals.

Changes in the latest round (2026-10-14 review)

Three gating findings, all confirmed by reading before any change.

• The reuse pre-gate no longer does engine and runtime work before --gpu is
  validated. This was a real ordering regression this PR introduced: the gate
  was !cpu_only && any_live_managed_service_for_model(..), but its only
  product, reuse_existing, is a term of a bail that can fire only when
  visible_gpu_indices == Some([]). So on any host with an overlapping live
  service name — GPU or no GPU — validate_engine_selection_runtime (which can
  write the runtime registry), ensure_self_managed_engine_ready (which can
  install Lemonade) and ResolveModel all ran ahead of
  validate_pinned_gpu_index, and a ? from any of them pre-empted the
  GPU-specific refusal. The comments at the --gpu validation and at the
  runtime resolution that claimed otherwise were false, and are corrected.

  The bail's host predicate is now computed once and handed to both through
  reuse_pregate_opens, which takes the record probe as a closure — on a
  host with a GPU the managed-service read, and the load_managed_services
  liveness refresh it entails, is not paid at all.
  the_reuse_pregate_stays_shut_on_a_host_that_has_a_usable_gpu was proved to
  fail first: with the predicate dropped from the gate the test panics (exit
  101), and it asserts both halves — the gate is shut, and the probe is never
  consulted.

• --help no longer promises that nothing is prepared for a live server this
  run cannot reuse. A same-model service that managed_service_reuse_refusal
  rejects (recipe mismatch, or --require-api-key against a keyless server)
  still passes any_live_managed_service_for_model, so preparation has already
  run by the time the refusal is known. The README was already correct; the help
  was the copy that drifted, which made the previous round's claim that all three
  surfaces had been corrected untrue. It is true now.

• docs/manual-testing.md §5a no longer tells testers a colourised line falls
  back to the canonical symptom. That is the engine's behaviour; the note
  quotes the row the terminal drew. §5a now splits the apostrophe case from the
  colour case and names
  the_colourised_round_trip_quotes_the_row_the_terminal_drew. This was the
  fourth surface of a claim a previous round fixed on three; the tree was grepped
  for the claim's meaning (a multiline search for colour/control-byte wording
  near "canonical"/"falls back"), and README, docs/vllm.md and
  rocm-core/src/lib.rs already state it correctly — there is no fifth.

Two of the four non-gating Blocking items were taken as well:

• The reuse refusal's device-policy term now covers every policy that reaches
  the GPU bail, not just gpu_required. serve() keys cpu_only off
  matches!(policy, CpuOnly), so a --device gpu_preferred serve goes through
  the bail and could have bought its bypass from a cpu_only service. The unit
  test now discriminates gpu_preferred and cpu_only separately. (The e2e
  record writer still plants device_policy: null, so the term remains without
  e2e coverage — that half is not taken; the state it guards needs a cpu_only
  record the CLI never writes.)
• serve-25 is retitled after what it proves. The reviewer is right that the
  OOM-absence assertion cannot fail on that path — the reuse path reports
  log_path: None, so the note is withheld on that ground alone — and the
  comment now says so. What only serve-25 proves is the exemption itself.
• the_utilization_hint_states_the_bound_the_parser_accepts now names
  gpu_memory_utilization_rejects_out_of_range_and_unparsable_values rather than
  the function, as AGENTS.md §3 asks.

Left, with reasons: the duplicate tail reader (main.rs's read_tail_lines
uses BufReader::lines() and errors on invalid UTF-8, so the note silently
disappears) is real, but it is shared by six call sites including rocm services logs, so swapping it for rocm_core::tail_lines is a wider change than a
gating-fix round should carry — raised rather than guessed at. The serve-25
already_running call-site argument is still held by nothing; covering it
means a new seam, which is the thing the previous round deliberately removed.

Changes in the previous round (automated review follow-up)

• The classifier now keeps step with the diagnose scorer across the whole
  line-break set. The log was split on \n/\r, but the scorer re-splits with
  rendered_lines, which also breaks on every non-SGR CSI (ESC[2K, ESC[1A),
  ESC E/ESC D, VT, FF, U+0085 and U+2028/U+2029. A real progress-bar
  repaint, ...90%\r\x1b[2Ktorch.OutOfMemoryError: ..., therefore left the
  vllm: anchor on an empty row, scored 0, and silently dropped both the
  engine hint and the serve note. Scoring is now done per rendered row while the
  raw segment is still what gets returned, so the quoting guard keeps
  rejecting colourised lines and the canonical fallback keeps firing. A segment
  that renders as more than one row must contain a control byte, and
  quotable_in_single_quotes rejects every one of those — so within one read
  it cannot reach the printed command as a stripped lookalike of the user's
  error. See the correction below for the bound on that, which is narrower than
  this paragraph originally claimed.

• The note now quotes the user's own failing line in real use. The supervisor
  and the engine share one log. When vLLM OOMs the engine bails with its
  startup-log context — which embeds rocm-cli's own OOM hint — and the CLI
  writes that error back into the same file, after the real line. That hint
  scores as an OOM and contains ', so the newest-first scan always selected it
  and the note fell back to the canonical symptom in every real run: the
  headline "we quote your failing line" promise never actually held in the field.
  Lines rocm-cli authored are now skipped, keyed off shared constants so the hint
  and the filter cannot drift. The e2e fixture now writes the engine's full error
  block, so serve-28 can see this shape.

• The exact contract is now stated, because the stronger one is false.
  vllm_oom_diagnose_symptom does not guarantee every returned line is
  diagnosable, and cannot without reintroducing the control-byte hole. What holds
  is: a line that passes quotable_in_single_quotes is diagnosable, and one that
  does not is replaced by the canonical symptom, which is. That is exactly what
  the callers gate on. Found by the new break-set test falsifying the old claim.

• Dead code removed. vllm_log_shows_oom was a pub fn with no production
  caller; its threshold assertions moved onto the gate that actually ships. The
  two serve_summary.rs tests that only re-tested it now go through
  oom_memory_note, so they can fail when that module changes.

• Pre-gate test gap closed. Two of the pre-gate's three conditions were
  deletable with the suite green. Added a case where only record.model_ref
  matches and one with a non-live record; both mutants are now killed.

• Doc/comment corrections. --help, README and docs/manual-testing.md no
  longer promise that nothing is prepared before the no-GPU refusal.
  docs/vllm.md states the narrowed hint trigger. The serve_cmd.rs comment
  claiming the pre-gate "reads the managed-service records and nothing else" is
  qualified against the liveness refresh it actually does.

Correction: the raw-split guarantee is per-read, not end-to-end

The automated round at 40e7b5c6 was right that for a colourised OOM line
the engine and the CLI print different rocm diagnose --symptom arguments, and
that the code and this description both claimed otherwise. Measured:

engine, read 1 : --symptom 'vllm: torch.OutOfMemoryError: HIP out of memory'   (canonical fallback)
CLI,    read 2 : vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.

The cause is the engine's own bail. It echoes the tail as rendered rows — it has
to, that text goes to a terminal — and the supervisor appends the whole bail to
the same shared log. By the time the CLI reads, the echo is the only copy of
the failing line left in the 80-line window: on a 200-line log the raw line sits
84 lines from the end, and the echo header has scrolled out as well.

Suppressing the echo to make the two agree is not available — measured, it
leaves no OOM line in the window at all and the serve note disappears for every
real managed launch. So the guarantee is stated for what it is, a property of a
single read, and the real two-read sequence is pinned by
the_colourised_round_trip_quotes_the_row_the_terminal_drew.

The difference is benign: the CLI's argument is a row the terminal really drew,
carries no quotes or control bytes, and quotable still implies diagnosable.

Scenario numbering. An earlier round of this PR had taken serve-24 — the
landed --require-api-key reuse refusal — for a new scenario and renumbered the
landed one, leaving two references on main pointing at the wrong scenario. The
landed scenario now keeps serve-24 and the new ones take 25-28 below it, so
both the landed id and the file's positional numbering hold.

Split out of this PR

The reuse exemption now ships as #703, off current main. Nothing about
its mechanics changed in the move; the scenarios are renumbered against main
(serve-28 through serve-29 there) and the OOM-flavoured dressing on the reuse
scenario — the planted OOM log, the vacuous "does not blame this invocation for
GPU memory" assertion — stayed here as OOM work rather than travelling with it.

Review history below that discusses the pre-gate, reuse_pregate_opens,
managed_service_reuse_refusal and the former serve-25/27/28/29 is kept for the
record, but that code is no longer in this branch. The
[!IMPORTANT] banner at the top still applies: 1de61359 is still in this
branch's history and this repo builds the squash body from commit messages.

Known deferrals

Non-blocking review items not addressed here, listed so they are not lost:
reuse is still decided without the launch lock; the note can in principle fire on
a still-loading server; nightly.yml's publishing builds are not covered by the
release-feature guard; and some feature-file comments still name drifted scenario
numbers. See the review reply comment for the full list and rationale.

[volen-silo]

A few observations, mostly around the signature list and the stacking:

• vllm_log_shows_oom treats engine core initialization failed as an OOM signature, but vLLM emits that as the terminal wrapper for any EngineCore startup crash (unsupported arch, shm size, TP misconfig, missing weights). Being the last line, it reliably lands in the 80-line tail — so unrelated failures get reported as "the serve attempt ran out of GPU memory".
• The e2e scenario plants exactly that string as its OOM log, and a unit test asserts it must produce a hint — so the only end-to-end coverage never exercises a real OOM message.
• This branch removes the DRM sysfs VRAM fallback, the --gpu auto count derivation, their tests and the docs bullet — all added by its base feat(vllm): Tackle out of memory errors (EAI-8058) #251, which is still open. Intentional? auto_select_gpu_indices' doc still refers to that fallback.
• collect_serve_notes and append_oom_serve_note each emit the full hint verbatim, so a low-VRAM serve that then OOMs prints it twice.
• Setting log_path/manifest_path on the already-running branch makes the OOM note reachable when no launch happened — re-running serve against a starting service attributes that process's OOM to the new invocation.
• feat(diagnose): add the vLLM out-of-memory failure mode to the catalog (EAI-8060) #290 edits the same oom_utilization_hint and still calls log_tail_shows_oom, which this PR deletes.

[r0x0r]

Thanks for the detailed review — addressed all of these in b9316f3 (rebased onto the current gpu-out-of-memory tip):

• Generic "engine core initialization failed" signature: removed it from vllm_log_shows_oom. It really is vLLM's terminal wrapper for any EngineCore startup crash, not just OOM, so treating it as an OOM signature was wrong. Detection now only fires on the allocator-level signatures (torch.OutOfMemoryError, hip out of memory, out of memory).
• E2E coverage only exercising the wrapper string: the e2e scenario now plants a real allocator OOM signature (torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.) instead of the generic wrapper phrase, and unit tests were updated the same way.
• DRM sysfs fallback / --gpu auto count derivation removed: that was purely a stale-base artifact — this branch was still on top of an older point in gpu-out-of-memory than your feat(vllm): Tackle out of memory errors (EAI-8058) #251 fix commit. Rebased onto the current tip (a97a39f) and restored the gpu_vram_usage/gpu_vram_usage_amd_smi/gpu_vram_usage_sysfs split that got flattened by the rebase's merge resolution — verified with cargo test/clippy that the dispatcher and DRM fallback tests are back and passing.
• Duplicate hint when a low-VRAM serve then OOMs: append_oom_serve_note now skips adding the note if the shared VLLM_GPU_MEMORY_UTILIZATION_HINT text is already present in the notes collected pre-launch, so it's never printed twice.
• log_path/manifest_path on the already-running branch misattributing OOM: append_oom_serve_note now also takes already_running and bails out immediately when true, so re-running serve against a live/starting service never blames that invocation for whatever the other process's log contains. Added a regression test for this (append_oom_serve_note_ignores_an_already_running_services_log) and repointed the e2e scenario at this exact case (serve-oom-memory-guidance), asserting the summary does not show the OOM note when reusing an already-running service.
• feat(diagnose): add the vLLM out-of-memory failure mode to the catalog (EAI-8060) #290 touching the same oom_utilization_hint/log_tail_shows_oom: noted for awareness — nothing in this repo currently defines log_tail_shows_oom (this PR's function is vllm_log_shows_oom), so there's no conflict today, but that PR will need to reconcile with whichever version of this lands first.

All changes covered by unit tests; cargo test -p rocm --bin rocm, cargo clippy -p rocm -p rocm-core -p rocm-engine-vllm --all-targets -- -D warnings, and cargo fmt --check all pass locally.

[volen-silo]

Round 2. Items 1, 4 and 5 are genuinely fixed — I checked the mechanisms, not just the claims. The dedup works (VLLM_GPU_MEMORY_UTILIZATION_HINT collapses to one line, notes holds raw text at the dedup point, render_summary does no wrapping, so contains really fires), and the already_running bail-out loses nothing (ManagedLaunchReport.already_running has exactly two producers, start_managed_service's final Ok is always false, and load_managed_services refreshes liveness before the check).

Blocking:

• The note fires on status == "running", which the codebase documents as healthy. Both append_oom_serve_note and oom_memory_note gate on status == "ready", but status_for_readiness produces three values and the comment four lines above it describes "running" as "the engine is up and loading normally" — deliberately distinguished from "starting" so rocmd doesn't kill a slow-loading model. So a serve whose endpoint is up and whose model is still loading, with out of memory anywhere in its last 80 log lines, is told "the serve attempt ran out of GPU memory." Both rustdocs claim they only fire for a serve that "failed to become ready" — the code contradicts its own contract. Gate on "starting" or an explicit failure predicate, not != "ready".

• The positive e2e case now has no scenario at all. @id:serve-oom-memory-guidance is the only scenario touching OOM guidance in the whole feature directory, and it now asserts the note is absent. Round 1's point was that the positive scenario planted a fake OOM string; repointing it at an unrelated negative case rather than repairing it leaves the PR's headline user-visible behavior unit-tested only. AGENTS.md §3: "User-observable behavior needs a scenario, not only a unit test... a unit test asserting the internal helper does NOT discharge this." Keep the negative, add a positive.

• Round-1 item 3 is only partly fixed. The production code is byte-identical to a97a39f0 — gpu_vram_usage/_amd_smi/_sysfs, effective_gpu_count, validate_pinned_gpu_index, auto_select_gpu_indices, and that last one's doc comment does match its code. But auto_selection_uses_vram_row_count_when_amd_smi_count_is_unknown is still deleted with no replacement — it's the only test removed anywhere in this PR, and effective_gpu_count still falls back to vram.map(<[GpuVramUsage]>::len), so it's a live path with no selection-level coverage for detected: None + populated vram. Separately, the new comment at main.rs:16882 ("No GPU count from amd-smi (unavailable, or genuinely zero devices)") contradicts the unchanged comment eight lines above it in the same function, which explains that vram may be populated precisely when amd-smi is absent. Reaching count == 0 needs both sources to fail. Revert the comment edit and restore the test.

• docs/vllm.md:123 says "two ways", the list below it has three items and is byte-identical to the base. Residue of the reverted-then-restored sysfs bullet — the bullet came back, the count word didn't.

• Dead signature entry. "hip out of memory" contains "out of memory", so .any() can never reach it. Cleanest fix that also un-narrows detection: ["outofmemory", "out of memory"]. The old log_tail_shows_oom matched bare "outofmemory"; replacing that with "torch.outofmemoryerror" means torch.cuda.OutOfMemoryError (the .cuda. breaks contiguity) and hipErrorOutOfMemory no longer match on their own. Usually the spaced phrase is also in the tail so impact is limited, but neither commit message mentions the narrowing.

• The spawn_managed_engine_child hunk is now inert. log_path/manifest_path on the already-running branch has zero reachable consumer: the three reads in the tree are append_oom_serve_note (bails on already_running) and print_managed_launch_plain twice (returns before reaching them). The danger round 1 flagged was fixed elsewhere; the now-purposeless hunk was left in.

• @requires-gpu puts the new scenario on a lane that gates nothing. ci.yml:675 skips those on the blocking mock job and the self-hosted GPU lane is continue-on-error. It isn't gratuitous as written — main.rs:4857 bails GPU-required serves before the already-running short-circuit — but that check is skipped for --device cpu_only, and existing_live_managed_service matches on (engine, canonical_model_id) only, so device policy is irrelevant to the reuse this scenario depends on. --device cpu_only + @requires-no-gpu (as scenario 11 already does) would put it on the blocking lane. The step's own comment flags the mismatch: "allocates no GPU memory despite running on the GPU lane."

On #290: the claim that nothing defines log_tail_shows_oom is wrong — git show origin/oom-diagnostics-catalog:engines/vllm/src/lib.rs still defines it at 1846, calls it at 1832 and asserts on it at 2252. The conclusion holds anyway: merge-tree is clean, #290 edits only the format string two lines below the if. The real coordination issue is that once both land there are two independent OOM detectors — rocm_core::vllm_log_shows_oom and #290's KEYWORDS_VLLM_OOM/check_16_vllm_oom table. Whichever merges second should fold the checker onto the shared helper; worth saying so in the body now.

Smaller things:

• vllm_log_shows_oom lives in rocm-core but has zero tests there — near-identical assertions now sit in serve_summary.rs:510 and engines/vllm/src/lib.rs:2227. cargo test -p rocm-core alone covers none of it.
• assert_no_oom_memory_guidance also asserts screen.contains("already running"), which the Gherkin doesn't state. Good guard against a vacuous negative, but it belongs in the feature file as its own And.
• read_optional_tail_lines runs before the already_hinted check — swap them.
• oom_memory_note is a policy function with no rendering, in serve_summary.rs, while its sibling collect_serve_notes is in main.rs, stitched together three lines apart at the call site.

Checked and clean: log lifecycle (File::create truncates per launch, service_id is timestamp-unique, so no stale OOM can be attributed), the e2e plumbing is real rather than vacuous, no orphaned steps, no temp-file collision.

Centralising the signature into rocm_core::vllm_log_shows_oom so the engine error and the CLI summary can't drift is the right call — it's what makes #290's duplicate detector visible as a problem.

[r0x0r]

Thanks for the Round-2 review — pushed 1adc674 addressing it.

Note could fire for a healthy running (still-loading) service. Fixed. Introduced serve_summary::serve_failed_to_become_ready(status) (status == "starting") and gated both append_oom_serve_note and oom_memory_note on it, instead of status != "ready". A healthy running service is no longer treated as a failed launch. New test: append_oom_serve_note_is_withheld_for_a_healthy_still_loading_service.

Dead signature / narrowing regression. vllm_log_shows_oom signatures are now ["outofmemory", "out of memory"]. The old "hip out of memory" was dead (any log with it already contains "out of memory"), and the space-free "outofmemory" matches the allocator error types whatever module path they carry (torch.OutOfMemoryError, torch.cuda.OutOfMemoryError, hipErrorOutOfMemory). Added rocm-core coverage: vllm_log_shows_oom_matches_allocator_signatures_but_not_generic_failures (also asserts the generic EngineCore wrapper is not matched).

Inert spawn_managed_engine_child hunk / deleted test / misleading comment. Reverted the already-running branch's log_path/manifest_path change back to None, reverted the select_auto_gpu_index comment churn, and restored auto_selection_uses_vram_row_count_when_amd_smi_count_is_unknown.

docs/vllm.md said "two ways" but the list has three. Fixed to "three ways".

The @requires-gpu reuse scenario gated nothing. Moved @id:serve-oom-memory-guidance onto the blocking no-GPU mock lane: @requires-no-gpu @requires-os:linux, with --device cpu_only so the GPU-required pre-flight is skipped and the already-running reuse short-circuit is reached. It now gates every PR and allocates no GPU memory. Also split the "already running" assertion into its own explicit Gherkin Then step.

Positive e2e scenario — not added, and here's why. The negative case (reuse must not blame this invocation) is the one the harness can drive without a GPU, because it reuses a pre-planted live service rather than launching. A positive end-to-end (this invocation launches, the launch OOMs, the note renders) needs the mock engine to emit a real allocator OOM signature in a log it owns (already_running == false, status == "starting"), which the mock can't do today. The positive behavior is currently covered by unit tests (append_oom_serve_note_reads_the_failed_serve_log_and_names_the_knobs and serve_summary::oom_note_*). If you'd like it in e2e, I'll add a mock hook to inject a failed-launch OOM log and verify it on the Linux lane as a follow-up — flagging the gap rather than shipping an unverifiable scenario.

Smaller items: added the missing rocm-core test for vllm_log_shows_oom; reordered append_oom_serve_note so the cheap already_hinted check runs before the log read; the "already running" assertion is now explicit in the feature file.

Coordination with #290: once both land there are two OOM detectors (vllm_log_shows_oom here and the diagnose catalog there) — happy to fold them onto a shared signature source in a follow-up if you'd prefer.

[r0x0r]

@volen-silo — correction on the scenario-15 lane, because my Round-2 reply was wrong and CI called it out.

--device cpu_only does not skip the GPU pre-flight — it's rejected before either branch is reached. parse_device_policy refuses cpu/cpu_only outright under the strict no-CPU-fallback policy ("rocm serve requires ROCm GPU execution; CPU mode is not a fallback path"), so serve() bails at argument parsing, before both the GPU-required pre-flight and the already-running reuse short-circuit. That's exactly why @id:serve-oom-memory-guidance went red on the no-GPU lane in 1adc674. Both my earlier claim and the "cpu_only skips the pre-flight" assumption were incorrect.

Proper fix (c0f39cf): reuse detection now runs before the GPU pre-flight, at the correct layer. Reusing an already-running managed service launches nothing and pins no GPU, so it should never be blocked by the GPU-required check. I moved model resolution + reuse detection ahead of that bail, gated by a cheap any_live_managed_service_for_engine pre-check so the common launch path still fails fast before any engine work. When a live managed service for the engine already exists and the model ref resolves to it (existing_live_managed_service match), the GPU bail is skipped; otherwise everything is byte-identical — including scenario 11's no-GPU/no-runtime refusal, which still fires because no live service exists.

Scenario 15 now runs on the blocking lane with the default device — @requires-no-gpu @requires-os:linux, no --device flag. It reaches the reuse short-circuit on the no-GPU mock host purely because the planted live service is reused, gates every PR, and allocates no GPU memory. Feature/step comments updated to match.

Follow-up 395ed9e is a fixup! (clippy map_unwrap_or → is_ok_and on the new helper); it'll autosquash into c0f39cf on the final rebase.

Note: the current red E2E tests (GPU) lane is unrelated — 9 scenarios fail identically with vLLM RuntimeError: Failed to infer device type (the runner's HIP device wasn't visible), spanning serve/chat/benchmark/lemonade cases this change doesn't touch; the prior commit's GPU lane was green. Re-running it.

Your other two Round-2 items (restore the deleted auto_selection_uses_vram_row_count_... test + revert the main.rs:16882 comment; positive-launch e2e scenario) are tracked separately and not part of this lane fix.

[volen-silo]

Round 3. I re-verified every Round-2 item against the code at 395ed9e, not against the claims. Genuinely fixed: the serve_failed_to_become_ready gate (both call sites), the restored auto_selection_uses_vram_row_count_... test, the reverted select_auto_gpu_index comment (now byte-identical to base), docs/vllm.md "three ways", the ["outofmemory", "out of memory"] signature pair, the reverted spawn_managed_engine_child hunk, and the lane move. cargo fmt --check, clippy --workspace --all-targets -D warnings and cargo test --workspace --all-targets are green locally at this head, and the blocking GitHub-hosted mock lane runs @id:serve-oom-memory-guidance and passes it. The two red checks (self-hosted GPU, readthedocs) fail identically on unrelated recently-merged PRs.

I also checked the new gate the other way, for over-narrowing: starting really is the only failure value that can reach append_oom_serve_note. start_managed_service always derives report.status from status_for_readiness, and its spawn-failure path returns Err before a report exists; the "failed"/"stopped" literals elsewhere belong to services restart/stop and never feed this. Gate is correct.

Blocking

The positive e2e scenario is still missing, and the infeasibility argument only covers half of it.

You are right about the post-failure path. append_oom_serve_note requires already_running == false; parse_device_policy rejects cpu_only outright; and the only other way past the GPU pre-flight is the reuse short-circuit, which forces already_running == true. There is no mock-lane route to that half — it needs a harness change, and I am not asking for one here.

The pre-launch half is coverable today, with no new harness capability:

• collect_serve_notes (apps/rocm/src/main.rs:5100) has no already_running gate — it runs on the exact reuse path scenario 15 already drives on the blocking no-GPU lane, and serve_gpu_low_memory_warning -> VLLM_GPU_MEMORY_UTILIZATION_HINT (main.rs:5190-5197) is reachable from there.
• The only missing ingredient is VRAM telemetry, and rocm_core::resolve_amd_smi_binary (crates/rocm-core/src/lib.rs:7301) checks $HOME/.rocm/runtimes/registry before PATH — and HOME is already isolated per scenario by the harness (tests/e2e-cucumber/tests/e2e.rs:259).
• So: plant a registry manifest plus a stub amd-smi emitting the {"gpu_data":[{"gpu":0,"mem_usage":{...}}]} shape parse_gpu_vram_usage accepts (main.rs:17128), and the hint renders into the same captured summary scenario 15 already asserts against. vram_capacity_is_meaningful (main.rs:17202) does not block it — it only suppresses on a single-GPU APU.

That is a fixture, not a new primitive. AGENTS.md §3 is explicit that a unit test does not discharge user-observable behavior, and its escape hatch — "state the gap in PR text and name the lane" — is also unmet: the explanation lives in a PR comment, while §2 treats bodies and comments as distinct surfaces. Minimum to unblock: put the post-failure gap and its lane in the PR body. Preferred: also add the pre-launch scenario, which then makes the body statement scoped to the one path that genuinely cannot run.

Nits (non-blocking)

• any_live_managed_service_for_engine (main.rs:14815) pre-gates on engine only, so when a live service exists for a different model of the same engine the probe runs and reuse_existing still ends up false — and the GPU fail-fast fires afterwards, weakening the invariant its own comment states ("BEFORE preparing or launching any engine"). Harmless for vLLM (ResolveModel is a pure in-process echo, and the "no usable AMD GPU" message is preserved), but for a self-managing engine ensure_self_managed_engine_ready can run a real install first when the pinned version is stale. ManagedServiceRecord already stores the raw model_ref, so the pre-gate could compare that and skip the probe entirely.
• Same block re-reads load_managed_services up to three times per serve (pre-gate, existing_live_managed_service, then again inside spawn_managed_engine_child), each with the 750 ms liveness probe budget.
• Widening to bare outofmemory reopens one plausible false positive: a torch/vLLM import-time crash whose traceback frame is from torch.cuda import OutOfMemoryError (missing libamdhip64.so, arch mismatch) lands in the tail with status == "starting", so it gets the memory-knob hint for a non-memory failure. Only a misleading hint, so not blocking — but worth a line in the doc comment.
• oom_memory_note is policy with no rendering, sitting in serve_summary.rs while its sibling collect_serve_notes is in main.rs; the two are stitched together three lines apart at the call site.
• The two-independent-OOM-detectors coordination point with the diagnose-catalog PR is still only in a comment thread; worth a line in the body so whichever lands second knows to fold onto the shared helper.

Everything else I checked came back clean: the shared-constant centralisation means the engine hint and the CLI note cannot drift, render_summary emits notes unwrapped so the dedup contains check really fires, the split Gherkin step is non-vacuous, and no doc or comment outside the diff still describes the old status != "ready" gating or the old signature list.

[r0x0r]

Thanks for the Round-3 review — and for re-verifying the Round-2 items against the code rather than the claims.

Blocking item — the positive scenario. You're right on both halves, so I've split them explicitly in the PR body (new "e2e coverage and the scenario gap" section), per §3's escape hatch and §2's body-vs-comment distinction:

• Post-failure hint (append_oom_serve_note): we agree this is genuinely unreachable on any current lane — already_running == false is required, parse_device_policy rejects cpu_only, and the reuse short-circuit forces already_running == true. It needs a harness capability that doesn't exist yet (a mock that clears the GPU pre-flight and reaches the post-launch summary), so it stays unit-covered; the gap + "no lane yet" is now stated in the body.
• Pre-launch hint (collect_serve_notes → serve_gpu_low_memory_warning → VLLM_GPU_MEMORY_UTILIZATION_HINT): your recipe is correct — the HOME-isolated runtimes/registry is resolved before PATH, so a stub amd-smi reporting a busy GPU makes the hint render into the same captured summary Scenario 15 asserts on, all on the @requires-no-gpu mock lane. The one thing it needs is a new harness step-def (a low-VRAM stub-amd-smi fixture); there's no existing step to reuse. I've deliberately not pushed that unverified: the e2e cucumber harness can't be built on my macOS box (engines/lemonade has an st_mode u16/u32 libc mismatch, and macOS is unsupported per §6), so a new step-def would go in blind and iterate through red CI. I've recorded it as a tracked follow-up in the body and I'm glad to land it as a dedicated, CI-verified change — say the word if you'd rather it block here and I'll add the fixture + scenario and drive it green on the Linux lanes.

Nits. Noted and deferred as non-blocking: the any_live_managed_service_for_engine engine-only pre-gate (harmless for vLLM's pure-echo ResolveModel, matters only for a self-managing engine's stale-version install), the repeated load_managed_services reads, the bare-outofmemory import-time-crash false positive (a misleading-but-additive hint), and the oom_memory_note policy/rendering split. The diagnose-catalog coordination point is now concrete — #290 merged, so whichever of the two OOM detectors lands here should fold onto the shared helper; I'll note that on the follow-up.

Two red checks (self-hosted GPU, readthedocs) fail identically on unrelated recently-merged PRs; the blocking GitHub-hosted mock lane runs and passes @id:serve-oom-memory-guidance.

[rominf]

I read through this PR's diff (base gpu-out-of-memory vs the current head), plus the surrounding code it touches in apps/rocm/src/main.rs, apps/rocm/src/serve_summary.rs, crates/rocm-core/src/lib.rs, and engines/vllm/src/lib.rs.

The change is well-scoped and I didn't find anything blocking:

• The new vllm_log_shows_oom signature detection is shared correctly between the CLI (append_oom_serve_note/oom_memory_note) and the vLLM engine's own startup-log summary (oom_utilization_hint), so the two surfaces can't drift in wording or matching logic. The exclusion of vLLM's generic "engine core initialization failed" wrapper as an OOM signature is deliberate and well-tested (it's the terminal line for any startup crash, not just OOM).
• append_oom_serve_note is correctly gated: vLLM only, real launch failure only (status == "starting", not the healthy still-loading running state), and already_running excluded so a reused service's log is never misattributed to the current invocation. The dedup check against the already-present hint text also works correctly against how collect_serve_notes stores it.
• The reordering that moves model resolution/reuse detection ahead of the GPU pre-flight bail is a real, deliberate fix (per the commit history) for a genuine ordering bug: reusing an already-running managed service pins no GPU and shouldn't be blocked by the GPU-required check. The any_live_managed_service_for_engine pre-gate keeps the common (non-reuse) launch path failing fast as before, and the non-reuse behavior (no live service, no runtime configured, etc.) is unchanged.
• Test coverage is solid: unit tests for the OOM signature matcher, the note builder in both withheld/fired cases, and a new Gherkin scenario (serve-oom-memory-guidance) that runs on the no-GPU mock lane and asserts a reused already-running service is never blamed for another process's OOM.
• No internal/company-only text made it into the diff.

This looks safe to approve after the usual final checks — passing CI and a maintainer's own pass — since this is a diff-only read and doesn't replace running the test suite or verifying against real hardware.

[r0x0r]

Rebased onto the updated gpu-out-of-memory base (now at e94ca5f) and force-pushed (e8e19e8).

One semantic merge decision worth flagging: the base branch had independently evolved oom_utilization_hint to route the user's actual failing line into a rocm diagnose --symptom 'vllm: …' example. The rebase converges that with this PR's shared detector, so the engine hint now uses rocm_core::vllm_log_shows_oom (shared with the CLI's serve summary) and keeps the base's --symptom routing. The local log_tail_shows_oom the base added is gone in favor of the shared function, and the engine tests assert both the shared-detection and the --symptom output.

The rest of the series replayed unchanged; cargo test --workspace and cargo clippy --workspace --all-targets -- -D warnings are green. This stays stacked on #251, so it depends on that landing first.

[rominf]

Re-reviewed the update pushed since the last review (commit e8e19e8a, rebased onto the current gpu-out-of-memory tip and force-pushed). This is a diff-only read of the new commit, e2e: cover positive OOM-launch memory guidance via feature-gated fault injection.

What's new since the last pass: it adds the positive e2e scenario the earlier review rounds flagged as missing (Scenario 16, serve-oom-launch-memory-guidance) — a fresh vLLM launch that runs out of GPU memory and whose interactive summary must blame this launch and name the --gpu-memory-utilization knob. Since a real OOM can't be produced deterministically on the no-GPU mock lane (the GPU pre-flight bails before a launch even starts) and a live-GPU OOM would be flaky, it introduces a compile-gated fault-injection seam: a new Cargo feature e2e-oom-fault-injection on the rocm package, off by default, enabled only by xtask e2e's mock-lane build (verified release.yml does not pass this feature, so shipped binaries never carry it). When armed per-child via ROCM_E2E_SIMULATE_OOM_LAUNCH=1, start_managed_service short-circuits to a helper that fabricates a starting, not-already_running managed-service record with a real allocator OOM signature in its own log, driving the exact append_oom_serve_note path the earlier unit tests already covered only in isolation.

I checked:

• The feature is correctly scoped (rocm/e2e-oom-fault-injection, not applied to rocmd) and compiled out of release builds.
• The fault-injection bypass is placed correctly relative to the GPU-required pre-flight (!e2e_simulate_oom_launch()), consistent with how the existing reuse_existing bypass works, and doesn't affect any non-e2e-feature build.
• The fabricated state (status: "starting", already_running: false, log carrying torch.OutOfMemoryError: HIP out of memory...) lines up exactly with the gating conditions in append_oom_serve_note/oom_memory_note fixed in earlier rounds, so the new scenario genuinely exercises the intended code path rather than a shortcut.
• --env-id on the new scenario resolves directly from the CLI flag (resolve_engine_selection's cli_env_id branch) with no on-disk manifest dependency, so it isn't fragile.
• The new TuiSession::spawn_with_env/spawn_binary_with_env helpers apply the env var per child process only (never a global set_var), which is the right way to keep this safe under concurrent no-GPU scenarios.
• The two OOM e2e scenarios use distinct model ids so their planted service records can't collide.

No findings from this pass. This looks safe to approve after the usual final checks — passing CI and a maintainer's own pass — since this is a diff-only read and doesn't replace running the test suite or verifying on real hardware.

[r0x0r]

Pushed `5bd1bfc`: gated the `serve-oom-launch-memory-guidance` scenario on a new `@requires-oom-fault-injection` capability so the WSL2 self-hosted E2E lane skips it instead of failing.

Why the lane failed: that scenario drives the test-only `e2e-oom-fault-injection` hook to fabricate a GPU-less OOM launch. The hook is compiled out of the shipping release binary. The self-hosted lanes test a prebuilt `ROCM_CLI_BINARY`, so xtask e2e skips its own feature build and the real GPU-required `serve` pre-flight ran instead — on WSL2 (no usable GPU) that bailed with "no usable AMD GPU detected" and the scenario exited 1. Only the GitHub-hosted mock lane, where `xtask` builds the binary itself with the feature, could ever pass it. This was a pre-existing design gap, not a rebase regression.

The gate: `xtask e2e` now sets `ROCM_E2E_OOM_FAULT_INJECTION=1` only when it built the binary with the feature; the harness probe reads it into `HostCapability.oom_fault_injection`, and `resolve()` skips the tagged scenario when the hook is absent. This preserves the mock-lane coverage that gates every PR and cleanly skips the feature-less self-hosted lanes. Added unit tests for the resolver gate (both directions) and the xtask env signal.

[rominf]

Re-reviewed the update pushed since the last review (commit 5bd1bfc, on top of e8e19e8a). This is a diff-only read of the delta plus a fresh full pass over the whole PR diff against gpu-out-of-memory.

What's new: the author gated the positive serve-oom-launch-memory-guidance scenario (added in the prior push) behind a new @requires-oom-fault-injection tag. That scenario drives the test-only e2e-oom-fault-injection hook, which is compiled out of the shipping release binary. The self-hosted lanes test a prebuilt ROCM_CLI_BINARY, so xtask e2e there skips its own feature build and the scenario was hitting the real GPU-required pre-flight instead of the fault-injection path — which correctly bailed with "no usable AMD GPU detected" on WSL2 and failed the scenario. That's a pre-existing lane-coverage gap surfacing when the positive scenario landed, not a rebase regression.

The fix: xtask e2e now sets ROCM_E2E_OOM_FAULT_INJECTION=1 only when it built the binary itself (with the feature); a prebuilt binary path clears/never sets it. The harness's HostCapability picks that signal up as oom_fault_injection, and resolve() skips @requires-oom-fault-injection scenarios when it's false. I checked:

• The env var name is kept in sync between xtask::e2e and e2e_cucumber::capability (both define the same ROCM_E2E_OOM_FAULT_INJECTION string, matched by a code comment cross-reference in each).
• configure_harness_env sets the var to "1" on the release-build path and explicitly env_removes it on the prebuilt-binary path, so a prebuilt lane can't accidentally inherit a stray 1 from the ambient environment.
• The new resolver test exercises both directions (hook absent → skip, hook present → run) against the same host capability, isolating the gate to the binary's build rather than the platform.
• release.yml never passes --features rocm/e2e-oom-fault-injection, so the hook stays out of shipped binaries, consistent with the doc comments' claims.
• The mock lane (where xtask builds the binary itself, so the feature and the env signal are both present) still runs and passes the scenario — confirmed on the current head's checks.

I also re-read the full diff against base again (not just this delta) given how long this PR has been through review rounds: the OOM signature matcher, the append_oom_serve_note/oom_memory_note gating (status == "starting", already_running excluded), the reuse-before-GPU-preflight reordering, and the two Gherkin scenarios (15 negative / 16 positive) are all unchanged from the last pass and still look correct together. No new findings.

This looks safe to approve after the usual final checks — passing CI (the self-hosted GPU/WSL lanes plus the mock lane) and a maintainer's own pass — since this remains a diff-only read.

[r0x0r]

The attribution reasoning here is the strongest part: gating the post-failure note on already_running == false and status == "starting" and the serve's own log_path is exactly the right defense against blaming this invocation for another process's OOM, and serve_failed_to_become_ready correctly keeps a healthy still-loading running service out of the failure path. The const false fault-injection seam compiling out of release builds is also clean.

One substantive question on precision: vllm_log_shows_oom is a case-insensitive substring scan for out of memory / outofmemory over the last 80 log lines, and the note fires whenever that substring appears in the tail of a failed-to-become-ready serve. It isn't anchored to the terminal/fatal error line. So a serve that died for an unrelated reason but whose tail happens to mention "out of memory" somewhere (a benign warning, a retried allocation that later succeeded, a model/path name containing the token) would be misreported as an OOM failure. The already_running/starting/own-log triad prevents cross-process misattribution, but not "wrong cause, same process." Is anchoring to the last matching line (the way oom_utilization_hint already picks the failing line) worth it here, or is substring-in-tail deemed precise enough in practice?

Smaller related point: the tail is read at 80 lines. If an OOM traceback is followed by more than 80 lines of shutdown/teardown noise, the signature scrolls out of the window and the note is silently withheld. Worth confirming 80 matches the engine's own DEFAULT_LOG_TAIL_LINES budget so the two surfaces don't disagree on what counts as "the tail."

[r0x0r]

Pushed c337c7c addressing the tail-window point; keeping the substring-in-tail detector on purpose, reasoning below.

Tail budget: the serve summary read a bare 80. It now reads rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES (the same budget the engine's OOM surfaces use), so the two surfaces can't silently drift on what counts as "the tail" of a failed launch. Added append_oom_serve_note_reads_the_shared_engine_tail_budget, which plants the OOM signature on the oldest line still inside the budget (must fire) and one row past it (must fall silent), pinning the read to the shared constant rather than a literal.

Anchoring vs substring-in-tail: deliberately keeping the whole-tail scan.

• Anchoring the gate to the last matching line wouldn't change whether the note fires — "any line matches" and "the last matching line matches" are the same boolean. Only tightening to fatal-line-only would change firing, and that trades a cheap, self-correcting false positive (an extra memory hint on an already-failed launch) for the far worse false negative of missing a real OOM whose traceback is followed by teardown noise — which is exactly the case the tail-window point warns about. The two pull in opposite directions.
• The two signatures (outofmemory, out of memory) are error-context tokens in vLLM/PyTorch — exception class names (torch.OutOfMemoryError, hipErrorOutOfMemory) and the RuntimeError: HIP/CUDA out of memory phrasing — not general prose, so a benign-mention false positive is unlikely in practice.
• Cross-process misattribution is already fully blocked by the already_running == false + status == "starting" + own-log_path triad, so the residual risk is bounded to "wrong cause, same already-failed process": an extra diagnostic line, never a wrong action on a healthy serve.

Meanwhile oom_utilization_hint still anchors the quoted --symptom line to the last matching line (.rev().find(...)), so the user always sees the actual failing line as evidence even though the gate stays whole-tail.

[rominf]

Reviewed as the delta against gpu-out-of-memory (#251), so #251's own work isn't re-litigated here — I left that separately on #251.

The note logic is good and the fault-injection seam is a nice piece of design: feature-gated with a #[cfg(not(feature))] const fn -> false fallback, armed per-invocation by an env var, reported to the harness as a capability so the scenario skips rather than silently passes on a prebuilt binary. oom_memory_note is properly unit-tested and the reuse-vs-launch distinction it draws is exactly right.

Three things I'd want fixed before merge.

The reorder is the significant one. To decide reuse_existing, the new block runs load_managed_services (side-effecting — liveness probes, record writes), a full ResolveModel round-trip, and ensure_self_managed_engine_ready before the no-usable-GPU bail. The comment right below it still says "BEFORE preparing or launching any engine (no wasted engine download …)", and that's no longer true. It also drags resolve_gpu_indices from before the no-runtime bail to after it — and the GPU lane on this head is failing serve-absent-gpu-index-rejected with exactly the message that predicts:

Step panicked. Captured output: expected the absent GPU index to be reported unavailable, got:
Error: device_policy: gpu_required; no active ROCm runtime is configured; ...

I checked #251's GPU lane as the control — same runner, same base — and it doesn't fail that scenario (its failures are all serve-timeout/resolved model shaped). So this looks caused by the reorder rather than inherited.

Second: xtask/src/e2e.rs replaces the single --features value with rocm/e2e-oom-fault-injection, but main now passes rocm/e2e-test-hooks there — added by #249, which isn't an ancestor of this branch. On rebase those compete for one slot, and workflow_contract.rs asserts the literal --features rocm/e2e-test-hooks string, so a naive resolution either breaks the contract test or silently compiles out lemonade's failure seams.

Third, smaller: the negative OOM assertion uses a raw contains while both positive ones normalize whitespace — by the PR's own stated reason (80-col soft wrap), so Scenario 15 can pass without testing anything.

On process — this is stacked on #251's branch but isn't draft and has no Depends on #251 in the body. AGENTS.md §11 asks for both, and it matters here: about half of what a reviewer sees belongs to the other PR, and #284's delta relocates code #251 introduces. Worth considering whether the rocm-core placement should just land in #251 directly.

Leak scan is clean and the EAI-8059 reference is fine per AGENTS.md §2.

[rominf]

This comment is now false, and the work it promises won't happen is exactly what runs above it.

"BEFORE preparing or launching any engine (no wasted engine download, and an actionable message instead of a late engine crash)" — but the reuse block at :4877-4897 already ran:

• any_live_managed_service_for_engine → load_managed_services, which is side-effecting: refresh_from_engine_state, refresh_managed_service_runtime_liveness (HTTP readiness/inference probes with a 750ms timeout each), and record.write();
• a full ResolveModel engine round-trip;
• ensure_self_managed_engine_ready for self-managed engines.

That last one is the expensive one. On a GPU-less host with lemonade selected, the user now sees "Preparing lemonade for GPU serving…" and potentially a multi-GiB install, and then gets told there's no usable AMD GPU. AGENTS.md §6's "preserve strict GPU-required behavior" is about the outcome, but the fail-fast part is what this comment is claiming and it's what's lost.

The !reuse_existing exemption itself is correct — nothing is launched when reusing, so the gate shouldn't fire. It's the cost of computing reuse_existing that's the problem.

A cheap ordering fix: gate the whole reuse block on usable_amd_gpu_indices() being non-empty (or None), so on a GPU-less host you fall straight through to the bail. Reuse can't be the right answer on a host with no GPU under gpu_required anyway. Alternatively determine reuse from a non-mutating read of the records and defer the ResolveModel/prepare work until after the gate.

Whichever way it goes, the comment needs to match — if the contract genuinely changed, say what the new one is.

[r0x0r]

Fixed in 4a108b9 — you were right that the comment promised something the block above it had already spent.

The reuse pre-gate keyed on the engine alone, so a live service for a model this invocation can never reuse was enough to pull the ResolveModel round-trip and — the expensive one you named — ensure_self_managed_engine_ready, complete with Preparing lemonade for GPU serving... and a possible multi-GiB install, in front of the no-usable-GPU bail. It now keys on the model too (any_live_managed_service_for_model), so on a GPU-less host with a live lemonade service for some other model the block is skipped entirely and the bail is once again the first thing that runs.

On the two options you offered: gating the whole reuse block on usable_amd_gpu_indices() being non-empty would have been cheaper, but it breaks @id:serve-oom-memory-guidance (serve-20), which reaches the reuse short-circuit precisely on a GPU-less host. So I took the second one — decide reuse from the records and let only a plausible match pay for the engine round-trip. Matching uses service_model_names_match, the same lenient relation the service-listing surfaces already use, so a short-vs-canonical spelling still reaches the probe; the probe then still decides reuse authoritatively on the canonical id via existing_live_managed_service. Reuse detection is not narrowed in practice.

The residual, stated plainly: when a live service does match this engine and model, ensure_self_managed_engine_ready still runs before the bail. That is the case where this invocation is about to reuse and legitimately skip the bail, so the work is not wasted — and the comment now says exactly that instead of claiming an absolute.

Regression tests (apps/rocm/src/main.rs): reuse_pregate_skips_engine_work_for_an_unrelated_live_model — this is the one that fails before the fix, since the old function body had no model term at all and returned true for any live service of the engine; plus reuse_pregate_admits_the_same_model_including_a_short_spelling, reuse_pregate_still_keys_on_the_engine, and reuse_pregate_admits_the_model_id_the_oom_reuse_scenario_plants, which pins the literal id serve-20 plants so a matcher change cannot silently turn that scenario into a launch attempt on the one lane it runs on.

Verified on a Linux MI300X box: full workspace cargo test + clippy -D warnings + fmt --check, and cargo xtask e2e with serve-20 and serve-21 both passing. I could not run the self-hosted E2E tests (GPU) or E2E tests (Strix Halo, Ubuntu) lanes — that hardware is not available to me, so CI is the only authority for those two.

[rominf]

resolve_gpu_indices moved from before the no-runtime bail to after it, and the GPU lane on this head is failing on exactly that.

On the base (:4873-4878 there), resolve_gpu_indices — the source of --gpu index N is out of range — ran ahead of the no active ROCm runtime is configured bail. Here it's at :4934, after. So a bad --gpu value now surfaces as an environment complaint instead of an argument complaint.

From run 33489993381 on c337c7c4:

Step panicked. Captured output: expected the absent GPU index to be reported unavailable, got:

Error: device_policy: gpu_required; no active ROCm runtime is configured; run `rocm runtimes list` ...

I pulled #251's GPU lane (run 33166192967, job 98832217235) as the control, since that lane has other unrelated problems and I didn't want to over-attribute. It does not fail serve-absent-gpu-index-rejected — its failures are all serve-timeout / missing-resolved model shaped. So the regression appears with this delta.

User-facing, argument validation before environment validation is the right order regardless: --gpu 99 is wrong no matter what runtime is active, and telling the user to go activate a runtime sends them down the wrong path.

The reorder that reuse needs shouldn't have to drag resolve_gpu_indices with it — gpu_vram_usage()/resolve_gpu_indices don't depend on resolved_model, so they can go back above the runtime bail.

[r0x0r]

Fixed in 7c09129 (already on the branch when you filed this); re-verified against the current head rather than the claim.

resolve_gpu_indices now runs at apps/rocm/src/main.rs:5209, immediately after the no-usable-GPU pre-flight and before the no-runtime bail at :5211. So --gpu 99 is an out-of-range argument error again regardless of runtime state, and a GPU-less host still refuses with "no usable AMD GPU" first. Your point that argument validation belongs ahead of environment validation is the ordering that shipped.

Lane evidence, since this was a GPU-lane regression: serve-16 / @id:serve-absent-gpu-index-rejected ran and passed on E2E tests (GPU) at 83ecfaad, and it also passes in my own cargo xtask e2e run on a Linux MI300X box at the new head 4a108b9. It is not among that lane's current failures.

Thanks for pulling #251's run as a control before attributing it — that was the right call and it is what made the delta unambiguous.

[rominf]

This replaces a --features value that main added, and there's a contract test asserting the old string.

On origin/main this call site is --features rocm/e2e-test-hooks (from #249, not an ancestor of this branch). Here it becomes --features rocm/e2e-oom-fault-injection, and apps/rocm/Cargo.toml in this diff declares only e2e-oom-fault-injection. git merge-tree origin/main pr/284 conflicts in apps/rocm/Cargo.toml, apps/rocm/src/main.rs, model_serving.feature, serving_steps.rs, and this file — and this one is literally the two feature strings competing for one slot.

The part that makes it more than a routine conflict: xtask/src/workflow_contract.rs:255-274 asserts every prebuilt E2E lane block contains the exact string

cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks

So a resolution that keeps only the new feature fails that test, and one that keeps only the old feature silently disables the new scenario (it degrades to a skip via the capability gate — a green run that tested nothing). Both failure modes are quiet in different ways.

On rebase this wants: a combined list (rocm/e2e-test-hooks,rocm/e2e-oom-fault-injection), e2e-test-hooks restored in apps/rocm/Cargo.toml, and the workflow_contract.rs expectation updated to match. Worth flagging in the PR text too, since whoever resolves the conflict may not know the contract test exists.

The configure_harness_env half is right — clearing the env var for a prebuilt binary rather than letting an inherited value through is the correct call.

[r0x0r]

Addressed across 7c09129 and 4a108b9.

The overwrite is gone: xtask/src/e2e.rs:125 now passes a single space-separated value, "rocm/e2e-test-hooks rocm/e2e-oom-fault-injection", so e2e-test-hooks (#249) survives alongside the new hook rather than losing the one --features slot to it. apps/rocm/Cargo.toml declares both.

On workflow_contract.rs:255-274: the assertion is a contains on cargo build --release -p rocm -p rocmd --features rocm/e2e-test-hooks, and every prebuilt lane still spells exactly that, so it passes unchanged. But your framing exposed that its doc and failure message had become false — they claimed the asserted string matched what cargo xtask e2e builds for itself, which is now a superset. 4a108b9 rewrites both to describe it as the shared hook floor, and records why the fault-injection hook is deliberately absent from the prebuilt lanes: those scenarios carry @requires-oom-fault-injection, so a missing hook is a reported skip, not the silent green you were worried about. That closes the third failure mode — a stale comment nobody re-reads on rebase.

Your two other resolution notes are now moot for this branch: this PR targets gpu-out-of-memory, not main, and the base already carries the serve-NN scenario naming, so merge-tree no longer conflicts in model_serving.feature. I have put the combined-feature-list requirement in the PR body anyway, since whoever eventually rebases this onto main will hit exactly the collision you describe and should not have to rediscover the contract test.

Agreed on configure_harness_env — it env_removes on the prebuilt path rather than letting an ambient 1 through, and that is unchanged here.

[rominf]

This negative assertion is whitespace-sensitive while both positive ones aren't, so Scenario 15 can pass vacuously.

assert_oom_launch_memory_guidance (:954) uses screen_without_whitespace(&screen).contains("ranoutofGPUmemory"), and the comment above it explains why: "The note is a long line the 80-column PTY wraps across grid rows … otherwise a soft wrap between 'GPU' and 'memory' would break a literal contains and mask a rendered note."

That reasoning applies identically here, in the direction that matters more. If the note were wrongly emitted on the reuse path and happened to wrap — which the positive step says is the normal case for this string — the raw contains wouldn't see it and the scenario would pass. So the regression guard is disarmed precisely when the note renders the way the code actually renders it.

screen_without_whitespace(&screen).contains("ranoutofGPUmemory") with the ! is the one-line fix, and it makes the pair symmetric.

The scenario itself is a good one — "don't blame this invocation for another process's OOM" is a real distinction and worth pinning.

[r0x0r]

Fixed in 7c09129; verified against the current code, not the claim.

tests/e2e-cucumber/tests/e2e/serving_steps.rs:1218 is now:

!screen_without_whitespace(&screen).contains("ranoutofGPUmemory"),

exactly the one-line fix you proposed, so the negative and the positive (:1272) are symmetric and the guard survives the soft wrap the positive step documents as the normal rendering. The comment above it now spells out why the whitespace collapse is load-bearing in the negative direction specifically — that a literal contains would have passed even if the note were wrongly emitted.

The scenario is now serve-20 (renumbered when the base landed serve-19); the @id: is unchanged. It ran and passed in cargo xtask e2e on a Linux MI300X box at 4a108b9, and it also runs on the blocking GitHub-hosted mock lane.

Thanks — that one really would have been a silently disarmed regression guard.

[rominf]

Two coupled issues with this detector and the note it drives.

The threshold contradicts #251's. This matches a bare out of memory substring anywhere in the tail. In the same stack, check_16_vllm_oom deliberately scores a bare "out of memory" at 25 — below MIN_SCORE_FOR_MATCH — with a comment explaining that it "is deliberately sub-threshold so this only claims the failure mode when the vLLM/HIP shape of the error is present," and gates the whole table behind VLLM_ANCHOR_PATTERN. Same failure class, same repo, two thresholds, and the looser one is the one driving user-facing serve output.

A managed-service log tail containing out of memory from any source — a kernel OOM-killer line, a dependency's log, an unrelated subprocess — produces the vLLM OOM note. The doc comment here is careful about excluding vLLM's generic wrapper line, which suggests the false-positive risk was on your mind; the bare-phrase case is the bigger one.

The guidance is wrong for the model-too-large case. The note only ever suggests --gpu-memory-utilization and --gpu <index>. Both help when the GPU is shared or busy. Neither helps when the model simply doesn't fit — and lowering the reservation there actively makes it worse, trading an earlier OOM for a later one.

#251 already gets this right: check_16_vllm_oom's summary says "Only lower --gpu-memory-utilization when the GPU is shared or already busy; on a GPU dedicated to this server it does not create room a too-large model needs." That caveat doesn't survive into the note this PR shows users, and --max-model-len isn't mentioned anywhere in the new guidance.

I'd reuse #251's anchored matcher rather than maintain a second looser one, and either carry the shared-vs-dedicated caveat into the note or have it point at rocm diagnose --symptom 'vllm: …', which already does the nuanced version. Pointing at diagnose is the lighter option, and it exercises the handoff the two PRs are building.

Small related note on the wording: the hint says --gpu-memory-utilization <0-1>, but parse_gpu_memory_utilization enforces (0, 1] — 0 is rejected.

[r0x0r]

All three points fixed in 83ecfaa. Taking them in your order:

Threshold. You were right that maintaining a second, looser matcher for the same failure class was the bug, and that the looser one driving user-facing output made it worse. rocm_core::vllm_log_shows_oom no longer does a bare substring scan — it classifies each tail line with the same rule as check_16_vllm_oom, scoring the line as vllm: <line> against the shared MIN_SCORE_FOR_MATCH threshold (crates/rocm-core/src/lib.rs:7424). The tail is vLLM's own process output, so the checker's required VLLM_ANCHOR_PATTERN is supplied by context rather than dropped. Sub-threshold lines that used to fire now correctly do not: a kernel OOM-killer line, a bare out of memory, a bare HIP error: out of memory, a bare torch.OutOfMemoryError with no allocator corroboration. Genuine allocator failures still clear it. One helper feeds both the engine hint and the CLI note, so there is no second matcher left to drift.

Model-too-large guidance. Also fixed, and I took your lighter option and carried the caveat, since the note is what the user actually reads. oom_memory_note (apps/rocm/src/serve_summary.rs:219) now says that if the model simply does not fit, lowering the reservation will not help, and to serve a smaller or quantized model instead — then routes to rocm diagnose --symptom '<their actual failing line>' for the case-appropriate fix, which exercises the handoff you point out the two changes are building. --max-model-len is deliberately left to diagnose, which does the nuanced version, rather than duplicated into a fixed string here.

Wording. <0-1> is gone. The shared hint now reads "a fraction greater than 0 and at most 1", matching what parse_gpu_memory_utilization actually enforces on (0, 1].

Verified at 4a108b9 on a Linux MI300X box: full-workspace cargo test + clippy -D warnings, and cargo xtask e2e with diagnose-13 (@id:diagnose-vllm-oom-is-conditional), serve-20 and serve-21 all passing. I could not run the self-hosted E2E tests (GPU) or E2E tests (Strix Halo, Ubuntu) lanes — no such hardware available to me — so CI remains the only authority on those two.

[rominf]

This comment claims an invariant that isn't enforced anywhere.

"Read the same tail budget the engine's own OOM surfaces use (rocm_engine_protocol::DEFAULT_LOG_TAIL_LINES) … rather than drifting apart on independent magic literals" — but the vLLM engine's OOM surface reads STARTUP_FAILURE_LOG_TAIL_LINES, a local const … = 80 at engines/vllm/src/lib.rs:34, used at :1817 and :1823. That's precisely the independent magic literal the comment says has been eliminated.

They're both 80 today, so nothing is broken. But the comment (and the commit message) describes an enforced coupling, and a future change to DEFAULT_LOG_TAIL_LINES would silently desynchronize the two surfaces — with the comment still asserting they can't.

Pointing STARTUP_FAILURE_LOG_TAIL_LINES at the protocol constant is a one-liner and makes the claim true. Otherwise the comment should describe the intent rather than a guarantee.

[r0x0r]

You were right, and this one was genuinely still open — the earlier response commits did not touch it. Fixed in 4a108b9.

engines/vllm/src/lib.rs:35 was still const STARTUP_FAILURE_LOG_TAIL_LINES: usize = 80;, the exact independent literal the serve-summary comment claimed had been eliminated. It now reads:

const STARTUP_FAILURE_LOG_TAIL_LINES: usize = DEFAULT_LOG_TAIL_LINES;

DEFAULT_LOG_TAIL_LINES was already imported in that file, so it is the one-liner you described. I took your first option rather than weakening the comment to intent, because the coupling is the thing worth having: a change to the protocol budget now moves both surfaces, and append_oom_serve_note and oom_utilization_hint cannot silently disagree on what "the tail" of a failed launch means. The const carries a doc comment saying so, so the next person is not tempted to re-inline the literal.

Deliberately scoped to vLLM: engines/lemonade has its own STARTUP_FAILURE_LOG_TAIL_LINES alongside a local DEFAULT_LOG_TAIL_LINES = 200, and it is not part of the OOM surface this PR couples, so I left it alone rather than widen the change.

Verified at 4a108b9: full-workspace cargo test + clippy -D warnings + fmt --check on a Linux MI300X box, plus the vLLM engine's own tail-budget tests. Not verified on E2E tests (GPU) or E2E tests (Strix Halo, Ubuntu) — that hardware is unavailable to me.

[rominf]

This constant is declared twice — here and in xtask/src/e2e.rs:24 — held together only by a "kept in sync" comment on each side.

A typo in either doesn't fail to compile and doesn't fail a test. xtask sets one name, the harness reads another, oom_fault_injection is false, @requires-oom-fault-injection maps to Expectation::Skip, and the scenario silently stops running. Green suite, zero coverage — the same failure mode the comment on probe_host_capability is careful about elsewhere.

Since the harness crate is already a build dependency relationship away, hoisting the constant into a shared crate would be cleanest. Failing that, a test in either crate asserting the two literals are equal costs three lines and closes it.

Minor, but this scenario is the only thing exercising the fault-injection path, so a silent skip is expensive.

[r0x0r]

Fixed in 83ecfaa, and I took the cleaner of your two options.

ROCM_E2E_OOM_FAULT_INJECTION is now declared exactly once, in the e2e-report crate that both xtask and e2e-cucumber already depend on:

• crates/e2e-report/src/lib.rs:30 — pub const OOM_FAULT_INJECTION_ENV: &str = "ROCM_E2E_OOM_FAULT_INJECTION";
• xtask/src/e2e.rs:30 — use e2e_report::OOM_FAULT_INJECTION_ENV;
• tests/e2e-cucumber/src/capability.rs:134 — pub use e2e_report::OOM_FAULT_INJECTION_ENV;

so the two former copies are re-exports of one source and a typo is now a compile error rather than a silent oom_fault_injection = false. Since it is a single constant, the equality test you offered as the fallback is no longer needed — there is nothing left to hold in sync.

Your framing of the cost is what decided it: this is the only thing exercising the fault-injection path, so a @requires-oom-fault-injection scenario degrading to Expectation::Skip would have been a green suite with zero coverage of the PR's headline behavior. The resolver still has tests for both directions (hook present → run, hook absent → skip), and serve-21 ran and passed under cargo xtask e2e on a Linux box at 4a108b9, so the signal is reaching the harness for real rather than only in principle.

[rominf]

These reuse scenario numbers that already exist on main.

origin/main's model_serving.feature has a Scenario: 15 at line 143 ("Serving with a temperature below zero is refused…"), another Scenario: 15 at 173 ("Selecting both a runtime and an environment at once…" — main already has a duplicate), and a Scenario: 16 at 184. This PR adds a third 15 and a second 16, in different hunks, so git merges them cleanly and nothing complains.

The cost is triage: a lane reports a scenario by name and number, and "Scenario 15 failed" stops identifying anything. Worth renumbering on rebase — or dropping the manual numbers in favor of the @id: tags the repo already uses everywhere, which is what the tooling actually keys on.

Both new scenarios read well otherwise; the positive/negative pair covers the distinction properly.

[r0x0r]

Fixed — the collision is gone as of 83ecfaa, though partly by the base moving under us.

origin/main no longer carries bare Scenario: 15 / 16 headings; every scenario in model_serving.feature is now Scenario: serve-NN - ..., running to serve-18 on main. The base branch (#251) adds serve-19, and this PR's two are serve-20 and serve-21, so the numbering is unique and sequential across the file and feature_naming is satisfied. Verified against origin/main and origin/gpu-out-of-memory at the current head rather than from memory.

On dropping the manual numbers in favour of @id:: agreed that @id: is what the tooling keys on, and the two scenarios here carry stable ones (serve-oom-memory-guidance, serve-oom-launch-memory-guidance) that survived both renumberings untouched — I have quoted the @id: rather than the number everywhere in the PR body for exactly the triage reason you give. Removing the numeric prefixes wholesale is a rename across every feature file plus the feature_naming test, which is a bigger and unrelated change than this PR should carry; happy to raise it separately.

Both scenarios ran and passed under cargo xtask e2e on a Linux MI300X box at 4a108b9. Thanks for the note on the pair reading well — the positive/negative split was the thing earlier rounds spent the most time getting right.

[r0x0r]

Thanks for the review. Rebased onto the updated #251 head and addressed the points; the branch is now two commits (the squashed feature + a review-response commit) and back in draft while it depends on #251.

• --gpu validation ordering (blocking). You were right that the reorder ran ResolveModel/ensure_self_managed_engine_ready before the pinned-index check, so serve-absent-gpu-index-rejected ("refused before any engine starts") would fail on the GPU lane. resolve_gpu_indices now runs immediately after the no-usable-GPU pre-flight and before any runtime/engine work, so a nonexistent --gpu index is rejected up front; a GPU-less host still refuses with "no usable AMD GPU" first.
• xtask feature collision. The e2e build no longer overwrites rocm/e2e-test-hooks (fix(lemonade): retry interrupted backend setup #249). It now passes a single --features "rocm/e2e-test-hooks rocm/e2e-oom-fault-injection" value so both hook surfaces are compiled into the mock-lane binary; workflow_contract.rs's prebuilt-lane assertion is unaffected (those lanes intentionally carry only e2e-test-hooks and skip the fault-injection scenario).
• Negative assertion. does not blame this invocation for GPU memory now collapses whitespace exactly like the positive checks, so it genuinely catches a note that wraps across the 80-column PTY grid rather than passing vacuously against a literal substring.
• Draft + dependency. PR is back in draft and the body now states Depends on feat(vllm): Tackle out of memory errors (EAI-8058) #251; I'll rebase onto main and mark it ready once feat(vllm): Tackle out of memory errors (EAI-8058) #251 merges.

Also renumbered the two OOM scenarios to 17/18 to sit alongside the runtime/env and Lemonade-recovery scenarios that landed on the base, and fixed a stale log_tail_shows_oom reference (the detector now lives in rocm-core as vllm_log_shows_oom). Full workspace cargo test + clippy -D warnings + smoke_local.py are green locally.

[r0x0r]

Merged main again (it moved 4 commits under this branch). Pushed as 28843727.

One conflict, in tests/e2e-cucumber/src/capability.rs: both sides added tests to the same module — this branch's only_the_exact_value_the_producer_writes_arms_the_hook (the value half of the @requires-oom-fault-injection contract) and main's three permission_probe_* tests. Kept both; nothing was dropped.

Verified on the Linux MI300X box with the merged tree applied onto main (81f4a46f): cargo clippy --workspace --all-targets -- -D warnings exit 0, cargo test --workspace --all-targets exit 0 across 76 suites with 0 failures, cargo fmt --all -- --check exit 0.

Heads-up on a scenario-number collision

This branch and #703 both currently declare serve-28. feature_naming requires the number to equal the scenario's declaration position, so this cannot be pre-emptively resolved on both sides — renumbering here before #703 lands would turn this branch's own check red.

#703 is approved and adds serve-28..32, so the natural order is: #703 merges first, then this branch renumbers its scenario to serve-33 on its next main merge. Flagging it so whoever merges does not hit it cold.

[jussielo-amd]

🔴 AI-assisted review (Claude Code) · head 28843727

This is an assisted human review, not an autonomous merge gate: no GitHub approval is being filed, and the merge decision stays with maintainers. The change request below is for the blocking items only.

Review — needs work

Full review of the PR's own diff at 28843727 (base 6eeec620, merge-base 81f4a46f; 28 files, +3174/−310). The ticket was not retrievable from here, so its scope was not checked. This round re-checks the earlier review at ebc26718 against the author's commit 4187652a and the main merge.

Blocking

1. The head cannot merge and no CI has run on it. engines/vllm/src/process.rs has 4 conflict hunks against main, which has since moved the startup diagnostics into engines/vllm/src/startup_diagnostics.rs. The PR's hunks and about 900 lines of tests have to be ported there. No workflow run exists for this head (GitHub skips PR workflows on a conflicting PR), and the previous commit's E2E self-hosted run was cancelled, so the Windows E2E that was red on ebc26718 has not been re-run to a result. Fix: rebase, port, get a full CI run.
2. The release-guard test passes when its guard is broken. xtask/src/workflow_contract.rs:4246-4275 vs :4317-4350: the_release_feature_guard_catches_every_spelling re-spells the predicate in its own closure instead of calling the real loop. Ran in a scratch copy: with offenders.push made a no-op and the feature added to release.yml, both tests stayed green. Fix: extract one release_feature_offenders() and call it from both.
3. The new helper-spawn guard is untested and its comment is false. apps/rocm/src/serve_cmd.rs:954-966: deleting if !e2e_simulate_oom_launch() leaves serve-28 green and a rocm daemon process running (ran). The comment says the note "is computed before it would have run", but the helper call (line 965) comes before the note computation (line 986 onward). Fix: assert no daemon state or autostart claim remains after the scenario, and correct the comment.

Non-blocking

1. Note vanishes on one invalid UTF-8 byte (open from last round): serve_summary.rs:429-435, services_text.rs:1289-1303. The CLI uses strict lines() over the whole file; the engine uses lossy rocm_core::tail_lines. Ran: a stray byte on line 1 gave no note. Use the engine's reader.
2. "may still be loading" prints beside "ran out of GPU memory" (open): serve_summary.rs:166-185, main.rs:510-523. Ran. --help says the note is never shown for a still-loading server, but for vLLM that state is normally starting, which is eligible. The README also omits that the log is read once after a fixed 45 s wait.
3. OOM selector is slow on the failure path (new): vllm_oom.rs:96-100, 221-229, diagnose.rs:2268-2285. It runs the whole diagnose registry on two hosts per row. Measured by a review pass: 234 ms for 80 lines, 14.3 s for 6,000 \r rows. Pre-filter, or score only fix-16.
4. Stale docs: terminal.rs:13-15, 415 and docs/architecture.md:160 ("two callers", now three). The stripper's only production use is an unreachable fallback.
5. Fault-injection guard covers release.yml only, and docs/testing.md:1957-1961 lists the wrong test. Ran: adding the feature to the nightly publishing builds and the ci.yml bundle build kept all 76 contract tests green.
6. docs/testing.md:1979-1981 is wrong: the only CI dispatch filter runs a prebuilt binary, so name_filter=serve-28 runs without the hook.
7. Inaccurate or history-narrating comments:
   • capability.rs, expectation.rs, xtask/src/e2e.rs, the feature file and the README call the lane binary "the shipping release build" and name the wrong skip gate (requires-no-gpu is checked first).
   • lib.rs:272, fix.rs:3142, serving_steps.rs:1378 and e2e-report narrate history.
   • service_launch.rs:568 links to crate::append_oom_serve_note, which has moved to serve_summary.
8. Hint/command rendering is duplicated in process.rs:721-727 and serve_summary.rs:246-251, and the fixture hand-copies the engine's bail text. One shared function would make "same guard" true by construction.
9. Two tests claim more than they check: the last block of the_colourised_round_trip… tests a filter built inside the test, and the_cli_window_can_miss_an_oom… pins a defect with a hard-coded 77.
10. serve-28 never runs the printed rocm diagnose command (it does route to fix-16, checked by hand), and the "no note when piped" promise has no test.
11. PR body is partly stale: verification is pinned to 7bc22302, it says "re-verified" with no CI behind it, and it names an internal review tool.

Decisions for the author

• CLI window can miss an OOM the engine diagnosed (answered-by-intent): the description's "Known gap" says this is deliberate. Confirm the structured-record follow-up.
• No scenario for the engine's startup-failure text (answered-by-intent): the description's "Stated gap" names the lane and why it is not added. Confirm.
• A bare HIP error: out of memory no longer gets a hint (tradeoff): the engine hint and rocm diagnose stay in step, but the docs/vllm.md reason for dropping this string is unsupported. Confirm it was deliberate.

Since the last review

• Fixed: serve-26 renumbered to serve-28; append_oom_serve_note moved to serve_summary.rs; the false vllm_oom.rs doc paragraph; the process.rs comment path; the --help apostrophe caveat; the advised_commands.rs exclusion; the workflow comments now say "floor".
• Open: non-blocking items 1, 2, 4, 5, 8, 9, 10, 11, and the conflict/CI gap (now on a different file).

Deployment notes

• The rebase is real work: main also has quoting.rs and the AIM serve-26/serve-27.
• The squash-body note in the PR banner still applies.
• The engine's startup-failure header text changed.
• The e2e-oom-fault-injection feature is built only by cargo xtask e2e.

What this covered

Three independent passes read the PR diff, and I re-verified the key findings against the PR head. Ran in scratch copies: engine and serve-summary tests, xtask and contract tests, a hooked serve-28 run, clippy, and mutation edits.

Not run: the full workspace gate and smoke_local.py, Windows paths, and a history pass on pre-existing regions. The review threads were not read or reconciled. The fixes on this PR were written by its author.

[r0x0r]

@jussielo-amd — all three blocking items addressed. Pushed as e229c692.

1. The head cannot merge / no CI has run

Merged. main had moved the vLLM startup diagnostics into engines/vllm/src/startup_diagnostics.rs, so all 4 conflict hunks in process.rs had an empty main side — code moved, not deleted.

Resolved by comparing bodies rather than names, because name-presence is not body-equivalence here: of the ~32 same-named symbols in the conflicted regions, 3 genuinely differ and carry this PR's whole contribution — startup_log_context, oom_utilization_hint, import_failure_hint. Those three are ported over main's copies in startup_diagnostics.rs; the rest take main's. In particular startup_log_context's switch to rocm_core::terminal::rendered_lines survived — that is the fix for stripping welding Loading weights: 90%\rtorch.OutOfMemoryError… into one fake line that then got scored and quoted into the rocm diagnose --symptom command.

All 7 PR-authored tests in those hunks were relocated into startup_diagnostics.rs and verified present by name in the final tree.

2. The release-guard test passed when its guard was broken

You were right, and the mutation reproduces exactly as you described. Extracted one release_feature_offenders(&str) -> (usize, Vec<String>); both the_release_workflow_builds_no_feature_gated_binaries and the_release_feature_guard_catches_every_spelling now call it, so the spelling test exercises the real loop instead of a copy.

Proven falsifiable rather than asserted: with offenders.push made a no-op, the_release_feature_guard_catches_every_spelling now FAILS (assertion failed: caught("cargo build --release -p rocm --features rocm/e2e-test-hooks"), workflow_contract.rs:4351), and passes again on restore. Before this change the same mutation left both tests green.

3. The helper-spawn guard was untested and its comment was false

Both fixed.

serve-28 now ends with And no background automation daemon was left behind, which asserts neither artefact the autostart path can leave survives the fabricated launch: no running: true in automations/runtime-state.json, and no autostart.claim. Both are checked because the daemon publishes its runtime-state only once it is up, so a spawn caught in the window before that shows up as a claim and nothing else.

On the comment — you were right that it was backwards. The suppression runs before the note is computed in the summary_mode block, so "the note is computed before it would have run" could not have justified it. It now states the real justification: there is no spawned process to supervise, and serve-28's new assertion is what holds the branch.

Verification

Linux MI300X box, merged tree applied onto main (6eeec620):

• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo test --workspace --all-targets — exit 0, 77 suites, 0 failures
• cargo fmt --all -- --check — exit 0

cargo-clippy/cargo-test were skipped on the push only because engines/lemonade cannot compile on Darwin (st_mode is u16 there, u32 on Linux); both ran on Linux as above. Every other hook ran, including the signature/sign-off gate.

Scenario numbering

This branch still declares serve-28, deliberately. Sibling PR #703 is approved and declares serve-28..32, and feature_naming requires the number to equal the declaration position — so renumbering here before #703 lands would turn this branch's own check red. Once #703 merges, this becomes serve-33 on the next main merge. Flagging so it is not read as an oversight.

Your three decision questions — all confirmed deliberate

• CLI window can miss an OOM the engine diagnosed — deliberate, and the structured-record follow-up is the intended fix rather than widening the window here.
• No scenario for the engine's startup-failure text — deliberate; the lane and the reason are named in the description.
• A bare HIP error: out of memory no longer gets a hint — deliberate, so the engine hint and rocm diagnose stay in step. You are right that the docs/vllm.md reason is thin; the real reason is step-keeping, not the one stated there.

Still open from your non-blocking list

Not fixed in this round, and I would rather say so than imply otherwise: #1 (strict lines() vs the engine's lossy reader — a real correctness bug), #2, #5, #8, #9, #10 and the remaining parts of #11. #4 and #7 are partly done. Happy to take #1 in the next round since it is the only one with user-visible wrong behaviour.

Not verifiable here either way: the box has no amd-smi/rocm-smi and no GPU telemetry, so a real GPU OOM — this PR's subject — rests on fault injection and unit coverage, not hardware.

All three blocking items addressed in e229c69: the branch merges again (main moved vLLM startup diagnostics to startup_diagnostics.rs; 3 production fns ported, 7 tests relocated and verified present); the release-feature guard is now one extracted release_feature_offenders called by both tests, proven by mutation to fail where it previously stayed green; and serve-28 now asserts no daemon runtime-state or autostart claim survives, with the false ordering comment corrected. Non-blocking items 1, 2, 5, 8, 9, 10 and parts of 11 remain open and are listed as such in the comment. Re-requesting your review.

[r0x0r]

Merged main now that #703 has landed, so this branch's scenario finally renumbers: serve-28 → serve-33, with every reference that belongs to this branch moved with it (docs/testing.md, capability.rs, serving_steps.rs, serve_cmd.rs). #703's own cross-references to serve-28/30/31/32 are deliberately left alone.

The substantive overlap was the no-usable-GPU bail in serve(). #703 refactored it onto no_usable_gpu_refusal(&device_policy) and reuse_existing && host_has_no_usable_gpu; this branch adds !e2e_simulate_oom_launch() so the fault-injected launch is not refused before it can fabricate its OOM. The merged condition carries both terms. In docs/manual-testing.md both sides claimed ## 5, so the reuse section keeps it and the memory-guidance section becomes ## 6, with the sections after it shifted to 7..11.

One thing worth recording, because it nearly shipped: my first resolution of serving_steps.rs welded two unrelated step functions together, leaving the file syntactically broken. Neither cargo check -p e2e-cucumber --all-targets nor cargo test --workspace --all-targets caught it — the e2e test target sets harness = false and is excluded from the default test set, so it is not built by either. Only cargo fmt --check noticed. The correct gate is cargo clippy -p e2e-cucumber --test e2e, which is now part of this branch's verification and is green.

Verified on a Linux MI300X box with the merged tree applied onto main (c39ee0c6): cargo clippy --workspace --all-targets -- -D warnings exit 0; cargo clippy -p e2e-cucumber --test e2e -- -D warnings exit 0; cargo test --workspace --all-targets exit 0 across 76 suites with 0 failures; cargo fmt --all -- --check exit 0.

[jussielo-amd]

Thanks for the work on this, and for the last round. I confirmed the release-guard test now calls the real release_feature_offenders, and the helper-spawn guard has its serve-33 assertion. I read the whole diff at fe213ce7 from the code. Two things block, and both are cheap:

Blocking

1. Today's renumber merge left the docs wrong.
   • The existing reuse scenario is still serve-28 - Reuse succeeds on a GPU-less host in model_serving.feature (L395), but docs/testing.md now calls it serve-33 in several places (~L2063, 2068, 2072, 2093-94, 2114). So serve-33 names two different scenarios, and -n "serve-33 - Reuse..." selects nothing. The new OOM scenario is the real serve-33; the reuse text should be serve-28 again.
   • The new "Serve OOM Guidance" section is spliced into the middle of a sentence: L1993 ends "E2E scenarios, in" and the sentence only resumes at ~L2047, so the reuse scenario list now sits under the OOM heading. Moving the OOM block after the serve-31 bullet fixes it.
   • docs/manual-testing.md: ## 6 has subsections ### 5a/### 5b, and terminal.rs:203 cites "§5a". They should be 6a/6b and §6a.
   • Still open from last round: "The CI dispatch filter takes the same route" (~L2142) is wrong. The dispatch filter runs a prebuilt binary, so name_filter=serve-33 runs without the hook.
2. The branch conflicts with main again (crates/rocm-core/src/lib.rs; GitHub shows it as dirty). It needs another merge and a full CI run.

Non-blocking

• The two notes can still print together for the exact launch this targets: "may still be loading" (serve_summary.rs ~L166-182) beside "ran out of GPU memory". No test asserts the first is absent.
• serve_summary.rs's module doc still says it is engine-agnostic, but this adds ~600 lines of vLLM-specific OOM logic. AGENTS.md §6 points to its own file.
• Still open from last round: the OOM selector's cost on the failure path, the note vanishing on one invalid UTF-8 byte, and no test for the "no note when piped" promise.
• service_launch.rs:701 links crate::append_oom_serve_note; it now lives in serve_summary.
• The release guard misses PowerShell backtick continuations (release.yml has a shell: pwsh job), counts each run: | build twice and doesn't strip comments.
• docs/architecture.md still says the terminal stripper has "two callers" and describes startup_diagnostics.rs as stripping the tail.
• The fixtures hand-copy the engine's bail header, so serve-33 would stay green if the real text drifts.

CI: hosted checks are green; the Strix Halo Windows lane is red (it fails on unrelated PRs too) and MI300X is still queued. Not run: tests (read-only review).

---

🤖 by agenthub on AMD AgentHub

[r0x0r]

Merged main again. Pushed as cd4f51f2.

The scenario renumbers again: serve-33 → serve-40. main has since added its own serve-33/34/35 (the native-Windows vLLM refusals) and now runs to serve-39, so this branch's OOM scenario takes the next free number. Its own references moved with it (docs/testing.md's OOM section, capability.rs, serving_steps.rs, serve_cmd.rs); main's serve-33/34/35 references in README.md, docs/testing.md and workflow_contract.rs are deliberately left alone.

This is the second renumber this branch has taken, both forced by scenarios landing on main first. feature_naming requires the number to equal the scenario's declaration position, so it cannot be pinned in advance — it is settled by merge order, not by choice.

Two other conflicts:

• main pluralised engine_env_note to engine_env_notes, so the OOM note block now does notes.extend(engine_env_notes.iter().cloned()).
• The release-feature guard doc and main's new comfyui/Windows paragraph in workflow_contract.rs are both additive; both kept.

Verified on a Linux MI300X box with the merged tree applied onto main (e6d2f7c0):

• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo clippy -p e2e-cucumber --test e2e -- -D warnings — exit 0
• cargo test --workspace --all-targets — exit 0, 74 suites, 0 failures
• cargo fmt --all -- --check — exit 0

The --test e2e clippy is in the gate deliberately: that target sets harness = false and is excluded from the default test set, so cargo test --workspace --all-targets does not build the step files at all. A syntactically broken serving_steps.rs passed both the workspace test run and cargo check --all-targets earlier in this branch's history and was caught only by cargo fmt --check.

[jussielo-amd]

Thanks Roman, the blockers from my last round are fixed in cd4f51f2: the head merges cleanly, the release-guard test calls the real function, serve-40 asserts no daemon is left behind, and the docs are consistent again (reuse scenario back to serve-28, OOM section no longer splices into the reuse list, 6a/6b and §6a). Hosted CI is green. The one red hosted check, remote control channel (containerised), failed on a rustup DNS lookup, so it needs a re-run and is not caused by this change. Approving.

Non-blocking, roughly in order of user impact:

• UX: for a vLLM launch that OOMs, the summary prints "the server did not report ready ... it may still be loading" (serve_summary.rs:166-182) and then "ran out of GPU memory". Suppressing the first line when the OOM note applies would avoid the mixed message. No test asserts they don't appear together.
• UX: the note disappears if the log holds one invalid-UTF-8 byte (strict lines() vs the engine's lossy reader). You mentioned taking this next, so good. Also worth a line in the README: the check is a single read after the ~45 s startup wait, so a later OOM only shows in rocm services logs.
• --help (main.rs:554-557) says a colourised line falls back to the canonical symptom, but the code, README and docs/vllm.md quote the rendered row. Narrowing it to the apostrophe case would match.
• The startup-tail echo expands every \r repaint into its own row (startup_diagnostics.rs:58-62), and the OOM scorer runs the diagnose registry per row. A progress-bar-heavy failure could print or spin a lot. Capping the echoed rows and pre-filtering on "memory" would bound both.
• The hint placeholder <fraction greater than 0 and at most 1> can't be pasted as-is and differs from --help's FRACTION. Putting the bound in prose would be friendlier.
• Small: the merge garbled two doc comments in workflow_contract.rs (~4216-4219 and ~4383-4385), and the serve-40 skip reason says the hook is "compiled out" when the env var simply isn't set.

---

🤖 by agenthub on AMD AgentHub

[r0x0r]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved and pushed: the conflict resolution is in 8dfd8a4, with the latest-main update in 33d5a84. Main’s no-helper scenario remains serve-40; the OOM scenario is now serve-41, with its references updated.