feat(vllm): Tackle out of memory errors (EAI-8058)

[r0x0r]

This pull request improves GPU selection and user guidance for ROCm and vLLM, especially in shared or containerized environments where the standard amd-smi tool may not be available. It adds a fallback for GPU VRAM telemetry, enhances user warnings and hints for out-of-memory (OOM) conditions, adds the vLLM startup OOM to the diagnose/fix catalog, and ensures consistent messaging across CLI and engine surfaces.

GPU selection and VRAM telemetry improvements:

• Added a fallback to read per-GPU VRAM usage from the amdgpu DRM sysfs counters (/sys/class/drm/card*/device/mem_info_vram_{total,used}) when amd-smi is not available, so --gpu auto ranking and low-VRAM warnings still have telemetry on a single-AMD-GPU host without amd-smi. The fallback deliberately withholds telemetry on a host with more than one AMD DRM card, because card<N> numbering is not guaranteed to match HIP's device ordinal there — so it is a single-GPU convenience, not a multi-GPU replacement.
• Explicit --gpu <index> validation was reworked on this branch to check membership in the KFD/DRM usable set rather than a device count. That is superseded by fix(serve): close serve races and translate GPU selection through visibility masks (EAI-7194) #267, which landed a validator consulting the visibility-resolved set directly (and with mask-aware wording), and the merge below drops this branch's version in favour of it — the two answered the same question, and fix(serve): close serve races and translate GPU selection through visibility masks (EAI-7194) #267's answer is the stronger one. The DRM sysfs VRAM rows are never used as a --gpu <index> bound; they stay scoped to --gpu auto ranking.
• --gpu auto ranks the actual reported VRAM row indices, which can be non-contiguous under a visibility mask, rather than a synthetic 0..count range that would look up absent rows and fall through to a bogus GPU 0.

User guidance and warnings for vLLM:

• Introduced a shared constant VLLM_GPU_MEMORY_UTILIZATION_HINT for the recommended workaround when running out of memory on a shared/busy GPU, ensuring CLI and engine logs use consistent wording.
• The serve summary prints a note about the --gpu-memory-utilization workaround when vLLM is selected and the GPU is busy, both interactively and in the deployment summary.
• vLLM engine startup logs append the same utilization hint if an OOM is detected.

vLLM startup OOM in the diagnose/fix catalog (EAI-8060):

