Repository navigation
Conversation
90bf93d to
1c2600b
Compare
|
Rebased onto current What the rebase droppedThe two That also retires the "Depends on #342" note at the top of the description — #342 has merged. What the rebase preservedFour conflicts, each resolved by keeping both sides rather than taking one:
Three things needed updating because
The dash clock was a second consumer#380 added a Rather than keep the feature alive for it, the same technique this PR already uses for Lemonade is applied to dash. The principle: what must not diverge between the tested and the shipped binary is the compile. That is not the same as "no test affordances" — a path- or state-based affordance is present identically in every build and inert until something plants the state, whereas a So The existence check there is load-bearing rather than an optimisation: that field selects wall time only on Why serve-18 was failingNot infrastructure — a real defect in this PR. All three red lanes reported the same single unexpected failure, and the logs showed the planted runtime being ignored while a real multi-gigabyte backend install ran and succeeded, so Before With the install skipped, The planting location was already correct: this scenario does not opt into the shared runtime tree, so the CLI resolves no env root and Status of the three previously-red lanes
Also green on this head: Verified locally
Not verified locally: serve-18 itself — this host has no usable AMD GPU, which is precisely why the scenario is now |
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
I ran a code review pass on this in addition to my own read of the diff, and it surfaced two things worth looking at before merge.
The main one: dash_test_clock_offset_path in apps/rocm/src/dash.rs now checks for a file on every rocm dash launch, in every build, not just E2E builds. If that file (telemetry/test-clock-offset) ever ends up in a real user's data directory (synced or copied from a dev or CI machine, some future bug that touches that path), the daemon silently drops onto a frozen logical clock instead of wall time. There's no log line when this happens, only a doc comment saying it should never occur outside the E2E harness. A one line warn log when the file is found and used would make this fail loudly instead of silently, without touching the test seam design itself.
The second one is minor, and actually already flagged by the author in a comment: the clock file name and its data/telemetry subpath are duplicated as literals between dash.rs and the E2E's dash_steps.rs, kept in sync only by that comment. Not asking to block on this one, just noting it since the review turned it up.
CI is otherwise clean aside from the Strix Halo Windows and Ubuntu self hosted lanes, which I checked are flaky on main independent of this PR (3 of the last 4 completed main runs failed or were cancelled on those two lanes), so that's not a signal against this change.
| /// `TestClockDirective::FreeRunning(0)` logical clock. Returning the path | ||
| /// unconditionally would therefore take production off `Utc::now()`. | ||
| fn dash_test_clock_offset_path(paths: &AppPaths) -> Option<PathBuf> { | ||
| let path = paths.telemetry_state_dir().join(DASH_TEST_CLOCK_FILE); |
There was a problem hiding this comment.
Every build now takes this path, not just E2E builds. If telemetry/test-clock-offset ever exists in a real user's data dir, the dashboard silently freezes its telemetry clock instead of using wall time, with nothing logged. Worth a warn log when this file is found and used, so a stray file fails loudly instead of silently skewing displayed timestamps.
There was a problem hiding this comment.
Agreed, and fixed in 7841012 — this is the inherent cost of the state-based seam and it should not be paid silently.
dash_test_clock_offset_path now logs at WARN when it finds the file, naming the path:
WARN rocm::dash: dashboard telemetry clock is in TEST mode, not wall time: displayed
timestamps come from a logical clock read from this file. Only the E2E harness creates
it; delete it to restore wall-clock timestamps.
test_clock_file=/…/telemetry/test-clock-offset
Why tracing and not stderr. logging.rs makes the client subscriber file-only on purpose — the TUI owns the raw-mode terminal and a stray stdout/stderr write corrupts the display. The only production caller of this function is maybe_spawn_embedded_daemon on the rocm dash path, so stderr is exactly the wrong channel here; a line printed there would also land on the PTY grid that the dash-* scenarios parse and assert against. The WARN lands in ~/.rocm/logs/rocm-cli.log.<date>, which is where you would look when investigating "my timestamps are wrong" — and it survives the alt-screen switch, which a pre-launch eprintln would not.
Verified live, not just by unit test. Planting telemetry/test-clock-offset under a throwaway ROCM_CLI_DATA_DIR and launching the release rocm dash under a pty produced exactly the record above in the client log; the same launch with no file planted produced an empty log. logging::init runs at the top of run(), before subcommand dispatch, so the subscriber is installed before this callsite is reached, and DEFAULT_FILTER (warn,rocm=info,…) admits the rocm::dash target.
Tests. Two, using a thread-local capturing subscriber (tracing::subscriber::with_default, so a concurrently-running test cannot see or pollute the records): one asserts the WARN fires and names the file when it is present, one asserts nothing at all is logged when it is absent — a warning on every launch would be noise and would drain the signal out of the first.
Both were mutation-probed rather than just observed green: deleting the tracing::warn! turns the first test red (expected a WARN record, got: ), and moving the warn above the existence check so it fires unconditionally turns the second red. So neither passes vacuously.
The existence check itself is unchanged — cycle_timestamp picks wall time only on None, so returning Some for an absent file would put production on FreeRunning(0). Only the logging is new.
| /// `<ROCM_CLI_DATA_DIR>/telemetry/test-clock-offset` and falls back to wall time | ||
| /// when it is absent, so a rename on either side silently drops the whole | ||
| /// mechanism rather than failing loudly. | ||
| const DASH_CLOCK_OFFSET_FILE: &str = "test-clock-offset"; |
There was a problem hiding this comment.
This risk is already spelled out in the comment above (rename on either side silently breaks the mechanism). Not blocking, just flagging that the fix (a shared constant, or a debug_assert tying the two together) is still open.
There was a problem hiding this comment.
Closed this one too, with the debug_assert-style option rather than the shared constant — reasoning, since you offered both:
A shared constant is not reachable here. apps/rocm is a binary crate, so the suite cannot import DASH_TEST_CLOCK_FILE at all; making it importable means hosting it in a library and adding a first-party edge from e2e-cucumber to that library. #422 landed xtask check-crate-edges on main while this branch sat, which deliberately makes every such edge a reviewed decision — spending one on a single string, to couple the black-box suite to a production crate it otherwise doesn't link, seemed like the wrong trade.
So instead dash::tests::e2e_harness_plants_the_file_rocm_dash_reads in apps/rocm/src/dash.rs reads the step file's source with include_str! and pins the literals to what rocm dash actually resolves. That is the same cross-tree reach the suite already makes (installer_fixture.rs does it for install.ps1), it adds no dependency, and it runs in the every-PR unit lane rather than only where the dash scenarios run.
It checks two things:
- the harness's
DASH_CLOCK_OFFSET_FILEdeclaration parsed out of the source, compared againstDASH_TEST_CLOCK_FILE— parsed, not substring-matched, so an unrelated constant containing the same text can't satisfy it; - every directory between the data root and the file, derived from
AppPaths::telemetry_state_dir()and required to appear in order insidedash_clock_path's own body. Anchoring on the builder rather than the whole file matters: a stray.join("telemetry")elsewhere in those ~1.2k lines would otherwise satisfy it. Taking the whole relative path rather than its last component matters too —telemetry_state_dir()being moved (say todata/dash/telemetry) leaves the leaf name intact, and an earlier version of this test passed under that mutation while the mechanism was broken.
Mutation-probed in all four directions: renaming the constant on the production side, renaming it on the harness side, relocating telemetry_state_dir(), and moving the .join out of the builder while leaving a decoy elsewhere in the file — each fails with a message naming the mismatch.
It does impose two constraints on that file, both written into the comment above the constant: keep the declaration on one line, and keep dash_clock_path building the path from .join(..) links. If either is broken the guard fails loudly with an explanatory message rather than passing silently, which is the failure mode I wanted.
One related thing the rebase surfaced, since it is the same gate: this PR's own e2e-cucumber -> rocm-deps edge (the pinned Lemonade version behind the planted runtime's env_id) was not in the allowlist, so check-crate-edges failed on the rebased branch even though the rebase itself was conflict-free. That entry is added in 7841012.
1c2600b to
7841012
Compare
|
Thanks — both findings were right, and the first one is the real cost of choosing a state-based seam over a compile-time one, so it shouldn't be paid silently. Pushed as 7841012, on top of a rebase onto current The warn log. The duplicated literals. Took the Rebase. Onto Ran locally on this head: Also closed out one loose end from the previous description: the three Leaving both threads open for you to close. |
CI status correction — two things changed after this review was writtenFlagging this because the review reasonably discounted the self-hosted lanes, and both facts behind that have since moved. Neither was wrong at the time. 1. 2. That lane has since gone green on
The first completed result landed about three hours after this review. So the flakiness read was sound at the time and simply predates the evidence. The Ubuntu lane is a different story and the original read still holds there: it fails at GPU preflight under contention, is red on What that means for this PR
This was invisible until now because the Windows lane was cancelled on the previous head, so this PR has never had Windows data for the planted-runtime rework. Working hypothesis, not yet confirmed: #404 changed Not proposing to retag or weaken the scenario while the cause is unknown. |
Follow-up: my #404 hypothesis was wrong, and the real cause is a pre-existing Windows defectCorrecting the guess I posted above before it misleads anyone: #404 is not involved. It touched The real mechanism
After the spawn the parent waits up to 45s for HTTP readiness, records the status, and returns This is a real user-visible defect on Why this PR surfaced itThe deleted The planted runtime deliberately moves the failure to the fake's Why there is no test-side fixBoth seam-side options are closed, checked rather than assumed:
What I am not doingI am not retagging or weakening serve-18 while the cause is understood but unfixed, and I am not bolting a production Windows fix onto a test-infrastructure PR — that change deserves its own PR, its own tests, and a maintainer with a Windows host. Flagging it here for a maintainer decision rather than picking one unilaterally. Happy to open a separate issue for the Observed vs inferred, stated plainly: every file/line and git fact above is observed. That the fake's |
|
🔴 Automated review · pr-review-watcher · 7841012 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. SummaryDeletes the e2e-test-hooks cargo feature of the rocm crate and both seams it gated, replacing them with state-based affordances: the Lemonade recovery scenario now drives a planted fake runtime whose 🚫 Blocking (must fix before merge)None. Non-blocking
|
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 7841012
This is a formal review recording our position on the record. It is deliberately non-gating: this automation files no approval, so no approving review will appear here whatever the outcome, and the merge decision stays with a human reviewer.
Blocking: 0 · Non-blocking: 6. This round reviewed removal of a compile-time test seam in favour of state-based affordances, with the new cross-file guard shown to be non-vacuous by two mutation experiments run in a scratch copy.
Check conclusions this review worked from, read at this exact head immediately before publishing: 26 success, 1 skipped, 2 failure.
The full findings, including the non-blocking items, are in the review comment posted alongside this one.
Splitting this up — it has three blockers welded togetherRebasing this onto current What the rebase foundThe rebase itself was nearly clean — one additive conflict in #421 landed two new consumers of the feature this PR deletes, in the newest commit on
That is the same thing that happened with #380's dash clock while this branch sat. Two consumers added in the time this has been open — the removal is chasing a target that keeps moving, which is itself a signal. The three blockers
Bundled, they all inherit the worst one. What I have done
What happens to this oneParked as draft, not closed — the planted-runtime work is sound and I would rather re-land it than rewrite it. It needs #467 fixed first, because the scenario legitimately asserts behaviour Windows does not currently implement, and the Windows lane is now a required check. The feature removal should come last, as a small mechanical PR converting the remaining consumers in one go, so it lands before I have not pushed the rebase here — it does not compile for the reason above, and a broken head would be worse than a stale one. It is kept locally if it is useful later. Nothing here needs a decision from a reviewer right now; #466 is the piece that is ready. |
`@id:serve-lemonade-preparation-recovery` (serve-18) needs Lemonade's llama.cpp backend install to fail twice, so it can pin the retry count and the recovery command. Two `#[cfg(feature = "e2e-test-hooks")]` branches provided that: one in the Lemonade engine's Install RPC that failed on `ROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILURE`, and one in `rocm serve` that waived its no-GPU pre-flight for the same variable. Both are deleted. The `e2e-test-hooks` feature stays: it has other consumers (the simulated host root among them), and removing it is the last step of #349, not this one. The failure now happens where a broken install fails, at a non-zero child process: - `fake-lemonade`, a `[[bin]]` of the suite, is planted as the runtime's `lemonade` and `lemond`. `lemonade backends` reports `llamacpp:rocm` as installable and `lemonade backends install` exits non-zero; `lemond` stays alive until killed. - The install manifest is written beside it. Its `env_id` is derived from `rocm_deps::LEMONADE_VERSION` (a dev-dependency, so no crate-edge entry): `serve` skips its own install only when `Detect` reports exactly that id, and a placeholder silently runs the real multi-gigabyte install instead. - The pre-flight runs unchanged against one KFD GPU node planted under the simulated host root. The root holds nothing else, so the scenario needs no GPU and a GPU runner's devices do not take part. `XDG_RUNTIME_DIR` points inside the scenario too, so the fake server writes nothing outside it. The scenario is now `@requires-os:linux` instead of `@requires-no-gpu`. It still runs on the GPU-less every-PR lane, and now on the Linux GPU lanes and WSL2 as well. It stays off Windows: the host root re-roots Linux paths only, and there `rocm serve` does not notice a managed engine dying at startup (#467), so the failure would never reach the user. A test in `expectation.rs` pins that gate against the real tag line. The engine's unit test now reads the backends table out of the fixture's source with `include_str!`, instead of a copy kept in step by a comment. Workflow and contract-test comments that cited the deleted seam as their example now cite the host root. Checked by mutation: the scenario fails when the fake install succeeds, and fails with its named pre-flight message when the GPU is not planted. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
7841012 to
e97c827
Compare
|
Head is now What changed in scope.
What carried over. Compared with the old diff over the kept files:
One design change from the old version: no GPU needed. Previously, removing the pre-flight bypass forced serve-18 onto
Verified locally on a GPU-less host. Mutations:
siloteemu's non-blocking items (automated review on
Why still a draft. It no longer has to come last: it removes no feature, and the scenario does not need #437 to pass. I am keeping it a draft as asked until #437 (the fix for #467) lands. Once that happens it can come out of draft as-is, and extending serve-18 to Windows would be a follow-up. Gate on this head:
|
rominf
changed the title
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · e97c827
Review — needs work
Full review of the whole change (12 files, 1c33248c318c72a9e837627ff71476c8583f1930...e97c827c0b69d684f64524794ed3d94116e86ae0), fanned out across three coupled-file-group workers plus centralized cross-cutting checks. Part of #349 (open tracking issue), explicitly re-scoped to convert one consumer of the e2e-test-hooks seam rather than remove the feature — the description says so and the diff matches that scope exactly. PR title cites ROCMAI-669; that identifier is an internal tracker reference not retrieved for this review.
Blocking
- CI check "E2E tests (MI300X, current)" is red at the reviewed head commit — CI status for
e97c827c0b69d684f64524794ed3d94116e86ae0(not a source location; run: https://github.com/ROCm/rocm-cli/actions/runs/40315449640/job/121546925225).
gh pr view 351 --json statusCheckRollup,headRefOidconfirmsheadRefOidmatches the reviewed commit and this check's conclusion isFAILURE, while every other GPU/OS E2E lane (Strix Halo ×4, W7900, Windows, WSL2) passed on the same run. The job log itself returned 403 in this environment, so the root cause could not be read directly — but the mechanism this PR introduces is a plausible direct cause, not just an unrelated flake: the scenario's gate changed from@requires-no-gputo@requires-os:linux, which is exactly what newly exposesserve-18to a real-GPU lane (MI300X) for the first time. Previously this scenario never ran there at all. Worth ruling out before assuming it's infra noise, since (unlike the Strix Halo/Windows self-hosted flakiness a prior reviewer already characterized as pre-existing) this specific lane has no established flake history against this scenario — it's new exposure.
Confidence 100 · architectural · Fix: pull the MI300X job log and confirm whether the planted fake runtime / simulated host-root GPU node interacts badly with a lane that also has a real AMD GPU and real Lemonade state, before merging.
Non-blocking
- Stale "scripted failure seams" wording in an assert message, contradicting the doc comment immediately above it —
xtask/src/workflow_contract.rs:296(theassert!message insideassert_prebuilt_e2e_lanes_enable_test_hooks), vs. the doc comment atxtask/src/workflow_contract.rs:273-274.
This PR correctly rewrote the function's doc comment from "the scripted Lemonade backend-install failure" to "the simulated host root,ROCM_CLI_TEST_HOST_ROOT" (the compiled-in failure branch is exactly what this PR removes). Theassert!panic message below it in the same function was not updated and still reads "...without it the suite's scripted failure seams are compiled out:\n{block}", describing the now-deleted mechanism. A future reader hitting this assertion failure gets pointed at machinery that no longer exists.
Confidence 85 · mechanical · Fix: update the assert message to match the revised doc comment (e.g. "...without it the suite's test seams, such as the simulated host root, are compiled out").
Decisions for the author
fake-lemonade.rs'srun_lemonade_clisilently succeeds on any subcommand it doesn't recognize — tradeoff. This keeps the fixture small and avoids breaking the scenario if the engine issues some other incidental call, but it also means a future, unexpected invocation shape from the real engine (a new flag, a new subcommand) would pass through unnoticed rather than failing loudly, which could mask a drift between the fixture and the reallemonadeCLI contract it stands in for. The alternative (fail loudly on anything unrecognized) was deliberately not chosen here (the code comments explain the three calls that matter and say "any other subcommand: succeed quietly"); confirm that's still the intended tradeoff as the fixture's surface grows.
Positive signals
- The new
backend_alignment.rsunit test pullsfake-lemonade.rs'sBACKENDS_TABLEviainclude_str!rather than a hand-copied literal, so the fixture and the parser it's meant to drive cannot silently drift apart — verified the extraction is byte-exact and the parser assertions are correct against it. - The planted runtime's
env_id(format!("lemonade-embeddable-{}", rocm_deps::LEMONADE_VERSION)in the test fixture) is verified byte-identical to the format string the real installer uses inengines/lemonade/src/install.rs:201, and the match againstDetectwas traced through to confirm the scenario actually skips the real multi-gigabyte install rather than silently falling through to it — exactly the property the PR's description claims. - Every cross-referenced issue in the description (#467, #437, #729, #510, #466) checks out factually against live GitHub state — titles, open/merged status, and the relationships claimed between them are all accurate.
Deployment notes
Both removed code paths (engines/lemonade/src/lib.rs's scripted install failure, apps/rocm/src/serve_cmd.rs's pre-flight waiver) were compiled only under the e2e-test-hooks feature, so this change has no behavioral effect on release/production builds.
What this covered
Every file in the diff, read completely at the PR head plus surrounding code (the full serve() pre-flight, the Lemonade install/detect flow, the E2eWorld harness, the tag-resolution machinery in expectation.rs), via three independent fanned-out workers grouped by contract coupling (engine/fixture contract; harness-and-steps; feature-tag-and-CI-gating), each of which activated the review-code skill itself and verified its own candidates before reporting. Centralized checks on top: live CI status for the exact head commit, a repo-wide grep confirming no dead references remain to the removed env var/feature seam, independent tracing of the env_id format-string parity between the fixture and the real installer, verification of the AGENTS.md crate-edge and GitHub-Actions-pinning conventions against this diff, and DCO/commit-quality checks (single well-described commit, signed off).
Did not run: a deep git log/blame history pass beyond confirming this is a single commit with no revision churn to review; a prior-PR-review-comments pass on these files (not retrievable at reasonable cost here); local build or test execution (no Rust toolchain available in this environment) — relied on live CI results instead, per the skill's guidance to take CI at face value rather than re-deriving its coverage.
Not reconciled against any PR discussion or review threads — those were deliberately excluded as input to this review's findings.
There was a problem hiding this comment.
No blocking findings.
Non-blocking
- MI300X lane red is unrelated to this PR.
E2E tests (MI300X, current)(job 121546925225) fails only ontherock-next-09(a vLLMuv pip install --reinstallstep), which this diff does not touch. serve-18 passed on that same lane. The identical single unexpected failure appears onmain(runs 40360152181, 40357677968, 40354126258) and on other branches in the same window (e.g. jobs 121795242877, 121882265448, 121704235820). It should not gate this PR. - The fake
lemondnever exits (tests/e2e-cucumber/src/bin/fake-lemonade.rs,run_lemond). The comment saysrocm serveterminates it when the serve fails, but the description says #729 tracks alemondleft running when the backend install fails during serve, which is this scenario's path. Until #729 lands, every Linux lane run can leave a sleeping process behind, and the scenario now runs on every Linux lane. Consider bounding the fake's lifetime, and aligning the comment with what serve does today. - The planted
env_idrepeats the format stringmanaged_engine_runtime_iduses (plant_failing_lemonade_runtimeintests/e2e-cucumber/tests/e2e.rs;apps/rocm/src/engines_cmd.rs:323). Deriving the version fromrocm_depscovers a version bump but not a change to thelemonade-embeddable-prefix. If Detect stopped matching, serve would start the real multi-gigabyte install instead of failing fast. An assertion that the install attempted was the fake's (it already prints a distinctive stderr line) would make that fail quickly. tests/e2e-cucumber/README.md(around lines 244-251) lists the fixture binaries (fake-uv,fake-python,fake-tailscale) and does not mentionfake-lemonade.
Decisions for the author
- The engine unit test reads the e2e crate's
fake-lemonade.rswithinclude_str!and extracts the raw-string literal textually. The table is then checked on every unit-test lane with no copy to drift. The cost is that a product crate's unit tests depend on a path intests/and on the literal's exact spelling. Please confirm that is intended. - The e2e crate gains a
rocm-depsdev-dependency, while the README's "Black-box only … No imports from the rocm-cli codebase" principle stays as written.rocm-coreis already such an edge. Either confirm this extension or adjust the README line. - serve-18 now runs on every Linux lane (GPU and GPU-less) and WSL2, where it previously ran only on the no-GPU lane. The new gate is pinned in
expectation.rs. This is more cadence, and each run starts the fake server noted above.
Good things
- The tag gate is pinned against the real feature-file tag line, so an edit fails a test.
- The no-GPU pre-flight is no longer waived; it runs against a GPU planted under the simulated host root.
- Both
e2e-test-hooksbranches are removed cleanly, and no references toROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILUREremain.
Part of #349.
Problem
@id:serve-lemonade-preparation-recovery(serve-18) needs Lemonade's llama.cpp backend install to fail twice, so it can pin the retry count and the recovery command. It got that failure from two#[cfg(feature = "e2e-test-hooks")]branches in product code:engines/lemonade/src/lib.rsfailed the Install RPC whenROCM_E2E_LEMONADE_BACKEND_INSTALL_FAILUREwas set;apps/rocm/src/serve_cmd.rswaivedrocm serve's no-GPU pre-flight for the same variable.So the scenario tested a code path that exists only in test builds.
Change
Both branches are deleted. The failure now happens where a broken install fails, at a non-zero child process.
The runtime.
tests/e2e-cucumber/src/bin/fake-lemonade.rsis a[[bin]]of the suite, planted as the runtime'slemonadeandlemond:lemonade backendsreportsllamacpp:rocmas installable;lemonade backends installexits non-zero;lemondstays alive until killed.The install manifest is written beside it. Its
env_idis derived fromrocm_deps::LEMONADE_VERSION(a dev-dependency, so no crate-edge entry).serveskips its own install only whenDetectreports exactly that id; a placeholder silently runs the real multi-gigabyte install instead.The GPU. The pre-flight runs unchanged, against one KFD GPU node planted under the simulated host root (
ROCM_CLI_TEST_HOST_ROOT). The root holds nothing else, so the scenario needs no GPU, and a GPU runner's own devices do not take part.XDG_RUNTIME_DIRalso points inside the scenario, so the fake server writes nothing outside it.The gate.
@requires-no-gpu→@requires-os:linux.rocm servedoes not notice a managed engine dying at startup (serve: on Windows a managed engine that dies at startup is reported as success #467, fix in fix(serve): fail the Windows managed launch when the engine dies at startup (ROCMAI-675) #437), so the failure would never reach the user.expectation.rspins this gate against the real tag line.The engine's unit test reads the fixture's backends table with
include_str!, instead of a copy kept in step by a comment.Workflow and contract-test comments that cited the deleted seam as their example now cite the host root.
Why this doesn't weaken verification
resolve_runtimeaccepts the planted manifest because it only checks that the manifest parses and thatlemondis a file. No checksum, version or signature is on this path; the sha-pinned archive download lives inprepare_embeddable, whichservedoes not reach onceDetectmatches. The no-GPU pre-flight is no longer waived. It now passes for the same reason it passes on a real GPU host.Test plan
serve-18passes locally viacargo xtask e2eon a GPU-less Linux (WSL2) host.installed: the engine unit test fails;@requires-no-gpuor removed: the tag pin fails.fmt;cargo check -D warnings;rocm-engine-lemonade --all-targets,rocm --bin rocm,e2e-cucumber --lib+feature_naming,check --test e2e;xtasktests,xtask manifest --check,check-crate-edges.Related
None), so no planted GPU would be needed.lemondleft running when the backend install fails during serve (found in review here).