[rominf]

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. — N/A, test-only.
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. — N/A.
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3. — N/A, no message changes.

Summary

The checks that decide whether something rocm downloads or runs is authentic — the download digest gate, the Lemonade archive digest, RSA signature verification, the pinned release key — and the code that decodes untrusted engine output had only example tests. Example tests miss exactly the inputs where such a check fails open: a digest one nibble off, a signature re-encoded with a leading zero, a key that differs in one character. This adds property and exhaustive tests aimed at that near-miss region, plus golden documents for the engine wire format.

What is covered:

Area	Tests	What they pin
Download digest gate (download_file_streaming)	crates/rocm-core/tests/digest_gate_properties.rs	every near-miss kind (one nibble, wrong suffix, truncated, padded, trailing newline, overlong, leading zero, empty, all zeroes) is rejected with the gate's own SHA-256 mismatch error, and the one-nibble, wrong-suffix and truncated kinds are walked over every position rather than sampled; the true digest is accepted in any ASCII case; a one-bit or past-the-first-chunk body change is rejected and leaves no file behind
Lemonade archive digest (verify_sha256)	engines/lemonade/src/archive_cache.rs	only the exact pinned digest is accepted (byte-exact, so an upper-case spelling is rejected too); an empty pin is rejected; each rejection must be the check's own mismatch error; a one-bit flip anywhere in a multi-buffer archive is rejected while the intact archive is accepted
RSA signature verification	crates/rocm-core/tests/integrity_properties.rs	payload bit flips, truncation and extension; signature byte changes, truncation, leading-zero and trailing-zero re-encodings; a signature from another key; corrupted or garbage public keys; PEM mangling (BOM, stray whitespace, blank lines, CR/CRLF) does not change the verdict
Model selection and readiness	crates/rocm-core/tests/model_selector_properties.rs	every advertised selector resolves to exactly its own recipe up to case and surrounding whitespace, and one inserted or deleted character selects nothing; no catalog model — or near miss of one — reads as ready for a different one, apart from the two shapes pinned below; a document naming no matching model never reads as ready; a model is ready under any of its catalog names
Engine protocol	crates/rocm-engine-protocol/tests/protocol_properties.rs	round trips, version skew (decodes exactly when the required fields are present), exact-method envelopes, the loopback allowlist against padded/re-cased/bracketed spellings and other local addresses, engine ids never ./.., --gpu values, golden documents for wire spellings a round trip cannot see; arbitrary and mutated input is smoke-tested for panics
Pinned release key (cargo xtask verify-pinned-keys)	xtask/src/verify_pinned_keys.rs	runs the real check over a scratch repository root and walks every position of the committed key in either installer (deletion, duplication, both base64 neighbours, case flip, swap with the next character); a correct copy elsewhere in the file never rescues a wrong constant; an empty or missing constant against a published key diverges; none of this depends on the key's type or size, so a rotation does not break it

Shared helpers live in crates/rocm-core/tests/common/: a loopback HTTP server that answers any number of requests and cannot hang when no client connects, and the PROPTEST_CASES floor below. Every download and readiness test asserts its request reached that server, so a transport failure cannot pass as a rejection or a "not ready".

docs/testing.md gains a section with the commands.

Two behaviours pinned rather than asserted away. Readiness, which confirms the engine is serving the model the user asked for, is looser than selection in two ways. Neither is changed here; each is pinned by a labelled characterization test, so it stays visible. If readiness or the catalog changes, these tests are expected to fail and should be updated with it.

• Bare family words. Readiness matches a bare family word against any model with a name token starting with it, in both directions. Asking for qwen being satisfied by a served Qwen3-0.6B-GGUF is already exercised by a unit test in endpoint_readiness.rs. The other direction means an engine whose /v1/models reports only qwen reads as ready for other Qwen catalog entries too, so readiness cannot tell models of one family apart when that is all the engine reports. Pinned by a_bare_family_word_satisfies_readiness_for_the_whole_family and its uncatalogued sibling.
• An alias that spells another model's basename. Readiness accepts a model's basename for the model. Two catalog aliases are also another recipe's basename: qwen3-32b (an alias of Qwen/Qwen3-32B-FP8, the basename of Qwen/Qwen3-32B) and qwen3.6-35b-a3b (an alias of unsloth/Qwen3.6-35B-A3B-GGUF:Q4_K_M, the basename of Qwen/Qwen3.6-35B-A3B). An engine launched under such an alias reads as ready for the other model. an_alias_spelling_another_models_basename_reads_as_ready_for_it derives the collisions from the catalog and compares them with these two, so a catalog change that adds one fails there, rather than being silently skipped by the pairwise property.

