[volen-silo]

Summary

• Resolve registered GGUF models from the installed Lemonade catalog instead of falling back to the default model. Only simple entries are served directly: one checkpoint with a variant, and no extra files or options. Anything else stays with the Lemonade router, including entries that need a vision projector. The direct path validates checkpoint metadata, uses the selected GPU backend, requires the requested alias at readiness, and keeps download and load errors.
• Keep a Linux fallback from a failed router model load or load verification to a direct llama-server, with two limits. It runs only when the registered model resolves to a safe checkpoint, and it uses the llama-server of the GPU backend that Lemonade selected, never a missing or CPU backend. Every other error after a serve starts returns its own error, stops a spawned server, and marks the engine state failed, for the router and the direct route. When the fallback runs, the router's server is stopped first and the state says starting for the whole fallback (so a reader never records the service as failed while it is still running). The router no longer marks failed for a load or verification failure; the fallback settles the state: starting while it runs, failed when no direct route is open or it produces no server. Tests read the state during the fallback's pull and after a failed fallback. The printed and logged line names the backend that was launched.
• State and health use the name that is actually served. The requested text stays in model_ref, and canonical_model_id holds the router name. The engine's own launch path still normalizes the model name once, as before, so it serves the same name that resolve reports.
• A registered entry whose GGUF is split into shards resolves to the first shard. The CLI readiness check accepts a router that selected the Vulkan backend as well as ROCm.
• Enable read-only local Lemonade discovery in rocm dash on Linux and WSL, with configurable host and port and an opt-out. Show every loaded model, remove unloaded or unreachable models, and group equivalent endpoints without adopting external services. A managed service bound to :: is matched against both loopback families.
• Lifecycle ownership comes only from the managed service registry. The Services overlay now lists only managed services. External Lemonade models and Docker-discovered containers stay visible in Observe but are not offered for stop or restart.
• Separate managed runtime installation from external availability. Leave throughput unavailable when last-request metrics cannot be attributed to one loaded model. A managed direct llama-server row stays visible with no throughput and no standing warning.
• A complete split model in one snapshot revision wins over an older partial download of the same model.
• When the selected backend is not ROCm (for example Vulkan), the serve output and the serve log name that backend and say it is not ROCm, on both the router and the direct route. A managed serve discards its stdout and stderr, so the log is where a user reads it. CPU is still refused.
• A managed Lemonade server that needs an API key (health answers 401 or 403) stays listed with its registry status and no throughput, and shows no standing warning. No credentials are read or sent. External discovery still warns on these statuses.
• The dashboard marks a Lemonade rate as the last request's rate: the Instances table shows a legend, and the instance detail reads last req tok/s. Rows for other engines are unchanged.
• Update command help, README, architecture, testing, and manual-testing documentation.

Regression coverage

Serving tests cover registered aliases, unsafe metadata, complex catalog entries, the pinned 11.5.1 catalog entries for the default and canary models, cache-root priority, the router route decision, the persisted served name, the pull command line, and failure states. Fallback tests cover a router load failure that falls back to the selected GPU backend, a non-load error, a non-Linux host, and a missing or CPU backend; each asserts the result with the state or log. A real router stop and a non-zero server exit are covered with a fake lemond. Dashboard tests cover ownership, endpoint grouping, rate attribution and reset, scrape-tick holds, and takeover of an external endpoint by a managed record.

Scenarios:

• @id:dash-lemonade-external-protocol-discovery and @id:dash-lemonade-external-protocol-transitions use an in-process protocol fixture, not real inference. The fixture records any request that is not a GET of health or stats, and the scenarios assert none arrived. They also assert that no managed records were created.
• @id:dash-lemonade-managed-inference-journey needs Linux, a GPU, and Lemonade. It now starts from an active managed runtime and has a bounded serve and a flaky expectations row, like the other Lemonade serve scenarios on Linux.
• @id:serve-lemonade-complex-registered-inference is new. It serves the smallest catalog entry that needs a vision projector and checks that the router loaded it. It is marked @requires-gpu and @nightly, so it only runs when the include-nightly input is set on a self-hosted GPU lane.
• @id:serve-lemonade-simple-registered-direct-route (serve-27) is new. It serves the simple registered entry Qwen3-0.6B-GGUF on its own port and reads the engine state: load_response.method must be lemonade-packaged-llama-server and the backend must not be CPU. It then runs inference and stops the service. It needs @requires-gpu, @requires-os:linux and @requires-engine:lemonade. It has no flaky expectations row, so a route regression fails the lane. Its route step passed on the Linux GPU lanes at the earlier head; its stop step failed there because it looked up the wrong model, which is fixed.
• The router-to-direct fallback has no scenario: there is no safe hook to force a real router load failure. See the testing notes in docs/testing.md. @id:serve-lemonade-complex-registered-inference remains the hardware check of the router route.
• The existing serve-lemonade-inference and serve-hf-checkpoint-inference scenarios stay in place.

Verification

• cargo test --workspace --all-targets with a Linux-only PATH, run serially
• cargo clippy --workspace --all-targets -- -D warnings
• cargo clippy -p e2e-cucumber --test e2e -- -D warnings
• python3 scripts/smoke_local.py
• cargo fmt --all -- --check
• cargo xtask check-crate-edges
• cargo xtask verify-commits: signed commits with DCO sign-off
• External Lemonade protocol scenarios dash-39 and dash-40: 2 scenarios, all steps passed on the latest head

Local limits:

• The normal PATH run fails in one unchanged xtask test because executable lookup returns permission denied. The suite passes with a Linux-only PATH.
• lychee is not installed here, so the full link check runs only in CI. A local check found no missing documentation targets.
• The new last step of runtime-lemonade-backend-alignment-reported checks that the selected backend's llama-server exists after rocm engines install lemonade --reinstall. It shows only that the flag is accepted and leaves a binary. It does not prove that the command repairs a deleted one. That step runs only on the nightly GPU lanes. A new dash-07 step also covers the positive ownership case on the hosted lane.
• Real GPU inference was not run locally. This host has no usable GPU runtime. No CPU fallback was added.
• The Windows build was not run locally, so Windows compile checks rely on CI.
• The GPU scenarios (the managed journey dash-38, serve-08, serve-27, and the nightly serve-26) need the self-hosted lanes. dash-38 sits under the same flaky expectations row as the other Linux Lemonade serve scenarios. The row matches every lane that can run it, so the whole journey is advisory there and cannot block a merge. Unit tests gate the managed-runtime installed wording.
• The throughput that Lemonade reports is the rate of the most recent request, and the dashboard refreshes that observation on every successful scrape. An idle server keeps showing its last rate until a new request or the row goes away. I tried to stop an unchanged report from refreshing it, but that made dash-lemonade-external-protocol-transitions flaky on hosted CI, so it is not part of this PR. External rows already behaved this way.
• No cucumber scenario was run locally, and the changed step in @id:dash-lemonade-external-protocol-transitions is only compiled here.
• The reinstall step shows only that --reinstall is accepted and leaves a binary. It does not prove that the command repairs a deleted one.
• The Linux direct route for simple registered names is gated by serve-27, which has not yet run to completion on hardware. The serving_route tests and released_catalog_default_model_routes_direct_with_binary_and_router_without cover the route decision.
• The original personal-machine failure is not reproduced.
• Existing flaky serving expectations stay, because the historical shutdown failure is not shown fixed on hardware.

Safety and scope

External discovery never creates managed service records or stops external processes. Tests use isolated roots and owned endpoints. Native Windows live dashboard support and general damaged-runtime repair are outside scope.

• Reviewed existing expected failures. None were removed without hardware evidence.
• Reused existing domain modules. The router path moved into its own module, router_serve.rs.
• Discovery and ownership messages are asserted together with the resulting state.

[rominf]

🔴 Automated review · pr-review-watcher · ace6ca1

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.

First automated round on this PR: every finding below is new.

Review — needs work

Full review of the whole change.
No ticket named. The description claims registered-GGUF resolution, read-only dash discovery, separate install and availability status, and doc updates. Findings 1, 3 and 4 below conflict with parts of those claims.

Blocking

• A registered llamacpp catalog entry the resolver doesn't fully understand now fails rocm serve on Linux, or is served without its extra files — engines/lemonade/src/direct_llama.rs:257-291, engines/lemonade/src/lib.rs:514-519
resolve_direct_llama_checkpoint requires a plain string checkpoint of the form owner/repo:variant and returns an error otherwise. serve_http passes that error up with ? on Linux for every model reference. Before this change these names went to Lemonade's router.
  I ran a probe test against the head code:

  • an entry with a checkpoints map returns Err(registered model Vision-GGUF has no checkpoint);
  • an entry whose checkpoint has no variant returns Err(... needs an explicit quantization variant ...);
  • an entry with checkpoint plus a top-level mmproj returns Ok(direct), and direct_llama_server_command never passes --mmproj, so the model is served without its vision projector and nothing reports it.
    Lemonade documents the checkpoints map (main/mmproj/draft) as the current format and shows built-in models using it (custom models, API). I could not confirm the pinned 11.5.1 catalog itself, because the repo has no fixture of it. A server_models.json that can't be read or parsed also hard-fails, including for names the file wouldn't contain.
    Confidence 85 · logic · Fix: serve directly only shapes the resolver fully understands (a single checkpoint with a variant, no mmproj/checkpoints/extra checkpoint types). Return Ok(None) for everything else and for catalog read or parse errors, so the router handles them. Add a test for each shape.

• The new serve routing decisions have no test that fails when they break — engines/lemonade/src/lib.rs:453-458, :514-519, :546-553; direct_llama.rs:297
  I mutated each of these on its own and ran cargo test -p rocm-engine-lemonade --lib; all 129 tests still passed every time:

  • disabled the bail for an HF checkpoint when no selected backend binary exists;
  • inverted the Linux check in launch_service;
  • forced direct_model to None, so registered names never take the direct path;
  • removed the backend_recipe != llamacpp guard in direct_serve_binary.
    The GPU scenarios serve-07, serve-08 and dash-25 cover only the happy path.
    Confidence 100 · logic · Fix: pull the direct / router / bail decision into a pure function (inputs: platform, parsed reference, catalog result, binary present) and unit-test each outcome, including the bail message.

• launch_service stops normalizing the model reference on Linux, with no stated reason, and engine state can then disagree with the served name — engines/lemonade/src/lib.rs:453-458, engines/lemonade/src/state.rs:152-153
rocm serve already passes the canonical name (serve_cmd.rs → resolve_model_response), so for that command this branch changes nothing.
  On the engine's own launch path, the state file now records the raw alias (for example qwen, or text with spaces around it) as model_ref and canonical_model_id. The router path, however, serves resolve_lemonade_model_ref(...). healthcheck_service reads the state value back, and its inference probe then uses the raw alias.
  WSL also counts as Linux here. With this branch, model-name normalization now happens in three separate places.
  Confidence 70 · logic · Fix: normalize once in launch_service as before. If the requested text must be kept, store it separately and keep canonical_model_id as the name actually served.

• External models found by discovery can be selected for "stop"/"restart" in the Services overlay, and the approval text calls them managed — crates/rocm-dash-tui/src/ui/services_manager.rs:93-110, :169-200
service_rows lists every daemon instance. Pressing s or r on a lemonade-<host>-<port>-model-<hex> row stages rocm services stop <id> --yes, with the text "This runs the CLI services stop for this managed service."
  The stop itself is harmless: the record lookup fails for an id that isn't in the registry, so nothing is killed. But the text is false for these rows, and the action can only fail.
  The code was already there, but this change makes discovery default-on in rocm dash, so these rows now appear on every host running Lemonade. docs/manual-testing.md works around it ("Do not use managed stop or restart actions…") instead of the code preventing it. AGENTS.md §3 says a promise that something will not happen must be tested together with the behavior.
  Confidence 85 · logic · Fix: give discovered rows an external/read-only marker. Hide or refuse lifecycle actions on them, and add a test that s on such a row stages nothing.

• A contract test's step claims something it cannot detect — crates/rocm-dash-daemon/tests/telemetry_contract.rs (step "adding another model clears previously attributed rate immediately")
  I deleted gen_trackers.remove(&row.id) in runner.rs:897, so nothing clears the old rate. lemonade_multiple_models_keep_identity_and_do_not_share_last_request_rate still passes; it took 7.1s instead of 1.2s because it waits for the held value to expire on its own. So the step does not prove "immediately".
  For contrast, the "last access is not last request" step is a real check: breaking stats_model's single-model rule fails it at line 192.
  Confidence 100 · logic · Fix: put a time bound on that wait that is shorter than the hold window, or check the first snapshot after the health change.

• Code left with no caller after the runner rewrite — crates/rocm-dash-collectors/src/lemonade.rs:169 (parse_health_model), :313 (fetch_health_model), :334 (discover), :117-121
  The runner now calls only discover_models and fetch_health. The other three have no non-test callers in the workspace (it is publish = false), yet their doc comments say they exist "for compatibility/legacy callers".
  In stats_model, the trailing if loaded.len() == 1 { … } else { None } can't be reached with any other length, because of the len() != 1 early return above it.
  Confidence 85 · mechanical · Fix: delete them, or make them test-only and move their tests to discover_models/fetch_health. Collapse the dead branch.

Non-blocking

• The new bail message has no remediation, and it is wrong for some cases — engines/lemonade/src/lib.rs:546-553. The old message said "run rocm engines install lemonade". The new "no selected GPU llama-server backend X is installed" also fires when backend_recipe isn't llamacpp or the backend is cpu, where "not installed" is not the problem. Confidence 70 · mechanical · Fix: handle the recipe and cpu cases separately, and name a real command (rocm engines install lemonade --reinstall is already used in backend_alignment.rs).
• Comments made stale by this change — Confidence 85 · mechanical · Fix: update each:
  • tests/e2e-cucumber/features/model_serving.feature:73-77 and tests/e2e-cucumber/tests/e2e/serving_steps.rs:613 cite the removed serve_hf_checkpoint and the removed "managed-lemonade fallback".
  • crates/rocm-dash-daemon/src/runner.rs:67-69 and :311-312 still say discovery is "Opt-in … off by default", but rocm dash now turns it on.
  • The registry.rs:26-31 module doc says record.host is honored only for the co-located daemon, but managed Lemonade is now scraped at record.host.
• Lemonade HTTP calls run one after another inside the daemon tick loop — runner.rs:~678-686, ~833-925. Each external or managed endpoint gets a health request and possibly a stats request, each with a 1.5s timeout (doubled by the 404 fallback), and a new reqwest client is built per managed endpoint on every tick. A hung endpoint delays every snapshot. The vLLM path next to it uses parallel_scrape. Confidence 70 · architectural · Fix: run the endpoint futures concurrently and reuse the collectors.
• One failed health poll removes a discovered model — lemonade.rs:323-330 (unwrap_or_default), runner.rs:~697-709. A timeout or 5xx while Lemonade is busy emits InstanceGone, and the next good poll emits InstanceDiscovered again, so the row flaps. Before, this applied to one instance and discovery was opt-in; now it applies to every loaded model by default. Health errors are logged at trace! only. Confidence 70 · logic · Fix: require several consecutive misses before removal, and report health errors through scrape_warns.
• dash-27 ends on a step that checks nothing — tests/e2e-cucumber/features/dash.feature:372-373. After "selects engine status in the Serving view", the final Then only waits for the heading text "Serving actions". Nothing checks that "Lemonade — externally running" is gone once the endpoint is unreachable. Confidence 85 · logic · Fix: replace it with an absence check on the engine pane.
• "External" is recognized only by string matching — crates/rocm-dash-tui/src/ui/tabs/pane.rs:393-398. The TUI matches container_name == "lemonade" plus a lemonade- id prefix against how the collector happens to build ids. Removing the starts_with half fails none of the four pane::tests, as I confirmed by running them. Confidence 70 · architectural · Fix: share a single predicate, or add an origin field to Instance; the Services-overlay fix above needs the same marker.
• One commit holds several separate changes — AGENTS.md §11. It bundles the engine serving rewrite, a new dashboard discovery feature with config schema, a default flip, the engine-pane status, and a protocol fixture, about 1,900 inserted lines, with an empty commit body. Consider splitting the serving fix from the dash discovery work. Confidence 75 · architectural.

Decisions for the author

• Direct llama-server is now tried first for every registered llamacpp name on Linux, and the router's load error no longer falls back to direct — tradeoff
  For: router load errors are no longer hidden, and the binary is chosen by the selected backend. Against: router-only per-model wiring (vision projector, draft and other checkpoint types, labels) is skipped whenever direct applies. Finding 1 is the correctness half of this; the question is whether bypassing the router for all registered names is intended.
• The direct-serve binary lookup is now limited to manifest.backend_name — tradeoff
  For: a stale ROCm or Vulkan install can no longer be picked. Against: a host whose manifest says rocm but has only vulkan on disk (or the reverse) used to serve HF checkpoints and now fails.
• rocm dash now probes 127.0.0.1:13305 by default — tradeoff
  It's documented. The cost is a poll on every discovery tick on hosts with no Lemonade, and any local listener on that port can put rows into the dashboard.

Positive signals

• The /api → /v1 fallback happens only on a 404, never on an auth failure or 5xx, and health_http_failure_never_falls_back_to_successful_route locks that in with .expect(0) plus verify().
• Throughput from Lemonade's server-wide "last request" stats is shown only when exactly one model is loaded, and breaking that rule fails both the unit test and the contract test, as I checked by mutation.
• Catalog metadata is treated as untrusted: checkpoints that look like path traversal or local paths are rejected and tested, and CPU or unknown backend names are refused.
• dash-26 checks the external fixture is still alive after the dashboard quits and that no service record was created, so the behavior is asserted, not just the wording.

Deployment notes

• rocm dash now reads the new dashboard.daemon.enable_lemonade setting (default true) and probes lemonade_host:lemonade_port (default 127.0.0.1:13305). Existing config files pick up these defaults. A standalone daemon built from RunnerOptions::default() stays opt-in.
• On Linux, rocm serve <registered llamacpp name> --engine lemonade now runs the packaged llama-server directly on the selected backend whenever the name resolves in the catalog, rather than going through Lemonade's router.

What this covered

Every file in the change at 34fbdfe9…ace6ca15, plus callers and siblings. Workers looked at:

• services_manager.rs and the services stop path;
• serve_cmd.rs and main.rs readiness, and the engine's state.rs, install.rs and backend_alignment.rs;
• expectations.toml and the serve step definitions.

