[bong-water-water-bong]

Closes #1049.

Root cause. engine_of (os/v3/puppy/console-tui/src/facts.rs) resolved the engine: <recipe> (<backend>) row from loaded.first(), where loaded is every entry of /api/v1/health's all_models_loaded[] in array order. Lemonade keeps one model per type in that array - its /v1/health docs show an embedding entry beside the llm under max_models: {llm:1, embedding:1, reranking:1, …} - and names the answering one separately in model_loaded ("model name of the most recently accessed model"). So an embedding model loaded first made the row describe the embedding, which is the opposite of what the row is for.

Measured before/after (same catalogue + health body as the docs, nomic-embed on llamacpp/cpu loaded first, qwen3-0.6b-1bit on flm/npu loaded second, model_loaded: qwen3-0.6b-1bit), on main 1cd4e4bc:

	engine_of answer
before (old body, new test - the negative control)	Some("llamacpp (cpu)") - the embedding
after	Some("flm (npu)") - what is answering

The change is one resolution order: model_loaded when the catalogue knows it, else the first loaded entry the catalogue knows (which also means a stale model_loaded no longer blanks the row), else nothing as before. Signature gains the serving argument; the one call site parses model_loaded from the same health body it already reads. 6 existing assertions updated for the new argument, one new test the_engine_is_the_model_answering_not_the_first_loaded covering both the embedding-beside-llm case and the stale-model_loaded case.

Verification (all on this branch, built and tested through ~/lb/thermal-run.sh):