Whether either is wanted is a question for maintainers: fixing the second means readiness stops accepting a basename that is another recipe's alias, or the catalog drops those aliases.

Can the tests fail?

Each mutation below was applied to the code under test, run against the suite, and reverted.

Mutation	Caught by
digest gate: prefix comparison, or an empty expected digest treated as absent	near_miss_digests_are_rejected
digest gate: last nibble ignored	positional_near_misses_are_rejected_at_every_position (every run), near_miss_digests_are_rejected
digest gate: both partial-file cleanups removed	a_tampered_body_is_rejected, tampering_past_the_first_chunk_is_rejected
Lemonade: starts_with instead of equality; hash only the first 64 KiB; skip an empty pin; last character ignored, including under PROPTEST_CASES=0	the two Lemonade archive-digest tests
signature truncated to 256 bytes before verifying	signature_mutations_are_rejected
readiness: substring instead of exact match; family match without its digit or length guard, by substring, or by equality only; recipe-alias branch removed	listing_a_different_catalog_model_is_never_readiness, readiness_accepts_a_catalog_model_under_any_of_its_names, a_bare_family_word_satisfies_readiness_for_the_whole_family
readiness: expected model ignored (any non-empty listing is ready)	a_models_document_naming_no_matching_model_never_reports_ready, listing_a_different_catalog_model_is_never_readiness, a_bare_family_word_satisfies_readiness_for_the_whole_family
readiness: a reported basename no longer matches its model	an_alias_spelling_another_models_basename_reads_as_ready_for_it, a_models_document_listing_the_expected_model_reports_ready
selection: prefix match, or interior whitespace stripped	a_mistyped_selector_selects_nothing
protocol: deny_unknown_fields on the healthcheck; extra_env serialized when empty; success envelope without data; loopback check trimmed, case-folded or widened to 0.0.0.0; ./.. engine-id guards removed; method, DevicePolicy, tokens_per_sec or port renamed; AUTO case-sensitive; engine-id helper returning nothing; a --gpu list read as its first entry	the matching protocol test for each
pinned key: divergence check disabled; body truncated; +// dropped from the alphabet; comparison case-insensitive or order-insensitive; extraction reading the whole file or from its start; an empty constant counted as a match; dormant guard or missing-source error removed	the pinned-key tests

Known survivors: removing only one of the two partial-file cleanups in the digest gate (the other still runs, and the test checks the end state). Renames of several protocol fields not listed above are not pinned by a golden document. The pinned-key tests' independence from key shape was not exercised against a key of another type.

Determinism, runtime, platforms

• Every property runs a fixed minimum number of cases (8–1024 per property). PROPTEST_CASES raises that minimum for a deeper run but never lowers it, so PROPTEST_CASES=0 cannot pass a property vacuously. The rocm-core and protocol suites use proptest's default random seed and persist a regression file next to the test on failure; the Lemonade tests use a fixed seed.
• Runtime in a debug build: integrity_properties takes roughly 7–15 s, almost all RSA-2048 key generation (two keys, generated once per test process); the readiness suite takes about 1.5–4 s, since its pairwise test makes about 6,200 requests against the 49-recipe catalog (one server per served name, not per request); every other suite takes under 1 s.
• The download and readiness tests serve responses from a loopback listener on an ephemeral port, with a read timeout.

AGENTS.md §3

Test-only: no CLI output, exit code, file layout or runtime selection changes, so no Gherkin scenario is needed.

Verification

• cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings: clean.
• cargo test --workspace --all-targets: everything this PR adds passes. Three tests outside it fail on my host only, because of local tooling: two build_metadata git_watch_paths tests, and one container stop test that the host's Docker-compatible shim breaks. They fail the same way on main.
• prek run --from-ref origin/main --to-ref HEAD: pass, except two hooks whose tools aren't installed here. hawkeye (license headers) did not run; both new files carry the SPDX header. lychee skipped itself. CI runs both.

Not verified: Windows; runtime under nextest's one-process-per-test model, where each integrity test pays its own key generation; cargo xtask coverage, which matters because one test that duplicated the catalog schema's own uniqueness check was removed.

[nowycondro]