Four coupled code passes, three design questions, and the four source-reading passes (agent-instruction adherence, history, prior PRs #80/#94/#104/#242/#295/#404, code comments) ran as separate workers.

What I ran myself:

• The unit and contract tests for the lemonade engine, the dash collectors and daemon, and the TUI pane, plus seven mutations and one probe test. Each run's test names and counts were checked.
• The leak scan was clean.
• At review time the Strix Halo Ubuntu (release) GPU lane had passed serve-07, serve-08 and dash-25 through dash-27.

Did not run or only partly ran:

• The e2e suite was not run locally. The dash-27 finding is from reading the step definitions.
• The TUI worker read event_loop.rs, app/mod.rs and instances.rs only as their one-line diffs plus call sites, not completely; it also did not read the rubric reference.
• MI300X, Strix Halo Windows and WSL2 had not reported; Ubuntu nightly was still running.
• The Lemonade catalog shape comes from Lemonade's public docs, not from the pinned 11.5.1 file.

Test verdicts:

• The engine serving_* tests and the rocm-core/dash.rs config tests would fail if the production change were reverted. The routing branches inside the change are not covered (finding 2).
• In the contract test, the "immediately clears" step passes with the clearing removed (finding 5); its other steps are real checks.
• The pane tests do not cover the id-prefix half of the "external" check.

This report is not reconciled against the PR discussion.

Addressed in 5f26173: preserve router wiring for complex catalog models, persist the served identity, restrict lifecycle controls to managed services, and strengthen routing and telemetry regressions. Rebased onto current main; local workspace and dashboard protocol checks passed. Updated CI is running.

[rominf]

🔴 Automated review · pr-review-watcher · ac02905

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.

Informational — describes ac029055, not the current head. The branch moved to 93324b5e while this round was running, so these findings were read against the older commit. Some may already be fixed, and anything the new commits changed was not reviewed. They are not attached inline for that reason, and this round files no change request: until the next round reviews 93324b5e, nothing here gates the merge. Round 2: the commit round 1 reviewed (ace6ca15) had been rewritten, so this was a full review. standing marks a finding round 1 also raised; the rest are new.

Review — needs work

Full review of the whole change.
No ticket named. The description claims the external endpoint staying alive after dashboard quit is "covered with resulting-state assertions". It is not; see Blocking.

Blocking

• [new] The new real-GPU scenario dash-25 hangs both Strix Halo Ubuntu lanes until they are cancelled — tests/e2e-cucumber/features/dash.feature:332-344
  • What CI shows. On jobs 114933031640 (X2-LA03-S03-D02) and 114933031567 (X2-LA03-S04-D08), dash-25 started at 17:57. Its first step passed. The next log line is ##[error]The operation was canceled. at 18:33. Both lanes produced an empty report.json, so none of the Linux GPU scenarios on this PR were verified there.
  • Likely cause. dash-25 serves with a GGUF model is being served on lemonade, the same step as serve-07. Unlike serve-07 (model_serving.feature:66), it omits Given a managed runtime is active. So the serve runs against an empty, isolated runtimes tree, inside a run_rocm call with no timeout.
  • Missing expectations row. That same serve is xfail/flaky on Linux under EAI-7423 in expectations.toml (serve-lemonade-inference and siblings, serve_timeout_secs = 90). The new id has no row.
  • Confidence 100 on the hang (lane logs); 70 on the cause. · logic
  • Fix: add the runtime precondition, add an EAI-7423 row with a bounded serve timeout, and re-run the Ubuntu lanes.
• [new] discover_models has no production caller; its three new tests check a copy of the logic the daemon never runs — crates/rocm-dash-collectors/src/lemonade.rs:292, crates/rocm-dash-daemon/src/runner.rs:800-814
  • On the base, the runner called LemonadeCollector::discover(). This change replaces it with discover_models(), which only tests call (lemonade.rs:388, 491, 512, 531, 542, 553).
  • The runner re-implements the same health → loaded_models → lemonade_service mapping inline, with different error handling: it propagates errors, while discover_models swallows them via unwrap_or_default.
  • So discovery_rejects_invalid_health_and_healthy_empty_router, discovery_recognizes_modern_loaded_model_without_legacy_field and discovery_accepts_v1_health_only_when_api_prefix_is_absent pass or fail independently of production.
  • Confidence 85 · architectural
  • Fix: extract one pure services_from_health(host, port, &LemonadeHealth) used by both, or delete discover_models and point the tests at the runner's path.
• [new] No test covers dropping external rows when a managed record takes over the endpoint — crates/rocm-dash-daemon/src/runner.rs:723-745
  • I deleted the for gone in &lemonade_ids { … } loop and lemonade_ids.clear(). All 7 lemonade_* contract tests and every other rocm-dash-daemon test (45 lib + 9 + 16) stayed green.
  • lemonade_external_endpoint_deduplicates_managed_owner_then_reappears_after_record_removal has the managed record present from tick 1, so lemonade_ids is always empty when this branch runs.
  • Without the loop, external rows that existed before the record appeared would stay "Ready" forever next to the managed row.
  • Confidence 100 (mutation run) · logic
  • Fix: add a contract case where external rows exist first and a matching registry record then appears.
• [standing — round 1's "routing decisions have no test", narrowed to this branch] The router-to-direct fallback removal has no test; the test added for it exercises unchanged code and is not hermetic — engines/lemonade/src/lib.rs:588-606, engines/lemonade/src/process.rs:815-856
  • The behaviour change is untested. On Linux, router load or verify failures no longer fall back to direct serving. Nothing tests that.
  • The test passes without the change. serving_gpu_model_load_process_error_is_returned drives only run_lemonade_model_load, which this diff does not change, so it passes with the production change reverted.
  • The test fails here. It fails deterministically on this host (cargo test -p rocm-engine-lemonade --lib: 134 passed, 1 failed; it also fails when run alone). The real error was preparing a runtime directory … at /home/rinflian/.rocm/data/lemonade (XDG_RUNTIME_DIR is unset …): Permission denied.
  • It writes outside its tempdir. LemonadeProcessEnvironment::default() reaches ParentRuntimeEnvironment::current(). Without XDG_RUNTIME_DIR that writes into the real user data dir; with XDG_RUNTIME_DIR set to a checkout dir the test passes. The runtime_dir.rs tests avoid this by passing an explicit parent.
  • Confidence 100 for the failure and the revert verdict; 85 for the untested behaviour · logic
  • Fix: inject the parent runtime env, or pass a LemonadeProcessEnvironment rooted in the tempdir. Then cover the no-fallback decision, for example by extracting it into a testable function.
• [new] The "external server remains alive after quit" check cannot fail, and the description cites it as coverage — tests/e2e-cucumber/tests/e2e/dash_steps.rs:194-205, tests/e2e-cucumber/src/lemonade_protocol.rs:25-28
  • The fixture is an in-process axum server with only GET /api/v1/health and /api/v1/stats routes. No product action can stop it or change its loaded list, so all_models_loaded.len() == 2 always holds.
  • That step backs the --help promise "quitting leaves external servers running" (apps/rocm/src/main.rs:596). It also backs the description's "covered with resulting-state assertions, including the external endpoint remaining alive after dashboard quit". AGENTS.md §3 requires "will not happen" promises to be exercised.
  • Only the data/services emptiness check in that step can go red.
  • Confidence 85 · logic
  • Fix: have the fixture record any non-GET or unload/load request and assert none arrived. Otherwise narrow the description.
• [new] Cache-root priority is lost when the same GGUF exists in several roots — engines/lemonade/src/direct_llama.rs:428-446, :476-478
  • On the base, hf_checkpoint_gguf_matches walked hf_cache_roots in order (HUGGINGFACE_HUB_CACHE > HF_HOME/hub > app cache > ~/.cache) and sorted within each root only.
  • Now files from every root are merged first. filter_ggufs_by_variant then runs ggufs.sort() over full paths before both the exact-filename find and the name dedup. So the alphabetically first path wins, not the configured root: /home/u/.cache/... beats /srv/hf/....
  • Confidence 85 (base vs head traced) · logic
  • Fix: filter per root in priority order, as the base did, and add a two-root test that asserts the chosen path, not just the file name.
• [new] lemonade_managed_installed has no test that tells "installed and managed" apart from "installed" — apps/rocm/src/dash.rs:252-261
  • Dropping the detect.managed_env == Some(true) conjunct, or mapping a failed Detect to Some(false) instead of None, is reached only by dash-25 on a GPU lane, and that scenario hangs (first finding).
  • Unit tests in dash.rs exercise only the config defaults.
  • Confidence 75 (read; I did not build the rocm bin here) · logic
  • Fix: inject the Detect result and unit-test the three outcomes.

Non-blocking

• [new] The Services details pane counts instances the Services overlay no longer shows — crates/rocm-dash-tui/src/ui/tabs/pane.rs:450-455, crates/rocm-dash-tui/src/ui/services_manager.rs:98
  • service_rows now filters i.managed, but the OpenServices preview still counts every is_serving() instance.
  • With only external Lemonade running, the pane says "Running now — 2" and pressing Enter shows "No managed services". service_rows governs, because it is what the lifecycle controls act on.
  • Confidence 75 · logic
  • Fix: apply the same filter in the pane, or label external rows.
• [standing] The "off by default" comments are now stale — crates/rocm-dash-daemon/src/runner.rs:88-90, :345-347
  • This change edited line 89 but kept "Off by default so hosts with no Lemonade server never poll a dead port", and "off unless a Lemonade endpoint is configured".
  • rocm dash now passes d.enable_lemonade, which defaults to true (crates/rocm-core/src/lib.rs default_dashboard_enable_lemonade). The config default governs; RunnerOptions::default remains false only for tests.
  • Confidence 85 · mechanical
  • Fix: reword both comments.
• [new] A failed /v1 fallback is reported against the wrong URL — crates/rocm-dash-collectors/src/lemonade.rs:263-274
  • After a 404 on /api/v1/..., a failing status from the retry is reported as GET {url} with the original /api URL.
  • A transport error on the retry prints only legacy_path, with no host or port. Both strings reach the dashboard warning badge.
  • Confidence 85 · mechanical
  • Fix: rebind the URL to the one actually requested.
• [new] dash-26/27 each bundle about five behaviours, and some step text leaks implementation — tests/e2e-cucumber/features/dash.feature:346-…
  • Examples of leaked implementation: "protocol fixture", "keeps only its first model loaded", "with truthful throughput".
  • In external_lifecycle_keys_do_nothing, the real lifecycle-refusal assertions sit inside a When step.
  • Confidence 70 · mechanical
  • Fix: split by behaviour and move the assertions into Then steps.

Decisions for the author

• Linux direct-vs-router routing is decided by an exhaustive key and label allow-list over Lemonade's external catalog (direct_llama.rs:339-355) — tradeoff
  • For: anything unrecognised falls to the router, so a model is never served directly on metadata it does not understand.
  • Against: the default model (Qwen3-4B-Instruct-2507-GGUF) no longer has a hard-coded direct path. Any extra label or key in a future catalog moves it, silently, onto the Linux router path that EAI-7423 records as broken. No in-repo fixture of the real entries pins its route.
• About 230 lines of endpoint grouping, scraping, attribution and reconciliation were added inline to run_loop (runner.rs:706-935) — non-blocking-improvement
  • A pure reconcile function in its own module would make the untested branches above testable without driving the whole loop.

Positive signals

• Instance.managed is #[serde(default)] false, and a legacy-snapshot test asserts that. Ownership is fail-closed: missing evidence never grants stop/restart.
• Removing the router-failure fallback ends a path that could quietly serve a different model than requested. It is replaced by an error that keeps the original cause.

Deployment notes

• rocm dash on Linux/WSL now polls 127.0.0.1:13305 every discovery tick by default. New config keys: dashboard.daemon.enable_lemonade, lemonade_host, lemonade_port.
• The Services overlay now lists only managed instances. Docker containers, external Lemonade, and snapshots or replays from an older daemon (which have no managed field) no longer appear there.

What this covered

• What was read. All 33 changed files, completely, plus callers and callees (registry/runner, the TUI consumers, engine_request, expectations.toml, the e2e harness). Range fe47c4891d5e…ac029055f224.
• How the review was split. Four parallel workers grouped by coupling: the Lemonade engine, the dash daemon and collectors, the TUI/app/e2e, and docs/AGENTS.md/history/prior-PR comments. Two more design-question workers followed.
• What I ran.
  • rocm-dash-daemon contract tests, plus one mutation.
  • The full rocm-dash-daemon suite.
  • rocm-engine-lemonade --lib, which reproduced the failure above.
  • The rocm-dash-tui services/pane tests.
  • CI rollup and lane logs for this head.

Did not run:

• rocm bin unit tests and local e2e, skipped for disk and build cost.
• No GPU on this host.
• The MI300X lane was still queued at review time.

Real Lemonade server_models.json entries were unavailable, so the default model's actual route is unverified.

Not reconciled against any PR discussion.

[rominf]

🔴 Automated review · pr-review-watcher · 93324b5

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.

Round 3. ac029055, the commit the previous round described, was force-pushed away, so this is a full review of 93324b5e. [new] marks findings this round raised for the first time. [standing] marks objections from round 2 (the informational comment on ac029055) that still hold. The last four Blocking items were not re-found by this round's blind review. They were settled afterwards by putting each one, in its original wording, to the reviewer that had just read this head. That check is anchored, meaning it was told what to look for, so it is weaker evidence than a blind finding; each item carries its verdict. The description finding would not gate on its own, because the squash commit is built from commit messages; the other blockers do gate.

Review — needs work

Full review of the whole change.
No ticket named. The description covers the registered-GGUF resolution and the dash discovery work. It leaves out several behaviour changes from the second commit (see Blocking).

Blocking

• [standing, in a new form: round 2's "dash-25 hangs both Strix Halo Ubuntu lanes"] The required GPU lanes never ran on this head, so the hardware-only scenarios are unverified. Locations: tests/e2e-cucumber/features/dash.feature:332 (dash-25), tests/e2e-cucumber/features/model_serving.feature:326 (serve-25), tests/e2e-cucumber/expectations.toml:96-101.

  • The self-hosted e2e run 38359301083 for 93324b5e has sat pending with no jobs since 06:53Z. No GPU lane appears in statusCheckRollup.
  • That means three things never ran: dash-25 (per-PR, @requires-gpu), serve-08, and the nightly serve-25. serve-25 is the only end-to-end check that complex catalog entries stay on the Lemonade router.
  • dash-25 reuses the step "a GGUF model is being served on lemonade". serve-lemonade-inference uses the same step and has a Linux/lemonade flaky xfail row (EAI-7423). dash-25 has no row, so it may fail hard on the Strix Ubuntu lane.
  • Confidence 100 · architectural · Fix: get the self-hosted run scheduled (it is stuck, not failed). Then either add a matching expectations row for dash-25 or show on hardware that EAI-7423 is fixed. Dispatch include_nightly once for serve-25 and cite that run.

• [standing: round 2's "router fallback removal has no test", now with new mutations] Several lemonade engine branches this change introduces are not exercised by any test. Locations: engines/lemonade/src/lib.rs:546, :569, :578, :585, :588-611; engines/lemonade/src/direct_llama.rs:152-159; engines/lemonade/src/process.rs:814-856.

  • Each mutation below left the whole lemonade lib suite green (143 tests):
    • persisting request.model_ref instead of router_model_ref at :546;
    • passing request.model_ref to the load, loaded-model and chat-smoke calls;
    • pulling the alias instead of checkpoint.model_ref();
    • swallowing the pull error with .ok().
  • The removed router-failure fallback has no test either. serving_gpu_model_load_process_error_is_returned calls run_lemonade_model_load, whose body this diff does not change, so it would still pass with the fallback restored.
  • That same test is also not hermetic. With XDG_RUNTIME_DIR unset it tries to create the real ~/.rocm/data/lemonade. On a clean tree here it failed with assertion failed: error.to_string().contains("Lemonade model load failed") (env -u XDG_RUNTIME_DIR cargo test -p rocm-engine-lemonade --lib serving_gpu_model_load_process_error_is_returned: 1 run, 1 failed).
  • Confidence 100 · logic · Fix:
    • Make the router branch's state persistence and failure handling testable, for example with a seam around spawn and load, and assert the id that gets persisted.
    • Assert the pull owner/repo:variant argv and that a non-zero exit is propagated, using the existing fake-executable pattern.
    • Give the load test an explicit ParentRuntimeEnvironment or a tempdir runtime dir.

• [new] Several dashboard branches this change introduces are not exercised by any test. Locations: crates/rocm-dash-daemon/src/runner.rs ~641-656, ~741-757, ~858-868; crates/rocm-dash-tui/src/ui/services_manager.rs:142; crates/rocm-dash-tui/src/ui/tabs/pane.rs (mark(...)).

  • Each of these mutations left the relevant suite green:
    • Model/host-change telemetry reset: turning it into a no-op left all 7 lemonade contract tests green.
    • Scrape-only cadence: forcing discover_external: true left everything green, because every new contract test sets discovery_tick == instance_tick. That leaves the "hold rows on a scrape-tick health failure" path unexercised.
    • Selection clamp: removing it left all 25 TUI tests green.
    • Pane glyph: forcing mark(false) left all 25 TUI tests green.
  • Confidence 100 · logic · Fix:
    • Add a contract test that rewrites a managed record's model while the clock is held, and asserts the rate resets.
    • Add one test with discovery_tick = 3 × instance_tick that covers the scrape-tick hold.
    • Add a TUI test that shrinks the rows below selected and then presses s.
    • Assert the ✓/· glyph.

• [standing in part: round 2's "external server remains alive after quit check cannot fail"; the other checks are new] Some new checks claim coverage they cannot fail on. Locations: engines/lemonade/src/direct_llama.rs:584-587, :461 and :875-876; tests/e2e-cucumber/tests/e2e/dash_steps.rs external_survives_quit (dash.feature:356,361) and real_lemonade_visible.

  • assert_eq!(requested.model_ref, "qwen-smoke", "direct aliases are not normalized") checks a value serving_route only borrows, so it is always true.
  • The test comment says the is_file() filter in collect_gguf_files is covered. The fixture is a directory that the earlier is_dir() branch handles first, so removing is_file() leaves everything green.
  • The e2e step "the protocol fixture remains alive" checks an in-process fixture thread that nothing the dashboard does can stop.
  • "Real managed Lemonade model is displayed" only waits for "Qwen3-0.6B" anywhere on screen. Managed rows were already shown before this change.
  • A check that names a branch it cannot fail on is worse than no check, because it stops the next reader from looking.
  • Confidence 85 · mechanical · Fix:
    • Assert the --alias produced by direct_llama_server_command along the launch_service path.
    • Add a dangling-symlink fixture (#[cfg(unix)]) and correct the comment.
    • Run the fixture as a child process and check its PID, or rename the step to what it really asserts ("no managed records").
    • Anchor the managed-row check to that row's port or status.

• [standing in part: round 2 flagged the "remaining alive" overclaim; the omissions are new] The description misstates the change's scope.

  • It does not mention four things the diff does:
    • It removes the Linux fallback to direct llama-server after a router load or verify failure (lib.rs:588-611).
    • The Services overlay now lists only managed instances, so Docker-discovered containers that used to appear are gone (services_manager.rs:98).
    • Complex catalog entries now go to the Lemonade router.
    • It adds the nightly scenario @id:serve-lemonade-complex-registered-inference.
  • It claims "the external endpoint remaining alive after dashboard quit" is covered "with resulting-state assertions". That assertion cannot fail (previous finding).
  • Confidence 85 · mechanical · Fix: update the description to list these changes. Either narrow the "remaining alive" claim or make the assertion real.

• [standing, not re-found blind, settled by objection check: HOLDS] discover_models has no production caller; its three tests check a copy of the logic the daemon runs — crates/rocm-dash-collectors/src/lemonade.rs:292-300, crates/rocm-dash-daemon/src/runner.rs:800-814

  • Only tests in the same file call discover_models, and a workspace grep finds no production caller. The runner repeats the health → loaded_models → lemonade_service mapping inline. Its error handling differs: it propagates the fetch_health() error with ?, while discover_models uses unwrap_or_default().
  • Partly mitigated: the shared helpers (fetch_health/get_text, parse_health, loaded_models, lemonade_service) are production code. Only the way they are put together, and the error-to-empty behaviour, are tested on a copy.
  • Fix: extract one pure function both paths use, or delete discover_models and point the tests at the runner's path.

• [standing, not re-found blind, settled by objection check: HOLDS, confirmed by a run] No test covers dropping external rows when a managed record takes over the endpoint — crates/rocm-dash-daemon/src/runner.rs:726-745

  • With the for gone in &lemonade_ids { … } loop and lemonade_ids.clear() deleted, RUSTC_WRAPPER= cargo test -p rocm-dash-daemon stayed fully green: lib 45, a second binary 9, telemetry_contract 16. All 7 lemonade_* contract tests ran by name.
  • lemonade_external_endpoint_deduplicates_managed_owner_then_reappears_after_record_removal writes the managed record before the runner starts (telemetry_contract.rs:328-329). So the transition from external rows to a managed record never happens.
  • Fix: add a contract case where external rows exist first and a matching registry record then appears.

• [standing, not re-found blind, settled by objection check: HOLDS, read from code] Cache-root priority is lost when the same GGUF exists in several roots — engines/lemonade/src/direct_llama.rs:428-446, :477, :115

  • The base served hf_cache_roots(..).into_iter().find_map(..), so the first root in priority order won (prw-base direct_llama.rs:327-329).
  • Now every root is flattened, filter_ggufs_by_variant sorts by full path, and matches.into_iter().next() is served. Nothing canonicalizes afterwards, so the alphabetically first path wins. It matters when copies differ, for example a stale ~/.cache snapshot against the configured HUGGINGFACE_HUB_CACHE.
  • The only multi-root test (:865-893) asserts file_name() alone, so it cannot see which root won.
  • Fix: filter per root in priority order, and add a two-root test that asserts the full path.

• [standing, not re-found blind, settled by objection check: HOLDS, read from code] lemonade_managed_installed has no test that tells "installed and managed" apart from "installed" — apps/rocm/src/dash.rs:252-261

  • The only unit test in dash.rs covers config. The TUI tests set the Option directly.
  • Hosted dash-26/27 reach only a Detect that succeeds with installed=false. So neither dropping the managed_env conjunct nor mapping a Detect error to Some(false) is caught there.
  • Only dash-25 exercises Some(true), and dash-25 did not run on this head (first finding). The check could not confirm round 2's claim that dash-25 hangs.
  • Fix: inject the Detect result and unit-test the three outcomes.

Non-blocking

• [standing] Stale "opt-in / off by default" comments — crates/rocm-dash-daemon/src/runner.rs:88-90, :345-347. rocm dash now enables discovery by default through DashboardDaemonConfig.enable_lemonade (crates/rocm-core/src/lib.rs:2413), which apps/rocm/src/dash.rs:61 passes through. Confidence 85 · mechanical · Fix: say discovery is on by default in rocm dash via config, and that only the bare RunnerOptions::default() is off.
• [new] rocm dash --help promises discovery on every platform — apps/rocm/src/main.rs:598-602. The embedded daemon returns None on Windows (apps/rocm/src/dash.rs:975-978), so nothing is discovered there. README already scopes the claim to Linux/WSL. Confidence 85 · mechanical · Fix: prefix the help text with "On Linux/WSL,".
• [new] A failed router serve leaves engine state at running, and this now happens on Linux too — engines/lemonade/src/lib.rs:588-611. Both error arms kill the child and return without mark_json_status(.., "failed"), unlike the direct path (process.rs:536-540). Before this change, Linux took the fallback instead. The rocmd record does flip to failed. Confidence 85 · logic · Fix: mark the state failed in both arms.
• [new] Remediation hints dropped — engines/lemonade/src/direct_llama.rs:56-75, :98-112. The missing-backend error no longer says to run rocm engines install lemonade (that command does fix it), and the variant errors lost their {model}:Q4_K_M examples. Confidence 85 · mechanical · Fix: restore both.
• [new] "open to check" points the user at an overlay that never shows install status — crates/rocm-dash-tui/src/ui/tabs/pane.rs (Lemonade and managed-runtime lines). draw_engine_manager (engine_manager.rs:252-300) shows no Detect status. The wording copies the existing vLLM line. Confidence 85 · logic · Fix: say "unknown — run rocm engines list", or show status in the overlay.
• [new] Install status is captured once per session — apps/rocm/src/dash.rs:314, crates/rocm-dash-tui/src/app/event_loop.rs:805. Installing Lemonade from the Engines overlay leaves "managed runtime not installed" on screen until restart. Confidence 85 · logic · Fix: re-run Detect when an engines job completes.
• [standing] Wrong URL in errors after the /v1 fallback — crates/rocm-dash-collectors/src/lemonade.rs get_text ~309-325. The message reports the original /api/v1 URL with the fallback's status, and the transport error drops the host. Confidence 85 · mechanical · Fix: format errors with the URL that was actually requested.
• [new] Leftover parameter from the fallback design — engines/lemonade/src/process.rs:451, lib.rs:528. reason: &anyhow::Error is now always a fixed string and only gets logged. Confidence 85 · mechanical · Fix: make it &str or drop it.
• [new] Stale doc and comment wording — three places:
  • docs/testing.md:852 still calls direct serving "the packaged llama-server fallback".
  • direct_llama.rs:240-244: the DIRECT_LLAMA_SERVER_BACKENDS doc describes lookup ordering it no longer drives.
  • direct_llama.rs:1098-1111: test names and comments refer to the removed find_llama_server_binary.
  • Confidence 85 · mechanical.
• [new] A managed service bound to :: can show up twice — crates/rocm-dash-daemon/src/registry.rs ~257-261 and endpoints_match. A dual-stack :: bind on a non-default port 13305 is also reached by the external probe, giving a false "externally running" row and double-counted gen_tps. Confidence 70 · logic · Fix: match a :: record against both loopback families, and add a test case.
• [new] A twin construction drops ownership — apps/rocm/src/dash.rs:621-628. launcher_serving_instances builds Instance from registry records without managed: svc.managed, while merge_instance carries it. Harmless today, because the launcher does not read the flag. Confidence 70 · mechanical.
• [new] A stale approval swallows one keypress — services_manager.rs:132-139. The modal is hidden, but the next key only clears it. Confidence 70 · logic · Fix: fall through and handle the key normally.
• [new] Managed direct llama-server rows may carry a standing "lemonade health" warning — registry.rs:257-270, runner.rs:862-868. Every Lemonade-engine record is scraped at /api/v1/health, which a direct llama-server likely does not serve the way the collector expects. Inferred from the code, not run. Confidence 70 · logic.
• [new] Match arms the current writers never reach — direct_llama.rs:258-259 (rocm-stable/rocm-nightly). Manifest writers only ever store rocm or vulkan; deleting both arms left all tests green. Confidence 70 · mechanical.
• [new] A dead branch in lemonade_service — lemonade.rs ~247-266. Every caller passes a trimmed, non-empty name, so the empty-name path is unreachable and untested. Confidence 85 · mechanical.
• [new] The manual-testing sequence cannot run as written — docs/manual-testing.md:219-224. rocm serve qwen-smoke … --port 11435 without --managed attaches to the log, so the next rocm services stop line is unreachable. Confidence 70 · mechanical.
• [new] README never says where config.json lives — README.md:196-199. Editing the file is the only way to set dashboard.daemon.*, but the path and the ROCM_CLI_CONFIG_DIR override are not documented. Confidence 70 · mechanical.

Decisions for the author

• Removing the router-failure → direct llama-server fallback on Linux — tradeoff. Pro: a real load error is no longer hidden behind a cached-model direct serve. Con: the narrow set of users whose router load failed while a direct server was available now gets an error on first serve. Confirm this is intended, and say so in the description.
• The default model's Linux route now depends on catalog labels — non-blocking-improvement. resolve_direct_llama_checkpoint serves directly only when every label is in {reasoning, tool-calling, tools}. If the pinned 11.5.1 server_models.json labels Qwen3-4B-Instruct-2507-GGUF with anything else (Lemonade's generated docs show hot), default rocm serve silently moves to the router path, which is the one the EAI-7423 xfail rows describe. A unit fixture with the pinned entry would lock this down.
• Provenance as managed: bool plus container_name == "lemonade" — non-blocking-improvement. A Docker vLLM container named lemonade is labelled "Lemonade — externally running". The lifecycle gate uses managed alone, so the impact is cosmetic. An explicit enum Provenance { Managed, Docker, ExternalLemonade { host, port } } would make the confusion unrepresentable and carry the endpoint the dedup needs.
• The external probe is awaited inside the daemon tick loop — non-blocking-improvement. With an unreachable or slow lemonade_host, each discovery tick can stall GPU sampling and the vLLM scrape for 1.5–3 s. This follows the vLLM collector's existing pattern. A separate task feeding a channel would isolate it.
• Scope — non-blocking-improvement. 33 files across two separable changes: lemonade serving-route resolution, and dash external discovery/ownership. Splitting them would make each easier to review and to revert.

Positive signals

• serving_route decides direct vs router before any download or spawn, and re-checks the GPU-required policy there for both platforms, so no CPU path is introduced (direct_llama.rs:44).
• Direct-server readiness now requires an exact alias match (process.rs:650). Mutating it back to fuzzy matching fails the tests.
• Ownership is assigned in exactly one place (registry.rs:169), and legacy snapshots default to managed: false. An old snapshot can never unlock stop/restart.
• stats_model refuses to attribute a last-request rate unless exactly one model is loaded. The dash-26 fixture puts the rate's model first so that naive attribution fails.
• The sr + single-Esc check in external_lifecycle_keys_do_nothing is a sound black-box probe: any staged approval or job console would keep the overlay open.

Deployment notes

• rocm dash now probes 127.0.0.1:13305 for Lemonade on every discovery tick by default (Linux/WSL embedded daemon). It adds the config keys dashboard.daemon.enable_lemonade, lemonade_host and lemonade_port. They have serde defaults, so existing configs keep loading.
• The Services overlay no longer lists unmanaged instances, Docker-discovered containers included.
• On Linux, a router load or verify failure in rocm serve with the lemonade engine is now an error instead of a fallback to direct llama-server.

What this covered

• What was read. Every file in prw-base…93324b5e (a02a859a…93324b5e, two commits), split across seven read-only workers grouped by coupling:
  • lemonade direct_llama/state;
  • lemonade lib/process;
  • dash daemon, collectors and core;
  • dash TUI plus apps/rocm/src/dash.rs;
  • e2e features and steps;
  • docs plus AGENTS.md adherence;
  • history, prior changes and code comments.
• Design questions. Two more workers took the design questions in Step 7. The callers of changed symbols were traced.
• Prior changes. The pass consulted the review comments of #80, #94, #242, #404 and #457.
• Tests run. Mutation verdicts came from real test runs, mostly in per-worker copies. The non-hermetic test failure was reproduced on a clean tree.
• Did not run / limits:
  • The TUI worker read only the diff hunks of crates/rocm-dash-tui/src/app/mod.rs and app/event_loop.rs (4- and 1-line changes), not those files in full.
  • Workers shared this checkout and briefly mutated tracked files in it. The local e2e run of dash-26/27 (2 scenarios, both passed, and also green in hosted CI) may have been built with another worker's pane.rs glyph mutation in place.
  • The apps/rocm test lemonade_discovery_defaults_and_overrides_reach_daemon was not run because of the time box.
  • GPU scenarios (dash-25, serve-08, serve-25) could not run: there is no GPU here, and CI never scheduled them.
  • The pinned Lemonade 11.5.1 catalog was not available offline, so the default-model label question is unresolved.
  • The ai-instruction-quality check was not needed, since no agent-instruction files changed.
  • The leak scan of the diff was clean, and no prompt-injection content was found.
• Not reconciled. This report was not reconciled against the PR's discussion.

[volen-silo]

Thanks for the round 3 review. I pushed 77781396, which is rebased on current main. It replaces 93324b5e, so the review and the round 2 comment were written against older heads. Here is the status of each item.

Blocking

• GPU lanes: not fixed, and I cannot fix it from here. The self-hosted run for the earlier head never started. For dash-25 I added the active-runtime step, a bounded serve (a stuck rocm serve is now killed at the serve timeout instead of stalling the lane), and a flaky expectations row like the other Lemonade serve scenarios. The nightly serve-25 still needs an include-nightly dispatch on a GPU lane. I have not run it.
• Engine branches: fixed. The router path now lives in router_serve.rs behind a small trait. New tests check the persisted name, that the router name (not the alias) reaches load and verify, and that each failure stops the server and marks state failed. Other new tests check the exact pull owner/repo:variant command line and that a failed pull is returned. The load test no longer touches the real user directory. The removed direct fallback is covered only structurally: serve_router has no direct step, but a test cannot catch someone adding a fallback in serve_http itself.
• Dashboard branches: fixed. New tests cover the model and host reset, the scrape-tick hold (discovery_tick of 3 times instance_tick), the selection clamp, and the glyphs. I checked each with a mutation that makes it fail.
• Checks that cannot fail: fixed. The alias test now checks the --alias that direct_llama_server_command produces. A dangling-symlink test covers is_file(). The fixture now records any request other than a GET of health or stats, and the scenarios assert none arrived. I renamed the step to say what it checks. The managed-row check now needs a full Observe row. I could not run that last change, because dash-25 needs a GPU.
• Description: updated. It now lists the removed router fallback, the managed-only Services overlay, the complex-entry routing, and serve-25.
• discover_models: removed. Collector tests now use the function the runner calls.
• Takeover of an external endpoint by a managed record: new test. It fails if the drop loop is disabled.
• Cache-root priority: fixed, with a two-root test that checks the full path.
• lemonade_managed_installed: the verdict is now a pure function with unit tests for all outcomes.

Non-blocking

• Fixed: stale "off by default" comments; the /v1 fallback URL in errors; --help now says Linux/WSL; a failed router serve now marks state failed; the hints are back (rocm engines install lemonade --reinstall and owner/repo:Q4_K_M-style examples); the unused reason error parameter; stale doc and test names; the rocm-stable and rocm-nightly arms (removed, since no writer produces them); the dead branch in lemonade_service; managed in launcher_serving_instances; a stale approval no longer swallows a key; the "unknown, run rocm engines list" wording; the manual-testing sequence; and the README config path.
• :: bind: a managed service bound to :: now matches both loopback families, with table tests and a contract test.
• Standing warning on managed direct llama-server rows: I confirmed it by running a fixture. Such a row now stays visible with no throughput and no warning. A 503 or a transport failure on a real router still warns.
• Install status is read once per session: I did not add a live refresh. That needs a new event path from engine jobs. The pane now says that status is read at startup.
• dash-26 and dash-27: assertions moved out of the When step into Then steps, and the wording describes what the user sees. I did not split the scenarios.

Decisions

• Removing the router-to-direct fallback is intended. The description now says so.
• I added a test with the real 11.5.1 catalog entries for the default and canary models. Entries that carry other labels, such as hot or coding, go to the router on purpose.
• I left the provenance enum, the probe in the tick loop, and the split of this PR as they are.

Verification: the full workspace suite passed serially (33 targets, 3,775 tests), and so did both clippy commands, fmt, crate edges, smoke, and the external protocol scenarios. I could not run GPU scenarios, the Windows build, or lychee locally. The previous head failed the Windows job on a test-only helper with no caller there. I gated it the same way as its callers. CI will confirm.

Fixed 7778139

[rominf]

🔴 Automated review · pr-review-watcher · 7778139

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

Review — needs work

Full review of the whole change.
No ticket named; the description's claims were checked against the code (see the first and third Blocking findings).

Blocking

• [new] Several serve error paths never mark the engine state failed, while the docs and description say every error after the spawn does — engines/lemonade/src/router_serve.rs:49-56, :95-111, :112; engines/lemonade/src/lib.rs:509-529; docs/architecture.md:161
serve_router only stops the server and calls mark_json_status(.., "failed") for errors raised inside the outcome closure. Three ? exits outside it skip both: the write_running_state_for_model right after the spawn, the merge_json_state once the model is ready, and wait_server(). LemondChild has no Drop, so on those paths lemond is left running and the state stays running or ready.
  The direct route has the same gap. Errors from serving_route and ensure_direct_llama_model_available (missing binary, invalid checkpoint, ambiguous variant, a failed pull) leave the state at the starting that serve_http wrote. This change fixed the router copy of this pattern but not the direct one.
  docs/architecture.md:161 says the module will "mark engine state failed on any error after the spawn". The code is what governs here, since it is what actually runs, so the doc claim is false.
  Confidence 85 · logic · Fix: one guard in serve_http that marks the state failed (and stops any spawned child) on every Err after starting is written. Otherwise narrow the doc line to what the code does.

• [new] The only end-to-end scenario for the managed engine pane's "installed" state can never go red — tests/e2e-cucumber/expectations.toml:106-111, tests/e2e-cucumber/features/dash.feature:332
  dash-25 is tagged @requires-os:linux @requires-gpu @requires-engine:lemonade. Its new row matches { effective_engine = "lemonade", os = "linux" } with flaky = true, and that covers every lane the scenario can run on. flaky accepts both pass and fail.
  So the "Lemonade — managed runtime installed" assertions have no scenario that can fail, which AGENTS §3 requires for user-observable behaviour. The dash assertions are hidden behind an unrelated serve bug (EAI-7423). The description says the row exists, but not that it leaves this behaviour without a gate.
  Confidence 85 · logic · Fix: assert the managed pane from a scenario that doesn't need the flaky real serve (for example a planted registry record plus a real Detect), or narrow the row to the lanes where the serve bug actually occurs.

• [new] Text about the CLI's own behaviour is pinned with no pointer to a test of that behaviour, despite the description's checkbox — apps/rocm/src/main.rs (dash_help_scopes_lemonade_discovery_to_linux_and_wsl); crates/rocm-dash-tui/src/ui/tabs/pane.rs:432-438 and startup_snapshot_note_shown_only_with_a_verdict; engines/lemonade/src/direct_llama.rs:65-73 and its test at :660-672
  AGENTS §3: "Where the text genuinely has to be pinned on its own, the assertion carries a comment naming the test that proves the behaviour". None of these three does:

  • "quitting leaves external servers running" (help text);
  • "Install status is read at startup. Restart the dashboard to refresh." (engine pane);
  • the advice to run rocm engines install lemonade --reinstall, which §3 says must be shown to actually fix the condition.
    The description ticks "Discovery and ownership messages are asserted together with the resulting state."
    Confidence 85 · mechanical · Fix: add the pointer comments (dash-26 / @id:dash-lemonade-external-protocol-discovery exists for the first), and add a test that restores the backend through the reinstall path, or name one that does.

• [new] A tracked file is cited as a bare backtick path in new docs/ text — docs/testing.md (the dash-25 bullet: "serve_timeout_secs in expectations.toml")
  AGENTS §5 requires repo files cited from docs/ to be relative links. The same section already links ../tests/e2e-cucumber/expectations.toml a few lines later.
  Confidence 85 · mechanical · Fix: make it [expectations.toml](../tests/e2e-cucumber/expectations.toml).

• [standing in part since 93324b5 — not re-found blind; settled by putting it to the reviewer after its report was frozen (anchored verdict): HOLDS IN PART] The GPU lanes have not run on this head, so the hardware-only scenarios are unverified. The part about dash-25 lacking an expectations row no longer holds — that row now exists (and is the second new finding above).

  • The only self-hosted run for 7778139, "E2E self-hosted" run 38386320833, has been pending with zero jobs since 10:52Z. No GPU, Strix or WSL check run appears for this head, so dash-25, serve-08 and serve-25 have no hardware result.
  • No include_nightly dispatch exists for this head. serve-25 is still the only end-to-end check that complex catalog entries stay on the Lemonade router.
  • Fix: get the self-hosted run scheduled (it is stuck, not failed) and dispatch include_nightly once for serve-25; cite both runs.

• [standing in part since 93324b5 — not re-found blind; settled by putting it to the reviewer (anchored verdict, read-only, no mutations run): HOLDS IN PART] Some lemonade engine connections this change introduces are still exercised only through fakes. The new router_serve.rs tests, the pull-argv test, the pull-error tests and the now-hermetic load test cover most of the original mutations. What remains:

  • The removed router-to-direct fallback is covered only structurally; nothing fails if one is re-added in serve_http (the author agrees).
  • The real LemondRouterSteps::load_model, model_loaded and chat_smoke (router_serve.rs:176-205) could ignore the name they receive and use self.request.model_ref; router_name_not_alias_reaches_load_loaded_and_smoke_checks drives FakeSteps only.
  • The real pull closure in ensure_direct_llama_model_available (direct_llama.rs:137-169) could pull request.model_ref instead of the checkpoint ref and stay green.
  • Fix: one test through the real LemondRouterSteps against a fake HTTP server asserting the name on the wire, and one through the real pull closure with the existing fake-executable pattern.

• [standing in part since 93324b5 — not re-found blind; settled by putting it to the reviewer (anchored verdict): HOLDS IN PART] Two checks still claim coverage they cannot fail on. The is_file() dangling-symlink test and the renamed stray-request step now fail as they should.

  • The alias test (direct_llama.rs:605-628) builds the command from its own requested value, so the launch_service / serve_http path to direct_llama_server_command is never exercised. That is also why the launch normalization change listed under Non-blocking is uncaught.
  • "Real managed Lemonade model is displayed": observe_row_cells (dash_steps.rs:302-307) needs a cell starting with "Qwen3-0.6B" followed by 7 more cells, with no port or status check. Managed rows already appeared before this change, so the step passes with it reverted — and dash-25 sits under a flaky row anyway.
  • Fix: drive launch_service or serve_http and assert the served --alias; anchor the managed-row check to that row's port or status.

Standing objections from 93324b5 that no longer hold

Each was not re-found by this round's blind review, then put to the reviewer one at a time with the author's reply (anchored verdict, from reading; no mutations could run). All were judged no longer to hold:

• Dashboard branches (telemetry reset, scrape-tick hold, selection clamp, glyph): each now has a named test that would plausibly fail under the original mutation.
• discover_models: removed; the collector tests call the runner's services_from_health.
• External rows dropped on managed takeover: lemonade_managed_record_taking_over_endpoint_drops_existing_external_rows exercises the transition.
• Cache-root priority: per-root filtering in priority order; checkpoint_matches_prefer_the_highest_priority_cache_root orders roots so alphabetical and priority order disagree, and asserts the full path.
• lemonade_managed_installed: the pure managed_installed_from_detect is unit-tested on all outcomes.
• Description scope: the description now lists the removed fallback, the managed-only Services overlay, complex-entry routing and serve-25. The commit messages still do not mention the removed fallback or the Services-overlay change; that is an omission, not a false claim, and does not gate.

Non-blocking

• [new] A stale cached collector can take over a new managed endpoint and dial the wrong IP family — crates/rocm-dash-daemon/src/runner.rs:770-797, crates/rocm-dash-daemon/src/registry.rs:251-272
  Grouping chains managed_lemonade_collectors.keys() into endpoints_match, which is deliberately non-transitive: :: matches 127.0.0.1. Say a :: record is removed and a 127.0.0.1 service takes the same port. It keeps the cached collector, which dials scrape_host("::") = "::1" against an IPv4-only listener, and the warning never clears. This contradicts the in-code comment "without … conflating IPv4 and IPv6 listeners". No test exercises the cached-key reuse branch.
  Confidence 70 · logic · Fix: reuse a cached collector only when scrape_host(cached) == scrape_host(target.host).

• [new] Managed Lemonade rows now show the last request's throughput as a fresh live rate indefinitely — crates/rocm-dash-daemon/src/runner.rs:925-935, crates/rocm-dash-collectors/src/lemonade.rs:121
/api/v1/stats reports the most recent request (module doc, lines 8-10). Every scrape calls observe_direct, which refreshes last_observed_at, so an idle server keeps showing, say, 42 tok/s as Fresh. External rows already behaved this way; this change extends it to every managed Lemonade row. The doc comment at :121 calls the value "instantaneous".
  Confidence 70 · logic · Fix: fingerprint the stats tuple and stop refreshing freshness (or report 0) while it is unchanged.

• [new] The engine's launch method no longer normalizes model_ref, and the description doesn't mention it — engines/lemonade/src/lib.rs:451 vs :409
  Through Launch, qwen-smoke is now served with --alias qwen-smoke, and direct readiness requires that exact name. resolve still reports Qwen3-0.6B-GGUF. rocm serve and rocmd are unaffected, because they pass canonical_model_id to serve-http. This still changes the rocm-engine-protocol contract (AGENTS §6).
  Confidence 70 · architectural · Fix: keep normalizing in launch_service, or pin the new identity with a protocol test and mention it.

• [new] The new lib.rs test reads with unwrap from sockets that are non-blocking on Windows — engines/lemonade/src/lib.rs:694-726 (serving_regression_persisted_identity_drives_health_chat_model)
  listener.set_nonblocking(true), then stream.read(..).unwrap() on the accepted streams. On Winsock, accepted sockets inherit non-blocking mode, so an early read returns WouldBlock and panics. The Windows lane runs cargo test; it passed once on this head.
  Confidence 70 · mechanical · Fix: call stream.set_nonblocking(false) after accept.

• [new] run_rocm_bounded limits the whole CLI serve to the harness's readiness window — tests/e2e-cucumber/tests/e2e/serving_steps.rs:150-151, tests/e2e-cucumber/tests/e2e.rs (run_rocm_bounded)
  For @serve-timeout:2400 the harness clock starts at spawn, but the CLI's own wait (ROCM_CLI_VLLM_READY_TIMEOUT_SECS) starts after its pre-launch work. The harness therefore kills the CLI first, and the CLI's own diagnostic is lost in the slow cold-load case. The 90 s rows now also have to fit any cold backend or weights fetch into that window.
  Confidence 70 · logic · Fix: give the CLI call timeout + margin, or a separate launch budget.

• [new] serve-25 runs rocm serve --managed through the unbounded run_rocm and has no EAI-7423 row — tests/e2e-cucumber/tests/e2e/serving_steps.rs:676-689, tests/e2e-cucumber/features/model_serving.feature:326
  This change adds run_rocm_bounded precisely because the unbounded call "stalls the whole lane with no report". serve-25 is the same Linux managed Lemonade serve as the rows that carry EAI-7423, but has no row.
  Confidence 70 · logic · Fix: use run_rocm_bounded, and add a row or say why the router path isn't affected.

• [new] write_dashboard_config overwrites config.json and drops the runtime that the previous step activated — tests/e2e-cucumber/tests/e2e/dash_steps.rs:92-101
  This is harmless today only because Lemonade skips the active-runtime check (serve_cmd.rs:722-730).
  Confidence 70 · mechanical · Fix: read the file, set only dashboard.daemon.*, and write it back.

• [new] No gating scenario asserts the new Linux direct route for simple registered names — engines/lemonade/src/lib.rs:512-530; tests/e2e-cucumber/features/model_serving.feature:61-70
  The only scenario serving Qwen3-0.6B-GGUF is covered by a flaky row on Linux and checks inference only. If the route were reverted to the router, every scenario would stay green.
  Confidence 70 · logic · Fix: assert load_response.method on a gating lane, or name the unit test covering the route in the PR text.

• [new] The README says unrecognized catalog shapes use the router, but on Linux a malformed registered checkpoint fails the serve — README.md (~681); engines/lemonade/src/direct_llama.rs:46-47, :359-385
resolve_direct_llama_checkpoint runs before direct_serve_binary and only when is_linux || explicit_checkpoint. On Linux the malformed entry fails even if no direct binary exists, while on Windows the same entry goes to the router. The comment "must not escape validation via routing" holds only on Linux. The code governs, because it is what runs.
  Confidence 70 · logic · Fix: send a malformed registered entry to the router unless the user gave an explicit owner/repo reference, or document the error.

• [new] A sharded GGUF variant in a registered entry goes direct, downloads every shard, then fails as ambiguous — engines/lemonade/src/direct_llama.rs:97-127, :508-540
collect_gguf_files recurses into the shard folder. The substring filter then matches every -0000N-of-0000M file and bails "matches multiple files". This contradicts the new docs line that multi-file entries stay with the router. It was not a working path before either. Whether the pinned catalog actually contains such an entry was not checked.
  Confidence 70 · logic · Fix: when every match shares one shard stem, pick shard 00001, or route split matches to the router before downloading.

• [new] A Lemonade router serving on Vulkan never reaches the CLI's readiness check, and more Linux serves now take the router — apps/rocm/src/main.rs:19055, :19113-19120
  The CLI accepts a router model only when its backend starts with rocm, while the engine side accepts vulkan (router_serve.rs tests record backend_requested: "vulkan"). The check existed before this change. What is new is that complex entries on Linux are now routed through the router, so a Vulkan host serving one waits forever.
  Confidence 70 · logic · Fix: accept the backend the engine recorded, and add a Vulkan router case to the readiness tests.

• [new] No GPU-lane check ran for this head — CI rollup
  Every hosted check passes. No self-hosted GPU-lane checks appear, so dash-25, serve-08 and nightly serve-25 have no result for 7778139.
  Confidence 70 · mechanical · Fix: run the required GPU lanes before merging.

Decisions for the author

• This PR makes two logical changes — non-blocking-improvement
  The Lemonade serve-route rework (registered-GGUF resolution, router extraction, removal of the direct fallback) and the dash discovery and ownership rework share almost no code. Together they come to about 5,300 lines. AGENTS §11 asks for one logical change per PR; splitting would let each be gated and reverted on its own.
• The Linux router-to-direct fallback was removed — tradeoff
  The description already discloses this. Router failures now surface as errors instead of being masked, at the cost of a first-serve failure on hosts where a direct server would have worked. Confirm this is the intended choice for Vulkan and WSL hosts, given the readiness gap above.

Positive signals

• Moving the router path into router_serve.rs behind a RouterSteps trait makes the failure paths testable without a real server. router_failures_return_error_stop_server_and_mark_state_failed would fail on the old code.
• Lifecycle ownership comes from a positive managed flag from the registry rather than id or engine string markers. The approval tests cover a stale target on both the key and draw paths.
• The e2e protocol fixture records every request that is not a health or stats GET, which turns "never touches external servers" into something the scenarios actually assert.

Deployment notes

• rocm dash now probes local Lemonade (default port 13305) by default on Linux and WSL; it can be turned off in config.
• The Services overlay now lists only registry-managed services. Docker-discovered containers and external Lemonade servers no longer appear there.
• A newer TUI attached to an older daemon gets no managed field, so its Services overlay is empty (by design: missing ownership is treated as unmanaged).

What this covered

Every file in the change was read completely (37 files), plus callers and callees in apps/rocm, apps/rocmd, rocm-core, the vLLM engine and the e2e harness. Diff prw-base…77781396 (merge base 2844d79).

What did not run, and why:

• No build or test ran. The host's free disk fell below the build floor during the review, so builds were stopped. Every finding is from reading, none is scored 100, and all mutation checks — including the ones behind the standing-objection verdicts above — are unverified.
• telemetry_contract.rs lines outside the diff were not re-read.
• The pinned Lemonade 11.5.1 server_models.json is not on this host, so the sharded-catalog question is unverified.
• The prior-changes pass was limited to inline comments on about seven earlier merged PRs.
• GPU and Windows execution is not possible here.
• No prompt-injection content was found.

The review ran fanned out across six independent workers: two for the engine, one for dash collectors/daemon, one for the TUI, one for e2e and docs, and one for the instruction, history, prior-change and comment passes. The findings were reached without the PR's discussion; the standing objections above were then settled against the author's reply of 10:53Z.

[volen-silo]

Thanks for round 4. I pushed 11b64524, rebased on current main. Here is the status of each item.

Blocking

• Failed state on every error: fixed. One wrapper in serve_http now marks the state failed on any error after starting, for the router and the direct route, and returns the original error. The router path also stops its server on the three exits you listed. I corrected the architecture line. One gap is left: the direct llama-server path still does not stop its child at its own post-spawn state writes. The state does end at failed.
• dash-25 cannot go red: agreed, and I did not add a hardware scenario that I cannot run. docs/testing.md now says dash-25 does not gate the installed wording, and names the unit tests that do. No other scenario shows the installed state through a real Detect.
• Pointer comments: added for the help text (dash-26), the startup status note (a new test shows the status does not change after startup), and the reinstall advice. For the reinstall advice I found no scenario that proves the backend binary is restored, and the comment says so. The two --reinstall scenarios prove only that the command and flag exist.
• Bare path in docs/testing.md: now a relative link.
• GPU lanes: unchanged. The self-hosted run is not mine to schedule, and the nightly serve-25 still needs an include-nightly dispatch.
• Fakes only: fixed. New tests drive the real router steps against a loopback fixture and a fake CLI, and the real pull closure with a fake executable. Both fail if the alias is used instead of the router name or checkpoint. A new test also runs the serve path and asserts --alias. The removed fallback is still covered only structurally.
• Checks that cannot fail: the alias test now goes through the serve path. For the managed row, the Observe row has no status or port column, so I check status and port in the instance detail view. The matcher has unit tests for wrong status, wrong port, and truncated rows. I could not run it on a GPU.

Non-blocking

• Stale cached collector: fixed. A cached collector is reused only when it dials the same host. A new contract test fails with the old behavior.
• Stale throughput shown as fresh: fixed. The collector keeps the last stats and refreshes freshness only when they change.
• Launch normalization: restored in the engine's launch path. The requested text is not kept separately there, because the serve request has one field. The protocol contract is unchanged.
• Non-blocking sockets in the Windows test: fixed through a shared fixture that sets blocking mode after accept. I could not reproduce the failure on Linux.
• run_rocm_bounded: the CLI budget is now the readiness timeout plus a margin.
• serve-25: now bounded. I did not add an expectations row, because it takes the router route, and a comment explains why.
• write_dashboard_config: now merges instead of overwriting.
• Simple-name direct route: no gating scenario. The description names the unit tests that cover it.
• README on malformed checkpoints: reworded. A malformed registered checkpoint is rejected on Linux with an error. This is intended.
• Sharded GGUF: confirmed. Two eligible entries in the 11.5.1 catalog are sharded. A complete shard set now resolves to the first shard, and an incomplete set is pulled again.
• Vulkan readiness: confirmed and fixed. The CLI check now accepts any GPU backend the engine selects, never cpu.

Not changed: the provenance enum, the probe in the tick loop, and the PR split.

Verification on the rebased tree: the full workspace suite passed serially (33 targets, 3,897 tests), and so did both clippy commands, fmt, crate edges, smoke, and the external protocol scenarios. GPU scenarios, the Windows build, and lychee were not run locally.

Addressed in 11b6452. Details are in the reply comment.

[rominf]

🔴 Automated review · pr-review-watcher · 788a1a3

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.

Informational — this describes commit 788a1a3, not the current head. The head moved to 5e0a33d while this round was running. A range-diff shows 5e0a33d is a rebase onto newer main that switches direct_llama.rs to crate::test_support::scratch_dir, which looks like it resolves the merge-conflict finding below. The other findings were not re-checked against 5e0a33d. This comment does not gate and files no change request. Its blocking findings are open, and the next round checks them against the current head.

Review — needs work

Full review of the whole change (two commits, 40 files, +6629/−741).
No ticket named. The description makes specific claims, and one of them is wrong (see the first Blocking item).

Blocking

• [standing since 7778139, re-found blind this round] The direct llama-server route still leaks a running server when a step after spawn fails, and both the description and a new doc comment say it doesn't — engines/lemonade/src/process.rs:577 (write_running_state(...)?), process.rs:604 (merge_json_state(...)?), process.rs:~624 (child.wait().context(...)?), engines/lemonade/src/lib.rs:545
  • In serve_direct_llama_server, only the readiness and smoke-check closure kills and reaps the child on error.
  • The three ? returns listed above exit without terminate_pid or child.wait(). std::process::Child does not kill its process on drop. Then serve_http_resolving marks the state failed, so stop and health can no longer find the server, and it stays orphaned on the port.
  • The PR fixed exactly this for the router twin (router_serve.rs:50-52, with the StateWrite, ReadyMerge and WaitServer tests).
  • The description says: "Any error after a serve starts now returns its own error, stops a spawned server, and marks the engine state failed, for the router and the direct route." The new doc comment at lib.rs:545 says "Servers spawned inside serve_with_context are stopped by the code that spawned them." The code governs here, because it is what runs, so both claims are false for the direct route.
  • Two workers found this independently. I confirmed it by reading the function at HEAD.
  • Confidence 85 · logic · Fix: wrap every post-spawn step of serve_direct_llama_server in the same kill-and-reap-on-error block serve_router uses. Add a regression test that makes the state path unwritable after spawn, as Fail::ReadyMerge does. Alternatively, narrow the description and the comment.
• [new] The branch conflicts with main and cannot merge as is — engines/lemonade/src/direct_llama.rs, engines/lemonade/src/lib.rs
  • git merge-tree prw-base HEAD reports content conflicts in both files, and the API reports mergeable: false, mergeable_state: dirty for head 788a1a3.
  • The cause is test: race-check scratch names in vllm and lemonade (follow-up to #603) #620 (6870dce), which moved the per-module scratch_dir helpers into crate::test_support::scratch_dir. This PR adds yet another local tempfile-based scratch_dir in direct_llama.rs (~L625-636).
  • Confidence 100 · mechanical · Fix: rebase, drop the local helper and use crate::test_support::scratch_dir. Re-run the Windows check after the rebase.
• [new] The suggested fix in the "matches multiple files" error does not resolve for split GGUF sets — engines/lemonade/src/direct_llama.rs:117-135 (message at L130), first_shard_of_complete_set (~L520), is_incomplete_shard_set (~L541)
  • Setup: a complete split set plus an unrelated file with the same label. The error then suggests the first shard's exact filename, e.g. o/r:model-Q4_K_M-00001-of-00002.gguf.
  • An exact-filename variant matches only that one shard, so the set reads as incomplete and direct_llama_model_path returns Ok(None). The serve then pulls and fails with "did not download". A probe test returned None.
  • serving_variant_errors_carry_concrete_examples checks that the example resolves, but only for non-split files.
  • AGENTS §3: remediation advice must actually clear the condition that printed it.
  • Confidence 100 · logic · Fix: when the matches include a split set, suggest the quantization label or shard folder instead. Add a split case to the test.
• [new] A shard set can be assembled across cache roots or snapshot revisions — engines/lemonade/src/direct_llama.rs:478-497, 520-538
  • Matches are deduplicated by file name across every root and revision.
  • In a run, shard 1 in root h1 plus shard 2 in root h2 counted as complete and returned h1/.../-00001-of-00002.gguf. llama-server looks for the remaining shards next to shard 1, so the launch fails after a spawn.
  • Separately, removing the (1..=total) index-range check at ~L530 leaves the whole suite green (run), so an out-of-range set such as {00001, 00003}-of-00002 is untested.
  • Confidence 80 · logic · Fix: count a set as complete only when every shard shares the first shard's parent directory. Add tests for the split-root and out-of-range cases.
• [new] A Lemonade last-request rate stays Fresh indefinitely, contradicting the PR's own contract comment — crates/rocm-dash-daemon/src/runner.rs:941, crates/rocm-dash-daemon/tests/telemetry_contract.rs:910-912, crates/rocm-dash-collectors/src/lemonade.rs:121
  • Every successful stats scrape calls observe_direct(rate, cycle_at), which refreshes last_observed_at. An idle server returning the same tokens_per_second body therefore never goes Held or expires.
  • The doc comment the PR adds at telemetry_contract.rs:910 says "An unchanged stats body is the same request, so it must not keep the rate fresh: once the validity window passes the rate becomes unavailable." The test beneath that comment checks something else (no standing warning on a direct llama-server row), so no test checks the stated contract.
  • The comment governs: it matches parse_stats's own doc ("not a live rate, stays the same until another request completes").
  • The runner logic predates the PR. Its blast radius is new: the PR turns on external discovery by default and starts scraping managed Lemonade.
  • Confidence 80 · logic · Fix: observe only when the stats body (or token counts) changes from the previous scrape for that row, and add the test the comment describes. Otherwise delete the orphaned comment.
• [new] The Serving preview says nothing is running while external models are serving, and a new test pins that — crates/rocm-dash-tui/src/ui/tabs/pane.rs:471, test at pane.rs:~706-713
  • The preview now counts managed services only but still says "Running now — N". With two external models Ready, it shows "Running now — 0" in the same tab where Engines says "Lemonade — externally running".
  • services_preview_ignores_external_only_instances asserts that text. AGENTS §3: "a pin over a false claim holds the claim in place."
  • Confidence 85 · logic · Fix: relabel it as managed services (e.g. "Managed services running — N", with an external read-only count). Do the same for the overlay's "No managed services." empty state.
• [new] The empty-state advice "check an external endpoint" points at a Serving action that doesn't exist — crates/rocm-dash-tui/src/ui/tabs/instances.rs:380, crates/rocm-dash-tui/tests/dash_characterization.rs:181-182
  • None of the Serving actions can check or configure an external endpoint, which is set by the dashboard.daemon.lemonade_host/lemonade_port config.
  • The characterization comment says dash-27 proves this message together with the behaviour. dash-27 only navigates to Serving.
  • Confidence 85 · logic · Fix: drop the clause or name the real control (the config keys), then correct the comment.
• [new] The new manual-testing text describes --managed behaviour the CLI no longer has — docs/manual-testing.md:272-273
  • The text says: "Without it, rocm serve stays attached to the log stream and the stop command above is not reachable."
  • apps/rocm/src/serve_cmd.rs:542: let _ = managed; // background is now the default; --managed is accepted as an explicit synonym. The serve only attaches with --foreground/--verbose, and the rocm serve --help doc comment agrees.
  • Confidence 95 · mechanical · Fix: delete the "Without it…" sentence.
• [new] SystemInfo.lemond_version is now dead — crates/rocm-dash-core/src/metrics.rs:59
  • The PR removed its only reader (the old Engines line in pane.rs). Its only writer, demo.rs:557, sets it to None.
  • Confidence 70 · mechanical · Fix: remove it, or mark it as a reserved or deprecated wire field if snapshot compatibility needs it.

Standing objections from 7778139 this round did not re-find

This round's blind review did not re-find these, and they were not settled this round. The next round settles them:

• Pinned CLI strings with no pointer to a behaviour test (help text, startup status note, reinstall advice).
• Some router-step and pull-step methods tested only through fakes.
• The alias test and the managed-row e2e check could not fail (the managed-row half overlaps the dash-25 item under Non-blocking).
• The bare-path citation in docs/testing.md (below the gating floor on its own).

Non-blocking

• [standing since 93324b5, re-found blind] No GPU or self-hosted lane ran on this head. statusCheckRollup lists only the hosted checks (all green). The router-route change, including removal of the Linux router→direct fallback, has no hardware evidence on this commit. dash-25, serve-08 and serve-25 have not run. The PR text acknowledges this. The dirty state may be what is keeping those lanes off. Confidence 85 · architectural.
• [standing since 7778139 as blocking, re-found blind and rated non-blocking this round] dash-25 can never gate. The flaky row at expectations.toml:106-111 matches every lane the scenario can run on (@requires-os:linux @requires-engine:lemonade), so its Observe-row and instance-detail assertions are advisory too. docs/testing.md:200-202 only concedes that the "installed" wording is ungated, which implies the other assertions do gate. Confidence 80 · mechanical · Fix: say in the docs that the scenario is advisory, or cover the Observe row and instance detail in a non-GPU scenario with a planted managed record.
• [new] dash-27's wording is stronger than what it shows.
  • "Lemonade returns to the managed installation status" (dash.feature:382): this scenario never shows the externally-running state first.
  • "stops responding" (dash.feature:380): the step only produces connection-refused, not a hung server.
  • Confidence 70 · mechanical.
• [new] Three host normalisers have already drifted — registry.rs:230-247 (host_reach, which lowercases), 271-277 (scrape_host) and 302-309 (discover_managed_services). As a result, LOCALHOST matches in endpoints_match but fails cached_collector_reusable. Today the only cost is a rebuilt collector. Confidence 75 · mechanical · Fix: one shared normaliser.
• [new] Inaccurate comments.
  • runtime_dir.rs:45-46 says rooted_at's "only callers" are in process.rs. It is also called from direct_llama.rs, lib.rs and router_serve.rs.
  • The launch_serve_request doc in lib.rs says "serve-http itself never normalizes", but the router route does normalize.
  • docs/testing.md:215 credits the rocm dash --help test to rocm-dash-tui. It lives in apps/rocm/src/main.rs (dash_help_scopes_lemonade_discovery_to_linux_and_wsl).
  • Confidence 75–90 · mechanical.
• [new] The two IPv6 contract tests return silently when the bind fails (lemonade_dual_stack_managed_bind_does_not_duplicate_external_row, lemonade_managed_ipv6_record_replaced_by_ipv4_record_on_same_port_recovers). On a host without IPv6, or on Windows where [::] is v6-only, they pass without testing anything. Confidence 70 · mechanical · Fix: emit a skip message or cfg_attr(..., ignore) with a reason.
• [new] is_external_lemonade keys on container_name == "lemonade" (metrics.rs:229-232). A Docker container a user names lemonade would show as "Lemonade — externally running". Confidence 70 · architectural · Fix: an explicit engine/source field.
• [new] The lemonade_managed_installed wiring in resolved_args is untested (apps/rocm/src/dash.rs:256-265, 328). Only the pure managed_installed_from_detect is tested, and only dash-26 catches the event_loop.rs:805 assignment. Confidence 70 · mechanical.
• [new] Pre-existing, made more visible: serve_http_resolving adds a second late failed write. This widens an existing race where a user-initiated stop (writes stopped) can end up recorded as failed. Confidence 70 · logic.

Decisions for the author

• This PR does two separable things — non-blocking-improvement
  • One is Lemonade serve routing and identity (engines/lemonade). The other is dashboard discovery and ownership (rocm-dash-*, TUI, e2e).
  • At 6.6k lines, each half reviews worse for sharing a PR with the other. Splitting would also let the dash half land without waiting on GPU-lane evidence for the serving half.
• Removing the Linux router→direct fallback — tradeoff
  • Errors are now honest, and a failed router load no longer hides behind a different serving path.
  • The cost: hosts where the router fails but a direct server would have worked now fail on the first serve.
  • The description states this; confirm it is deliberate given that no hardware lane has run.
• A malformed registry entry fails the serve on Linux even when no direct binary is installed — tradeoff
  • The ? on resolve_direct_llama_model (direct_llama.rs:48-50) runs before the direct_serve_binary check, so Linux errors where Windows would route to the router.
  • The comment near L378 calls the error deliberate. Confirm it is also meant for hosts where the direct path cannot run.

Positive signals

• Extracting router serving behind the RouterSteps trait (router_serve.rs) made post-spawn failure paths testable with a fake. Seven single-branch mutations in that area each turned a test red.
• Ownership is a positive, opt-in managed flag with #[serde(default)], so a legacy snapshot never gains lifecycle control. Tests confirm that id prefixes cannot fake it. dash-26 failed under the is_listed_service → true mutation (run).
• The e2e Lemonade protocol fixture logs every non-GET-health/stats request, which gives a real negative check against lifecycle calls reaching external servers.
• The 404-only /v1 fallback never falls back on 401/403/5xx, and that is tested with .expect(0).
• find_llama_server_binary_for_backend now rejects cpu and unknown backends instead of mapping them to vulkan, so the no-CPU-fallback rule is enforced and tested on every route.
• The docs/ citations the PR adds are relative links, README stays link-free, and the leak scan of the diff came back clean.

Deployment notes

• rocm dash now enables read-only local Lemonade discovery by default on Linux and WSL (dashboard.daemon.enable_lemonade defaults to true, 127.0.0.1:13305). Existing users will see external models appear in Observe on upgrade.
• Docker-discovered containers are no longer listed in the Services overlay, so they can no longer be stopped or restarted from the dashboard.
• On Linux, a failed router load no longer falls back to a direct llama-server; it now fails the serve.

What this covered

Every file in prw-base...HEAD (6870dce…788a1a3f). Six independent workers covered: lemonade direct_llama/runtime_dir/state; lemonade lib/process/router_serve/test_http; dash collectors/core/daemon plus apps/rocm/src/dash.rs; the dash TUI; main.rs, rocm-core and the docs; the e2e harness and features.

• Not read end to end: the TUI worker read only the changed regions of app/mod.rs, event_loop.rs, types.rs and instances.rs (1–4 changed lines each in files of 500–3400 lines), plus every user of the new symbols.
• What was run: lemonade lib tests (166 passed), daemon telemetry_contract (14), the collectors/core/daemon/TUI lib tests, clippy -D warnings on the touched crates, and e2e dash-26/dash-27 (both passed, both failed under targeted mutations). About 25 production mutations in total.
• Caveat on the runs: the workers ran concurrently in the same disposable checkout, and some mutation runs overlapped another worker's edits. The kills reported were each attributable to that worker's own mutation.

What did not run, and why:

• The history, prior-changes and code-comment passes could not get an independent worker. Spawns hit the concurrent-subagent cap repeatedly, and three retries failed. I ran a shortened history and prior-changes pass myself (git log of the touched files; review threads on fix(lemonade): align ROCm llama.cpp backend with the active ROCm SDK (EAI-8272) #404, ROCMAI-52: Modularize engines/lemonade/src/lib.rs #457 and test(dash): hold the observation clock so gen_tps scenarios cannot race the host #412), so it was not independent. Code-comment adherence was covered only through the per-group workers.
• The Step 7 design questions were not fanned out as separate workers for the same reason. The decisions above come from the consolidating context.
• GPU, Windows-hardware and nightly scenarios (dash-25, serve-08, serve-25) were not run, and no self-hosted lane appears in CI for this head.
• No prompt-injection content was found in the checkout.

Not reconciled against any PR discussion here. The driver read the author's reply of 09:19Z. The standing objections that reply addresses and this round did not re-find are listed above as unsettled.

[rominf]

🔴 Automated review · pr-review-watcher · 5e0a33d

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

Review — needs work

Full review of the whole change.
No ticket named. The description matches the change closely. It discloses the removed Linux router-to-direct fallback, that the Services overlay now hides Docker containers, and the scenario gaps it knows about.

Round 6. This round reviewed 5e0a33d in full and blind, because the previous review point, 788a1a3, is no longer in the branch. Each finding is marked [new] or [standing]. Standing objections the blind pass did not re-find were each put to the reviewer afterwards. That check is anchored: the reviewer had been told the objection. Each is listed below with its outcome.

Blocking

• [standing since 788a1a3, re-found blind] An idle Lemonade's last-request rate stays "Fresh" forever, against the contract the PR's own test file states — crates/rocm-dash-daemon/src/runner.rs:932-941, crates/rocm-dash-daemon/tests/telemetry_contract.rs:910-912, crates/rocm-dash-collectors/src/lemonade.rs:117-122
  • /api/v1/stats tokens_per_second is the rate of the most recent request. The PR's own doc on parse_stats says it "stays the same until another request completes".
  • The runner calls observe_direct(rate, cycle_at) on every stats scrape, and observe_direct re-stamps last_observed_at without checking whether the body changed. So an idle server's old rate shows as Fresh on every cycle and never reaches the validity-window expiry.
  • The doc comment added in 5e0a33d says the opposite: "An unchanged stats body is the same request, so it must not keep the rate fresh: once the validity window passes the rate becomes unavailable". No code does this. The test the comment sits on (lemonade_managed_direct_llama_server_row_has_no_standing_warning) tests something else.
  • The comment governs: it is the only statement of intent, and it agrees with parse_stats' own description of the value.
  • The base branch had the same weakness for external discovery, but discovery was off by default there. This PR turns it on by default and scrapes managed Lemonade services for the first time, which widens the blast radius.
  • dash-27's external_single_model_rate step asserts "38" tok/s from a fixture that serves no requests.
  • Confidence 85 · logic · Fix: call observe_direct only when the stats body changes (output tokens or decode times differ), and add the test that comment describes. Or remove the comment and label the value "last request".
• [standing since 7778139, re-found blind] The new remediation advice is pinned as wording only, and its own comment says no test proves it — engines/lemonade/src/direct_llama.rs:67-75, :712-733
  • The error tells the user to run rocm engines install lemonade --reinstall to restore a missing llama-server binary.
  • The test comment says: "This pins the wording only … NO test proves that the llama-server binary is back afterwards: that needs a real backend download, which no scenario asserts."
  • AGENTS.md §3 requires remediation advice to be asserted together with the command actually clearing the condition. A wording-only pin is allowed only when it names a test that proves the behaviour.
  • The comment's reason does not hold. @id:runtime-lemonade-backend-alignment-reported (runtime-16, @nightly @requires-gpu) already runs that exact reinstall with a real backend download. One added Then there would prove the binary is present afterwards.
  • Confidence 80 · mechanical · Fix: add a step to runtime-16 asserting that the selected backend's llama-server exists after the reinstall (or that a direct serve then starts). Point the pin's comment at that step.
• [standing since 7778139, not re-found blind; confirmed in full by the follow-up check] The direct llama-server route leaves a running server behind when a step after spawn fails, while a doc comment says it doesn't — engines/lemonade/src/process.rs (serve_direct_llama_server, from L504), engines/lemonade/src/lib.rs:547-548
  • After command.spawn(), three ? returns exit without stopping the child: write_running_state(..., Some(child.id()), "running")?, merge_json_state(&request.state_path, …"ready"…)? and child.wait().context(…)?. Only the readiness/smoke closure's error arm calls terminate_pid and child.wait().
  • std::process::Child does not kill the process when dropped. serve_http_resolving then marks the state failed, so neither stop nor health can reach the server that is still running.
  • The doc comment on serve_http_resolving says "Servers spawned inside serve_with_context are stopped by the code that spawned them". That is false for these three sites. The PR description says the same for "the router and the direct route". The author's reply on 7778139 conceded the gap, and the code has not changed since.
  • The router twin got this fix (router_serve.rs StateWrite/ReadyMerge/WaitServer tests). No test makes a post-spawn step fail on the direct route.
  • Confidence 85 · logic · Fix: put one terminate-and-reap guard around every post-spawn step, as serve_router does. If wait() itself fails, still attempt terminate_pid. Add a failure-matrix test for the direct route.
• [standing since 788a1a3, not re-found blind; confirmed by a run in the follow-up check] The suggested fix in the "matches multiple files" error does not resolve for split GGUF sets — engines/lemonade/src/direct_llama.rs (ambiguity error ~L117-135, first_shard_of_complete_set, is_incomplete_shard_set)
  • Setup: the existing split_gguf_variant_resolves_to_its_first_shard fixture, which has a complete two-shard set plus other-Q4_K_M.gguf. The error suggests …:model-Q4_K_M-00001-of-00002.gguf. Fed back through direct_llama_model_path, that suggestion returns Ok(None).
  • The reason: an exact-filename variant matches only that one shard, so the set reads as incomplete.
  • The serve would then pull and fail with "did not download". That part was read from the code, not run.
  • serving_variant_errors_carry_concrete_examples only covers non-split files. AGENTS.md §3 requires that remediation advice actually clears the condition that printed it.
  • Confidence 100 · logic · Fix: when the matches include a split set, suggest its quantization stem instead. Or let an exact shard filename resolve to its whole set. Add a split case to the test.
• [standing since 788a1a3, not re-found blind; confirmed by runs in the follow-up check] A shard set can be assembled across cache roots or snapshot revisions — engines/lemonade/src/direct_llama.rs (dedup ~L484-497, collect_hf_checkpoint_ggufs ~L459-468, first_shard_of_complete_set ~L520)
  • Matches are deduplicated by file name only. Shard 1 in root h1 plus shard 2 in root h2 counted as complete and returned the h1 shard. The same happened with shard 1 under snapshots/revA and shard 2 under snapshots/revB.
  • llama-server looks for the remaining shards next to shard 1, so the launch fails after spawn. On the direct route it then also hits the cleanup gap above.
  • Deleting the (1..=total) index-range check leaves the engine suite green (166 passed), so the {00001, 00003}-of-00002 case has no test.
  • Confidence 85 · logic · Fix: count a set as complete only when every shard shares the first shard's parent directory. Add the cross-root and out-of-range cases as tests.
• [standing since 788a1a3, not re-found blind; preview half confirmed, overlay half dropped] The Serving preview says nothing is running while external models are serving, and a new test pins that — crates/rocm-dash-tui/src/ui/tabs/pane.rs:463-477, test services_preview_ignores_external_only_instances (L706-713)
  • The preview now counts only managed, serving instances (is_listed_service(i) && i.status.is_serving(); base counted every serving instance). The label was not changed and still reads "Running now — {n}" / "none running".
  • With two external models Ready, the Serving tab says "Running now — 0 / none running", while the Engines pane in the same tab says "Lemonade — externally running". The new test asserts exactly that text. AGENTS.md §3: "a pin over a false claim holds the claim in place."
  • The overlay's "No managed services." empty state is correctly scoped and is proven by dash-26. That half of the earlier objection is withdrawn.
  • Confidence 85 · logic · Fix: relabel the preview as managed (e.g. "Managed running — N" / "no managed services running") and update the pinned strings.
• [standing since 788a1a3, not re-found blind; confirmed by the follow-up check] The new manual-testing text describes --managed behaviour the CLI no longer has — docs/manual-testing.md:278
  • The doc says: "Without it, rocm serve stays attached to the log stream and the stop command above is not reachable." But apps/rocm/src/serve_cmd.rs:542 reads let _ = managed; // background is now the default; --managed is accepted as an explicit synonym., and only --foreground/--verbose attach (both conflicts_with = "managed").
  • AGENTS.md §5 requires docs to match the code path.
  • Confidence 95 · mechanical · Fix: delete that sentence, or say that --managed is the default.
• [standing since 788a1a3, not re-found blind; confirmed by the follow-up check] SystemInfo.lemond_version is now dead — crates/rocm-dash-core/src/metrics.rs:59, demo.rs:557
  • The PR removed its only reader. Its only writer is the demo, which sets it to None.
  • Removing it is wire-safe: there is no deny_unknown_fields, and a missing Option deserializes as None.
  • Confidence 85 · mechanical · Fix: remove the field and its demo initializer. The siblings llama_server_build and ccr_version were already dead before this PR and can go in a separate change.

Non-blocking

• [new] First contact with the external endpoint swallows every error, not just "nobody is listening" — crates/rocm-dash-daemon/src/runner.rs:879-880
  • lemonade_discovery_warn = (!endpoint.rows.is_empty()).then(...) stays quiet on 401, 403, 5xx and body errors whenever the endpoint has never been seen. So a Lemonade server that is present but needs auth, or is broken, produces no warning.
  • docs/testing.md promises only that "a never-seen absent endpoint remains quiet".
  • Confidence 80 · logic · Fix: stay quiet only on connection-refused or transport errors.
• [standing, re-found blind] dash-25's expectations row makes the whole journey advisory on every lane that can run it — tests/e2e-cucumber/expectations.toml:103-111
  • @requires-engine:lemonade forces effective_engine = "lemonade", and the scenario is @requires-os:linux. So the row {effective_engine="lemonade", os="linux"} with flaky = true matches every eligible lane.
  • As a result, the dashboard, instance-detail and post-quit inference checks never gate a merge. The description concedes this only for the installed wording.
  • Confidence 80 · logic · Fix: keep only the serve step under the flaky scope, or say plainly that dash-25 is advisory everywhere.
• [new] No scenario covers the positive half of the new ownership rule — crates/rocm-dash-tui/src/ui/services_manager.rs:95-105, tests/e2e-cucumber/features/dash.feature
  • dash-26 proves that external rows are excluded from the Services overlay.
  • No scenario opens Services with a live managed service and sees it listed and stoppable. server-records-02 covers only non-running records.
  • Unit and contract tests do catch a dropped managed flag (mutation run).
  • Confidence 75 · mechanical · Fix: extend dash-07 to open the overlay, assert the managed row, and stage a stop approval.
• [new] Serving behaviour the CLI now chooses differently is covered only by unit tests — apps/rocm/src/main.rs:15578-15598, engines/lemonade/src/direct_llama.rs (serving_route)
  • Two choices changed: Vulkan-backed router models now read as ready, and simple registered names now take the direct route on Linux.
  • AGENTS.md §3 treats which runtime the CLI selects as user-observable. serve-lemonade-inference exercises the direct route, but it neither asserts the route nor gates on Linux.
  • Confidence 70 · mechanical · Fix: assert the route (load_response.method) in serve-lemonade-inference, and name or add the lane that covers router+Vulkan readiness.
• [new] serve-25 cannot tell old behaviour from new on Windows — tests/e2e-cucumber/features/model_serving.feature:338
  • Registered names took the router on Windows both before and after this change.
  • Confidence 75 · logic · Fix: tag the scenario @requires-os:linux, or say that its Windows run only re-confirms existing behaviour.
• [new] run_rocm_bounded was applied to only some of its twin serve call sites — tests/e2e-cucumber/tests/e2e/serving_steps.rs:557, :928, :982
  • setup_gpu_model and two serve --managed calls still use the unbounded run_rocm.
  • Confidence 75 · mechanical · Fix: use run_rocm_bounded(..., serve_cli_budget(...)) at those sites as well.
• [new] The isolated dash scenarios now probe a real 127.0.0.1:13305 by default — crates/rocm-core/src/lib.rs (default_dashboard_enable_lemonade), tests/e2e-cucumber/tests/e2e/dash_steps.rs
  • Only dash-25 turns discovery off.
  • dash-03/04/07/08/09 and the replay/GPU scenarios do not. On a self-hosted lane with Lemonade on 13305 they gain extra rows and warnings.
  • Confidence 70 · mechanical · Fix: default the isolated e2e config to enable_lemonade: false.
• [standing, re-found blind] The IPv6 contract tests return early and pass silently when IPv6 is unavailable — crates/rocm-dash-daemon/tests/telemetry_contract.rs (lemonade_dual_stack_managed_bind_does_not_duplicate_external_row, …ipv6_record_replaced_by_ipv4_record…)
  • Confidence 75 · mechanical · Fix: print a skip, or mark the tests ignored when the bind fails.
• [standing, re-found blind] Instance::is_external_lemonade keys on container_name == "lemonade" — crates/rocm-dash-core/src/metrics.rs:230-231, crates/rocm-dash-tui/src/ui/tabs/pane.rs:394-397
  • An unmanaged Docker container named lemonade makes the Engines pane say "externally running". The effect is display-only.
  • Confidence 70 · logic · Fix: carry an explicit provenance or engine field.
• [standing, re-found blind] No self-hosted GPU-lane check has reported on this head. The status rollup shows 23 hosted checks, all green, with Skill checks skipped, and no GPU lane. dash-25, serve-08 and serve-25 have not run anywhere for 5e0a33d. This is missing evidence, not a defect. Confidence 85 · mechanical.

Standing objections settled this round without being kept

These checks are anchored: each objection was put to the reviewer after its blind pass ended.

• Merge conflict with main: resolved. The API reports mergeable: true for 5e0a33d.
• Empty-state advice "check an external endpoint" points at a Serving action that does not exist: not kept. The Engines action in Serving does show live external availability. The dash_characterization comment is accurate, because dash-27 asserts the empty message and that "externally running" clears. What remains is that nothing in the TUI names the probed host/port or the dashboard.daemon.lemonade_* keys. That is an observation. The next round re-reads instances.rs blind.
• Pinned CLI strings with no pointer to a behaviour test (help text, startup status note): does not hold. Both now point at tests that prove the behaviour: dash-26, and managed_install_verdict_is_not_changed_by_live_events. The reinstall-advice part is kept, as the second Blocking item above.
• Router and pull steps tested only through fakes: does not hold. Real-step tests against a loopback fixture and a fake executable exist and catch the alias/router-name mutations. As the author notes, "a router failure never spawns a direct server" is still guaranteed only by how the code is structured.
• The alias test and the managed-row e2e check could not fail: does not hold. Changing --alias failed 2 of the 9 alias tests (run). The instance-detail matcher rejects a wrong status, port or model in its unit tests. dash-25 being advisory on every lane is the separate Non-blocking item above.

Decisions for the author

• Linux direct serving does not apply the router's per-model launch defaults — tradeoff
  • Direct serving passes -m, --n-gpu-layers 999, --alias and the recipe flags. A simple catalog entry can therefore run with different settings, such as context size, on Linux than the same name gets through the router on Windows. In return, direct serving gives an exact alias and avoids the router.
  • One comment in serving_route on why Windows stays on the router would let a reader confirm the split is deliberate.
• Direct-served models still get a /v1/unload on stop — tradeoff
  • llama-server has no such route, so engine_stop reports an error, and the process is then stopped by PID.
  • The behaviour is pre-existing, but this PR makes direct serving reachable for registered names.
• The PR bundles three changes — non-blocking-improvement. The three are the serving-route and identity rework, default-on external discovery in the dashboard, and the Services ownership model. Each would review and revert more safely on its own.

Positive signals

• Ownership is a positive managed flag that defaults to false (#[serde(default)]), so legacy snapshots fail safe: controls are hidden rather than wrongly offered. A staged approval whose target vanishes or turns external is dropped both on keypress and at draw time.
• The external-protocol fixture records every request that is not a GET of health or stats, and dash-26 asserts the log is empty after the stop/restart keys and again after quit. That is the proof of behaviour AGENTS.md asks for.
• router_serve.rs hides the router steps behind a trait, so the failure-matrix tests check stop-then-failed state for every step.

Deployment notes

• rocm dash on Linux/WSL now polls 127.0.0.1:13305 on every discovery tick by default. To opt out, set dashboard.daemon.enable_lemonade: false.
• On Linux, a failed router load no longer falls back to a direct llama-server, so affected hosts get an error on the first serve.
• The Services overlay no longer lists Docker-discovered containers.
• Version skew: a new rocm dash that attaches to an older dash's still-running embedded daemon gets snapshots with no managed field. It then shows "No managed services." until the old dash exits.

What this covered

• Read: every file in 21be0034…5e0a33d9 (prw-base...HEAD, 40 files), plus callers and siblings: backend_alignment.rs, install.rs, observation.rs, the readiness and stop paths in main.rs, and local_server_records.feature.
• Fan-out: two coupled-group workers (Lemonade engine/serving; dashboard/telemetry/docs), then one design worker.
  • The engine worker split off a read-only sub-worker for the e2e files and the main.rs / rocm-core hunks. That sub-worker read only the serving-related sections of e2e.rs.
  • The dashboard worker read event_loop.rs, app/mod.rs, app/types.rs and instances.rs selectively, because their changes are one-line field plumbing.
  • The history and prior-changes passes were run by the consolidating context and were not independent.
• Runs:
  • cargo test -p rocm-engine-lemonade --lib: 167 passed, and mutations M1–M7 were all caught.
  • The telemetry_contract Lemonade tests: 14 passed, and every mutation was caught except one equivalent mutant.
  • The services_manager and pane mutations were all caught.
  • cargo xtask e2e -- -n 'External Lemonade protocol': 2 scenarios, 31 steps passed.
  • The follow-up objection checks ran the split-shard, cross-root, index-range and alias probes.
• Did not run: any GPU scenario (dash-25, serve-08, serve-25), because there is no GPU here; Windows cfg arms (CI's windows-build-and-test passed); mutations for the e2e step files; local clippy (CI's passed).
• No prompt-injection content was found.
• The findings were not reconciled against the PR discussion. Only the standing objections above were weighed against the author's replies.

[volen-silo]

Thanks for round 6. I pushed aa28578d, rebased on current main. Here is the status of each item.

Blocking

• Idle rate stays fresh: not fixed. I tried to stop an unchanged stats report from refreshing the rate. It made dash-lemonade-external-protocol-transitions flaky on hosted CI, so I reverted it. I removed the comment that claimed this behaviour and rewrote the docs to say what the code does: the last request's rate stays shown until a new request or the row goes away. The description says so. A real fix needs a scenario that varies the stats, so I left it for its own change. External rows already behaved this way.
• Reinstall advice: fixed. runtime-lemonade-backend-alignment-reported now ends with a step that checks the selected backend's llama-server exists after rocm engines install lemonade --reinstall. The helper that finds the binary has unit tests. The scenario needs a GPU and a real download, so I could not run it. The pinned test comment now names that step.
• Direct route leaves a server running: fixed. All post-spawn steps now sit inside one guard that stops the child, reaps it, and marks state failed, including when wait() itself fails. New tests inject each failure with a fake llama-server and check that the process is gone and the state is failed. Mutating the guard makes them fail.
• Split GGUF suggestion: fixed. An exact shard filename now resolves to its whole set. A test feeds the suggestion from the real error back through resolution.
• Shard set across roots or revisions: fixed. A set counts as complete only when every shard is in the first shard's directory. New tests cover cross-root, cross-revision, and out-of-range indexes. One limit remains: matches are still deduplicated by file name, so a complete set in a later root is not found if an earlier root holds a same-named shard. The result is a clean error, not a launch that fails after spawn.
• Serving preview label: fixed. It now reads "Managed running — N" and "no managed services running", and the pinned strings changed.
• --managed doc: fixed. It says --managed is the default and only --foreground or --verbose attach.
• lemond_version: removed, with a test that an older snapshot carrying the key still loads.

Non-blocking

• First contact with the external endpoint: it now warns on a 401, a 5xx, or a body error. It stays quiet only when the connection is refused.
• dash-25 row: it is advisory on every lane that can run it. The docs and description now say so. I cannot scope the row to one step.
• Positive ownership: dash-07 now opens the Services overlay, sees the managed service listed with its port, and stages a stop approval, then declines it. It passes on the hosted lane.
• serve-25 is now @requires-os:linux. The other serve --managed sites now use the bounded run.
• Isolated dash scenarios now turn Lemonade discovery off unless a scenario sets it. dash-26 and dash-27 turn it on.
• The IPv6 tests now print a skip line when they cannot bind.
• The comment on why Windows registered names stay on the router is added.
• Not changed: the Docker container named lemonade display case, a scenario that asserts the Linux direct route, and the /v1/unload call on stop for direct servers. The first is display-only. The other two need hardware runs.

dash-25 had a real bug that the flaky row hid. On the GPU lane my instance-detail check failed on the real screen, because a panel border touches the model: label. I fixed the matcher and added a fixture captured from that run.

Verification: the full workspace suite passed serially (33 targets, 3,993 tests), and so did both clippy commands, fmt, crate edges, smoke, and the external protocol scenarios. After the last two rebases onto main I re-ran the affected crates and clippy. I could not run the GPU scenarios, the Windows build, or lychee locally.

[rominf]

🔴 Automated review · pr-review-watcher · aa28578

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.

Informational: this review describes aa28578, not the current head. The head moved to dd0ac46 while this round was running, and the code differs. These findings are not anchored to the current diff, are not a binding round, and file no new change request. The change request already standing from round 6 is untouched and still gates. The next round reviews dd0ac46 and settles everything below. Until then, a blocker first found here at aa28578 is not gating.

Review — needs work

Full review of the whole change.
No ticket named. The description matches what the diff does: catalog resolution, removal of the router-to-direct fallback, persisted served identity, read-only dashboard discovery, and registry-only ownership.

Round 7 (at aa28578). This round reviewed aa28578 in full and blind, because the previous review point, 5e0a33d, is no longer in the branch after the rebase. Each finding is marked [new] or [standing]. The eight blocking objections from round 6 were not re-found blind as written, so each was put to the reviewer afterwards. That check is anchored: the reviewer had been told the objection and the author's reply. One still holds in part and is kept below. The other seven are listed at the end of Non-blocking with their outcome.

The failing check at aa28578 is caused by this PR. windows-build-and-test fails on lemonade_never_seen_absent_external_endpoint_is_quiet. That test does not exist on main, and main's latest completed CI runs are green.

Blocking

• [new] The required windows-build-and-test check fails on a test this PR adds — crates/rocm-dash-daemon/tests/telemetry_contract.rs:477 (lemonade_never_seen_absent_external_endpoint_is_quiet), crates/rocm-dash-collectors/src/lemonade.rs:122-137, crates/rocm-dash-daemon/src/runner.rs:356-360,797-801,881-884
  • The job log for head aa28578 shows 24 passed, 1 failed. The panic is "normal absent external endpoint is not a noisy warning".
  • Likely cause, traced but not run on Windows: the collector has a 1500 ms total timeout. On Windows, a connection to a closed localhost port is retried for about 2 s rather than refused at once. So reqwest reports a timeout, not is_connect(). send_error then maps it to Transport, absent is false, and a warning is raised.
  • The test does not exist on prw-base.
  • Confidence 100 (CI run) · logic
  • Fix: native Windows has no live dashboard per the README, so cfg-gate these daemon contract tests to unix. Alternatively, set a short connect_timeout and classify connect timeouts explicitly.
• [new] A managed Lemonade service with an API key shows a permanent "lemonade health … 401" warning and never shows throughput — crates/rocm-dash-daemon/src/runner.rs:817-822,887, crates/rocm-dash-collectors/src/lemonade.rs:258-311
  • This PR newly scrapes managed Lemonade endpoints, and the collector sends no credentials.
  • A public-bind serve gets a minted key: engines/lemonade/src/process.rs:263-270 sets LEMONADE_API_KEY, which gates /api and /v1.
  • A managed record run against a 401 fixture produced, on every snapshot: lemonade health 127.0.0.1:PORT [managed-lemonade]: transport: GET …/api/v1/health: status 401 Unauthorized. That real lemond answers 401 here is taken from the engine's own comment, not from a live server.
  • Confidence 85 · logic
  • Fix: present the CLI-owned key for registry targets, or treat 401/403 on a managed target as "throughput unavailable" with no standing warning. Add a regression test.
• [new] A stale partial download in an older snapshot revision makes a complete split model permanently unservable — engines/lemonade/src/direct_llama.rs around 599-616 (hf_checkpoint_gguf_matches_in_roots), 639-677 (first_shard_of_complete_set / is_incomplete_shard_set), 359 (ensure_model_with_download)
  • Matches are de-duplicated by file name across all snapshot revisions, in sorted path order. So revision aaa/…-00001-of-00002 shadows the bbb/…-00001 of a complete set. The kept shards then span two directories, the set reads as incomplete, and resolution returns Ok(None).
  • The re-pull resolves to None again, and the serve fails with "did not download".
  • The shard logic is new in this PR.
  • A probe test reproduced it: aaa holds shard 1 only, bbb holds both shards, and the result is Ok(None).
  • Confidence 100 · logic
  • Fix: group matches per snapshot directory, and choose a complete set within one directory.
• [new] The "previously seen external endpoint went away" warning has no test — crates/rocm-dash-daemon/src/runner.rs:881
  • Mutating absent to drop its endpoint.rows.is_empty() half silences that warning, and all 16 Lemonade telemetry_contract tests still pass. dash-27 only checks that the rows disappear.
  • The code comment promises the code "reports why they disappeared". AGENTS.md §3 requires a message to be asserted together with its behaviour.
  • Confidence 100 (mutation survived) · mechanical
  • Fix: add a daemon test in which the endpoint refuses connections after rows were seen, and expect the warning.
• [standing since 788a1a3, not re-found blind; the follow-up check found it holds in part] An idle Lemonade's last-request rate is still presented as a live, fresh rate — crates/rocm-dash-daemon/src/observation.rs (observe_direct, ~L249), crates/rocm-dash-daemon/src/runner.rs (stats scrape), crates/rocm-dash-tui/src/ui/tabs/serving.rs:57
  • Resolved: the comment that claimed the rate expires is gone. The test doc (telemetry_contract.rs ~L971) and the collector doc (lemonade.rs ~L144) now say what the code does: the last request's rate stays shown.
  • Still holds: observe_direct re-stamps last_observed_at on every scrape. An idle server's old tok/s therefore always reads as fresh, and the held/stale legend can never fire for it. The TUI shows it as a plain tok/s figure (e.g. the "… status · tok/s" header), with no "last request" qualifier there, in README.md, or in docs/testing.md. A user watching an idle server still sees what looks like current throughput.
  • Round 6 offered two remedies; the author's reply explains why the first was reverted. The second was half done: the comment is gone, but the value is not labelled.
  • Confidence 80 (anchored check, read not run) · logic
  • Fix: label the value where it is displayed (for example "last req tok/s", or a held/stale marker once no new request has completed), or stop re-stamping when the stats body is unchanged.

Non-blocking

• [new] Typo in the scenario tag line: @id:runtime-lemonade-backend-alignment-reported@requires-gpu — tests/e2e-cucumber/features/runtime_setup.feature:340
  • The gherkin parser still splits this into two tags, so the GPU gate holds and feature_naming passes (4 tests ran).
  • Whitespace-splitting readers, such as feature_tags in expectation.rs, see a different id.
  • Confidence 85 · mechanical · Fix: insert the missing space.
• [new] runtime-16's new last step never starts from the missing-binary condition — tests/e2e-cucumber/tests/e2e/runtime_steps.rs (assert_lemonade_backend_server_installed)
  • It proves the binary exists after a reinstall, not that the reinstall restores a missing one. Restoration holds only by reading install.rs remove_dir_all.
  • Confidence 70 · mechanical · Fix: delete the selected backend's binary before the reinstall step.
• [new] The direct readiness check now matches names exactly, but its healthcheck twin still normalizes them — engines/lemonade/src/process.rs around 762 (models_payload_has_loaded_model) vs around 782 (models_payload_has_ready_model)
  • A direct server can fail the serve-time check yet read as ready in healthcheck.
  • Confidence 70 · logic · Fix: match direct entries (those without recipe_options) exactly in both.
• [new] The engine picks a nested llama-server build directory in read_dir order, while the e2e copy of the lookup sorts — engines/lemonade/src/direct_llama.rs around 325-337 (find_binary_in), tests/e2e-cucumber/src/lemonade_backend.rs (binary_in)
  • The function predates this PR, but direct serving is now the default Linux route for catalog names.
  • Confidence 70 · logic · Fix: sort, or prefer the newest build.
• [new] is_connect() is treated as "nothing listening", so DNS failures and connect timeouts are silent — crates/rocm-dash-collectors/src/lemonade.rs:122-129
  • A mistyped lemonade_host never warns.
  • Confidence 70 · logic · Fix: treat only a ConnectionRefused found in the error's source chain as "nothing listening".
• [new] The fixture's premise "a plain llama-server: neither Lemonade health route exists" looks wrong — crates/rocm-dash-collectors/src/lemonade.rs:416
  • Real llama-server serves /v1/health and returns 503 while loading. So a managed direct server would show a transient health warning that the fixture, which serves only /health, cannot reveal.
  • This rests on recall of llama.cpp, not on a run.
  • Confidence 70 · mechanical · Fix: serve /v1/health in the fixture, including a 503 phase.
• [new] HTTP status errors are labelled "transport:" in warnings — crates/rocm-dash-collectors/src/lemonade.rs (status mapped to CollectorError::Transport)
  • This is visible in the 401 output above.
  • Confidence 85 · mechanical
• [new] Stale test name — crates/rocm-dash-collectors/src/lemonade.rs:680 (discover_is_clean_noop_when_endpoint_absent)
  • The discover() it names was removed, and the test now asserts an error.
  • Confidence 85 · mechanical
• [standing, re-found blind] The IPv6 daemon tests skip with eprintln! + return and report ok — lemonade_dual_stack_* and lemonade_managed_ipv6_record_replaced_* in telemetry_contract.rs
  • They ran on this host. On a host without IPv6 they are a silent false green.
  • Confidence 70 · mechanical · Fix: use #[ignore] with a reason, or fail loudly in CI.
• [new] The leak this PR fixes for Lemonade is still in vLLM (pre-existing, pattern relevance) — engines/vllm/src/process.rs around 118-120 and 147-148
  • write_running_state(...)? runs after the spawn, and a child.wait() error does not mark the state failed.
  • Confidence 85 · logic · Fix: a follow-up issue or PR.
• [standing] No self-hosted GPU-lane check ran for aa28578. The removed router-to-direct fallback, Vulkan readiness acceptance, dash-25, serve-08 and runtime-16 have no hardware evidence there. The description says the same. This is missing evidence, not a defect. Confidence 85 · architectural.

Standing objections settled this round without being kept

These checks are anchored: each objection was put to the reviewer, with the author's reply, after the blind pass ended.

• Direct route leaves a server running after a post-spawn failure: no longer holds. supervise_direct_server puts recording, readiness and the wait under one stop-and-reap guard. Removing the stop on the wait-error path fails direct_server_step_failure_stops_the_child_and_marks_state_failed (mutation run).
• The split-GGUF suggestion does not resolve: no longer holds. Breaking the shard branch fails split_set_ambiguity_suggestion_resolves_to_the_whole_set (mutation run).
• Shard set assembled across roots or revisions: no longer holds as written. Dropping the same-directory check fails the cross-root and cross-revision tests (mutation run). The out-of-range case is tested by shard_indexes_outside_the_declared_range_are_not_a_complete_set. The stale-partial-revision case is the separate new Blocking item above.
• --reinstall advice pinned as wording only: no longer holds as filed. runtime-16 now ends with the requested binary-present step, and the pin names it. The residual is the Non-blocking item above.
• "Running now — 0" in the Serving preview: no longer holds. It now reads "Managed running — N" / "no managed services running", and the tests are updated.
• Old --managed text in manual-testing: no longer holds. docs/manual-testing.md:278-280 now matches the CLI.
• Dead SystemInfo.lemond_version: no longer holds. It was removed, and gpu_system_info_ignores_retired_lemond_version_key covers old snapshots.

Decisions for the author

• Removing the router-to-direct fallback — tradeoff
  • Failures are now honest, and a failed serve stops its server and marks the state failed.
  • The cost is that a host where the router load fails, but a direct server would have worked, now gets an error on the first serve. The description states this. Confirm it is intended for Linux users on the default model.
• Scope — non-blocking-improvement
  • The change is about 7.5k lines across engine catalog resolution, router error semantics, daemon telemetry, TUI ownership and docs. AGENTS.md §11 asks for one logical change per PR.
  • The serving half and the dashboard half share little beyond canonical_model_id. They would review and bisect better as two PRs.
• About 250 lines of Lemonade endpoint logic added inline in run_loop (crates/rocm-dash-daemon/src/runner.rs:709-951) — non-blocking-improvement
  • AGENTS.md §6 prefers a domain module, such as a lemonade_scrape module.

Positive signals

• Every step after a spawn (router, direct server, download lemond) runs under one guard that stops and reaps the child. Each path has a failure-injection test, and 18 production mutations were each caught by a named test.
• Ownership defaults to managed: false through serde, so older snapshots and replays cannot grant stop or restart. A staged approval is re-checked against current ownership before any verdict.
• The dash-26 fixture logs every request that is not a health or stats GET. That proves the dashboard takes no lifecycle action against an external server, rather than only asserting the wording.
• Throughput is attributed only when exactly one model is loaded, instead of inventing a per-model rate.

Deployment notes

• Behaviour change: on Linux, a failed Lemonade router load no longer falls back to a direct llama-server. The serve fails instead.
• rocm dash now probes 127.0.0.1:13305 by default. The new dashboard.daemon.enable_lemonade, lemonade_host and lemonade_port keys in config.json control this.
• The Services overlay now lists only registry-managed services. Docker-discovered containers can no longer be stopped or restarted from the dashboard.

What this covered

• Read: every changed file in prw-base...aa28578d (merge base f9a868a), completely, by two parallel read-only workers. One took the Lemonade engine, rocm-core, main.rs and the serving and runtime e2e. The other took the dash crates, dash.rs and the dash e2e. Docs, history and the agent-instruction pass were done by the review driver. git merge-tree against prw-base is clean.
• Ran:
  • rocm-engine-lemonade --lib (177 passed), with 19 single-branch mutations.
  • rocm-dash-daemon --test telemetry_contract, plus the collector, services_manager, pane and dash unit tests, with 24 mutations.
  • e2e-cucumber --test feature_naming (4 passed).
  • The windows-build-and-test job log was read, and the failing test was checked against main.
• Did not run:
  • The cucumber e2e suite (dash-03/25/26/27, serve-08/25, runtime-16).
  • A Windows-target cargo check, because no mingw linker is available here. Windows cfg arms were reviewed by reading only.
  • Real lemond or llama-server behaviour, so the 401 and /v1/health claims rest on the engine's comments and on recall.
  • The prior-changes pass.
  • The Step 7 design questions were investigated by the review driver, not by independent workers, because of a 2-worker cap.
• Caveat: both workers mutated the same checkout concurrently. A small window of cross-interference between their mutation verdicts cannot be excluded.
• CI rollup at aa28578: every check passes except windows-build-and-test. Skill checks were skipped.
• Findings were reached without the PR discussion. Only the standing objections were weighed against the author's reply, which was acknowledged before this round.

[rominf]

🔴 Automated review · pr-review-watcher · dd0ac46

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

Review — needs work

Full review of the whole change.
No ticket named. The description is detailed and accurate except for the reinstall-proof claim (the fourth Blocking item).

Round 8 (at dd0ac46). This round reviewed dd0ac46 in full and blind, because the previous review point, aa28578, is no longer in the branch after the rebase. Each finding is marked [new] or [standing]. The five objections carried from round 7 (posted as an informational comment at aa28578) were not re-found blind, so each was put to the reviewer afterwards, one at a time, with the author's reply where there was one. That check is anchored: the reviewer had been told the objection. Four still hold and are appended under Blocking; one no longer holds and is listed at the end of Non-blocking. This change request replaces the round-6 one as this account's complete position.

The fourth new Blocking item (reinstall "proof") is the residual round 7 rated non-blocking. It is reported as the method found it, but it does not on its own decide this change request; the other blocking items do.

Blocking

• [new] The new router serve path's real stop and its failed-exit branch are never tested — engines/lemonade/src/router_serve.rs:121-133, engines/lemonade/src/router_serve.rs:220-225, engines/lemonade/src/process.rs:686-698
  I made two changes together: turned LemondRouterSteps::stop_server into a no-op, and made a non-zero wait_server status return Ok(()). cargo test -p rocm-engine-lemonade --lib still passed all 177 tests. FakeSteps::wait_server always succeeds, and no test spawns a real lemond and then stops it. The architecture doc and the PR say this new module stops the server and marks state failed on any error. Neither claim is tested.
  Confidence 100 · logic · Fix: add a failing-exit variant to FakeSteps that asserts both the error and state failed. Add a Linux test that spawns the fake lemond (exec sleep 30), calls the real stop_server, and asserts the process is gone.

• [new] New test helpers write fake executables with std::fs::write instead of the shared ETXTBSY-safe helper — engines/lemonade/src/test_http.rs:112-116 (write_executable), plus the fake_pull_manifest, fake_llama_server and inline-script helpers in engines/lemonade/src/process.rs (the from_mode(0o700) sites the diff adds)
  crates/rocm-core/src/fake_executable.rs exists to prevent this race. Writing a script in-process and then running it while other test threads fork fails intermittently with Text file busy (os error 26). engines/lemonade/src/install.rs in this same crate already uses that helper. One worker reported 5 such failures in 70 serial runs of cargo test -p rocm-engine-lemonade --lib, but its logs did not contain the error. I got 0 failures in 25 serial and 45 parallel runs, so I am relying on the in-repo explanation of the race, not my own reproduction. CI's per-process nextest would hide it. The local cargo test --workspace gate in AGENTS.md §8 would not. This also breaks AGENTS.md §6 (reuse the shared component).
  Confidence 85 · mechanical · Fix: call rocm_core::fake_executable::write_fake_executable from these helpers.

• [new] Resetting telemetry when only canonical_model_id changes is never tested — crates/rocm-dash-daemon/src/runner.rs:650
  The worker replaced old.canonical_model_id == new.canonical_model_id with true, and all Lemonade telemetry contract tests stayed green. The model_ref and host comparisons next to it are each caught by a test, so this is the one part of the new rewrite detection that nothing tests.
  Confidence 100 · logic · Fix: add a case to assert_record_rewrite_resets_attributed_rate that rewrites only canonical_model_id.

• [new] The reinstall advice is said to be "proved" by a step that never creates the condition the advice is for — tests/e2e-cucumber/tests/e2e/runtime_steps.rs:532-575, tests/e2e-cucumber/features/runtime_setup.feature:337-339, docs/testing.md (the runtime-lemonade-backend-alignment-reported paragraph), engines/lemonade/src/direct_llama.rs:775-785, PR description ("Local limits")
  The serve error at direct_llama.rs:74-79 says to run rocm engines install lemonade --reinstall when the selected backend's llama-server is missing. Runtime-16 runs --reinstall on a runtime it never damaged, then checks that the binary exists. So it proves that an install leaves a binary, not that --reinstall restores a missing one. AGENTS.md §3 requires the named command to "actually clear the condition that printed it", with the message and the resulting state asserted together. The step's doc comment, docs/testing.md and the PR body all call this step "the proof", so the description overstates what the step covers.
  Confidence 70 · logic · Fix: before the reinstall, delete <runtime_dir>/bin/llamacpp/<backend>/, ideally also check that rocm serve <owner/repo:variant> prints the advice, then reinstall and assert the binary is back. Otherwise, weaken the claim in all four places.

• [standing since aa28578, not re-found blind; the follow-up check found it still holds] A managed Lemonade service with an API key shows a permanent "lemonade health … 401" warning and never shows throughput — crates/rocm-dash-daemon/src/runner.rs (managed branch, ~L817-822 and ~L886-888), crates/rocm-dash-collectors/src/lemonade.rs (fetch_health_if_router, ~L335)

  • engines/lemonade/src/process.rs:263-270 sets LEMONADE_API_KEY whenever rocm serve protects the endpoint; its comment says Lemonade then gates /api, /v0 and /v1 on it.
  • Nothing in the collector or daemon sends credentials. A 401/403 from the health check is kept as an error on purpose (and tested as such), so the managed branch pushes a "health" warning and skips the row on every cycle.
  • At aa28578 a managed record against a 401 fixture produced that warning on every snapshot. At dd0ac46 this was re-checked by reading, not re-run. That real lemond answers 401 there rests on the engine's own comment.
  • Confidence 85 · logic · Fix: present the CLI-owned key for registry targets, or treat 401/403 on a managed target as "throughput unavailable" with no standing warning. Add a regression test.

• [standing since aa28578, not re-found blind; reproduced again by the follow-up check] A stale partial download in an older snapshot revision makes a complete split model permanently unservable — engines/lemonade/src/direct_llama.rs (hf_checkpoint_gguf_matches_in_roots ~L514-534, first_shard_of_complete_set, is_incomplete_shard_set, ensure_model_with_download)

  • Matches are still de-duplicated by file name across every snapshot revision in a root, in path order. So revision aaa/…-00001-of-00002 shadows the bbb/…-00001 of a complete set; the kept shards span two directories, the set reads as incomplete, and resolution returns Ok(None).
  • A probe test at dd0ac46 (aaa holds shard 1 only, bbb holds both) returned None, then ensure_model_with_download failed with "Lemonade did not download req for direct serving".
  • The author's earlier note ("a clean error, not a launch that fails after spawn") is accurate that nothing spawns, but nothing the user is told clears it: the automatic re-pull finds the same cache (inferred, not run against a real pull), and the message points at the download rather than the stale revision. That note also describes one root shadowing another; here both revisions are in the same root. The existing shards_spread_over_snapshot_revisions_are_not_a_complete_set does not cover this case.
  • Confidence 100 · logic · Fix: group matches per snapshot directory, and choose a complete set within one directory.

• [standing since aa28578, not re-found blind; the mutation survived again] The "previously seen external endpoint went away" warning has no test — crates/rocm-dash-daemon/src/runner.rs (let absent = endpoint.rows.is_empty() && …is_connect_error(&error))

  • Dropping the endpoint.rows.is_empty() half silences that warning, and cargo test -p rocm-dash-daemon stayed green (47 unit, 9 end_to_end, 25 telemetry_contract).
  • lemonade_external_health_failure_warns_and_removes_rows_without_grace uses HTTP 503, which is not a connect error, so absent is false either way. A seen endpoint that stops, the commonest way a server goes away, is never exercised.
  • The code comment promises the code "reports why they disappeared". AGENTS.md §3 requires a message to be asserted together with its behaviour.
  • Confidence 100 (mutation run) · mechanical · Fix: seed rows, close the listener, and expect a warning naming the endpoint.

• [standing since 788a1a3, not re-found blind; the follow-up check found the remaining part still holds] An idle Lemonade's last-request rate is presented as a live, fresh rate — crates/rocm-dash-core/src/observation.rs (observe_direct, ~L249), crates/rocm-dash-daemon/src/runner.rs (stats scrape), crates/rocm-dash-tui/src/ui/tabs/serving.rs:57, crates/rocm-dash-tui/src/ui/tabs/instances.rs:57

  • observe_direct re-stamps last_observed_at on every scrape, so the held legend can never fire for an idle Lemonade. Serving and Instances show the value as plain "tok/s". No "last request" qualifier appears in README.md, docs/testing.md or the TUI; only the collector's code comment and the PR description explain it.
  • The author's reply explains why the deeper fix (not re-stamping an unchanged report) was reverted, and that is credible. It is also true that external rows already behaved this way on main. But this PR newly scrapes managed Lemonade services through the same path, so a model started with rocm serve now shows its last request's rate as current throughput, which it did not before.
  • The cheap remedy needs no new scenario.
  • Confidence 80 (anchored check, read not run) · logic · Fix: label the figure where it is displayed (for example "last req tok/s"), or document what it means in the user-facing docs.

Non-blocking

• [standing, re-found blind] A missing space joins two tags — tests/e2e-cucumber/features/runtime_setup.feature:340 (@id:runtime-lemonade-backend-alignment-reported@requires-gpu)
  gherkin 0.16 still reads it as two tags, because tag_char excludes @ and __ allows zero separators. But feature_naming.rs splits on whitespace, so it reads the id as runtime-lemonade-backend-alignment-reported@requires-gpu. The guard and the harness now see different ids for this scenario.
  Confidence 100 · mechanical · Fix: restore the space.

• [new; the test-name part standing] Some new comments say things the code does not do — engines/lemonade/src/runtime_dir.rs:45-46, engines/lemonade/src/direct_llama.rs:145-147, crates/rocm-dash-collectors/src/lemonade.rs:640-653

  • runtime_dir.rs says the only callers of rooted_at are the Linux tests in process.rs. It is also called from lib.rs:898, router_serve.rs:543 and direct_llama.rs:1482.
  • The isolation comment says tests read no real user directories, but hf_cache_roots still reads HF_HOME, HUGGINGFACE_HUB_CACHE and HOME.
  • discover_is_clean_noop_when_endpoint_absent says it checks "an empty discovery result", but it asserts that fetch_health and fetch_stats return errors.

  Confidence 85 · mechanical · Fix: reword the comments, and rename the test or change what it asserts.

• [new] Three host normalizers in the registry disagree on case — crates/rocm-dash-daemon/src/registry.rs:67, :105, :301-306
host_reach lowercases and strips brackets; the other two are case-sensitive. A LOCALHOST record only groups correctly because the grouping step happens to lowercase.
  Confidence 70 · mechanical · Fix: route all three through one normalizer.

• [new] The docs describe the serve bound imprecisely — docs/testing.md (dash-25 paragraph) and the comment on the new expectations.toml row
  Both say the serve is bounded by serve_timeout_secs (90s). Readiness polling does use that, but the CLI process is only killed after 90 + 300 s (serve_cli_budget).
  Confidence 70 · mechanical · Fix: mention the margin.

Standing objections settled this round without being kept

This check is anchored: the objection was put to the reviewer after the blind pass ended.

• windows-build-and-test failed on lemonade_never_seen_absent_external_endpoint_is_quiet: no longer holds. send_error now maps connect errors and timeouts alike to "nothing listening", and closed_and_unanswered_endpoints_both_read_as_nothing_listening covers both cases. The test is not cfg-gated, and the windows-build-and-test log for dd0ac46 shows it ran and passed.

The seven other round-6 objections were settled at aa28578 as no longer holding (see the round-7 comment); none was re-found blind here.

Decisions for the author

• The router→direct fallback after a failed load is removed — tradeoff
  An error now stops the server and fails the serve every time, which is more honest. The cost: a host where the router load fails but a direct server would have started now gets an error on its first serve. The PR states this, so this is only a request to confirm it is deliberate.
• Vulkan counts as GPU-ready — tradeoff
  service_model_reports_gpu_backend (apps/rocm/src/main.rs) and direct serving now accept whichever GPU backend Lemonade selected. docs/manual-testing.md previously required "does not fall back to CPU or Vulkan". CPU is still refused, so the no-CPU-fallback rule holds. But a ROCm-capable host whose Lemonade picked Vulkan will now serve through Vulkan with no notice.
• The real-GPU dashboard journey can never block a merge — tradeoff
  The new dash-lemonade-managed-inference-journey row in expectations.toml matches every lane where the scenario can run, so every assertion in it is advisory. This is disclosed. One option is to split the dashboard and instance-detail checks into a scenario that uses a planted managed Lemonade record, so they gate without the flaky serve.
• The PR combines two independent changes — non-blocking-improvement
  It is about 7.6k lines. Lemonade catalog routing and serve-path rework is one change; dashboard external discovery and ownership is the other. Each commit mixes both. Splitting them would make each easier to review and revert.

Positive signals

• Lifecycle ownership is a positive managed flag that only the registry sets, with serde defaulting it to false for old snapshots. The Services overlay re-checks that the approval target is still managed before acting on a "yes". Mutation testing showed each of those branches is caught.
• serving_route decides the route before any download or spawn and refuses CPU and unknown backends. The shard-resolution edge cases are covered by mutation-verified tests.
• The external-discovery scenarios use an in-process fixture that records any request other than a GET of health or stats. That is a real behavioural check that the dashboard never sends a lifecycle request to a server it does not own. Both scenarios pass locally (2 scenarios, 31 steps).

Deployment notes

• dashboard.daemon.enable_lemonade defaults to true (new config fields enable_lemonade, lemonade_host, lemonade_port). Every Linux/WSL dashboard now probes 127.0.0.1:13305 read-only on each discovery tick.
• Behaviour change: a failed router load no longer falls back to a direct llama-server.

What this covered

Every file in the change, 12342b3ac7be… (prw-base, equal to the merge-base) … dd0ac46a7141…. The code was split across three independent workers: Lemonade engine plus rocm-core, dashboard telemetry plus TUI, and the e2e harness plus docs. Some parts of large files were read only as diff hunks plus their surroundings, not end to end:

• runner.rs lines 1000-1739 (snapshot assembly and existing tests);
• the rest of registry.rs and of the pane.rs production code;
• direct_llama.rs lines 1300-1412 and 1810-2001 (test bodies);
• parts of process.rs and lib.rs;
• the unchanged parts of serve_log.rs, serving_steps.rs and runtime_steps.rs.

Tests run, all in the checkout's own target/ with RUSTC_WRAPPER= empty:

• lemonade lib: 177 tests, plus mutations;
• dash-core, dash-collectors, dash-daemon (including telemetry_contract) and dash-tui: all green except 3 sandbox-only agent::clients failures unrelated to this PR;
• e2e-cucumber lib (161) and feature_naming (4);
• the two external Lemonade protocol scenarios.

CI is all green, but no self-hosted GPU or Windows-GPU lane appears in the rollup for this head. That is missing evidence for dash-25, serve-08 and serve-25, not a defect.

Did not run:

• Agent-instruction adherence, history, and design passes: run by the driver in a single context, so not independent.
• Prior-changes pass: not run (time).
• Windows cfg arms: checked by reading only, not compiled locally. CI windows-build-and-test passed.
• GPU scenarios: not run (no GPU).

Not reconciled against any discussion here. The standing objections above were weighed against the author's reply after the blind pass, and that is the only part of the discussion that reached the reviewer.

[volen-silo]

Addressed in a133657, rebased on current main.

• A complete split model in one snapshot now wins over an older partial download. The regression test failed before the fix.
• Router stop and failed-exit paths now have tests, including a real stop of a fake lemond.
• Test helpers use the shared fake-executable helper.
• A change of only canonical_model_id now has a rewrite-detection test.
• The "previously seen external endpoint went away" warning has a test.
• A managed server that needs an API key (health returns 401 or 403) stays listed with its registry status and no throughput, and no longer shows a standing warning. No credentials are read or sent. External discovery still warns on these statuses.
• The reinstall claim now says only what the step checks: --reinstall is accepted and a binary exists afterwards.
• Fixed the missing space between two tags, the serve-timeout wording, and several comments and doc lines. The README and manual-testing notes now say the tok/s figure is the last request's rate.

On the decisions:

• The idle rate is now labelled last req tok/s in the instance detail, with a legend in the Instances table.
• The router-to-direct fallback is back on Linux. It runs only for a router load or verification failure, only for a model that resolves to a safe checkpoint, and only through the selected GPU backend's llama-server. A missing or CPU backend, Windows and any other error return the router's error. There is no scenario for it because there is no safe way to force a real router load failure; the unit tests assert the result together with the state or log.
• A non-ROCm backend such as Vulkan is now named in the serve output and log.
• The PR stays one change.

Not verified here: no GPU lane or cucumber scenario ran locally. CI covers those.

[rominf]

🔴 Automated review · pr-review-watcher · a133657

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

Review — needs work

Full review of the whole change.
No ticket named; the description was read and compared with the change (see the description finding below).

Round 9 (at a133657). This round reviewed a133657 in full and blind, because the previous review point, dd0ac46, is no longer in the branch after the rebase. Each finding is marked [new] or [standing]. Of the round-8 objections, one was re-found blind (the canonical_model_id clause: the added test does not catch its removal) and one came back as non-blocking (the reinstall advice). The other six were not re-found blind, so each was put to the reviewer afterwards, one at a time, with the author's reply (comment 6216331509). That check is anchored, because the reviewer had been told the objection. None of the six still holds as a blocker; they are listed at the end. This change request replaces the round-8 one as this account's complete position.

Two Blocking items do not on their own decide this change request. The merge conflict blocks the merge on GitHub anyway. The description items appear only in the PR description, not in any commit message, so they do not land on main. The fallback-window state, the vacuous leak test, the canonical_model_id clause and the missing route scenario do decide it.

CI: the one failing check, E2E tests (Strix Halo, Windows, current), fails scenario diagnose-38. Main's own run of that lane at 1f19198 fails it the same way, and this PR touches no diagnose or fix files, so the failure is not counted against this PR.

Blocking

• [new] The PR no longer merges, because its new dash scenario numbers collide with ones added on main — tests/e2e-cucumber/features/dash.feature:560-613

  • The PR adds dash-35, dash-36 and dash-37. Main has since added its own dash-35 (dash-gpu-vram-uses-binary-units) and dash-36 (dash-onboarding-adopt-whitespace-cwd-quotes-approval-preview).
  • git merge-tree --write-tree prw-base HEAD reports CONFLICT (content): Merge conflict in tests/e2e-cucumber/features/dash.feature. This is the only conflicting file.
  • Confidence 100 · mechanical · Fix: rebase, then renumber the three new scenarios to dash-37–dash-39. Main's numbers stay because they are already merged.

• [new] While the router-to-direct fallback runs, the engine state says failed, and any reader in that window locks the service record at failed for good — engines/lemonade/src/router_serve.rs:127-131, engines/lemonade/src/direct_llama.rs:110-149, crates/rocm-core/src/service_record.rs:224-246

  • serve_router stops lemond and calls mark_json_status(.., "failed"). serve_direct_after_router_failure then runs ensure_model_in, which can be a long download, and spawns llama-server. Nothing rewrites the state until that spawn succeeds.
  • refresh_from_engine_state copies failed into the record. Its callers then persist it: load_managed_service and load_managed_services in main.rs, and providers.rs:529. That covers rocm services, chat and dash.
  • Once the record says failed, refresh_from_engine_state returns early for any status outside starting|running|recovering|ready. A fallback that later succeeds therefore stays recorded as failed.
  • The fallback test (lib.rs:1195) asserts only the final state.
  • Confidence 85 (path traced, not run) · logic · Fix: write starting before the fallback starts its work. Add a test that asserts the state during the fallback.

• [new] A test that claims to prove the direct server is never leaked stays green when nothing kills it — engines/lemonade/src/process.rs:1009-1016, process.rs:1107-1164 (direct_server_stops_when_state_cannot_be_written_after_spawn)

  • The fake server is #!/bin/sh\nexec sleep 30. After the exec, the process's /proc/<pid>/cmdline is sleep 30, so the test's search for the script path can never match.
  • I removed terminate_pid and child.wait() from stop_and_fail in supervise_direct_server. This test still passed, and a sleep 30 process was left running.
  • The sibling direct_server_step_failure_stops_the_child_and_marks_state_failed did fail under the same mutation, so only this test is vacuous.
  • Confidence 100 · logic · Fix: have the fake write $$ to a file before exec, and check that PID.

• [standing since dd0ac46, re-found blind] One clause of the runner's same-endpoint check can be deleted without any test failing — crates/rocm-dash-daemon/src/runner.rs:655-656

  • With && old.canonical_model_id == new.canonical_model_id removed, all 19 telemetry_contract Lemonade tests passed.
  • lemonade_managed_canonical_model_rewrite_resets_attributed_rate passes only because the rewritten name stops matching health, and the unmatched-row tracker removal clears the rate anyway.
  • Confidence 100 · mechanical · Fix: add a case where canonical_model_id changes while a name that still matches health remains.

• [new] No scenario covers the new route choice for registered names: simple ones are now served directly on Linux — tests/e2e-cucumber/features/model_serving.feature (serve-07/serve-08), tests/e2e-cucumber/tests/e2e/serving_steps.rs:746

  • Which runtime a serve uses is user-observable. AGENTS.md §3 says such a change needs a Gherkin scenario, and that "a unit test asserting the internal helper does NOT discharge this".
  • The only route assertion in tests/ is "lemonade-cli", in serve-26 (router route). "lemonade-packaged-llama-server" appears nowhere under tests/.
  • serve-07 stays green whichever route a simple registered name takes.
  • The description admits the gap and points to unit tests, which is the substitution §3 rules out.
  • Confidence 85 (cited rule, verified with grep) · logic · Fix: add a @requires-os:linux Then step that reads the engine state for a simple registered name and asserts the direct method and a non-CPU backend.

• [new; the reinstall part standing] The description overstates two things

  • The non-ROCm notice. The description says "When the selected backend is not ROCm … the serve output and log name that backend". The direct path writes the notice to the log (process.rs:524-548). The router path only calls eprintln! (router_serve.rs:87-89). A managed serve runs under spawn_serve_http_background, whose stdout and stderr go to Stdio::null() (state.rs:141-143), so a managed Vulkan router serve shows the notice nowhere. a_non_rocm_backend_is_named_in_the_notice_and_matches_the_state tests the helper, not the emit.
  • The reinstall advice. The description says the advice "is proved by a new last step of runtime-lemonade-backend-alignment-reported". Later it says the step "does not prove that the command repairs a deleted one". The second statement is the accurate one.
  • It also names scenarios by stale numbers ("dash-25", "the nightly serve-25"); the PR's scenarios are dash-35 and serve-26.
  • Confidence 85 · logic · Fix: for the notice, either share one emit-and-log helper between the two routes and assert it from the log, or narrow the description. For the rest, correct the wording.

Non-blocking

• [standing, re-found blind] The new reinstall advice is not shown to fix the condition it reports — engines/lemonade/src/direct_llama.rs (the error added by this PR), tests/e2e-cucumber/tests/e2e/runtime_steps.rs (assert_lemonade_backend_server_installed)

  • The error added by this PR tells the user to run rocm engines install lemonade --reinstall. AGENTS.md §3 requires that a named command actually clears the condition.
  • The step never deletes the binary first, so it passes even if the reinstall does nothing. It also runs only on Windows nightly, while the advice is printed on Linux too.
  • The PR states this gap openly.
  • Confidence 70 · logic · Fix: delete the selected backend's llama-server, reinstall, then assert it is back.

• [new] Comments this change made stale

  • router_serve.rs:91-92 says a load failure is preserved "rather than masking it with direct serving". The next call tags it RouterLoadFailure, which is what turns on direct fallback.
  • crates/rocm-dash-tui/src/ui/mod.rs:258-260 still refers to the "Running now" listing, which this PR renamed to "Managed running".
  • ui/mod.rs:353-357 says service_rows can include an unmanaged Docker/Lemonade entry. It no longer can.
  • In each case the code governs, because the comment describes behaviour the code no longer has.
  • Confidence 85 · mechanical.

• [new] The last req tok/s label appears only in the Instances tab — crates/rocm-dash-tui/src/ui/tabs/pane.rs (OpenServices gen span), ui/services_manager.rs (gen_tps_compact)

  • Managed Lemonade rows on these surfaces show a stale last-request rate as plain gen throughput. Home and Observe also add it into their totals.
  • The README scopes its claim to the Instances table and detail, so this is an inconsistency, not a false claim.
  • Confidence 70 · logic.

• [new] External discovery can flap and stall the daemon loop

  • One failed or slow /health call removes every external row with no grace period. That behaviour is pinned by lemonade_external_health_failure_warns_and_removes_rows_without_grace.
  • parallel_scrape is awaited inline in the tick loop (runner.rs:813-886). Up to three sequential 1.5 s requests can delay GPU ticks.
  • Both matter more now that discovery is on by default against 127.0.0.1:13305.
  • Confidence 70 · architectural · Fix: put an overall timeout on each endpoint and allow one missed discovery.

• [standing] The PR does several separable things in about 8,400 lines. These are:

  • Lemonade catalog routing;
  • the router-to-direct fallback;
  • dash external discovery;
  • Services-overlay ownership;
  • rate labelling.

  AGENTS.md §11 asks for one logical change per PR. Splitting engine routing and fallback from the dash work would make each part reviewable.

Standing objections settled this round without being kept

Each was put to the reviewer after the blind pass ended, one at a time, with the author's reply. These checks are anchored, because the reviewer had been told the objection.

• A managed Lemonade service with an API key shows a permanent 401 warning: no longer holds. A 401 or 403 on a managed health check now maps to "no telemetry" with no warning. External discovery still errors on these statuses. lemonade_protected_managed_endpoint_keeps_registry_status_without_telemetry_warning passes, and fails with that branch disabled.
• A stale partial download in an older revision makes a split model unservable: no longer holds. Complete sets within one snapshot directory now sort first. complete_snapshot_is_not_shadowed_by_an_older_partial_download passes, and fails with that ordering disabled.
• The "previously seen external endpoint went away" warning has no test: no longer holds. With the round-8 mutation re-applied, lemonade_seen_external_endpoint_closing_warns_and_removes_rows fails in 3 of 3 runs.
• The router's real stop and failed-exit paths are untested: no longer holds. Re-applying both mutations fails nonzero_server_exit_returns_error_and_marks_state_failed and real_router_stop_kills_and_reaps_its_server. The second checks the PID it spawned, so it does not share the exec sleep weakness above.
• New fake-executable helpers skip the shared ETXTBSY-safe helper: no longer holds, by reading only. Every fake executable this PR adds goes through write_fake_executable.
• An idle Lemonade's last-request rate is shown as a live rate: no longer holds as a blocker. README, docs/testing.md, docs/manual-testing.md and the Instances table and detail now label it. The Serving and Services surfaces still show plain gen, which is the non-blocking finding above.

Decisions for the author

• External Lemonade discovery is on by default — tradeoff
  • For: zero-config visibility of a local Lemonade server.
  • Against: every Linux/WSL dashboard now probes a third-party port. Any other service there that answers 404, 401 or 503 produces a standing warning, and the probe adds tick latency.
  • The other option is off by default, with the dashboard telling the user how to turn it on.
• Registered aliases on Linux now go direct, launched with basic flags only — tradeoff
  • Simple catalog entries no longer receive the router's per-model launch defaults.
  • Restricting the route to "simple" entries limits this, but confirm that losing those defaults is acceptable for the default model.

Positive signals

• serve_router wraps every post-spawn step in one fallible block behind the RouterSteps trait, so any failure stops and reaps lemond. The old path could leak it.
• Fallback is limited by a typed RouterLoadFailure marker, checked by downcast. It is Linux-only and refuses cpu and unknown backends, which keeps the no-CPU-fallback policy.
• Ownership is a serde-defaulted-false Instance.managed, set only by registry discovery and checked in one predicate. The Services manager revalidates a pending approval against fresh rows. A mutation run showed the tests catch removing that check.
• The external-protocol scenarios assert a negative: the fixture logs every non-GET request, and the scenarios assert that no lifecycle call reached the external server.

Deployment notes

• A new TUI attached to an already-running older daemon, or replaying an old recording, sees managed = false on every row. The Services overlay then says "No managed services" and silently disables stop/restart until the daemon restarts.
• dashboard.daemon.enable_lemonade defaults to true for existing configs.
• rocm dash now runs a builtin Detect call at startup.

What this covered

• Read: every file in prw-base...HEAD (merge-base 1f19198…a1336572, 46 files), in two coupled groups, each run as an independent read-only worker:

  • the Lemonade engine, main.rs, config.rs and the serving/runtime e2e;
  • the dash daemon, collectors, TUI and dash e2e.

  The engine worker split its group across four sub-workers. Their reported gaps:

  • the unchanged parts of runtime_dir.rs;
  • e2e.rs, serving_steps.rs and runtime_steps.rs, read at the changed hunks plus the functions they depend on;
  • only the changed hunks of apps/rocm/src/main.rs.

  The driver read the docs, README, expectations.toml and the feature files.

• Tests run:

  • cargo test -p rocm-engine-lemonade -p rocm-core: 189 and 748 passed.
  • The dash worker ran the rocm-dash-tui services_manager (23), telemetry_contract lemonade (19) and rocm dash::tests (37) tests.
  • Branch mutations were run on supervise_direct_server, the runner's endpoint check, and the services-manager approval and selection guards.
  • No e2e or GPU lanes were run.

• CI: the only failing check, Strix Halo Windows diagnose-38, fails the same way on main at 1f19198 and is outside this diff, so it is not counted. Skill checks (skillscope) was skipped.

• Did not run:

  • The design-review step was done in the driver context, not as independent workers, because of the 2-worker budget.
  • The prior-changes pass did not run: the PR's earlier reviews and comments are out of bounds here.
  • The history pass was limited to checking for drift against main, which found the conflict.
  • ai-instruction-quality was not activated, because no agent-instruction files changed.

• Not reconciled against the PR discussion, except as stated in the round note: after the blind pass, the standing objections were weighed against the author's reply. No prompt-injection content was found.

[volen-silo]

Addressed in 93aacb9, rebased on current main.

• Scenario numbers: rebased, and the new dash scenarios are now dash-37 to dash-39 (main keeps dash-35 and dash-36). The merge conflict is gone.
• Fallback state: the state is now starting for the whole fallback and goes back to failed if it produces no server, so a reader no longer records the service as failed while it runs. A new test covers the failed-fallback end state. The starting write itself is not observable in a unit test, because the download runs inside the engine call; I did not add a test for that part.
• Vacuous leak test: the fake server now writes its own PID before exec, and the test checks that PID. With the stop code removed, both direct-server tests fail.
• canonical_model_id clause: a new telemetry test rewrites only that field while the row still matches health through its model_ref. It fails when the clause is removed.
• Route scenario: serve-27 (@id:serve-lemonade-simple-registered-direct-route, Linux, GPU) reads the engine state and asserts the direct method and a non-CPU backend. It sits under the same flaky Linux expectations row as the other direct serves and has not run on hardware yet.
• Description: the non-ROCm notice is now also written to the serve log on the router path (a managed serve discards stdout and stderr), with a test on the log. The reinstall wording and the scenario numbers are corrected.
• Stale comments fixed (router load failure, "Managed running", the Services listing).

Not changed: the last req label on the Serving and Services surfaces, the discovery grace period and tick latency, and the PR split. These are decisions I am leaving open. CI on the previous head failed only on diagnose-38 (the Windows lane), which fails the same way on main at that base.

[rominf]

🔴 Automated review · pr-review-watcher · 93aacb9

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

Review — needs work

Full review of the whole change.
No ticket named. The description is a detailed account of the change. It misstates serve-27 (see the first finding) and contradicts itself on whether any scenario gates the direct route.

Round 10 (at 93aacb9). This round reviewed 93aacb9 in full and blind, because the previous review point, a133657, is no longer in the branch after the rebase. Each finding is marked [new] or [standing]. The author's reply (comment 6336068412) was read before this round and acknowledged; its claims were weighed only after the blind pass, as below.

Of the round-9 objections: the dash scenario-number conflict is gone (the branch is 0 commits behind main and GitHub no longer reports a conflict). The other four were put to the reviewer after the blind pass, one at a time, with the author's reply. Those checks are anchored, because the reviewer had been told the objection. Two no longer hold (the leak test and the canonical_model_id clause, each confirmed by a mutation run). Two still hold in part and are appended below as [standing]. This change request replaces the round-9 one as this account's complete position.

What decides this change request: serve-27's always-failing stop step hidden by a flaky row (with the standing route-scenario remainder), and the fallback-state remainder with its missing test. What does not decide it on its own: the reinstall advice (non-blocking in round 9, and no new evidence since), the non-ROCm notice clobbering (confidence 70, an informational line), and the red E2E tests check — main fails the same hosted job at the merge base 32834b2 in two consecutive runs with other dash quit/gen-tps scenarios, so it is not counted against this PR. Skill checks (skillscope) was skipped in CI.

Blocking

• [new] serve-27 always fails at its stop step, and a flaky expectations row hides the failure — tests/e2e-cucumber/tests/e2e/serving_steps.rs:815-830 (stop_complex_model_service, assert_complex_model_stopped), tests/e2e-cucumber/features/model_serving.feature (serve-27), tests/e2e-cucumber/expectations.toml (serve-lemonade-simple-registered-direct-route row)
  • serve-27 serves Qwen3-0.6B-GGUF, but it reuses serve-26's stop steps. Those steps call complex_model_service, which is hard-coded to look up Qwen3.5-0.8B-GGUF. It finds no record and panics before rocm services stop ever runs, so the served model is left running.
  • CI head 93aacb9 shows this on all three Linux GPU lanes that ran it (Strix Halo Ubuntu release, Strix Halo Ubuntu nightly, W7900): Step panicked ... expected exactly one scenario-owned Qwen3.5-0.8B-GGUF service.
  • The lanes stay green only because the new row sets flaky = true and blames EAI-7423. That bug is the intermittent ready-then-shutdown, not a failure that happens every run.
  • docs/testing.md and the PR description both claim the scenario "runs inference and stops the scenario-owned service". The description also says "No scenario gates the Linux direct route for simple registered names", which contradicts its own serve-27 bullet.
  • Confidence 100 · logic · Fix: have both stop steps call registered_model_service(world, world.model_name…). Then check whether serve-27 still needs the flaky row, and correct the docs and description to match.
• [new] The fallback's starting state is untested, but a test comment says it is covered — engines/lemonade/src/direct_llama.rs:134-138, test a_fallback_that_cannot_get_the_model_ends_failed_not_starting in engines/lemonade/src/lib.rs
  • I deleted the merge_json_state(... "starting") call and all 190 lib tests still passed. Deleting the failed-mark is caught.
  • The test's doc comment, and the description's "the state says starting for the whole fallback", both claim a reader "never sees a stale failed". No test checks that.
  • Confidence 100 · logic (test quality) · Fix: capture the state when the fallback's pull/spawn step begins (for example, have the fake lemonade pull copy state.json aside) and assert it is starting.
• [standing since round 8; non-blocking in round 9] The new remediation advice names a command that nothing shows actually fixes the problem — engines/lemonade/src/direct_llama.rs:71-79, tests/e2e-cucumber/features/runtime_setup.feature (runtime-16), tests/e2e-cucumber/tests/e2e/runtime_steps.rs:537-588
  • The error "run rocm engines install lemonade --reinstall to restore it" is new in this PR; base has no such text.
  • AGENTS.md §3 requires that remediation advice naming a command "actually clear the condition that printed it". The covering step never removes the binary first, as the PR itself admits.
  • That step also runs only under @requires-os:windows @nightly, while the direct route that prints the advice is chiefly Linux. So the in-code comment "restores the file whatever state Lemonade recorded" is unverified.
  • Confidence 85 · logic · Fix: delete the selected backend's llama-server, run the reinstall, assert the binary is back, and ideally re-run the serve. Run this on a lane where the advice is actually printed.
• [new] The non-ROCm backend notice is likely overwritten in the serve log — engines/lemonade/src/state.rs:79-97 (announce_non_rocm_backend), engines/lemonade/src/router_serve.rs:87, engines/lemonade/src/process.rs:110
  • The notice is appended to the log through a separate append-mode handle. lemond writes to the same file through a File::create handle without O_APPEND, so its next write lands at its own offset, on top of the appended line. A shell run of that file-handle setup showed the clobber.
  • A managed serve discards stderr, so per the doc comment the log is "the only place the user can read it". That makes the claim fragile right after a load, which is when lemond logs.
  • FakeSteps has no concurrent writer, so the notice test cannot catch this. The load command's own append (process.rs:477) has the same issue, but that predates this PR. This change puts a new user-facing guarantee on it.
  • Confidence 70 · logic · Fix: open lemond's log with .append(true) (truncate first if needed), and test with a real child process writing through that handle.
• [new] The hosted E2E tests check is red on this head — tests/e2e-cucumber/features/dash.feature (dash-08, dash-gen-tps-held-after-scrape-failure)
  • The check fails, so the PR is not mergeable as it stands. From the code, the cause looks pre-existing rather than introduced here:
    • The quit path is not in the diff: quit_and_wait, QUIT_CONFIRM_MARKER and the quit_tui step are unchanged.
    • The quit-confirm gate has_live_instance_with is also unchanged, and dash-08's mock serving model triggers the prompt on base as well.
    • open_dashboard now writes enable_lemonade: false, so no new Lemonade rows reach dash-08.
    • Base fails the same job with two other load-sensitive dash quit/gen-tps scenarios.
  • One unproven indirect factor: the two new always-run mock-lane dash scenarios (dash-38/39) add lane load, and extra load has tipped dash-gen-tps scenarios before. That could change which flaky scenario fails.
  • Confidence 85 (failing check) · logic · Fix: re-run the job. If dash-08 keeps failing, check how much dash-38/39 add to lane load.
• [standing since a133657; still holds in part] Engine state reads failed during the router-to-direct fallback — engines/lemonade/src/router_serve.rs (the failed marks around lines 129, 136 and 140), engines/lemonade/src/direct_llama.rs:134-138, crates/rocm-core/src/service_record.rs:224-230
  • Not re-found blind as stated; settled by putting it to the reviewer afterwards with the author's reply (anchored).
  • Fixed: the fallback now merges starting before ensure_model_in and the spawn, and re-marks failed on error, so a reader no longer sees failed during the download or spawn.
  • Still holds: the router path still writes failed before control reaches the fallback's starting merge. A reader in that gap (milliseconds, not a download) still locks the record at failed, because refresh_from_engine_state returns early for any status outside starting|running|recovering|ready. Closing it means not marking failed on load or verification errors that are eligible for the fallback.
  • Still holds: no test asserts the state during the fallback (the [new] finding above). The reply's "not observable in a unit test" does not match the code: ensure_model_in shells out to lemonade pull, which the lib tests already replace with a fake script that could record state.json mid-fallback.
• [standing since a133657; still holds in part] The direct route for simple registered names has a scenario, but nothing it asserts can gate — tests/e2e-cucumber/expectations.toml (serve-lemonade-simple-registered-direct-route row)
  • Settled by putting it to the reviewer afterwards with the author's reply (anchored, from the CI logs of head 93aacb9).
  • No longer holds: serve-27 exists, and its "managed engine state records direct serving on a GPU backend" step passed on all three Linux GPU lanes, so the route did run on hardware (the reply's "has not run on hardware yet" is out of date).
  • Still holds: the row is flaky = true and matches every lane that runs serve-27, so a route regression back to lemonade-cli would leave every lane green, exactly as the stop-step panic does today. Fixing the stop step and dropping or narrowing the flaky row resolves both.

Non-blocking

• [new] rocm chat scenarios still probe the real Lemonade port — tests/e2e-cucumber/tests/e2e/dash_steps.rs:578-588 (open_chat), tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:1021
  • The new disable_lemonade_discovery_by_default is documented as "call before every dashboard spawn", but only the dashboard, demo, replay and launcher spawns call it.
  • rocm chat also starts the embedded daemon (apps/rocm/src/dash.rs:1047), and discovery now defaults to on. A runner with anything on 127.0.0.1:13305 therefore leaks rows, and with them the quit prompt, into chat scenarios.
  • Confidence 70 · mechanical · Fix: call the helper there too, or move it into TuiSession::spawn for TUI-launching arguments.
• [new] Two new label branches have no test — crates/rocm-dash-tui/src/ui/tabs/instances.rs (card " · last req " label, ~485; combined held + last-req legend, ~205)
  • Only the table legend alone and the detail label are covered.
  • Confidence 70 · mechanical · Fix: add one assertion for each branch.
• [new] A new test passes with the change reverted — crates/rocm-dash-core/src/metrics.rs:370-378 (gpu_system_info_ignores_retired_lemond_version_key)
  • It guards deserialization against deny_unknown_fields; it does not cover the field's removal.
  • Confidence 70 · mechanical · Fix: reword the name or comment so it does not read as coverage of the removal.
• [standing] The PR does several separable things at once — about 8.6k lines across two areas: (1) Lemonade serving (registered-name catalog routing, router-to-direct fallback, Vulkan readiness, served-identity persistence), and (2) dash work (external Lemonade discovery, the registry-only ownership model for Services, engine-pane install status, last-request rate labelling). Each is reviewable alone, and splitting would make later reviews cheaper. Confidence 70 · architectural.

Standing objections settled this round without being kept

Each was put to the reviewer after the blind pass ended, one at a time, with the author's reply. These checks are anchored, because the reviewer had been told the objection.

• dash-35/dash-36 number collision and merge conflict: no longer holds. The branch is rebased onto main (0 behind) and the new scenarios are numbered from dash-37.
• The direct-server leak test stays green when nothing kills it: no longer holds. The fake server writes its own PID before exec. With terminate_pid and child.wait() removed from stop_and_fail, both direct-server tests failed in 3 of 3 runs and passed with the file restored. Residual: the second test passes if the PID file never appears within its 40-iteration poll, so a very slow host could read a leak as a pass.
• The canonical_model_id clause can be deleted without a test failing: no longer holds. With the clause deleted, lemonade_managed_canonical_rewrite_that_still_matches_health_resets_rate fails (28 passed, 1 failed).

Decisions for the author

• External-only Lemonade rows now trigger the "May still be serving — quit anyway?" prompt — tradeoff
  • The gate (crates/rocm-dash-tui/src/app/mod.rs:688-697) counts any serving instance, managed or not. With discovery now on by default, a user running their own Lemonade on 13305 sees the prompt on every q. Yet the help says quitting leaves external servers running, and the prompt offers nothing to act on for those rows.
  • Keeping it is consistent with how unmanaged Docker rows already behave. Gating it on managed would match the new ownership model.
  • dash-38 auto-answers y, so neither choice is pinned by a scenario.

Positive signals

• Lifecycle ownership comes from a serde-defaulted Instance.managed flag set only by registry discovery, not from endpoint names. The services-manager tests caught every mutation of that filter (5 of 5 tests failed when it was forced true).
• telemetry_contract.rs is strong. Ten targeted mutations of the daemon's Lemonade merge and scrape logic were each caught by a named test.
• Moving the router path into router_serve.rs behind a step trait lets failure, stop and fallback paths be driven without a real server, and the tests use it to assert results and state together.

Deployment notes

• rocm dash now probes 127.0.0.1:13305 by default on Linux/WSL. Opt out with dashboard.daemon.enable_lemonade: false in config.json. Older configs pick up the new defaults through serde.
• Older snapshots without the managed field deserialize as unmanaged, which leaves them out of Services stop/restart.

What this covered

• What was read: every file in prw-base...HEAD (32834b2…93aacb9f), in two coupled worker groups:
  • Lemonade engine, the serve CLI and serving e2e.
  • Dash collectors, daemon, TUI and dash e2e.
  • I reviewed README, docs/ and main.rs/config.rs myself.
• What was run:
  • rocm-engine-lemonade lib tests (190 passed).
  • telemetry_contract (29 passed), dash_screen unit tests, and 17 dash-side plus several engine-side single-branch mutation runs.
  • CI logs for the three Linux GPU lanes.
  • No cucumber scenarios were run locally.
  • The apps/rocm unit tests were not built; those verdicts are from reading.
  • Windows cfg arms were not compile-checked locally (the windows-build-and-test check is green).
• What did not run:
  • Prior-changes pass: no earlier reviews were read, by instruction.
  • Step 7 design questions ran in the driver context rather than as independent workers, because of the 2-worker budget. That pass was not independent.
  • The one lib-test file section read only in diff form: the lemonade collector test module past line 400.
• Discussion: this review was not reconciled against the PR's discussion.

[volen-silo]

Addressed in 18ab14a.

• serve-27 stop steps: both stop steps now find the service of the model the scenario served, so serve-27 no longer panics at the stop step. I dropped its flaky expectations row, so a route regression fails the lane. The route step passed on the three Linux GPU lanes at the previous head. It has not yet run to completion since this fix.
• Fallback state: I was wrong that this could not be observed in a unit test. The fake lemonade pull now copies the state file as it is during the pull, and the_state_says_starting_while_the_fallback_pulls_the_model asserts starting. It fails with the starting write removed. The router also no longer marks failed for a load or verification failure: the fallback settles the state (starting while it runs, failed when no direct route is open or it fails), which closes the gap between the two writes.
• Non-ROCm notice in the log: lemond's log handle is now opened in append mode, and a test shows a line appended beside the child's handle survives the child's next write.
• Chat scenarios now turn off Lemonade discovery too. The two label branches (card label, combined legend) have tests. The retired-key test is renamed to say what it guards.
• Description: it no longer says no scenario gates the direct route, and it describes the stop-step fix.

Not changed: the reinstall advice. The runtime-16 step still does not delete the binary first. Making it do so would change a nightly step I cannot run here, so I left it as the PR describes it. The quit prompt for external-only rows, the discovery grace period and the PR split are still open decisions.

[rominf]

🔴 Automated review · pr-review-watcher · 18ab14a

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

Review — needs work

Full review of the whole change.
No ticket named. The description is detailed and mostly accurate, but it says serve-27's stop step "is fixed", and at this head that is not true (see the first finding).

Round 11 (at 18ab14a). This round reviewed 18ab14a in full and blind, because the previous review point, 93aacb9, is no longer in the branch. Each finding is marked [new] or [standing]. The author's reply (comment 6341755344) was read before this round and acknowledged; its claims were weighed only after the blind pass, as below.

Of the round-10 objections: four were put to the reviewer after the blind pass, one at a time, with the author's reply. Those checks are anchored, because the reviewer had been told the objection. None of them still holds as worded (details at the end). The reinstall advice was re-found blind and stays [standing]. This change request replaces the round-10 one as this account's complete position.

What decides this change request: two red checks that this change causes and that are green on main. The four Linux GPU lanes fail serve-27 as a regression. Main's latest completed self-hosted run at the merge base 32834b2 failed only the Windows lane. windows-build-and-test fails to compile the lemonade tests, and it is green on main's latest runs. What does not decide it on its own: the reinstall advice (non-blocking since round 9, and no new evidence), the untested call-site guards in the third finding, and the hosted E2E tests failures (dash-09, serve-01, serve-02), which also fail on main at ea1d871 and 32834b2. GitHub also reports a merge conflict with main (README.md), so the branch needs a rebase.

Blocking

• [new] The new scenario serve-27 fails on all four Linux GPU lanes at this head — tests/e2e-cucumber/tests/e2e/serving_steps.rs:835-845, apps/rocm/src/main.rs:6763-6797 (render_service_action_result), apps/rocm/src/main.rs:7076-7081

  • @id:serve-lemonade-simple-registered-direct-route failed with "FAIL … expected to pass on this host but FAILED — a regression" on MI300X, W7900, Strix Ubuntu release and Strix Ubuntu nightly.
  • The route, inference and identity steps pass. The failing step is "Then the managed service is stopped and its endpoint is closed". It requires the stop output to contain the service id. The real output is only Local server stopped / stopped processes: 1.
  • Why the id is missing: the in-process stop report has result.service_id, but the renderer only prints a service: line from result.service.service_id. The unit test service_actions_require_yes_and_render_sandbox_result feeds the renderer the shape it expects, not the shape production sends.
  • The renderer mismatch was already on main. This PR's new scenario is the first thing to assert on it, and the PR makes the scenario required on those lanes with no expectations row.
  • Which side governs: the renderer. Its own unit test promises a service: line, and production never prints one.
  • The description says this step "failed there because it looked up the wrong model, which is fixed". The failure at this head has a different cause.
  • serve-26 uses the same stop step, so it will fail the same way. It has not run anywhere, because it is @nightly and was not included on the PR lanes.
  • Confidence 100 · logic
  • Fix: have render_service_action_result also read result.service_id and result.status, and add a unit test fed the real stop_internal_managed_service report shape. Then correct the description.

• [new] The Windows build fails to compile the lemonade tests — engines/lemonade/src/lib.rs:1156-1192 (fallback_fixture_with)

  • The required windows-build-and-test check is red with five errors: E0432 no fake_runtime in test_http, and E0425 for write_registry, install_fake_llama_server, fake_request and fake_context.
  • The sibling fallback_fixture has #[cfg(target_os = "linux")]; this function does not, and its body uses only Linux-gated helpers.
  • The worker reproduced the five errors by mapping the Linux cfg to an unmatchable OS. Adding the one missing cfg made the same check pass.
  • Confidence 100 · mechanical
  • Fix: gate fallback_fixture_with with #[cfg(target_os = "linux")].

• [new] New behaviour that the added tests do not pin (mutation-checked) — engines/lemonade/src/lib.rs (serve_with_context, the runtime_is_linux() argument to with_direct_fallback), crates/rocm-dash-daemon/src/runner.rs:870 (lemonade_discovery_warn = None), engines/lemonade/src/lib.rs test serving_regression_persisted_identity_drives_health_chat_model

  1. Replacing the call-site runtime_is_linux() with true leaves all 192 lemonade lib tests green. Nothing stops a Windows router load failure from falling back to direct serving, which the README and serving_route both rule out.
  2. Deleting the line that clears the external-discovery warning on success leaves all 20 telemetry_contract lemonade tests green. A recovered endpoint would keep its warning.
  3. serving_regression_persisted_identity_drives_health_chat_model passes with the production change reverted. It writes the state itself, and healthcheck_service_at already read canonical_model_id on the base. The name claims regression coverage it cannot provide; the real coverage is router_state_keeps_requested_text_and_records_router_name.
  • Confidence 100 (all three were run) · logic
  • Fix:
    • For item 1, compute the Linux flag inside with_direct_fallback, or test the call site through a seam.
    • For item 2, extend lemonade_seen_external_endpoint_closing_warns_and_removes_rows to bring the endpoint back and assert the warning clears.
    • For item 3, rename the test as a characterization test or remove it.

• [standing since round 8; non-blocking since round 9] The new remediation advice is not proven to clear the condition it is printed for — engines/lemonade/src/direct_llama.rs (the "run rocm engines install lemonade --reinstall to restore it" bail), tests/e2e-cucumber/features/runtime_setup.feature (runtime-16), docs/testing.md

  • AGENTS.md §3: remediation advice that names a command must "actually clear the condition that printed it", and the covering test asserts the message and the resulting state together.
  • The only behavioural check is the new last step of runtime-16. By its own comment it does not remove the binary first, so it never shows the reinstall restores a missing one.
  • runtime-16 is also @requires-os:windows @nightly. The Linux direct route, where this error mostly fires, is never exercised.
  • docs/testing.md says this step "runs only on the nightly GPU lanes"; in fact it runs only on the Windows nightly GPU lane.
  • Confidence 85 (cited rule) · logic
  • Fix: delete the selected backend's binary, run the reinstall, and assert the binary is back. This could be a unit test against the install code with a fixture archive. Otherwise drop the "to restore it" claim. Either way, correct the lane note in docs/testing.md.

Non-blocking

• [new] A Lemonade last-request rate is still summed as live throughput everywhere except the Instances tab — crates/rocm-dash-tui/src/ui/tabs/home.rs:407-420, ui/widgets.rs (node tok/W), ui/tabs/observe.rs:180-205, agent/snapshot.rs:49-80, ui/tabs/pane.rs, ui/services_manager.rs

  • observe_direct stamps the repeated /stats rate as Fresh on every scrape, so an idle server's last rate feeds the Home hero sum, tok/W and the "gen N" rows.
  • The assistant JSON calls it "live generation throughput", freshness "fresh". Only instances.rs reads gen_tps_last_request.
  • This was already true on main, but this PR turns discovery on by default, so it now reaches every dashboard user. The description acknowledges the per-row half of this, not the totals or the agent snapshot.
  • Confidence 85 · logic
  • Fix: leave last-request rows out of the totals and tok/W (or mark them there), and add the flag to the agent snapshot.

• [new] Stale comments: the router no longer marks the state failed on a load failure — engines/lemonade/src/lib.rs (doc comment on with_direct_fallback), engines/lemonade/src/direct_llama.rs:132, and the doc comments of the_state_says_starting_while_the_fallback_pulls_the_model and a_fallback_that_cannot_get_the_model_ends_failed_not_starting

  • router_serve.rs:129-135 skips the failed mark for RouterLoadFailure, and router_failures_return_error_stop_server_and_mark_state_failed asserts running.
  • Those two fallback tests hand-mark failed first, a state production never produces at that point.
  • The code governs: it is what the router test asserts.
  • Confidence 85 · mechanical
  • Fix: reword the comments to "left at running" and drop the artificial pre-mark.

• [new] Two ownership predicates; the quit dialog can point at a Services list that is empty — crates/rocm-dash-tui/src/ui/services_manager.rs:93-97 (i.managed) vs app/mod.rs:726-733, :823-848 and the quit confirmation in ui/mod.rs (snapshot.managed_ids)

  • Against a separately running older daemon whose snapshot lacks managed (defaults to false), the quit dialog prints "Press s to open Services and stop it.", but the overlay filters to managed rows and is empty.
  • Confidence 70 · architectural
  • Fix: use one predicate for both.

• [new] Managed Lemonade and llama-server rows show a health warning for their whole startup — crates/rocm-dash-daemon/src/registry.rs:298-321, runner.rs:840-847, runner.rs:885-888

  • Records in starting or recovering are scraped, and connection-refused or 503 health responses push a warning on every tick until the model is ready.
  • The direct-llama fixture serves 404 on /v1/health, so no test reaches this path.
  • Confidence 70 · logic
  • Fix: suppress the warning while the registry status is Starting.

• [new] Host normalisation written twice — crates/rocm-dash-daemon/src/registry.rs:305-311 vs normalized_host/scrape_host (:229-273)

  • The two copies agree today only because endpoints_match re-normalises.
  • Confidence 70 · mechanical
  • Fix: share one helper.

• [new] gen_tps_last_request is never reset — crates/rocm-dash-core/src/observation.rs:265, runner.rs:1104-1106

  • After expiry or invalidation, the card reads "last req —".
  • Confidence 70 · logic
  • Fix: derive the flag from is_direct() && gen_tps.is_some().

Standing objections settled this round without being kept

Each was put to the reviewer after the blind pass ended, one at a time, with the author's reply. These checks are anchored, because the reviewer had been told the objection.

• serve-27's stop step looks up the wrong model, and a flaky row hides it and any route regression: no longer holds as worded. Both stop steps now look up the model the scenario served, and the stop command ran on all four lanes. serve-27 has no expectations row, and the lanes report it as a regression, so a route regression would also fail them. The scenario still fails at the same step, for the different cause in the first finding. Still to check once that is fixed: that the endpoint-closed and stopped-record assertions pass. They come after the failing assertion and have not run in CI.
• The fallback's starting state is untested: no longer holds. With the starting write removed, the_state_says_starting_while_the_fallback_pulls_the_model failed (8 fallback tests ran, 1 failed, 7 passed).
• Engine state reads failed during the router-to-direct fallback: no longer holds, from reading the code. Load and verification failures are both tagged RouterLoadFailure, and the router skips the failed mark for those. The fallback then writes failed when no direct route is available (Windows, or an ineligible model or backend), and also when the fallback itself fails. The one production caller always goes through with_direct_fallback. The call-site Linux flag is not pinned by a test (third finding, item 1).
• The non-ROCm notice is overwritten in the serve log: no longer holds, from reading the code. open_service_log truncates, then reopens in append mode for lemond. The new test exercises that production function and announce_non_rocm_backend. No test asserts that an old log is truncated.

The round-10 non-blocking items were not put to the reviewer. Except for the PR's size, the author reports them all fixed: the chat-discovery leak, the label-branch tests and the retired-key test name.

Decisions for the author

• Lemonade discovery is on by default — tradeoff
  • For: zero configuration for the common local-Lemonade case.
  • Against: every existing rocm dash user now probes 127.0.0.1:13305 every discovery tick, and existing configs gain three keys on their next save.
  • Confirm on by default is what you want, rather than opt-in.
• dash-37 can never block a merge — tradeoff
  • Its flaky row matches every lane that can run it, so the real managed-dashboard journey is advisory everywhere. The docs say so and point to unit tests for the wording.
  • The positive ownership path is gated only by the hosted dash-07 extension.
• The e2e harness re-implements the engine's backend lookup — non-blocking-improvement
  • tests/e2e-cucumber/src/lemonade_backend.rs mirrors backend_server_dirs / find_llama_server_binary_for_backend.
  • Drift would show up as a false red, not a false green, but the directory list now lives in two places.

Positive signals

• The RouterSteps / DirectServerSteps seams inject a failure at every step after a spawn, including state writes and the final wait, and assert the child is stopped exactly once.
• Pending approvals in the Services overlay are re-validated on every key and every draw, so a vanished or external target can never run a job. Mutating either guard fails a test.
• dash-38/39 make the read-only promise checkable: the protocol fixture logs every non-observational request, and the scenario asserts that log is empty after the stop/restart keys and again after quit.

Deployment notes

• dashboard.daemon.enable_lemonade defaults to true, with lemonade_host 127.0.0.1 and lemonade_port 13305. Users with something else on that port will see it probed.
• The merge has a conflict in README.md against prw-base (ea1d871, the #616 copyedit), so the PR must be rebased.

What this covered

What was read: all 46 files of prw-base...HEAD (32834b2…18ab14a5), split three ways.

• Worker 1: the lemonade engine, the main.rs hunks and config.rs, read completely.
• Worker 2: the dash crates and dash.rs.
• Driver: the e2e harness, features, steps and docs, plus CI.

Every finding marked 100 was confirmed by a CI log or a local mutation run.

CI, compared with main's latest runs:

• windows-build-and-test: red here from a compile error introduced by this PR. Main's failures are only in E2E tests, not this job.
• Hosted E2E tests: dash-09, serve-01 and serve-02 fail here. They fail identically on main's two latest CI runs (39793631227, 39631143323), which also fail dash-34; this PR fixes dash-34. So these are not attributed to this change.
• Four Linux GPU lanes: red here only through the regression gate on serve-27. Main's latest self-hosted runs were green on those lanes.

What did not run, or ran with limits:

• Worker 2 read only part of its group. It read every diff hunk in full, and read runner.rs, collectors/lemonade.rs, services_manager.rs, registry.rs (production half) and pane.rs (production half) completely. For the unchanged parts of app/mod.rs, event_loop.rs, ui/mod.rs, the instances.rs tests, format.rs, demo.rs, docker.rs, the observation.rs tests, and dash.rs outside its hunks, it read only the code around the changed fields. It said so. Neither re-spawning it nor re-reading those parts here fitted the two-worker budget and the time budget, so those unchanged regions were not read in full.
• Design questions (Step 7) were worked through in the driver's context, not as independent workers, because of the same budget.
• History and prior-change passes: limited to git log on touched files and the merge-conflict check. Earlier review comments were deliberately not read.
• No real Windows cross-compile (no mingw toolchain here). The Windows compile finding rests on CI plus the simulated-cfg check.
• No GPU scenarios run locally. GPU evidence comes from CI lane logs.

One worker finding was refuted and dropped: that discovery stalls the telemetry loop on Windows. The embedded daemon is #[cfg(unix)] only (apps/rocm/src/dash.rs maybe_spawn_embedded_daemon). Two 40-confidence candidates were dropped: the container_name == "lemonade" heuristic and the IPv6-only :: bind on Windows. No prompt injection was found in the files read.

This report was not reconciled against the PR's discussion.

[rominf]

🔴 Automated review · pr-review-watcher · dd428d7

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.

Round 12 (at dd428d7). This round reviewed dd428d7 in full and blind, because the previous review point, 18ab14a, is no longer in the branch. Each finding is marked [new] or [standing]. No author reply was outstanding.

What decides this change request: the fourth blocking finding. A suggested model from Lemonade's own released catalog cannot be served on Linux at this head. The reviewer rated it 70 because it could not see a real catalog. The driver then confirmed it:

• The released v11.5.1 src/cpp/resources/server_models.json in lemonade-sdk/lemonade contains "gpt-oss-120b-mxfp-GGUF": {"checkpoint": "ggml-org/gpt-oss-120b-GGUF:*", "recipe": "llamacpp", "suggested": true, ...}.
• A probe test in the checkout called serving_route with that entry. With is_linux = true it returned Err("registered model gpt-oss-120b-mxfp-GGUF has an invalid HF checkpoint"). With is_linux = false it returned the router route (1 test ran, 1 passed, output inspected).
• The error comes before the binary check, so it fires on every Linux host, with or without a direct-serve binary.

What does not decide it on its own. The other blocking items are test-strength or test-hermeticity gaps. Each one leaves correct production code behind, and CI is green on them. They should still be fixed, but on their own they would not hold this PR. The reinstall advice has been non-blocking since round 9, and there is no new evidence.

CI at this head, compared with main:

• Every GPU lane is green, including serve-27 on MI300X and Strix Ubuntu, with its stop step passing. windows-build-and-test is green too.
• The hosted E2E tests job fails dash-09, serve-01 and serve-02. The same scenarios fail on main's latest CI run at ea1d871, so they are not attributed to this change.
• GitHub no longer reports a merge conflict.

Round-11 objections:

• serve-27 fails on the four Linux GPU lanes: no longer holds. The lane logs at this head show every step passing, including "the managed service is stopped and its endpoint is closed".

• The Windows build fails to compile the lemonade tests: no longer holds. windows-build-and-test is green.

  Both are settled by CI runs at this head, not by the reviewer's silence.

• The three test gaps (call-site Linux flag, warning cleared on recovery, misnamed characterization test): these were not gating. From the driver's reading of the code, they look addressed:

  • is_linux now flows through ServeContext into serve_with_context.
  • lemonade_seen_external_endpoint_closing_warns_and_removes_rows now brings the endpoint back and asserts the warning clears.
  • The characterization test is gone.

  They were not put to the reviewer this round, so treat that as unverified rather than settled.

This change request replaces the round-11 one as this account's complete position.

Review — needs work

Full review of the whole change.
No ticket named. The description is detailed and matches the diff, with one disagreement folded into the third blocking finding below.

Blocking

• [new] A new lemonade unit test reads the host's real environment, so it fails on any host without XDG_RUNTIME_DIR — engines/lemonade/src/router_serve.rs:203-211, engines/lemonade/src/backend_alignment.rs:641, test at engines/lemonade/src/lib.rs:1341-1377

  • The router passes parent (the environment it uses to set up the spawned server's runtime directory) to every step except select_backend. That step calls ensure_best_llamacpp_backend, which ends up in ParentRuntimeEnvironment::current().
  • So a_router_load_failure_is_not_served_directly_when_the_host_is_not_linux depends on the host's own environment.
  • With XDG_RUNTIME_DIR unset, the test failed (0 passed, 1 failed): "failed to select a supported Lemonade llama.cpp backend". With XDG_RUNTIME_DIR pointed at a directory inside the checkout, it passed (1 passed).
  • The worker's run showed the full error chain: it tried to create ~/.rocm/data/lemonade and got permission denied.
  • The test passes in CI only because the CI environment is writable. That contradicts what ServeContext.parent says it is for: letting tests supply a confined parent environment.
  • Confidence 100 · mechanical
  • Fix: pass parent through ensure_best_llamacpp_backend, run_lemonade_backends_list and the backend install call.

• [new] The O_APPEND fix to the service log has a test that still passes when the fix is reverted — engines/lemonade/src/process.rs:127-134, test a_line_appended_beside_the_childs_log_handle_survives_its_next_write (process.rs:1143-1156)

  • I changed .append(true) to .write(true) and ran the test: 1 passed.
  • Without append, the child's second write lands on top of the first 15 bytes of the appended notice. The assertion contains("not ROCm") matches text further into the notice, so it survives.
  • Confidence 100 · mechanical
  • Fix: assert the whole notice line (for example the string announce_non_rocm_backend writes), or compare the exact file contents.

• [new] Three new GGUF-matching branches survive mutation with the full lemonade suite — engines/lemonade/src/direct_llama.rs:676 (other == stem in first_shard_of_complete_set), :487 (size >= 0.0), and the exact-.gguf-filename empty-result branch in filter_ggufs_by_variant (:733 onward)

  • A worker ran each mutation over all 193 tests in rocm-engine-lemonade --lib, and I checked those outputs.
  • In each run, the only failing test is the environment-dependent test from the first finding, which fails without the mutation too.
  • The stem check matters most: without it, shards from two different models with the same shard count in one snapshot count as one complete set and get served together.
  • Confidence 100 · mechanical
  • Fix: add one test per branch: two stems with equal totals, a negative size, and an exact .gguf filename that does not exist.

• [new] On Linux, a registered catalog entry whose variant is not a "safe" filename (e.g. *) now makes rocm serve fail instead of using the router — engines/lemonade/src/direct_llama.rs:473-476 (with is_safe_hf_component, :365-372)

  • parse_hf_checkpoint(checkpoint_ref)...? runs before the check that would send non-simple entries to the router, so a variant like * becomes a hard error.
  • The code comment and the README ("A registered checkpoint that is malformed or unsafe is rejected on Linux") say this is deliberate. The description says "Anything else stays with the Lemonade router".
  • A sub-worker reports that the pinned 11.5.1 catalog has gpt-oss-120b-mxfp-GGUF with checkpoint ggml-org/gpt-oss-120b-GGUF:*. I could not open a real catalog to confirm. If it is there, a catalog model the router served on the base branch is now unservable on Linux, while Windows still routes it.
  • The twin call site, direct_fallback_route, swallows the same error with .ok().flatten(), so the two paths disagree.
  • Which side governs: the router-compatible behaviour. * is how Lemonade's own catalog says "every file", not a malformed path, and the base branch served it.
  • Confidence 70 · logic
  • Fix: for a registry-sourced checkpoint, route a variant that does not parse to the router (Ok(None)), and keep the hard error for user-typed owner/repo:variant only. Add the real * entry to released_catalog_entries.

• [standing since round 8; non-blocking since round 9] The new remediation advice (rocm engines install lemonade --reinstall) is never shown to clear the condition that prints it — engines/lemonade/src/direct_llama.rs (the serving_route bail "no selected GPU llama-server backend ... run rocm engines install lemonade --reinstall to restore it"), with its comments in the unit test and in runtime_steps.rs

  • AGENTS.md §3 requires that advice naming a command be backed by a test where "the named command must exist, accept those flags, and actually clear the condition that printed it". Where the wording is pinned on its own, the comment must name the test that proves the behaviour.
  • The pinning comment names runtime-16, but runtime-16's new step does not delete the binary first.
  • The unit-test comment, the step's doc comment and the description all say restoring a missing binary is not covered.
  • Confidence 85 · logic
  • Fix: add a scenario step that removes the selected backend's llama-server, runs the advised command, and asserts the binary is back.

Non-blocking

• [new] The non-ROCm backend notice says Lemonade "is serving" before the model has loaded, and prints twice on the fallback path — engines/lemonade/src/router_serve.rs:87, engines/lemonade/src/process.rs (serve_direct_llama_server)
  • The router announces right after select_backend, before load_model and verification.
  • If the load fails and there is no fallback, the log claims serving that never happened. If the fallback runs, the direct path announces again.
  • AGENTS.md §3 says outcome messages must be true. Traced by reading only.
  • Confidence 70 · logic
  • Fix: announce after verification succeeds.
• [new] serve_direct_after_router_failure loses the router's error if writing the starting state fails — engines/lemonade/src/direct_llama.rs:110 onward (the merge_json_state(...).context(...)?)
  • That early return drops router_error, so the user sees only the state-write error.
  • Confidence 70 · logic
  • Fix: chain router_error into that error.
• [new] The quit-confirm doc comment now contradicts the code — crates/rocm-dash-tui/src/ui/mod.rs:259 vs :350-358
  • The doc comment says the body mirrors the Serving tab's "Managed running" listing. That listing is now managed-only.
  • The quit body still lists every serving instance, as the comment the PR edited at :350 itself says. The code and the :350 comment govern.
  • Confidence 85 · mechanical
  • Fix: reword the :259 doc.
• [new] IPv6 bracketing in LemonadeCollector::url is covered only by daemon tests that print SKIPPED and pass when an IPv6 bind fails — crates/rocm-dash-collectors/src/lemonade.rs:273, crates/rocm-dash-daemon/tests/telemetry_contract.rs:1022,1077,1082
  • The worker disabled the bracket condition and all 16 collector tests stayed green. Only the IPv6 daemon tests caught it, and those skip on hosts without IPv6.
  • Confidence 85 · mechanical
  • Fix: add a pure unit test for url() with ::1, [::1] and an IPv4 address.
• [new] The "Coverage matrix" module doc in telemetry_contract.rs is stale — crates/rocm-dash-daemon/tests/telemetry_contract.rs:13
  • It still lists the 9 original scenarios and none of the 20 new Lemonade contract tests.
  • Confidence 70 · mechanical
  • Fix: update the matrix.
• [standing] The PR is too large to review as one unit — 46 files, +8.9k/−0.8k lines.
  • It combines two separable changes. One is Lemonade serve routing and fallback (engines/lemonade, readiness in main.rs, serve scenarios). The other is dashboard external discovery plus registry-only lifecycle ownership (rocm-dash-*, dash scenarios).
  • Splitting them would let each get a full review.
  • Confidence 70 · architectural
  • Fix: consider splitting along that line.

Decisions for the author

• External Lemonade discovery is now on by default, polling 127.0.0.1:13305 — tradeoff
  • For: users see an already-running Lemonade server with no setup.
  • Against: every dashboard user now polls that port. External models appear in Observe and keep the quit-confirm prompt live (has_live_instance_with counts unmanaged rows). The e2e harness turns discovery off by default, so the shipped default is exercised only by a unit test.
  • Confirm that on by default is intended.
• The Services overlay now lists only registry-owned instances, which also removes Docker-discovered containers — tradeoff
  • The overlay could not actually stop those containers, so hiding them is more honest.
  • But it is a visible change for Docker vLLM users, and the description mentions it only in passing. Confirm it is intended.

Positive signals

• run_rocm_bounded (tests/e2e-cucumber/tests/e2e.rs) sends child output to files and kills the child at the deadline. A serve that never returns now fails its own scenario instead of stalling the whole lane, and serve_cli_budget leaves the CLI room to print its own diagnostic first.
• The lemonade_protocol fixture records every request other than GET health/stats. That is a direct, falsifiable check that discovery stays read-only, not just an absence of errors.
• serve-27 checks load_response.method in the engine state, not just whether the endpoint answers. It is the only check that tells the direct route from the router route.

Deployment notes

• New config fields dashboard.daemon.enable_lemonade (default true), lemonade_host (127.0.0.1) and lemonade_port (13305). Existing config files pick up these defaults, so external discovery switches on for existing users after upgrading.
• The router readiness check now also accepts a Vulkan backend, as well as ROCm.

What this covered

Every file in prw-base...HEAD (0314db3…dd428d7b), read in full, plus the callers and twin implementations of the changed symbols: the backend-dir lookup in the engine and its e2e mirror, the readiness backend checks, and the ownership predicate.

• How it was split: two independent read-only workers took the coupled code groups:

  • the Lemonade engine with config.rs;
  • the dashboard daemon, collectors and TUI with dash.rs.

  I read the docs, the feature files, the e2e harness and step files, and the main.rs CLI changes myself, so that group was not an independent pass.

• Re-runs: I re-ran the first two blocking findings myself. For the third, I checked the workers' saved outputs rather than re-running the mutations.

• Did not run:

  • Step 7 design questions: not run as independent workers, because of the 2-worker cap and the time budget. The two decisions above come from my own reading only.
  • Prior-changes pass: not run; earlier review comments are out of bounds here.
  • E2E scenarios: not run locally.
  • Windows check: the cross-target check could not run (no mingw linker). Windows cfg arms were checked by reading and by CI's green Windows build.
  • The *-variant catalog entry: not confirmed against a real Lemonade catalog.
  • CI: read as given by the caller. The hosted E2E failures (dash-09, serve-01, serve-02) are treated as pre-existing.

• Not reconciled: this report was not checked against the PR's discussion.

Addressed at 1ae0844. The lemonade unit tests pass with XDG_RUNTIME_DIR unset (the backend selection takes the parent environment). The O_APPEND test now fails when .append(true) becomes .write(true). Each of the three GGUF branches (shard stem, negative size, exact .gguf filename) has a test that fails when the branch is mutated. A registered checkpoint with a * variant routes to the router on Linux, and the real gpt-oss-120b-mxfp-GGUF entry is in the test data. All required checks pass, including windows-build-and-test.

[rominf]

🔴 Automated review · pr-review-watcher · 1ae0844

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.

Round 13 (at 1ae0844). This round reviewed 1ae0844 in full and blind, because the previous review point, dd428d7, is no longer in the branch. Each finding is marked [new] or [standing]. No author reply was outstanding.

This round lifts every standing change request from this automation. Nothing gates any more:

• The round-12 gating objection no longer holds. That objection was that on Linux, gpt-oss-120b-mxfp-GGUF (checkpoint ggml-org/gpt-oss-120b-GGUF:*) failed with "invalid HF checkpoint" instead of going to the router.
  • resolve_direct_llama_checkpoint now returns Ok(None) for a registry checkpoint that does not parse, so the entry routes to the router on Linux and Windows. The fallback declines it cleanly.
  • The real entry is now in released_catalog_entries. released_catalog_default_model_routes_direct_with_binary_and_router_without and a_registered_wildcard_variant_routes_to_the_router_not_an_error pass (2 ran).
  • With the old hard error put back, both fail.
• The round-12 test gaps no longer hold:
  • The XDG-dependent test now passes with XDG_RUNTIME_DIR unset, because select_backend uses the given parent environment.
  • Each of the three GGUF-matching mutations now fails a named test, run over all 198 lemonade lib tests.
  • The blind pass found O_APPEND pinned by a whole-file assertion.
• The round-11 test gaps:
  • The recovery warning-clear check now passes.
  • The retired-field test is renamed and its comment matches what it checks.
  • The call-site Linux flag partly holds. The flag is pinned from ServeContext down. But the line that fills it from the host, is_linux: runtime_is_linux() in serve_http, can be changed to false with all 198 lemonade lib tests still passing. Only GPU scenario serve-27 would catch that.
  • Under the floor this is a test-strength gap over correct code. It is reported, not gated.

These settlements are anchored: each objection was put to the reviewer after its blind pass ended. They are not blind re-findings.

Why the report below still reads "needs work". Its Blocking items are not gated here, for these reasons:

• Non-ROCm notice before readiness: rated non-blocking at round 12. The router copy is now fixed. The direct-path copy that remains is a log line, which is not new evidence of a larger consequence.
• The --reinstall advice: non-blocking since round 9.
• Last-request rate in the totals: non-blocking since round 11.
• The description omission: concerns the PR description only. A squash merge builds its body from the commit messages, and those make no false claim.

All of them should still be addressed.

CI at 1ae0844: every hosted check is green, including windows-build-and-test and E2E tests; skillscope is skipped. No self-hosted GPU lane has reported at this head, so serve-27 is still unproven on hardware. That is missing evidence, not a defect.

Review — needs work

Full review of the whole change.
No ticket named. The description covers registered-GGUF resolution, the Linux router-to-direct fallback, read-only external Lemonade discovery in rocm dash, and registry-only lifecycle ownership. The code does all of these. The one unmentioned change is listed under Blocking.

Blocking

• [standing since round 12, non-blocking there] The "Lemonade is serving on the X GPU backend" notice is printed and logged before the direct server is ready — engines/lemonade/src/process.rs:544-560, compared with engines/lemonade/src/router_serve.rs:107-109

  • The router path waits until verification passes before announcing. Its comment explains why: "Only now is it true that Lemonade is serving on this backend: announcing earlier would claim it for a load that fails".
  • The direct path is the other copy of the same announcement. serve_direct_llama_server calls non_rocm_backend_notice and writes "Lemonade is serving on the {backend} GPU backend, not ROCm; ..." to stderr and to the serve log before it spawns the server or checks readiness.
  • If the direct server then fails readiness, the log claims it is serving next to a failed state.
  • This announcement is new in this change; prw-base has no non_rocm_backend_notice. No test covers the failing case.
  • AGENTS.md §3 requires a message claiming an outcome to be asserted together with the resulting state.
  • Confidence 85 · logic · Fix: announce from the ready transition (record_ready, or after supervise_direct_server reports ready), as the router does, and add a test where readiness fails and the log does not claim serving.

• [standing since round 8; non-blocking since round 9] The new --reinstall advice for a missing llama-server is never shown to fix that condition — engines/lemonade/src/direct_llama.rs:75, tests/e2e-cucumber/features/runtime_setup.feature (runtime-16), tests/e2e-cucumber/tests/e2e/runtime_steps.rs:537-588

  • This change rewrites the advice from rocm engines install lemonade to ... --reinstall to restore it.
  • AGENTS.md §3 requires that remediation advice naming a command "actually clear the condition that printed it", asserted in the same test.
  • The new step only checks that the binary exists after a reinstall. Its own doc says "The runtime is not damaged first, so this does not prove the reinstall restores a missing binary".
  • That step is also @requires-os:windows @nightly, while the message fires mostly on the Linux direct route.
  • The unit test pins the wording only, and the scenario its comment points to does not prove the behaviour.
  • Reading install.rs (reinstall forces re-extraction) makes the claim plausible, but it is unverified.
  • Confidence 85 · logic · Fix: delete the selected backend's llama-server before the reinstall step, and run that check on a lane that exercises the Linux route.

• [standing since round 11, non-blocking there] A Lemonade last-request rate is counted as live throughput everywhere except the Instances tab — crates/rocm-dash-tui/src/ui/widgets.rs:99-115 (node_efficiency), crates/rocm-dash-tui/src/ui/tabs/observe.rs:96-107, crates/rocm-dash-tui/src/ui/tabs/home.rs:276-300, crates/rocm-dash-tui/src/agent/snapshot.rs:58-113

  • gen_tps_last_request is read only in tabs/instances.rs (:121, :486, :754).
  • node_efficiency adds up every instance's gen_tps and divides by current GPU power. An idle Lemonade server keeps reporting its last request's rate as fresh, because every successful scrape refreshes the observation. The tok/W hero, the Home T/S hero and the agent snapshot therefore show a past rate as current, with no marker.
  • README states the opposite: "not live throughput: it stays the same while the server is idle".
  • On prw-base, apps/rocm/src/dash.rs set enable_lemonade: false. This change turns discovery on by default and scrapes managed Lemonade rows, so these figures now include such rates on every Linux dashboard.
  • Confidence 85 · logic · Fix: leave gen_tps_last_request rates out of the summed figures and node_efficiency, or label them there too, and carry the flag into the agent snapshot.

• [new] The description omits a user-visible output change — apps/rocm/src/main.rs:6770-6787

  • render_service_action_result now also reads service_id/status directly from result. As a result, the output of rocm services stop for the in-process stop report now includes service: and status: lines.
  • The description's bullets do not mention this. Its only nearby reference is that serve-27's stop step "looked up the wrong model, which is fixed".
  • Confidence 85 · mechanical · Fix: add a line to the description (the code and its test service_stop_render_prints_the_in_process_stop_report_shape are fine).

Non-blocking

• [new] The direct fallback erases lemond's router-failure log when it has to download the model — engines/lemonade/src/process.rs:127 (open_service_log uses File::create), engines/lemonade/src/direct_llama.rs:148, :250

  • A worker seeded serve.log with a marker line in the_state_says_starting_while_the_fallback_pulls_the_model. After the fallback the marker was gone (instrumented run, rc=101).
  • Only the router_error text inside the "Launching ... directly" line survives.
  • Confidence 100 · logic · Fix: open the log in append mode for every spawn after the first.

• [standing] The router_serve.rs module doc contradicts its code — engines/lemonade/src/router_serve.rs:10-12 against :129-137

  • The doc says that after any failure "the engine state is left at failed".
  • The code is the side that is right here: it deliberately skips failed for RouterLoadFailure, and tests assert running. The doc should change, not the code.
  • Confidence 85 · mechanical.

• [new] Health and stats warnings name rows by hex-encoded ids instead of model names — crates/rocm-dash-daemon/src/runner.rs:56-64, crates/rocm-dash-collectors/src/lemonade.rs:209-216

  • LemonadeEndpoint::warning joins row.id, and lemonade_model_id encodes the model name as hex bytes, so users see ...-model-50726f....
  • Confidence 85 · mechanical · Fix: put the model names in the warning text.

• [new] serve-27 has no flaky-expectations row, though it does the same real Linux serve as the existing flaky rows (EAI-7423) — tests/e2e-cucumber/features/model_serving.feature:349, :368; tests/e2e-cucumber/expectations.toml:96-112

  • serve-26 justifies having no row because it takes the router route. serve-27 takes the direct route, is not @nightly, and has never run to completion on hardware.
  • Confidence 70 · logic · Fix: add a row, or explain in the scenario's comment why this serve is exempt.

• [standing] A non-Lemonade HTTP service on port 13305 now produces a warning on every rocm dash — crates/rocm-dash-collectors/src/lemonade.rs:327-331, crates/rocm-dash-daemon/src/runner.rs:637-641

  • A 404 on both routes counts as "answered with an error", and discovery is now on by default.
  • Confidence 70 · logic · Fix: treat "neither route exists" on a never-seen endpoint the same as "absent".

• [standing] Lemonade polling is awaited inline in the run loop — crates/rocm-dash-daemon/src/runner.rs:813-865

  • The worst case per endpoint is about 6 s: four 1.5 s timeouts across health, stats and their fallbacks.
  • A hung, never-seen listener on the default port silently delays each discovery tick's snapshot by about 1.5 s.
  • This follows the existing vLLM scrape pattern.
  • Confidence 70 · architectural.

• [standing] Two IPv6 contract tests pass without running when the bind fails — crates/rocm-dash-daemon/tests/telemetry_contract.rs:1030-1032, :1085-1092

  • They eprintln!("SKIPPED ...") and return, so a CI host without IPv6 reports them green.
  • Confidence 85 · mechanical · Fix: make a skip visible in CI, for example with #[ignore] plus a lane that has IPv6, or by failing when the environment variable that requires IPv6 is set.

• [new] Stale names and references — apps/rocm/src/main.rs:11542, apps/rocm/src/main.rs:16875

  • The parameter require_rocm_backend now means any GPU backend; rename it.
  • The test doc cites "serve-27" by index. Indices go stale; cite @id:serve-lemonade-simple-registered-direct-route.
  • The PR description calls the managed journey "dash-37", but the feature file numbers it dash-38.
  • Confidence 85 · mechanical.

• [standing] The PR mixes two separable changes across 47 files and about 9k lines — the lemonade serve routing and fallback (engines/lemonade), and dash external discovery, ownership and labelling (rocm-dash-*). They share no code path beyond the docs. Splitting them would make each easier to review and to revert.

Decisions for the author

• Simple registered names now serve directly on Linux, without the router's per-model launch defaults — tradeoff

  • direct_llama.rs:47-50 gives this as the reason Windows keeps registered names on the router: "direct serving ... does not apply the router's per-model launch defaults". That reason applies equally to Linux.
  • On the other side, the Linux direct route avoids the router's intermittent load failures and is limited to entries with no extra files or options.
  • Please confirm that dropping Lemonade's per-model defaults (context size, template flags) for those entries on Linux is intended.

• A new TUI connected to an older daemon shows no managed services — tradeoff

  • Instance.managed defaults to false through serde. A new rocm dash attached to an older daemon's socket therefore shows "No managed services" and offers no stop or restart.
  • This fails safe, but it will look like a bug to the user.

Positive signals

• Lifecycle ownership comes only from the registry. service_rows filters on managed, and a staged approval is re-checked on every key and draw. A worker removed that re-check and three tests failed.
• The external-protocol fixtures record every request that is not a GET of health or stats, so dash-39 and dash-40 would catch any load, unload or stop sent to an external server.
• The RouterSteps and DirectServerSteps seams let tests exercise stop-on-failure and the state transitions with real child processes, and one guard stops the spawned server on every step after spawn.
• O_APPEND on the shared log is pinned by a whole-file assertion, and the router announcement itself is correctly withheld until the load is verified.
• CPU and unknown backends fail closed on both the route and the fallback, so there is no CPU fallback.

Deployment notes

• rocm dash on Linux/WSL now probes 127.0.0.1:13305 by default (dashboard.daemon.enable_lemonade defaults to true), including for existing configs.
• The router load passes --save-options, which persists to Lemonade's own config. This predates the PR.

What this covered

Read: the whole change, 1c33248c…1ae0844d, split across two independent workers:

• Lemonade engine group: engines/lemonade, rocm-core config, the changed main.rs hunks with their functions and callers, and the serving/runtime e2e files.
• Dash telemetry group: collectors, dash-core, daemon, TUI, dash.rs, and the dash e2e files.
• Read directly here: README, the docs/ changes, the feature files, expectations.toml, a leak scan and the CI rollup.

Verdicts on the added tests:

• Would they fail if the production change were reverted? Nearly all would, because they exercise new API or new fields. The exceptions:
  • restart_requires_approval_then_spawns_job pins behaviour that already existed.
  • old_recordings_with_a_retired_lemond_version_key_still_load passes either way, as its comment says.
  • dash_help_scopes_lemonade_discovery_to_linux_and_wsl pins wording only, and cites dash-39 for the behaviour.
• Branches that can break while the tests stay green:
  • The log truncation and the notice-before-ready case above.
  • A wait_ready failure in the direct-server step tests, because the fake always returns Ok.
  • Removing managed_lemonade_collectors.retain(...).
  • The IPv6 skip paths.
• Mutations actually run:
  • Router fallback is_linux forced true: caught.
  • Unique-attribution filter removed: caught.
  • cached_collector_reusable guard removed: caught.
  • Approval re-check removed: caught.

Runs:

• rocm-engine-lemonade --lib: 198 passed.
• telemetry_contract, Lemonade tests: 20/20.
• Dash crate suites: green.
• Clippy on the touched crates and the e2e target: clean.
• CI at this head: every check passes; skillscope is skipped.

What did not run, or was only partly done:

• The cucumber e2e suite and GPU scenarios were not run.
• A Windows-target check could not build (ring needs mingw), so the Windows code paths were reviewed by reading only.
• main.rs (21k lines) was reviewed by hunk plus the surrounding functions and callers.
• Not read beyond their changed hunks: the unchanged parts of runtime_dir.rs, backend_alignment.rs, runner.rs (outside lines 330-1140) and the older sections of telemetry_contract.rs.
• The prior-changes pass did not run (I did not read review comments on earlier PRs).
• The history pass was light: recent log only, plus a check against #683's ASCII-output rule, which exempts the TUI.
• No prompt-injection content was found.

The PR discussion was not reconciled here.

Round 13 at 1ae0844: the gating objection (registered wildcard-variant catalog entry failing on Linux) no longer holds, verified by tests and a revert-mutation; the remaining items are non-gating. See #631 (review)