• Added a keyword-scored OOM signature (check_16_vllm_oom) plus a print-only fix-16-vllm-oom recipe. The remediation stays conditional — it distinguishes a tenancy collision with vLLM's fixed ~90% VRAM reservation from a model that genuinely does not fit — rather than prescribing a lower --gpu-memory-utilization unconditionally.
• Only the lines carrying a vLLM anchor (vllm / gpu_memory_utilization) are scored — never the whole pasted log — so an incidental vllm mention cannot misattribute another framework's OOM. The anchor is not a boolean gate: because regex matching ignores line boundaries, gating on the anchor and then scoring the whole symptom still counted keyword hits from unanchored lines at full weight (a benign gpu_memory_utilization=0.9 config echo plus an unrelated torch.OutOfMemoryError: CUDA out of memory on the next line reached 95, above HIGH_CONFIDENCE). The same-line requirement now falls out of the scoring itself: an anchored line with no OOM token contributes nothing. Pinned by only_the_anchored_lines_are_scored_not_the_whole_paste.
• gpu_memory_utilization remains an anchor even though it is also a 20-point scoring entry: with only anchored lines scored, a bare flag echo (a config dump, or a paste of rocm-cli's own low-VRAM hint, which prints --gpu-memory-utilization) is worth 20 — far below MIN_SCORE_FOR_MATCH — so it can surface as a weak signal but can never carry a verdict, nor lend its anchor to an OOM elsewhere in the paste (rocm_cli_s_own_low_vram_hint_is_never_a_verdict_on_its_own).
• The vLLM OOM table alone is scored through keyword_score_collapsing_overlaps, which de-duplicates overlapping match spans, so one phrase yields one evidence bullet and one weight (HIP out of memory is not also counted as the nested out of memory). The rest of the catalog keeps the default "every matching entry is an independent signal" scoring: several older tables pair a greedy .* pattern with a second, independent keyword that the greedy span swallows, and collapsing there would push real diagnoses below MIN_SCORE_FOR_MATCH.
• The engine's printed rocm diagnose --symptom '...' is decoupled from its own coarse detector: it consults vllm_oom_symptom_is_diagnosable and falls back to VLLM_OOM_CANONICAL_SYMPTOM when the user's real line would score sub-threshold, so the command it hands the user always reports a cause. The user's actual failing line stays visible in the human-readable message.
• The diagnosis-embedded Fix and the rocm fix catalog are cross-checked for shared fix ids, so the two surfaces cannot silently drift.
• On WSL, suppression of the routing note is gated on has_match (a checker actually cleared MIN_SCORE_FOR_MATCH), not on matched being non-empty, so a weak sub-threshold signal cannot bury the WSL setup guidance.

Documentation:

• Updated docs/vllm.md, README.md, and skills/rocm-cli-assistant/SKILL.md to explain the behavior on shared/busy GPUs, the single-GPU-only telemetry fallback, the rocm diagnose --symptom pointer, and the recommended OOM workaround.

Behavior coverage (e2e) — per AGENTS.md §3, with the lane named for each:

@id:	Behavior	Tags / lane
diagnose-vllm-oom-is-conditional (diagnose-20)	A vLLM startup OOM is diagnosed, its remedy distinguishes a busy GPU from a model that does not fit, and the rocm fix <id> the report names actually runs on the host that produced it	@requires-os:linux — runs on every Linux lane including the GitHub-hosted mock lane; WSL2 matches this tag, so the self-hosted e2e-wsl lane is the one that can observe the platform-gate half
serve-vllm-low-vram-oom-guidance (serve-22)	The serve plan warns the pinned GPU is low on VRAM and explains vLLM's total-VRAM reservation, not merely the flag name	@requires-gpu @requires-engine:vllm @requires-os:linux — vLLM GPU hardware lane only
serve-absent-gpu-index-rejected (serve-16)	An explicit --gpu <index> that is not usable is rejected outright, never silently remapped — under either validation authority	@requires-gpu @requires-os:linux — GPU hardware lanes only

Why the two GPU-gated ones cannot run on the mock lane: on a no-GPU host the GPU-required pre-flight refuses with "no usable AMD GPU" before an index is ever validated or a plan is ever built, so neither the index-specific rejection nor the pre-launch VRAM note is observable there. serve-22 additionally injects its near-full reading through the e2e-test-hooks-gated ROCM_E2E_FORCE_LOW_VRAM=<ordinal> seam, because the GPU lane's real cards are comfortably free; the engine still launches against the real device. The low-VRAM seam is unit-tested on every lane by forced_low_vram_hook_trips_the_serve_plan_warning.

A coverage gap this branch had stated is now closed by #267. The masked sub-case of --gpu validation (a visibility mask leaving a non-contiguous usable set) had no scenario of its own here, because it was reachable only where amd-smi is absent. #267 revalidates against the visible set unconditionally and ships serve-19 (serve-masked-gpu-index-rejected) and serve-20 (serve-rocr-reindexed-gpu-index-rejected) for it, so the gap and the serve-16 comment that recorded it are both gone.

serve-22's two Then steps divide the work deliberately: the warning step is the scenario's premise (the low-VRAM warning predates this change) and pins only that the warning names the GPU the serve was pinned to; the memory-knob step is the verdict and requires the note's own reservation rationale, because matching the bare flag name would also be satisfied by a recipe echo of a user-supplied --gpu-memory-utilization. Both literals are additionally pinned in the always-run unit test serve_notes_pair_low_vram_with_the_vllm_utilization_hint, so a rewording fails on every lane rather than silently on the one lane that can observe the scenario.

Review follow-ups (2026-09-11).

• fix-16-vllm-oom now applies on WSL2. check_16_vllm_oom is registered for ["linux", "wsl"], but the recipe was LINUX_ONLY, and rocm fix re-gates a fix-id on the recipe's list against the running OS — where WSL2 is its own family. So rocm diagnose printed apply with: rocm fix fix-16-vllm-oom and that command then printed the plan and exited 3 with "This fix only applies on: linux. Running OS is: wsl.", leaving the supported way to serve from a Windows host with a diagnosis and no fix path. Nothing about the remediation is bare-metal — it is vLLM CLI flags. The invariant is now asserted for the whole catalog by every_checker_platform_is_covered_by_its_recipe, which requires every platform family a checker answers on to be one its recipe will act on (containment, not equality: rocm fix <id> is reachable without a diagnosis). It surfaced no other mismatch across the remaining 24 entries (25 in the catalog, recounted after the merge).
• The shared --gpu-memory-utilization hint said 0.1 while every command said 0.5. VLLM_GPU_MEMORY_UTILIZATION_HINT is printed verbatim by the serve low-VRAM note and the vLLM post-failure hint, and interpolated into the fix-16-vllm-oom diagnosis summary — so a single rocm diagnose printed "e.g. 0.1 for a small model" directly above rocm serve <model> --gpu-memory-utilization 0.5. The const is now 0.5, and the_utilization_hint_example_matches_the_recipe_command pins the hint's worked value to the value the recipe commands. Neither existing guard covered it: the diagnose/fix cross-check exempts the summary as prose framing, and a line-based grep misses the const because the flag and the value sit on different continuation lines.

Review follow-ups (2026-09-14).

• The rocm diagnose --symptom '...' command the OOM hint prints is no longer built from unescaped subprocess output. The vLLM engine routed the user's actual failing line — taken verbatim from the engine's own startup-log tail — into a single-quoted shell argument in a message that invites the user to paste it. A single apostrophe, routine in Python error text (can't allocate, model 'foo'), closed the quote, and a line only has to mention running out of memory to be selected; the neighbouring human-readable echo passed the same bytes through including ANSI. A line that cannot be rendered as one intact single-quoted word is now treated as unusable and takes the VLLM_OOM_CANONICAL_SYMPTOM fallback that already existed for exactly that case, rather than being escaped: escaping keeps the exact bytes but yields a command a reader cannot check by eye, and a wrong escape is runnable and misleading instead of obviously broken, while control bytes would still reach the terminal. The user's own line stays visible in the human-readable sentence (and in the log tail printed with it), now stripped of ANSI sequences and control bytes. Pinned by an_apostrophe_in_the_failing_line_cannot_break_out_of_the_printed_command and control_bytes_from_the_log_are_stripped_from_the_echoed_line, both of which fail on the previous code.
• DiagnoseReport::out_of_scope's doc comment matched the code again. It had been changed to assert that matched may hold sub-threshold entries alongside the field and that the field is set when nothing clears MIN_SCORE_FOR_MATCH. Neither is true: diagnose clears matched whenever out_of_scope is set, and the field is decided purely by catalog_covers — platform coverage, never a score. The (e.g. WSL2) example can no longer occur at all now that WSL2 is a covered platform. Since this is the serialized JSON contract, a consumer trusting it would wait for rows that can never arrive. an_out_of_scope_report_never_carries_matched_entries now pins the relationship across all four platform families.

Coverage note for the first item. The engine's post-failure OOM hint is only emitted when a real vLLM server process fails to start on a GPU host with an out-of-memory error, which no CI lane can produce (the GPU lanes' cards are free, and serve-22 forces only a low-VRAM reading, not a failed launch). The behaviour is therefore covered by the two unit tests above, which run on every lane, and the gap is stated here per AGENTS.md §3 rather than papered over with a scenario that cannot run. diagnose-20 continues to cover the rocm diagnose side, which is unchanged.

These improvements make GPU selection more robust in diverse environments and provide clear, actionable guidance to users encountering memory issues with vLLM.

Merge with main + a blocking fix (2026-09-15).

main moved under this branch (#331, #327, #267), leaving six conflict hunks — five in apps/rocm/src/main.rs, one in model_serving.feature. Merged (never rebased).

• --gpu validation. This branch's resolve_pinned_gpu_index / validate_pinned_gpu_index_against are dropped in favour of fix(serve): close serve races and translate GPU selection through visibility masks (EAI-7194) #267's validate_pinned_gpu_index, which consults the visibility-resolved set and words its refusal differently depending on whether a mask is actually set. Keeping both would have been two validators for one question. effective_gpu_count survives: it is what lets --gpu auto rank devices when amd-smi is absent but the DRM sysfs fallback produced rows.
• select_auto_gpu_index is a genuine union. Candidate ordinals come from the actual VRAM rows (this branch — a mask makes reported ordinals non-contiguous), and are then narrowed to the visible set (fix(serve): close serve races and translate GPU selection through visibility masks (EAI-7194) #267 — never target a masked-out device). Neither side alone is correct after the merge.
• Scenario index collision. main published serve-19/20/21 and they are cross-referenced from src/expectation.rs and from each other, so they keep their numbers; this branch's low-VRAM OOM scenario shifts to serve-22. Its @id: is unchanged, so expectations.toml is unaffected.
• Three defects the textual merge produced with no conflict markers, all caught by compiling: a validate_pinned_gpu_index tail that kept this branch's usable bail inside fix(serve): close serve races and translate GPU selection through visibility masks (EAI-7194) #267's function body; a vec![all[0]] fallback whose all binding lived on the other side of the hunk; and three select_auto_gpu_index call sites still on the old arity. A fourth was marker-free and compiled: a serve-16 comment naming a unit test the merge had deleted.

Blocking fix: the all-busy auto-selection fallback returned a GPU that was never reported. Once --gpu auto candidates come from the VRAM rows, the terminal "every candidate is busy, pick one anyway" fallback could no longer return a hardcoded 0. On the base branch that was safe by construction (candidates were 0..count, so ordinal 0 was always real). With rows reported at [2, 3] and both pinned by a managed service the candidate list empties, all three passes fall through, and the function returned [0] — an ordinal absent from the reported set, exported verbatim as HIP_VISIBLE_DEVICES. Under the strict GPU-required / no-silent-fallback policy that is a silently wrong pin, not a fail-fast. The fallback now returns the lowest ordinal that was actually reported, and the adjacent comment (which claimed "Every detected GPU is pinned... We still return GPU 0") is corrected. This is a fabrication removed, not a policy change: "all busy, pick one anyway" still holds, and rocm serve refuses in exactly the same situations as before.

auto_selection_all_reported_busy_falls_back_to_a_reported_ordinal covers it. The pre-existing auto_selection_ranks_the_actual_reported_row_indices claimed pass 3 "never returns a fabricated index 0" but only exercised the partially busy case, which never reaches the fallback. Falsified: reverting the fallback to vec![0] fails the new test with left: [0], right: [2]; restoring it passes. Equality assertion, not fragment absence.

Why no new scenario for the fallback (AGENTS.md §3). Reaching it needs every GPU the host reports to be simultaneously pinned by a live rocm-cli managed service, on a host whose reported ordinals are sparse. No lane can arrange that without a seam that would make the scenario assert against the seam rather than the product, and a serve under those conditions is a deliberate over-commit that the low-VRAM warning already reports. The selection function is pure and injectable, so the unit test covers the decision exactly; what is not covered live is the sparse-ordinal host itself, which is the same hardware gap #267 records on serve-20.

Review follow-ups (2026-09-17).

• rendered_lines's never-merge guarantee is qualified where a consumer reads it. The doc stated it as an unqualified absolute, while the one counterexample was written down in a body comment inside the private next_token — so it reached no generated documentation — and the public doc pointed at it with See [next_token], a link to a private item that rustdoc will not resolve. The exception now has a single authoritative statement in the module documentation, which is public and rendered: a line break inside a well-formed string-argument body (OSC, DCS, SOS, PM, APC) is consumed with that body, worked example included. rendered_lines carries the qualifier inline so the only absolute a consumer can read is true, and links to the module docs instead. The behaviour is unchanged; following the grammar rather than guessing where an unterminated body ends is still the right call. a_line_break_inside_a_well_formed_string_body_is_consumed_with_it pins the worked example, plus the two shapes that bound the hole (the same break outside a string body still splits; an unterminated body still stops at the following ESC). cargo doc -p rocm-core --no-deps warned on the dangling link and goes from 6 warnings to 3 with this change — the remaining three are pre-existing on main.

Docs scope for this PR. docs/testing.md and docs/manual-testing.md are deliberately untouched. Neither file itemises fix IDs or diagnose signatures today — docs/testing.md documents how to run the suites and docs/manual-testing.md walks the install/serve/dash flows — so the three user-observable additions here (the fix-16-vllm-oom catalog entry, the OOM diagnosis, and the pre-launch low-VRAM serve warning) have no row to update in either. The user-facing surfaces that do describe them — README.md, docs/vllm.md, and skills/rocm-cli-assistant/SKILL.md — are updated in this PR, and each behaviour is covered by the @id:-tagged scenarios tabulated above. (Worth noting precisely: this branch's AGENTS.md carries no unconditional same-change docs rule. Its only such requirement is the scenario rule — "User-observable behavior needs a scenario, not only a unit test" — which this PR discharges in the table above. main's copy has since grown a stronger clause, "when a command's flags, defaults, arguments, or observable behavior change, update README.md, its --help/doc comment, docs/testing.md, and docs/manual-testing.md in the same change"; that text is not in this branch's AGENTS.md, and it will apply on the next merge from main, at which point the reasoning above is the answer: no flag, default or argument changed, and neither file states a behaviour claim this PR falsifies.)

On the three effective_gpu_count assertions (apps/rocm/src/main.rs): they are characterisation tests. They are revert-sensitive to the filter(|&count| count > 0) guard, but that guard predates this branch and this PR's own change to the function is doc-only — so they close a pre-existing coverage gap on a function the merge kept, rather than pinning behaviour this PR introduces.

[volen-silo]

A few observations from a read of this change:

• No Gherkin scenario covers the new user-visible behavior (the extra serve note, --gpu auto picking from the sysfs fallback); AGENTS.md §3 asks for one or a stated reason. The unit tests cover helpers rather than behavior.
• validate_pinned_gpu_index still sees only the amd-smi count, so on the exact environment this targets (no amd-smi, sysfs works) an out-of-range --gpu <n> is accepted and fails later in the engine.
• read_sysfs_u64(...mem_info_vram_used).unwrap_or(0) makes a card with an unreadable used counter look 100% free — which is what --gpu auto prefers first.
• read_drm_vram_usage orders by ascending card<N>; an AMD APU passes the same vendor + mem_info_vram_total filter, so on APU + dGPU the ordinal can diverge from HIP's, and it feeds HIP_VISIBLE_DEVICES directly.
• docs/vllm.md: the fallback probes amd-smi only, never rocm-smi.

On the stack: #284's diff against this branch removes the sysfs fallback, its tests and the docs bullet — looks unintended.

[r0x0r]

Thanks for the detailed review — pushed a fix commit (a97a39f) addressing the concrete bugs:

• validate_pinned_gpu_index only saw the amd-smi count: resolve_gpu_indices now derives an effective_gpu_count that falls back to the DRM sysfs row count when amd-smi is unavailable, so an out-of-range --gpu <n> is rejected up front on exactly the environment this PR targets (no amd-smi, sysfs works), instead of failing later inside the engine.
• read_sysfs_u64(...mem_info_vram_used).unwrap_or(0): a card whose used counter can't be read is now skipped entirely rather than treated as 0 bytes used (100% free).
• read_drm_vram_usage ordinal divergence on APU + dGPU: ascending card<N> order only mirrors HIP's KFD-topology ordinal when there's exactly one AMD DRM card. The function now returns no rows at all when more than one AMD card is found, instead of guessing an ordinal that could feed the wrong device into HIP_VISIBLE_DEVICES.
• docs/vllm.md: fixed — the fallback only ever probes amd-smi, never rocm-smi; wording corrected.

Added unit tests for all four (effective_gpu_count_*, resolve_gpu_indices_rejects_out_of_range_index_from_sysfs_fallback_count, read_drm_vram_usage_skips_a_card_with_an_unreadable_used_counter, read_drm_vram_usage_withholds_telemetry_when_multiple_amd_cards_are_present).

On the Gherkin scenario: I didn't add one for the "no amd-smi, sysfs fallback" path — there's no runner in the current fleet shaped like that (real GPU hardware with amd-smi absent), so I couldn't author or verify a @requires-gpu scenario for it. The corrected behavior is covered at the unit level instead (pure functions, planted sysfs fixtures). Happy to add e2e coverage if/when a suitable lane exists.

On #284: worth double-checking before merging that stack — its diff against this branch appears to drop the sysfs fallback, its tests, and the docs bullet, which does look unintended given this PR is what introduces them.

[volen-silo]

Round 2. Items 3 and 5 are genuinely fixed. Items 2 and 4 are fixed in intent, but the implementations leave a hole and add a new regression.

• The multi-card guard counts surviving rows, not AMD cards found. In read_drm_vram_usage, three continues drop a card before cards.push(...) — vendor mismatch, missing mem_info_vram_total, missing mem_info_vram_used (the new item-3 skip). if cards.len() > 1 { return Vec::new() } then runs on the already-shrunk vector. On a genuine 2-AMD-card host where one card is filtered out, the guard doesn't fire and the survivor is assigned ordinal 0 regardless of its real card<N> — which feeds HIP_VISIBLE_DEVICES. That is the APU+dGPU misattribution the guard exists to prevent, and it's worst in exactly the case the doc comment reasons about (an APU whose mem_info_vram_total is absent gets dropped, the dGPU becomes "ordinal 0"). read_drm_vram_usage_skips_a_card_with_an_unreadable_used_counter uses a single-card tree, so it can't catch it. Count AMD cards in a separate counter incremented before the telemetry filters.

• effective_gpu_count turns best-effort telemetry into a hard input-validation bound. A sysfs row count of 1 now hard-rejects --gpu 1..n at the CLI. Two ways that refuses a legitimate index: via the bug above (transient used read failures shrink the count), and via DRM-vs-KFD divergence — combine_amd_gpu_counts exists for precisely this and its own test asserts combine_amd_gpu_counts(Some(3), Some(1)) == Some(3) ("KFD larger than DRM, e.g. multi-partition compute nodes"). usable_amd_gpu_indices() is the amd-smi-independent authority here and is already used for this class of check in main.rs:4858, engines/vllm/src/lib.rs:1122, engines/lemonade/src/lib.rs:3784,3826. Deriving the count from it would also keep read_drm_vram_usage's withholding scoped to VRAM ranking rather than disabling the feature outright on multi-GPU hosts.

• resolve_gpu_indices_rejects_out_of_range_index_from_sysfs_fallback_count doesn't call resolve_gpu_indices. It hand-composes validate_pinned_gpu_index(1, effective_gpu_count(None, Some(&single_gpu))). Reverting the production fix leaves it green. resolve_gpu_indices has no test caller at all.

• On the Gherkin decline: agreed for the low-VRAM note: line — the harness has only tag-based host selection plus HIP_VISIBLE_DEVICES masking, no amd-smi shim or fixture sysfs tree. But AGENTS.md §3 wants the reason in the PR text, and @id:serve-absent-gpu-index-rejected (model_serving.feature:152) already covers the out-of-range rejection this PR changes behavior for — §3 asks that be named rather than silently relied on. Body edit, no code change.

Smaller things:

• After the guard, cards.sort_by_key(...) + .enumerate() can only ever see 0 or 1 elements — the sort, the tuple sort key and the "Placeholder ordinal" comment are dead, and the doc comment's "assigns sequential 0-based ordinals in ascending card order" is not what the code does.
• drm_device_is_amd checks vendor == 0x1002 only; rocm_core::is_amdgpu_device checks vendor or uevent containing DRIVER=amdgpu. The weaker one feeds the guard. The card walk also duplicates linux_drm_amdgpu_card_count line for line — both rocm-core helpers are private, so a shared exported enumerator would be better than a third, weaker predicate.
• README.md:353 and skills/rocm-cli-assistant/SKILL.md:30 both still describe amd-smi as the only VRAM source.
• docs/vllm.md:132 says the note prints "before launch" — true for the plain path, but in summary mode collect_serve_notes runs after start_managed_service and the smoke test. And the --gpu 3 example at :145 sits under the paragraph explaining the fallback only works single-GPU, where --gpu 3 is now rejected.
• PR body says the fallback works "in stripped-down containers and shared nodes", but it yields nothing on any host with >1 AMD DRM card; docs/vllm.md:129 states the real scope correctly.
• #[allow(clippy::too_many_arguments)] on collect_serve_notes at 8 positional args — a params struct would read better at the 4 call sites.

Untested: the gpu_vram_usage() amd-smi→sysfs chaining itself, the plain-path note: print, non-numeric sysfs contents.

[r0x0r]

Thanks for the Round-2 review — pushed 3dcaa7b addressing it.

Multi-card guard counted surviving rows, not AMD cards found. Fixed. read_drm_vram_usage now increments a dedicated amd_cards_found counter the moment drm_device_is_amd returns true — before any telemetry (readable-counter) filtering — and withholds all rows when amd_cards_found > 1, so a second AMD card that yields no counter still suppresses the potentially-misnumbered telemetry. New test: read_drm_vram_usage_withholds_telemetry_when_a_second_amd_card_has_no_counter.

effective_gpu_count hard-bounded --gpu from best-effort telemetry. Fixed. --gpu validation now goes through pinned_index_bound, which prefers the amd-smi count and falls back to rocm_core::usable_amd_gpu_indices().map(|i| i.len()), never the VRAM-row count. effective_gpu_count is now used only for auto-ranking (doc updated). New tests: resolve_pinned_gpu_index_validates_against_the_detected_count, pinned_index_bound_prefers_the_amd_smi_count.

The "rejects out-of-range index" test didn't call resolve_gpu_indices. Fixed by extracting resolve_pinned_gpu_index(index, detected) (the Index arm) and testing it directly, replacing the old test that never exercised the path.

Body should name the declined scenario + its Gherkin reason. Done in the body: @id:serve-absent-gpu-index-rejected's premise (an index-specific rejection) can only be observed on a real GPU host, since a no-GPU host refuses at the GPU-required pre-flight first.

Smaller items: removed the dead sort/enumerate tail after the guard; drm_device_is_amd now matches rocm_core::is_amdgpu_device (vendor 0x1002 or uevent DRIVER=amdgpu); README and the assistant SKILL now mention the DRM sysfs VRAM fallback.

[volen-silo]

Round 3. The multi-card guard (A), the dead sort/enumerate tail, drm_device_is_amd, and the README/SKILL updates are all genuinely fixed — verified against source, not just the reply. Two things still block.

Blockers

1. apps/rocm/src/main.rs:16751-16756 — pinned_index_bound collapses usable_amd_gpu_indices() to .len() and uses it as an upper bound. That function returns absolute, unrenumbered ordinals after applying the visibility mask — crates/rocm-core/src/lib.rs:11548 asserts usable_amd_gpu_indices_from(4, Some("2,0")) == Some(vec![2, 0]). So on a 4-GPU host with HIP_VISIBLE_DEVICES=2 and no amd-smi (detected == None — exactly this PR's target environment), pinned_index_bound yields Some(1) and --gpu 2 is hard-rejected as "out of range", even though 2 is the only usable device. The mirror case also holds: with usable == [2, 3], --gpu 0 passes CLI validation for a device that isn't visible at all.

   This is a regression: pre-PR, detected == None meant no CLI-side validation, and the request reached the engine gate, which does it correctly — engines/vllm/src/lib.rs:1139-1150 checks usable.contains(index), and engines/lemonade/src/lib.rs:3784 does the same. main.rs:16754 is the only call site in the repo that reduces that API to a length. The check wants membership, not a count.

2. apps/rocm/src/main.rs:23962-23967 — pinned_index_bound_prefers_the_amd_smi_count only exercises Some(4) / Some(0); nothing calls pinned_index_bound(None). The .or_else fallback is the entire behavior this round added, and it's the branch carrying the bug above. As written it isn't injectable (it calls the real probe), so testing it means splitting a pinned_index_bound_from(detected, usable) seam — the same shape resolve_gpu_indices_against already uses in the engines. Same gap as round 2's item 3, one layer down.

3. AGENTS.md §3 — the reply says the Gherkin decline reason and @id:serve-absent-gpu-index-rejected went into the PR body, but the body is unchanged from the original. §3 wants the named scenario and the gated-lane reason in the PR text. Body edit, no code change.

Nits (non-blocking)

• docs/vllm.md:133 says the note prints "before launch". True on the plain path; in the default interactive summary mode collect_serve_notes runs at main.rs:5058, after start_managed_service and the smoke test.
• docs/vllm.md:145 — the --gpu 3 example still sits under the paragraph explaining the fallback withholds telemetry on multi-GPU hosts, so the reader gets a 4-GPU example in the section about the case where none of this telemetry exists. Both new examples also drop --engine vllm, unlike every other bash block in the file.
• PR body still claims the fallback makes things work "in stripped-down containers and shared nodes" and that "the device count for selection now also derives from these sysfs rows" — the first yields nothing on any host with >1 AMD DRM card, and the second is now auto-ranking only.
• read_drm_vram_usage's card walk is still a line-for-line third copy of the enumeration in rocm_core::linux_drm_amdgpu_card_count + is_amdgpu_device. A single exported enumerator would keep the two from drifting again.
• log_tail_shows_oom matches "out of memory" anywhere in the 80-line tail, so an unrelated failure whose tail happens to mention memory gets the hint appended. Low impact — the hint is additive — but it isn't guarded.

cargo fmt and cargo clippy --workspace --all-targets are clean; -p rocm --bins and -p rocm-engine-vllm --lib pass (one unrelated flake in providers::tests under parallel run, passes in isolation, untouched by this PR).

[r0x0r]

Thanks for the Round-3 review — pushed 59c7f31 (rebased onto the current branch tip, which now carries #290's squash; one docs/vllm.md example-block conflict resolved to keep both your diagnose line and the --engine vllm fix).

Blocker 1 — the fallback reduced the usable set to a length. Fixed at the layer you named. The count bound is gone; --gpu <index> now validates membership. When amd-smi is present its renumbered 0..count is still the authority (index >= count → out of range); when it's absent, the index must be a member of usable_amd_gpu_indices() — the absolute, unrenumbered ordinals the engine gate already trusts. On your 4-GPU / HIP_VISIBLE_DEVICES=2 case, --gpu 2 (usable [2]) now passes and the mirror case — a hidden --gpu 0 with usable == [2, 3] — is rejected, with a message that names the usable set and the visibility vars. An empty usable set is deliberately not authoritative here (the no-usable-GPU fail-fast owns that message), so it doesn't reject.

Blocker 2 — the fallback branch wasn't injectable. Split the seam you asked for: validate_pinned_gpu_index_against(index, detected, usable). resolve_pinned_gpu_index supplies the real usable_amd_gpu_indices().as_deref(), and the new validate_pinned_gpu_index_falls_back_to_usable_set_membership test drives the detected == None branch directly (sole-usable passes, hidden rejected, empty set allowed through, amd-smi count still wins). The pre-existing tests now go through the same seam.

Blocker 3 — scenario + gated lane in the body. Done in the PR body (not just this comment). The user-observable "explicit --gpu <index> is rejected outright, never remapped" behavior is @id:serve-absent-gpu-index-rejected (Scenario 13, tests/e2e-cucumber/features/model_serving.feature), tagged @requires-gpu @requires-os:linux: on a no-GPU host the GPU-required pre-flight refuses with "no usable AMD GPU" before the index is ever validated, so the index-specific rejection is only observable on the GPU hardware lanes. The membership refinement is unit-covered on every lane.

Nits. Fixed the two docs/vllm.md inaccuracies (the "before launch" timing now also names the interactive post-readiness path; both workaround examples gained --engine vllm, and the stray 4-GPU --gpu 3 example is now --gpu 1). Corrected the PR-body overclaims you flagged — "stripped-down containers and shared nodes" is now scoped to single-GPU, and the "device count for selection derives from sysfs rows" line is gone (that path is auto-ranking only). Left read_drm_vram_usage's card-walk dedup and the broad log_tail_shows_oom match as noted-but-deferred; happy to fold the DRM enumerator into rocm_core in a follow-up if you'd rather have it here.

Verification note: the full cargo test/clippy workspace build can't run on my macOS box — an unrelated st_mode u16/u32 libc mismatch in engines/lemonade, and macOS is unsupported per AGENTS §6 — so the Linux CI lanes are the authoritative gate for the compiled and e2e checks. cargo fmt and rust-analyzer are clean locally.

[rominf]

This is a well-tested change and the core GPU-selection/validation logic (the amd-smi-independent membership check, the DRM sysfs fallback and its multi-card guard, the conditional OOM diagnosis) looks sound after the several rounds of review already visible on the thread. I only found one new issue, introduced in the latest commit while resolving a rebase conflict in the docs.

In docs/vllm.md, the "Explicitly, the workaround for an OOM on a shared card is:" example has a duplicated ```bash fence opener right before the example commands. As written, the block opens, immediately closes/reopens on the literal text, and the commands after it are no longer inside a fenced code block for the rest of that section (until the next ```` ) closes it) - so the example won't render as a code block. Please drop the duplicate line.

[r0x0r]

Thanks — good catch, the merge-conflict resolution had left two consecutive bash fence openers before the shared-card workaround example, so the block reopened on the literal text and the commands rendered outside the fence. Fixed in e94ca5f (now the PR head): dropped the duplicate opener so it renders as a single fenced block. No logic touched — docs-only.

[r0x0r]

@rominf the duplicated ```bash fence opener in docs/vllm.md is removed, so the "workaround for an OOM on a shared card" example is a single fenced code block again (the commands render inside it rather than as literal text). Ready for another look — thanks.

[rominf]

Re-reviewing at e94ca5f1 — the docs fence I flagged earlier is fixed, thanks. The four layers here are individually well built: the keyword table's own comment explains why a bare "out of memory" is sub-threshold, VLLM_ANCHOR_PATTERN exists precisely because the torch OOM shape isn't vLLM-specific, and the WSL rework is careful to keep sub-threshold hits in matched rather than dropping them. --gpu auto ranking on free VRAM with the multi-card ordinal guard in read_drm_vram_usage is a good piece of work.

My concerns are all at the seams between the layers, where each side is individually reasonable and the pair isn't.

The biggest: the engine detects an OOM with a bare substring scan, then tells the user to run rocm diagnose --symptom 'vllm: <the failing line>' — but the diagnose table scores several of the lines the engine accepts below MIN_SCORE_FOR_MATCH. I replicated keyword_score against the table in this diff rather than eyeballing it:

DET score=  0 NONE   vllm: RuntimeError: hipErrorOutOfMemory
DET score=  0 NONE   vllm: torch.cuda.OutOfMemoryError
DET score= 25 WEAK   vllm: HIP error: out of memory
DET score= 25 WEAK   vllm: the process was killed: out of memory
DET score= 90 LIKELY vllm: torch.OutOfMemoryError: HIP out of memory. Tried to allocate 7.21 GiB.

DET means log_tail_shows_oom accepted it, so the engine printed the command. The first two produce nothing at all. Details inline.

Second: hip out of memory (45) and out of memory (25) both match the same token span, and keyword_score sums the top two, so one phrase yields 70 and two "independent" evidence bullets. That's the exact overlap torch_oom_class_alone_is_not_a_vllm_match guards against for the PyTorch pattern — the guard just isn't applied here.

Third: making out_of_scope depend on the symptom turns it from a host property into a host+symptom property, and diagnose-states-whether-the-platform-is-covered asserts it agrees with examine's status — which is unconditionally "wsl" on WSL. There's no WSL lane in CI (the scenario's own comment says so), so this lands on a developer.

For what it's worth I checked and disproved several things, so they're not worth your time: applies_on: LINUX_ONLY does not block fix-16 on WSL; \boutofmemory\b and torch\.outofmemoryerror don't double-count; wsl_out_of_scope_message is byte-identical to main; fix-16 is the only checker registered for "wsl".

On CI — the red E2E tests (GPU) lane looks like a stale base rather than this PR. All four failures share device_policy: gpu_required; no active ROCm runtime is configured, which 283811f (#322) fixed on main; main's latest self-hosted run is green. A rebase should clear it, and is worth doing before merge so the lane is actually informative.

[r0x0r]

Thanks @rominf — pushed e1adff1 (rebased onto latest main) addressing each point. Summary of what changed:

Engine hint vs. scorer coupling (the printed --symptom could report nothing).
The vLLM engine no longer hard-codes vllm: <failing line> into the rocm diagnose --symptom '…' it prints. oom_utilization_hint now consults rocm_core::vllm_oom_symptom_is_diagnosable(&candidate) and, when the user's actual line would not itself clear the match threshold, falls back to rocm_core::VLLM_OOM_CANONICAL_SYMPTOM (which carries two independent signals and is guaranteed to score). The user's real failing line still shows in the human-readable message; only the copy-pasteable command is normalized. New loop-invariant test every_emitted_oom_symptom_is_diagnosable asserts that whatever the engine emits is diagnosable — including bare lines that trigger the fallback.

Double-counted keyword spans / "one bullet per phrase".
keyword_score now collects each match's span and drops a hit whose span overlaps an already-kept stronger hit before taking the top two, so out of memory nested inside HIP out of memory is not counted twice. The KEYWORDS_VLLM_OOM table now deliberately splits allocator messages (hip out of memory, cuda out of memory, hipErrorOutOfMemory → 50, clear alone) from the exception class name (torch.OutOfMemoryError → 45, sub-threshold on its own), which keeps torch_oom_class_alone_is_not_a_vllm_match and a_bare_out_of_memory_stays_below_the_match_threshold honest.

Same-line anchor.
The vllm / gpu_memory_utilization anchor must now appear on the same line as an OOM token (vllm_oom_line_is_anchored), not merely somewhere in a pasted log — closing the misattribution where a stray vllm mention re-enabled attribution of an unrelated framework's OOM. New test vllm_anchor_must_be_on_the_same_line_as_the_oom_token.

WSL out-of-scope routing.
The WSL branch now suppresses on any_cleared_threshold(&wsl_matches) rather than "matches is non-empty", so a genuine vLLM OOM on WSL is surfaced while a sub-threshold signal is preserved and still routed out of scope. Tests: a_genuine_vllm_oom_match_on_wsl_is_surfaced_not_routed_out_of_scope, wsl_sub_threshold_vllm_signal_is_preserved_but_still_routed_out_of_scope.

serve --gpu <index> rejection wording.
The usable-set membership rejection now reads --gpu index N is not available; the usable AMD GPU(s) are […], matching the engine wording and the e2e serve-absent-gpu-index-rejected contract ("not available"). Test updated.

--gpu auto on non-contiguous ordinals.
select_auto_gpu_index now ranks the actual reported VRAM row indices (which can be non-contiguous when devices are hidden by a visibility mask) instead of a synthetic 0..count, so it can no longer look up absent rows and fall through to a bogus GPU 0. New test auto_selection_ranks_the_actual_reported_row_indices constructs the real amd-smi non-contiguous-index state.

Duplicated fix-16 remediation text.
Reconciled the verify/notes/executable commands between the rocm fix catalog (fix::RECIPES) and the diagnosis-embedded Fix for fix-15/fix-16 (they had diverged on a two-vs-three-space verify and a reworded note), and added a cross-check test diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids so they cannot silently drift again. The prose framing (catalog rationale vs. diagnosis summary) legitimately differs by surface, so that is intentionally not asserted equal.

Pre-launch low-VRAM note scenario.
The pre-launch low-VRAM guidance is still unit-covered here; its user-observable Gherkin scenario (@id:serve-oom-memory-guidance) lands in the stacked follow-up #284, which depends on this branch. Called out so the coverage gap is explicit rather than silent.

Full workspace cargo test --all-targets, clippy -D warnings, and scripts/smoke_local.py are green locally; the pre-push gate (fmt/clippy/test) passed on push.

Superseded. This objection was filed at an earlier commit; it is retired here and replaced by a fresh review at the current head, so that only one of our change requests is live at a time. Retiring it is bookkeeping, not a withdrawal on the merits: the selection defect it named is confirmed fixed on the path where the host's usable device set is known, and confirmed still present on the path where it is not. The replacement states both precisely.

[siloteemu]

🔴 Automated review · pr-review-watcher · 73b8bfb

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

Summary

Adds vLLM out-of-memory diagnosis (new catalog entry, recipe, terminal-escape module), a DRM sysfs VRAM fallback for --gpu auto, and serve-plan low-VRAM guidance, across 14 files and 44 commits. Outcome: our GPU-selection objection is only half discharged — the head commit fixes the defect on the Linux/probeable path but leaves it live on the visible == None path, which is every non-Linux host. Verified: I extracted select_auto_gpu_index at the base tip, at the previously reviewed head and at this head into a scratch tree, compiled all three and ran them over a scenario matrix, plus a single-branch mutant of the new gate and a variant that re-adds the removed Pass-2 guard; the full unit, integration and e2e suites were not run here, and neither was a workspace-wide build. Checks at review start: 25 success, 1 failure, 1 pending; at publication the pending one had finished, giving 27 success and 1 failure on the same head. Blocking: 2 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

apps/rocm/src/main.rs:20933 — the fix is gated on an unrelated condition, so the reported defect still reproduces on every non-Linux host.

The head commit unions the detected 0..count range back into the candidate set, but only inside if visible.is_some(). The fault being fixed has nothing to do with visibility: it is caused by parse_gpu_vram_usage dropping any device entry missing its used_vram/total_vram pointers, so a device the lighter list enumeration counts simply has no row. Gating the repair on visible therefore misses the hosts where it is needed.

usable_amd_gpu_indices() returns None unconditionally for not(target_os = "linux") (crates/rocm-core/src/lib.rs:5197-5199), and visible is threaded straight from it through select_gpu_indices_under_launch_lock (apps/rocm/src/main.rs:20707) — the single production serve selection path, engine-agnostic. So visible is always None on Windows, a first-class supported platform, and also None on Linux hosts whose topology is unprobeable.

Running the three extracted revisions over count = 2, one VRAM row for GPU 0, GPU 0 service-pinned, both GPUs present:

• with visible = Some([0, 1]) — base returns 1 (idle), previous head returned 0 (busy), this head returns 1 (idle). Fixed.
• with visible = None — base returns 1 (idle), previous head returned 0 (busy), this head still returns 0 (busy). Unchanged regression against the base branch.

That is exactly the failure the commit message describes: "serve pinned to an already-occupied GPU, then failing on VRAM, which is the fault this selection exists to reduce." The inline rationale at apps/rocm/src/main.rs:20916-20930 asserts that fault is now avoided, without qualifying that the guarantee only holds when visible is known.

The PR's new test does not hold the gate either. Forcing if visible.is_some() to true leaves all three of its assertions green, so nothing pins the condition, and no test anywhere in the suite passes detected: Some(n) together with visible: None and a short row set — every visible: None telemetry case passes detected: None, where count is derived from the rows and the mismatch cannot arise. The commit message notices the neighbouring gap ("every all-busy case passed detected: None") but closes only half of it.

Concrete fix: discriminate on the actual cause rather than on visible. The rows are a re-indexed/masked set exactly when some row index falls outside the detected range; otherwise they are a subset of a dense range the count already asserts exists. So union 0..count whenever detected is Some and every row index is below count, and keep rows-only otherwise — the existing visible retain still runs on top when it is available. That keeps the masked case the rows-only construction was introduced for intact (rows 2 and 3 with count 2: a row index is at or above the count, so rows stay authoritative) while repairing the untelemetried-device case on all platforms. Add a test with detected: Some(2), visible: None, one row, an all-busy reported set, asserting the idle ordinal — it is red today.

apps/rocm/src/main.rs:20974-20977 — a load-bearing rationale comment that this same commit falsified.

The Pass 2 comment reads: "No "does this ordinal have a row?" guard is needed: inside this branch every candidate came from a reported row, so usage_for is total over candidate_indices. It was needed when candidates were a synthetic 0..count range, where an ordinal could be selected that telemetry had never reported."

The union added at line 20933 puts synthetic 0..count ordinals back into candidate_indices, so that statement is false at this head, and the PR's own new test disproves it: the first assertion returns ordinal 1, which has no VRAM row, and it returns from Pass 2. usage_for is not total over candidate_indices.

I checked whether this is also a behaviour bug and it is not — I re-added the removed usage_for(index).is_some() guard to the head version and ran both over mixed rowed/rowless, all-rowless, and busy variants: zero behavioural differences, because when every candidate lacks a row the max_by tie-break already returns the lowest index, which is what Pass 3 would return anyway. So the guard removal is sound; only the stated reason is wrong. It must still be corrected, because it advertises an invariant a future maintainer would rely on while editing exactly this ranking code. Replace it with the true reason: usage_for is partial over the candidates, None sorts below every Some, so a rowless ordinal can only win when no candidate has a row, and in that case Pass 2 and Pass 3 agree.

Non-blocking

• docs/vllm.md:183 — "Auto-selection avoids busy cards" is unqualified, but the terminal all-busy fallback deliberately returns a busy ordinal (with a low-VRAM warning). Say it prefers idle cards and returns the lowest busy one as a last resort.
• apps/rocm/src/main.rs:20841 and README.md:489 describe the three preference passes but omit that all-busy fallback, implying auto either picks a non-busy GPU or picks nothing.
• engines/vllm/src/lib.rs:2701-2703, 2733-2743 — both new display call sites split the log tail with str::lines() before stripping, so a bare CR progress redraw glues frames into one run-on line. crates/rocm-core/src/terminal.rs::rendered_lines exists for precisely this and is already used by the diagnose path; no test covers a bare-CR tail either way.
• apps/rocm/src/main.rs — the DRM sysfs cluster (gpu_vram_usage_sysfs, read_drm_vram_usage, read_sysfs_u64, drm_device_is_amd, ~110 lines plus ~120 of tests) is self-contained and already takes an injectable directory for testing; a candidate for its own module next time the file is touched.

Prior round

Our earlier review text was not available while this review ran, so nothing is quoted verbatim; instead both counts were re-verified from the source at this head, and all 44 commit objects in the range were read whole rather than as summaries.

Count 1, selection behaviour — PARTIALLY DISCHARGED, and the remainder STANDS. Discharged on the visible.is_some() path: where the previously reviewed head returned the busy GPU and the base returned the idle one, this head returns the idle one. Not discharged on the visible == None path, where this head still returns the busy GPU against a base that returns the idle one. Because usable_amd_gpu_indices() is None for all non-Linux targets, that path is every Windows host. Determined by compiling and running the function extracted from all three revisions over a scenario matrix in a scratch tree, not by reading the diff and not by inferring anything from the author having pushed.

Count 2, docs contradicting code — STANDS, in two confirmed places at this head: the Pass 2 rationale comment falsified by the same commit's union (and by the commit's own new test), and the union rationale at lines 20916-20930 claiming a guarantee the code delivers only under visible.is_some(). Two further prose claims drift in the same direction but are softer and listed as non-blocking. I did not find a third hard contradiction of the strength of the first two, so if our earlier count of three included one of the prose items, that one has been downgraded rather than refuted.

What misled a previous cycle, and the cheap guard against a repeat. This is codebase-invited confusion, not reviewer error: the repair and the mask logic are fused into one expression, so the diff reads as a general fix while the gate silently narrows it to one platform family, and the surrounding comment asserts the general guarantee. A future reviewer will make the same misread. The cheap prevention is the test named in the first blocker — one case with detected: Some(n), visible: None and a short row set — plus one clause on the comment naming the platforms where visible is never known. Both are a few lines, and either alone would have surfaced this.

No prompt-injection content was found in the diff, the commit objects or the touched docs; the leak scan over the diff and over all 44 raw commit objects is clean, and the bare ticket IDs present are the form this project expressly permits.

[siloteemu]

pr-review-watcher · 73b8bfb

Two blocking findings at this head; the full report is in the round comment.

1. The GPU-selection repair is gated on whether the host's usable device set is known. The fault it fixes is unrelated to that: it comes from a counted device having no telemetry row. On hosts where the usable set is never known -- which includes every Windows host -- the selection still returns the busy card where the base branch returns the idle one. Forcing that gate true leaves all three of the new test's assertions green, so nothing pins it, and no test anywhere combines a known device count with an unknown usable set and a short row set. Discriminating on the actual cause rather than on the usable set, plus one test covering that combination, would close it.

2. The rationale comment on the second ranking pass states an invariant that this same commit falsified, and the new test disproves it: a selected ordinal can now have no telemetry row. Re-adding the removed guard and running both variants shows no behavioural difference, so the guard removal itself is sound -- only the stated reason is wrong. It still needs correcting, because it advertises a property a maintainer would rely on while editing this ranking code.

Our earlier change request on this PR was filed at an older commit and has been retired, so this is the only one of ours that is live.

Withdrawing this change request: both counts are discharged at 2aa6434.

Count 1 — the usable-set gate is gone. The detected range is now unioned back in under a condition that requires the count to come from the enumeration and the rows to be a dense subset of it, with the mask applied strictly on top. Verified by reproducing the original probe in a scratch copy: restoring the old gate now fails the new unprobeable-host test, and forcing either half of the new condition true fails a different assertion of that same test, so both halves are independently pinned rather than covered only by a wholesale revert.

Count 2 — the rationale comment no longer claims the falsified invariant. It states the opposite and gives the correct reason, which checks out against the comparator.

A separate change request has been filed at this head for a new defect found in this round; it is unrelated to either count above.

[siloteemu]

🔴 Automated review · pr-review-watcher · ce7a234

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

Summary

The PR adds OOM diagnosis and a shared terminal-sanitising module; the new commit closes the quoting guard on the two mandatory line-break scalars. Our prior objection is DISCHARGED on code and on pins — the fix is the shared predicate we asked for, not a local patch. Outcome: no blocking findings; one residual from the recurrence half of our own objection. Verified by mutation in scratch copies (never the tree under review): (1) reverting the gate to the narrow is_control_or_format alone turns the vLLM stripper test RED naming "a line separator must make a line unquotable, not ride into the command"; (2) dropping U+2028 alone and (3) dropping U+2029 alone from the new shared predicate each turn tests RED in both crates, so neither scalar is left unpinned by the other; (4) narrowing the gate to U+2028 only turns the paragraph-separator quoting assertion RED, so both new assertions are individually load-bearing rather than one covering for the other; (5) a scratch end-to-end probe through the real hint builder with a raw tail torch.OutOfMemoryError: HIP out of memory<U+2028>PWNED-TAIL confirms str::lines does not split (2 lines), that the pre-fix gate embedded the scalar and the trailing text verbatim in the printed command, and that at this head the same input falls back to the canonical symptom with no U+2028 anywhere in the hint — so the defect was reachable exactly as described and is now closed on the real path. Also confirmed: only one --symptom command builder exists, and no other call site was left on the narrow test (the stripper's own Token::Text filter cannot see these scalars, since the classifier already labels them line breaks, and the merge-property test derives its expectation independently on purpose). Ran the vLLM engine crate's unit tests and the terminal module's unit tests, green at this head; the full suite and e2e were not run. The 2 red checks are not attributed to anything in this diff — I did not look at check state and am not inferring a defect from the count. Blocking: 0 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/terminal.rs:111-128 — the recurrence half of our objection is only partly discharged: is_control_or_format's doc comment is untouched, still opens "carries no glyph of its own but changes how the text around it renders" and still says of the display-control concern that "the answer has to be the same", with no note that Zl/Zp fall outside it and no pointer to the new sibling; only the wider predicate's doc names the gap, which the next author reaches for second. One cross-reference line on the narrow one would close it.
• engines/vllm/src/lib.rs:2786-2789 — the gate ORs two predicates that both already include char::is_control, so the site still answers "what breaks a line" twice; a single named union predicate would leave exactly one answer where the confusion arose.
• engines/vllm/src/lib.rs:3520-3522 — the new test comment says the quoting assertions "go red" under that mutation, but the stripper table assertion panics first and they are never reached; they are genuinely load-bearing (proved separately), so the comment is imprecise rather than wrong.
• engines/vllm/src/lib.rs:3548-3560 — the two new pins call the guard directly, so the mechanism our objection named (candidate built from the raw line, lines() not splitting on U+2028) is still pinned nowhere end-to-end; this matches the existing bidi pin, and the control-byte case does have an end-to-end test, so one more would complete the set.
• Commit ce7a2340 carries an AI co-author footer — the only one of the PR's 46 commits that does; the contributor rules say to avoid AI-generated boilerplate footers.

[siloteemu]

🔴 Automated review · pr-review-watcher · 2aa6434

Blocking: 1. Both counts of our previous change request are discharged and that review has been withdrawn; this one is new and unrelated to either.

engines/vllm/src/lib.rs:2780 — the quoting guard admits the two mandatory line-break scalars into the printed diagnose command, contradicting the contract this same pull request states for them.

The predicate rejects a single quote and anything the shared control-or-format test matches, but that test is character-class based and returns false for both LINE SEPARATOR and PARAGRAPH SEPARATOR. This pull request's own character classifier widens past that same test precisely for these two, and documents at its definition that the shared test is false for both and that a mandatory line break is exactly the kind of non-drawing character the stripper exists to remove. So the two call sites disagree about the same question, and the narrower one is the one guarding output.

The gate is load-bearing rather than cosmetic. The value it admits is built from the raw, unstripped line, not from the sanitised one, and line splitting does not break on LINE SEPARATOR — so such a scalar survives inside a selected line. A log line carrying one after an out-of-memory phrase clears the scan, clears this gate, scores above threshold, and is embedded verbatim in the command printed for the user to copy and paste. The bytes stay inside the quotes, so this is display control rather than a shell-quoting escape, but it is the same class the guard's own doc comment says it closes.

Nothing pins it. The neighbouring test deliberately exercises this guard for the bidi override, with a comment explaining that the two call sites are tested separately, yet the two line-break scalars are pinned against the stripper only.

Fix, and it is worth doing as the shared predicate rather than the local patch: export one line-boundary test and use it at both call sites, then add the missing assertion for both scalars alongside the bidi one. The local patch would work, but the confusion recurs otherwise — the shared test's own doc comment advertises itself as the answer to this exact concern while the classifier's documents that it is not sufficient, so the next call site reaches for the narrower one again.

Five non-blocking items, including a detector that disagrees with its twin about what counts as evidence, are in the review comment posted alongside this one.

Discharged at ce7a234. The shared line-boundary predicate was exported and both call sites now use it, and mutation confirms each of the two scalars is pinned on its own: dropping either one from the predicate turns tests red in both crates, and reverting the gate to the narrower test fails naming the right thing. Withdrawing this change request. One residual doc-comment point is non-blocking and left in the review comment.

[siloteemu]

🔴 Automated review · pr-review-watcher · 5379d56

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

Summary

This pull request adds a shared ECMA-48 escape-sequence walk, routes the vLLM out-of-memory hint and the diagnose scorer through it, shares one constant for the --gpu-memory-utilization wording, and withholds DRM sysfs VRAM telemetry when more than one AMD card is present: No blocking findings. Verified: targeted library tests for the core crate and the vLLM engine crate pass, and three mutations on a throwaway copy confirm the new tests are load-bearing — narrowing the shared line-boundary predicate back to the control-or-format test turns two escape-grammar tests and the engine's stripper test red; reverting only the quoting guard while leaving the shared predicate intact turns the new LINE/PARAGRAPH SEPARATOR assertion red on its own, so that guard is pinned independently of the classifier; and removing the multi-card early return turns both withholding tests red. The prior round's single blocking finding is genuinely resolved, by exactly the shared-predicate remedy rather than a local patch, and both missing assertions are present. I also confirmed the pull request text against the code: the DRM fallback counts AMD cards before any telemetry filter, so a partially-reporting second card still suppresses; --gpu auto ranks the indices actually reported rather than a synthetic range, and that index survives unchanged into the visibility mask; the shared hint constant is the single source of its wording and its 0.5 example agrees across the command summary, the engine startup path and the documentation; and the out-of-memory recipe is genuinely conditional — not auto-applicable, no runner, both branches always printed with the "if it genuinely doesn't fit, this won't help" caveat — with the checker's platform list and the recipe's agreeing. I made no claim about the separately referenced pull request. The full suite and the end-to-end suite were not run here. Checks at review time: 21 success, 0 failure, 0 pending. Blocking: 0 · Non-blocking: 2.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/terminal.rs:211 — the doc comment states "only two classes are Token::Ignorable" and that "everything else with an ESC in it … is a boundary", but a third arm at :268 makes a bare or doubled ESC ignorable too; the claim is an exhaustive enumeration a reader would audit the never-merge property against, and only the end-of-input half is pinned (ESC ESC is asserted nowhere — it yields ["a"] for "a\u{1b}\u{1b}"), so name the third class in that sentence and add that row to the table.
• engines/vllm/src/process.rs:778 — the printed --symptom candidate is built from the unstripped line while the stripped one is computed alongside it, so a colourised failing line — the case the comment two lines above names as the motivating one — always fails the quoting gate and degrades to the canonical symptom instead of quoting the user's actual line; substituting the stripped value keeps the gate load-bearing and satisfies the stated intent, or a line saying only pristine lines are quoted would settle it.