🤖 Automated review (Claude Code) · f995307

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 — no blocking findings

Full review of the whole change (10 files, 2 commits, +2688/-1) via four independent blind fan-out passes grouped by coupling (rocm-core digest-gate + RSA integrity; rocm-core model-selector/readiness; rocm-engine-protocol; lemonade archive-digest + xtask pinned-key + docs), plus driver-level direct reading of the same production source as a cross-check. No narrowing applied — this is this PR's first review.

No ticket named. The PR description's claimed scope (property/exhaustive tests for the download digest gate, the Lemonade archive digest, RSA signature verification, model selection/readiness, the engine protocol, and the pinned release key, plus a docs/testing.md section) matches the diff exactly: the five new test files, two Cargo.toml dev-dependency additions, the one-line Cargo.lock update, and the docs section are the entirety of the change. No production code is touched.

Blocking

None.

Non-blocking

None.

Decisions for the author

• The new proptest files hardcode cases: N and never read PROPTEST_CASES — non-blocking-improvement. Three existing modules in this repo (examine_driver_proptests.rs, archive_props.rs, and apps/rocm/src/providers/proptests.rs after a sibling, not-yet-merged commit) expose a helper that lets PROPTEST_CASES raise a property's case count for a deeper local/CI sweep, while clamping it so it can never lower the count below the safe default (a lesson that sibling commit learned the hard way: an unclamped read let PROPTEST_CASES=0 pass every property vacuously). This PR's five new files instead write ProptestConfig { cases: N, ..ProptestConfig::default() } directly, which is not vulnerable to that vacuous-pass bug (the literal field wins over whatever Default would have read from the env), but it also means PROPTEST_CASES has zero effect on any of these new suites — an operator cannot opt into a deeper sweep the way they can elsewhere in the repo. Worth adopting the same floor-respecting helper for consistency, not required for correctness.
• The exhaustive listing_a_different_catalog_model_is_never_readiness test in model_selector_properties.rs opens a real loopback TCP socket (bind + spawned thread + HTTP response) for every pair in the full catalog cross-product plus near-miss names — on today's 14-entry catalog this is already on the order of hundreds of real socket round trips in one test, growing quadratically as the catalog does. Nothing here is incorrect (ephemeral ports avoid collisions, server.join() can't hang because the client always connects), but it is a heavier design than necessary for what is fundamentally a pure-function property; an in-process fake of openai_models_endpoint_has_model's input, or a lighter transport stub, would keep this test's runtime from scaling with the catalog as new models are added.
• The family-word readiness cross-match is pinned, not fixed — tradeoff, already flagged by the author for maintainers. a_bare_family_word_satisfies_readiness_for_the_whole_family documents, in its own doc comment, a known false-positive surface in pre-existing (unchanged by this PR) production matching logic: an engine reporting only a bare family word like qwen reads as ready for any catalog model of that family, not just the one actually loaded. The test explicitly states this is characterization, not endorsement, and that tightening the behavior is a decision for maintainers. Since the behavior itself is untouched by this PR, this is correctly out of blocking scope — flagging only so the tradeoff is visible to whoever merges.

Positive signals

• digest_gate_properties.rs's and integrity_properties.rs's near-miss generators defensively re-assert that each generated "near miss" candidate actually differs from the true digest/signature before using it (e.g. prop_assert!(!candidate.eq_ignore_ascii_case(&truth), ...)) — a guard against the generator itself silently producing a non-near-miss that would make the property pass vacuously.
• a_correct_copy_elsewhere_in_the_file_never_rescues_a_wrong_constant-style coverage in xtask/src/verify_pinned_keys.rs plants a correct copy of the canonical key under a different token name before and after the real constant and confirms the check still reports divergence — this genuinely exercises the per-token isolation extract_constant relies on, rather than only checking "a wrong value exists somewhere in the file."
• Every test file in this PR calls the real production function under test (download_file_streaming, verify_sha256, verify_rsa_pkcs1_sha256_signature, ModelRecipeRecord::matches_ref, openai_models_endpoint_has_model, check_pinned_keys, the real serde impls in rocm-engine-protocol) rather than a reimplementation of the logic under test — confirmed in every file by at least one independent pass.

Deployment notes