• cargo test --offline 52/52 (was 51), cargo fmt --check and cargo clippy --all-targets clean.
• Negative control: the new test fails on the old engine_of body with exactly the measured pair above, passes after.
• build-static.sh -> fd8e80d9e32cb993, 1,581,392 B, still static (ldd = "statically linked", no PT_INTERP).
• --snapshot 80x25/240x67 --fixture are byte-identical to the committed tests/snapshots/*.txt, so this PR does not move the frame at all (the fixtures load one model; os/v3/puppy: the console names the engine that is serving, and stops printing a URL before it has one #1041's engine row is unchanged) - no baseline churn for the next cut.
• No gate file, no vm-check.sh/puppy-check.sh string touched; tools/copyright.py --check clean; no new files.

Found by pr_agent on #1041.

[bong-water-water-bong]

#1050 ready for review at 1301f42f, closes #1049.

For the reviewer, the two things worth a look: (1) the resolution order in engine_of - model_loaded, then the first loaded entry the catalogue knows, then None; a stale model_loaded deliberately falls through rather than blanking the row, and the new test covers both; (2) this PR does not move the frame - --snapshot 80x25/240x67 --fixture are byte-identical to tests/snapshots/*.txt, so the #1041 engine row and its gate-checked position (CONNECT block on lines 2-7, vm-check.sh U2 menu connect) are untouched.

The negative control is in the body: the new test run against the old engine_of body fails with llamacpp (cpu) where the box answers flm (npu). Artifact fd8e80d9e32cb993, 1,581,392 B, static (the size matches #1041's because only the resolution order changed).

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 4f061e9)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1049 - Partially compliant Compliant requirements: The console's LEMONADE engine: row now picks the model that is currently serving based on model_loaded The fix correctly prefers model_loaded, then falls back to the first loaded entry the catalogue knows The PR includes new tests that verify the correct behavior with both embedding and LLM models loaded Non-compliant requirements: The PR description mentions measuring on hardware but does not provide specific measurements or benchmarks Requires further human verification: The specific hardware measurements and performance benchmarks should be verified on actual AMD Strix Halo's XDNA 2 NPU and Radeon iGPU hardware

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Engine resolution logic change The change modifies the engine_of function to prefer model_loaded over the first loaded model, which fixes the issue where the console would display the wrong engine when an embedding model was loaded before the chat model. This is a correctness fix for the engine identification logic. pub fn engine_of( loaded: &[String], serving: Option<&str>, engines: &[Engine], devices: &[(String, String)], ) -> Option<String> { // Lemonade loads one model per type (`max_models`: llm, embedding, …), so `loaded` is load order and its first // entry can be an embedding while the llm is the one answering: `model_loaded` is the box's "last used". let e = serving .and_then(|n| engines.iter().find(|e| e.model == n)) .or_else(|| { loaded .iter() .find_map(|n| engines.iter().find(|e| &e.model == n)) })?; let device = devices .iter() .find(|(n, _)| n == &e.model) .map(|(_, d)| d.as_str()); Some(match device.or(e.backend.as_deref()) { Some(backend) => format!("{} ({backend})", e.recipe), None => e.recipe.clone(), Test coverage for new logic The new test the_engine_is_the_model_answering_not_the_first_loaded covers the specific scenario described in the ticket where an embedding model is loaded first but the LLM model is the one answering. This ensures the fix works as intended. #[test] fn the_engine_is_the_model_answering_not_the_first_loaded() { // Lemonade's own /v1/health shape: `max_models` allows one model per type, `all_models_loaded[]` carries // the embedding beside the llm in load order, and `model_loaded` names the one last accessed. let models = r#"{"data":[ {"id":"nomic-embed","recipe":"llamacpp","downloaded":true,"recipe_options":{"llamacpp_backend":"cpu"}}, {"id":"qwen3-0.6b-1bit","recipe":"flm","downloaded":true,"recipe_options":{"onebit_backend":"npu"}} ],"object":"list"}"#; let m = models_from(models); let loaded = vec!["nomic-embed".to_string(), "qwen3-0.6b-1bit".to_string()]; let health = r#"{"model_loaded":"qwen3-0.6b-1bit","all_models_loaded":[ {"model_name":"nomic-embed","type":"embedding","device":"cpu"}, {"model_name":"qwen3-0.6b-1bit","type":"llm","device":"npu"} ],"status":"ok"}"#; let devices = health_devices(health); // load order says embedding; the box says the NPU chat model is the one answering assert_eq!( engine_of( &loaded, json_str(health, "model_loaded").as_deref(), &m.engines, &devices ) .as_deref(), Some("flm (npu)") ); // a `model_loaded` the catalogue no longer knows must not blank the row: the loaded list still resolves let stale = r#"{"model_loaded":"gone","all_models_loaded":[]}"#; assert_eq!( engine_of( &loaded, json_str(stale, "model_loaded").as_deref(), &m.engines, &devices ) .as_deref(), Some("llamacpp (cpu)") ); }

[bong-water-water-bong]

VERDICT: READY at 1301f42f — the bug is real, the fix is the right resolution order, and the negative control reproduces exactly as described.

Ran it here (cargo test --offline, thermal-run):

test result: ok. 52 passed; 0 failed; 0 ignored          (51 before, +the new case)
cargo fmt --check                     rc 0
cargo clippy --offline --all-targets  no warnings, no errors

Negative control, reproduced independently. I put the old engine_of body back inside my own review worktree (keeping the new serving parameter, unused) and ran only the new test:

thread 'facts::tests::the_engine_is_the_model_answering_not_the_first_loaded' panicked:
assertion `left == right` failed
  left: Some("llamacpp (cpu)")     <- the embedding, loaded first
 right: Some("flm (npu)")          <- what model_loaded names
test result: FAILED. 0 passed; 1 failed; 51 filtered out      (exit 101)

So the test fails on the old body with precisely the pair in the body, and passes at this head. Restored afterwards; git status clean.

The frame does not move, checked rather than assumed:

$ 1bit-console-tui --snapshot 80x25  --fixture | diff -q - tests/snapshots/80x25.txt   -> identical
$ 1bit-console-tui --snapshot 240x67 --fixture | diff -q - tests/snapshots/240x67.txt  -> identical
  LEMONADE up 8882 engine: llamacpp (gpu) | GPU HRX0 43C | NPU idle | MODELS 2

One detail worth calling out as done right: the device lookup moved to the resolved model (n == &e.model) rather than staying on the old name. Without that, engine_of would have picked the NPU model and then reported its device from the embedding's entry — the test would have caught it, but it is the half of the change that is easy to miss.

Residual, not blocking: when model_loaded is absent, null, stale or names a model the catalogue does not know, the fallback is still "first catalogue-known loaded entry", which in a two-type box can be the embedding again. all_models_loaded[] carries type (llm / embedding), so preferring llm there would close the last case — worth a line in the comment if you touch this again, not worth another push tonight.

[github-actions]

Persistent review updated to latest commit 4f061e9

[bong-water-water-bong]

VERDICT: READY at 4f061e97 (rebase-only; my review stands).

4f061e97 is a rebase of the same change onto a main that has since taken #1045, #1046 and #1047, so a raw diff between the two heads shows those PRs, not this one. The PR's own lines are unchanged:

head 1301f42f  own-diff hash  311fb6b6835aaed812a2abfae2cc2d3845c8ab909592a2c00daa56e3235ee2c3
head 4f061e97  own-diff hash  311fb6b6835aaed812a2abfae2cc2d3845c8ab909592a2c00daa56e3235ee2c3

Everything in my previous verdict therefore still holds at this head: cargo test --offline 52/52, the negative control failing on the old engine_of body with Some("llamacpp (cpu)") vs Some("flm (npu)"), both snapshots byte-identical, fmt/clippy clean.

I re-ran the suite at this head rather than assuming the rebase was inert:

$ cargo test --offline        -> 52 passed; 0 failed

[bong-water-water-bong]

Stronger selector added at eff0bb4f (on top of the READY 4f061e97, same issue, same file - only the model-selection rule changed again).

Reviewing Lemonade's schema further showed my first rule was still a proxy: model_loaded is documented as "the most recently accessed model", so a request to the embedding model that sits beside the llm moves it, and the row would name the embedding again. The authoritative field is the per-entry type - Lemonade types every all_models_loaded[] entry and keeps max_models.llm = 1, so there is exactly one chat engine to name.

New rule: take the type: "llm" entry, fall back to model_loaded when type is not reported, then the first loaded entry the catalogue knows, then nothing. Implemented as llm_model_of(&body) next to health_devices; engine_of's resolution order is unchanged from what you reviewed.

Negative control for this arm (same test, llm_model_of reduced back to model_loaded alone):

left:  Some("llamacpp (cpu)")   <- model_loaded moved to the embedding
right: Some("flm (npu)")        <- the llm-typed entry

Re-verified on the rebased branch: cargo test 52/52 (three assertions in the one new test: llm beside embedding in load order, embedding accessed last, stale model_loaded), fmt/clippy clean, frames still byte-identical to tests/snapshots/*.txt at 80x25 and 240x67 - still no pixel moves. Artifact is now c22f0176efb88f98, 1,581,392 B, static.

I also measured the shape the box has not shipped yet, since it can change the 80x25 layout: with a hybrid ryzenai-llm (gpu npu) value (Lemonade's own documented hybrid device string, and the longest plausible recipe) the facts block re-wraps within its existing two rows and the log pane keeps its three rows - so no defect and no extra test was needed.

[bong-water-water-bong]

VERDICT: READY at eff0bb4f — the selector change is the right fix and I re-ran everything at this head.

cargo test --offline   ->  52 passed; 0 failed          (the third assertion lives inside the existing test, so the count is unchanged)
--snapshot 80x25  --fixture   byte-identical to tests/snapshots/80x25.txt
--snapshot 240x67 --fixture   byte-identical to tests/snapshots/240x67.txt

Negative control, done the right way round. Reverting the call site to json_str(&body, "model_loaded") does not fail the test — the test drives llm_model_of() directly, so the wiring is not what it exercises. The control that does bite is making llm_model_of() itself ignore the type:

(make llm_model_of skip the "type": "llm" pass)
thread '…the_engine_is_the_model_answering_not_the_first_loaded' panicked:
  left: Some("llamacpp (cpu)")     right: Some("flm (npu)")

So the new assertion genuinely covers the "an embedding request moved model_loaded" case, and the llm-typed entry wins as intended.

One gap worth naming, and it is the same shape as the bug being fixed: nothing tests the call site. Facts::refresh could go back to json_str(&body, "model_loaded") and all 52 tests would still pass, because every assertion starts from llm_model_of(...) rather than from a health body through refresh. A test that drives the facts fetch with the embed_last body (the way the snapshot fixtures already drive the frame) would close it; it is a five-line test and not worth another push on its own.

Two small things I checked while reading llm_model_of(): the checkpoint fallback is a good belt-and-braces for a health entry without model_name, and returning model_loaded when no entry is typed llm keeps the previous behaviour for any Lemonade that does not report types — so an older box degrades to exactly what it did before this PR.