• This PR's head (f995307) has a genuine content merge conflict against current main in engines/lemonade/src/install.rs: a later, unrelated refactor on main (after this PR's base commit) moved verify_sha256 out of install.rs into archive_cache.rs. GitHub's own mergeable_state for this PR independently reports dirty, consistent with this. The two new tests this PR adds to install.rs will need to move with that function when this PR is rebased onto current main.
• Test-only change: no production code, CLI behavior, API, or file-layout change for any consumer.

What this covered

Entire diff (10 files) read completely at PR head f9953072119a32f91edaa5ea86a93985918b6197 against merge-base 4f485bdfb4af367d1274eaa0a9563a1c821ded8c, by four independent blind fan-out workers plus the driver's own direct reading of the production source each test file exercises (net.rs's download digest-gate path, crypto.rs, excerpts of model_catalog.rs/model_readiness.rs, rocm-engine-protocol/src/lib.rs, and the pre-existing production logic in xtask/src/verify_pinned_keys.rs).

• Agent-instruction adherence checked against the root AGENTS.md (read in full): no violation found; the PR's own checklist items are correctly marked N/A for a test-only change with no message/behavior changes.
• History pass: done directly by the driver (git log on the five pre-existing touched files) — surfaced the install.rs divergence noted under Deployment notes; nothing else of note.
• Prior-changes pass: searched this author's own recent commit history for established conventions a new proptest file should follow (surfaced the PROPTEST_CASES-floor pattern above); no prior review comments existed to check against, since this is the PR's first review.
• Code-comments pass: folded into each file-group pass; no contradiction between comments and code beyond one sub-threshold wording imprecision (a doc comment on decode_all_without_panic describing some request-direction types as "output," noted by a worker but not scored as a finding).
• CI: every required check on the reviewed commit reports success (fmt, clippy, the full workspace test matrix, Windows build/test, all engine E2E lanes, the commit-signature/sign-off gate, license headers, lychee, third-party notices). Two early "E2E tests (Strix Halo, Ubuntu, {nightly,release}, current)" runs show cancelled, but later runs of the same check names on the same commit report success — no outstanding CI gap.
• Not independently executed: no Rust toolchain was available in the review environment, so no test in this PR was actually compiled or run by any pass. All conclusions are from tracing the test logic against the real production code (in two cases cross-checked by independently re-deriving the relevant SHA-256/matching-function outputs in Python), not from cargo test. The PR description's own mutation-testing and runtime claims could not be independently re-executed for the same reason; nothing found contradicts them.
• Not reconciled against any discussion on this PR — there was no existing review on this PR at the time this one was written.

---

🤖 Generated with Claude Code

[jussielo-amd]

Thanks Romain, this is a thorough suite and every test calls the real production function. One blocker, and it only shows up once the branch meets current main.

Blocking

• listing_a_different_catalog_model_is_never_readiness (model_selector_properties.rs:372-426) should fail on current main. #794 grew the catalog from 14 to 49 recipes after this branch's CI ran. Two aliases now trip it: qwen3-32b (on Qwen/Qwen3-32B-FP8) and qwen3.6-35b-a3b read as ready for Qwen/Qwen3-32B / Qwen/Qwen3.6-35B-A3B, because model_refs_match (net.rs:909-917) returns true when basename(right) == left. So the test is catching a real readiness false positive, and the description's "no catalog model reads as ready for a different one" holds only at the old base. Please decide when rebasing: fix readiness, or pin it as a labelled characterization like the bare-family case. (Hand-traced against main's catalog; I could not run it here.)

Rebase note

• One textual conflict, in engines/lemonade/src/install.rs: #825 moved verify_sha256 to engines/lemonade/src/archive_cache.rs. The two new lemonade tests are still valid but need to move into that module (proptest is already a dev-dep there). The description's mention of install.rs will be stale.

Non-blocking

• The digest near-miss generator picks position/keep randomly over 24 cases, so a last-nibble mutant is caught in roughly half the runs; looping all 64 positions makes it deterministic.
• Rejection tests assert only is_err(), so they'd also pass on a transport failure; serve_once + join() can hang in accept() if the client never connects.
• A few weak/redundant checks: model_selector_properties.rs:248-277 (effectively no-panic), :88 (subsumed by validate()), protocol_properties.rs:394, :408-427, :548.
• The pairwise readiness test is quadratic: ~770 loopback requests now, ~6,200 on main's catalog.
• xtask tests hard-require the current RSA-2048 key shape, so a key rotation breaks them for a non-bug reason.

---

🤖 by agenthub on AMD AgentHub

[rominf]

Thanks. All addressed in b8a6bcc and 282e946, on a branch rebased onto current main.

Blocker. Reproduced: after the rebase listing_a_different_catalog_model_is_never_readiness fails on exactly the two pairs you traced (qwen3-32b → Qwen/Qwen3-32B, qwen3.6-35b-a3b → Qwen/Qwen3.6-35B-A3B). I went with a labelled characterization rather than changing readiness, since this PR is test-only and the fix is a product decision: either readiness stops accepting a bare basename when it is another catalog recipe's alias, or the catalog drops those aliases. an_alias_spelling_another_models_basename_reads_as_ready_for_it derives every alias/basename collision from the catalog, compares the list against the two known ones, and asserts each still reads as ready, so a catalog change that adds a collision fails there instead of being silently excluded from the pairwise property. One correction to the trace: qwen3.6-35b-a3b belongs to unsloth/Qwen3.6-35B-A3B-GGUF:Q4_K_M, not an FP8 recipe. Happy to open an issue for the readiness side.

Rebase. The two Lemonade digest tests moved into archive_cache.rs with verify_sha256.

Non-blocking.

• Digest near misses: the one-nibble, wrong-suffix and truncated-prefix kinds are now also walked over all 64 positions deterministically (positional_near_misses_are_rejected_at_every_position); a mutant that ignores the last nibble fails it every run.
• Rejection assertions now require the gate's own SHA-256 mismatch error, and every download/readiness helper asserts the request actually reached the test server, so a transport failure cannot pass as a rejection or a "not ready".
• The one-shot serve_once is replaced by a shared LoopbackServer that serves any number of requests, sets a read timeout, and wakes its own accept loop on drop, so it cannot hang in accept().
• Quadratic readiness test: one server per served name instead of one per request (about 130 servers for about 6,200 requests on main's catalog; the suite takes about 1.5 s).
• xtask: the tests no longer assume an RSA-2048 key; the coverage check compares the walked characters with a count taken straight from the committed file.
• Weak checks: a_models_document_never_reports_ready_without_naming_a_model is replaced by a_models_document_naming_no_matching_model_never_reports_ready, whose ids carry no letter so no matching rule can apply. no_advertised_selector_matches_two_recipes is removed, since ModelRecipeIndexDocument::validate already rejects a duplicated ref. The two implied protocol assertions are gone, and the --gpu and engine-id properties point at the example tests that serve as their positive controls.

Also from the automated review: every property's case count can now be raised by PROPTEST_CASES but never lowered (shared helper crates/rocm-core/tests/common/proptest_cases.rs); the Lemonade properties previously took it unchecked, so PROPTEST_CASES=0 passed them without running a case. The family-word cross-match stays pinned and labelled, next to the new alias/basename pin.

[jussielo-amd]

Thanks Romain. My earlier blocker is resolved: I traced the pairwise readiness property against main's 49-recipe catalog and the only alias/basename collisions are exactly qwen3-32b and qwen3.6-35b-a3b, which are now excluded from the property and pinned by an_alias_spelling_another_models_basename_reads_as_ready_for_it. The Lemonade digest tests moved into archive_cache.rs, the LoopbackServer can't hang (accept loop is woken on drop, reads time out), the case-count floor holds at PROPTEST_CASES=0, and the pinned-key tests no longer depend on key shape. CI on this head is green apart from two hardware E2E lanes that are still queued. This was a static review; I did not run the tests.

Non-blocking points are inline (a third copy of the case-count helper, and a hand copy of the download chunk size). One more: the description says a one-bit flip "anywhere" in the Lemonade archive is rejected, but that test flips the last byte plus sampled positions, so "sampled positions" would be more accurate.

---

🤖 by agenthub on AMD AgentHub

[jussielo-amd]

Non-blocking: this is a third copy of proptest_cases/raised_case_count (see crates/rocm-core/src/test_support.rs:192 and apps/rocm/src/test_support.rs:61), and the doc comment on the rocm-core one still says only archive_props keeps its own. Including this file from test_support.rs by #[path], or updating that comment, would keep them from drifting. The #[test] in this file also runs once per includer.

[jussielo-amd]

Non-blocking: CHUNK_BYTES is a hand copy of the private DOWNLOAD_CHUNK_BYTES in download.rs, with nothing tying them together. The test is built to survive drift (3 chunks plus 1 byte), so this is fine to leave, but exposing the constant to tests would remove the copy.