From c351f51c5ca961a1bd3331f72f6caaa7bb0cc2d4 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Thu, 13 Aug 2026 20:51:45 +0000 Subject: [PATCH 01/12] spec(LTX25-DEVICE-SEAM-SIBLING): the sibling that never adopted the device seam, and the gate that cannot see it (#659, #660) FOLLOWING_AGENTS_PROTOCOL `11cc1d589` routed LTX-2.5's device question through the platform seam. It fixed the lane it aimed at and left two things standing, both found by a peer session's reviewer while reviewing that repair for landing. #659 -- the seam was adopted, its companion guard was not. `ltx2_video.cpp:549` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)`, but never `supports_model_architecture`, which is the third question the cited precedent `model_loader.cpp:97` asks and the reason a PARTIAL backend can decline by name. `metal.cpp:70` and `tenstorrent.cpp:52` are the two overriders. On those builds a `device = 1` load was refused BY NAME and is now accepted into a kernel bind. CUDA is unaffected -- which is exactly why it is invisible on the gate box. #660 -- the gate that certified that repair is a token grep. `check-device-leakage.py:78` is `\bkCUDA\b`; `minimax_h3_video.cpp:221-226` writes `static_cast(device)` and scores ZERO. The sharpest form: `test_minimax_h3_video_fold.cpp:162` asserts `... == vt::DeviceType::kCUDA`, so the gate counts the TEST's honest spelling and misses the SOURCE's laundered one. "kcuda 2 -> 0" is a true statement about the token and a weaker statement about the property than it reads. The spec's load-bearing constraint is on the new bucket: it must be derived from the property, not from the one spelling in the tree, and it must go RED for at least three spellings that are not that one. A bucket that only catches the known site is a regression test wearing a gate's clothes, and the row says so rather than claiming coverage. Explicitly out of scope: the leakage baseline. A peer measured today that `--write-baseline` refuses to RAISE the baseline, but hand-editing the JSON to a higher number makes the checker PASS -- so the file's only real defence is a visible, reviewed diff, and this row touches it only for its own new entries. Gate: `agent-preflight.sh --staged` is green except `test_cpu_x86_llamacpp_floor`, which failed under six concurrent build agents (`busy=163% load=60.36`, the NO_QUIET_WINDOW retry path) and is a recorded load artifact; this change is a spec plus two roadmap rows and cannot reach a CPU x86 llama.cpp floor. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- .agents/roadmap_v1.md | 4 +- .agents/specs/ltx25-device-seam-sibling.md | 198 +++++++++++++++++++++ 2 files changed, 200 insertions(+), 2 deletions(-) create mode 100644 .agents/specs/ltx25-device-seam-sibling.md diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index c335bd797..439607fcb 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -186,8 +186,8 @@ issue is not yet placed. Keyed record: update in place, never append. | [#607](https://github.com/mudler/vllm.cpp/issues/607) | `ENG-MM-INPUT-PIPELINE` | **Premise corrected 2026-08-13, see `specs/multimodal-track.md` §1.5.** Not "skip the vision encoder": `--language-model-only` sets every modality limit to **0** (`multimodal.py:78,321-327`) and is sugar over `--limit-mm-per-prompt`. Two consequences follow, and we have NEITHER — upstream then **refuses every multimodal request** (`processing/context.py:409-428` raises "At most 0 image(s) may be provided in one prompt", from `:461` and `chat_utils.py:662`), and builds the tower uninitialised (`interfaces.py:293`). We have no `MultimodalConfig`, no `limit_per_prompt` and no enforcement at all, so this is a PORT of the limits mechanism (L1-L4), not the exposure of a boolean. 43 of 157 recipes pass the flag and we abort on it. The flag appears in this repo only in `tools/bench/run_serve_low.py`, which passes it to the ORACLE — a grep reads as coverage and is not | feature | | [#651](https://github.com/mudler/vllm.cpp/issues/651) | — | `test_agent_record`'s MODEL-ratchet docstring is two contradictory paragraphs spliced together, and the surviving half records a pin transition that never happened | bug | | [#652](https://github.com/mudler/vllm.cpp/issues/652) | — | `model-matrix.md` prose counters drifted: LTX-2.5 reached the rows and the CI-enforced rollup but none of the five sentences that count them | bug | -| [#659](https://github.com/mudler/vllm.cpp/issues/659) | — | LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing) | bug | -| [#660](https://github.com/mudler/vllm.cpp/issues/660) | — | `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553) | bug | +| [#659](https://github.com/mudler/vllm.cpp/issues/659) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing) | bug | +| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553) | bug | | [#608](https://github.com/mudler/vllm.cpp/issues/608) | `TOOLS-PARSER-BREADTH` | **W0 (record backfill) landed 2026-08-13; row `INVENTORIED` → `PARTIAL`, 41 shipped names / 37 families recorded.** Re-derived from the two REGISTRIES rather than from recipe usage: **five** `--tool-call-parser` names are upstream-only at the pin — `openai`, `inkling`, `minimax_m3` (W1, recipe demand) and `cohere_command3`, `cohere_command4` (W2, ZERO recipe uses, so usage-driven audits miss them). **Only `inkling` is PORTABLE from vLLM source**: `minimax_m3` is backed by the Rust crate, `openai` is a declared Harmony stub that raises on both methods, and both Cohere names are shims over the out-of-tree `cohere_melody` package, so W1/W2 each owe a recorded decision before code rather than a text port. `nemotron_json`, `kimi_k3` and `ling3` are in NEITHER registry and arrive with the pin advance, not here. W3 ports upstream's shared `ToolParserTestConfig` harness. The earlier "six missing" framing was usage-derived: it listed `nemotron_json` as portable (it is not registered at the pin) and missed both Cohere entries | feature | | [#647](https://github.com/mudler/vllm.cpp/issues/647) | — | Oracle policy had no fallback and no pin concept: five upstreams beyond vLLM are already compared against (vLLM-Omni, SGLang, llama.cpp, `transformers`, tt-forge) with their pins scattered across individual specs or absent entirely. AGENTS.md now admits a named secondary oracle where vLLM implements nothing, `.agents/oracles/.md` pins each one file-per-oracle, and `check-oracle-pins.py` enforces both directions. The gateability debts for `sglang`, `diffusers` and `tt-forge` stay open on this issue | feature | | [#649](https://github.com/mudler/vllm.cpp/issues/649) | `TOOLS-CALLING-CORE` | That row's prose still records `tool_parser_names()` 40 / `reasoning_parser_names()` 7; both enumerations have grown since 2026-07-24 and are now **41** (`tool_parsers/abstract.cpp:269`) and **12** (`reasoning_parsers/abstract.cpp:72`). Code and tests are correct — `test_detect.cpp:221` already pins 41 — only the record drifted. Halves belong to two other rows (#608, #605), so it is filed rather than repaired inside #643 (found while implementing #643's review findings) | bug | diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md new file mode 100644 index 000000000..e083f3554 --- /dev/null +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -0,0 +1,198 @@ +# The diffusion lane's device seam — the sibling that never adopted it, and the gate that cannot see it + +Row: `LTX25-DEVICE-SEAM-SIBLING` +Issues: [#659](https://github.com/mudler/vllm.cpp/issues/659), [#660](https://github.com/mudler/vllm.cpp/issues/660) +Campaign: [#644](https://github.com/mudler/vllm.cpp/issues/644) +Base: `11cc1d5896b480a1b652db9249319242053aca93` + +Both defects were found by a **peer session's reviewer** while reviewing the +main-red repair for landing, not by this campaign. They are recorded here +because they are in this lane and this campaign owns them. + +## 0. What is wrong today + +`11cc1d589` routed LTX-2.5's device question through the platform seam. It fixed +the lane it was aimed at and left two things standing. + +**(a) #659 — the seam was adopted, its companion guard was not.** +`src/vllm/multimodal/ltx2_video.cpp:549-562` now asks two questions: + +```cpp +const vt::DeviceType accelerator = vllm::platforms::CurrentPlatform().device_type(); +if (accelerator == vt::DeviceType::kCPU || + vt::TryGetBackend(accelerator) == nullptr) { Fail(...); } +``` + +That is "is there an accelerator, and is a backend registered for it". The +precedent it cites, `SelectQueueForModel`, asks a third question — +`src/vllm/entrypoints/model_loader.cpp:97`: + +```cpp +(architecture.empty() || plat.supports_model_architecture(architecture)) +``` + +`supports_model_architecture` exists precisely so a **partial** backend can +decline by name. Two platforms override it and both declare a short list: +`src/vllm/platforms/metal.cpp:70` and `src/vllm/platforms/tenstorrent.cpp:52`. +On such a build a `device = 1` LTX-2.5 load **was refused by name** and is now +accepted, so the failure moves from a refusal that says which piece is missing +into a kernel bind that says nothing. CUDA is unaffected, which is exactly why +this is invisible on the box that runs the gates. + +The refusal that *is* there argues, correctly, that serving the CPU forward +behind an accelerator handle "would make every later timing and every 'it ran on +the GPU' claim false". A partial backend that binds and dies has the same +property one level down: it is a device claim the build cannot honour. + +**(b) #660 — the gate that certified (a) is a token grep, and the sibling lane +spells its way past it.** `scripts/check-device-leakage.py:78` is + +```python +RE_KCUDA = re.compile(r"\bkCUDA\b") +``` + +`src/vllm/multimodal/minimax_h3_video.cpp:221-226` never writes that token: + +```cpp +vt::DeviceType MiniMaxH3VideoDeviceType(int32_t device) { + if (device != 0 && device != 1) { throw ...; } + return static_cast(device); +} +``` + +It hardcodes the ABI's `0/1` into the enum by integer cast and scores **zero** +in the `kcuda` bucket. The same defect, in the sibling diffusion lane, under a +different spelling. + +**The sharpest form of it:** `tests/vllm/models/test_minimax_h3_video_fold.cpp:162` +asserts `MiniMaxH3VideoDeviceType(1) == vt::DeviceType::kCUDA`. The *test* spells +the token honestly and is counted; the *source* launders it and is not. The gate +therefore reads the confession and misses the act. + +This is the shape the project already has a name for — an instrument that cannot +report its own blind spot returns a pass. "`kcuda` 2 → 0" is a true statement +about the token and a weaker statement about the property than it appears to be. + +## 1. Scope + +**In.** + +1. `minimax_h3_video.cpp` resolves its device through the platform seam rather + than by integer cast, mirroring what `ltx2_video.cpp` now does. The public + `MiniMaxH3VideoDeviceType(int32_t)` contract in + `include/vllm/multimodal/minimax_h3_video.h:63` is preserved — `0` is CPU, + anything else is refused or resolved, never cast. +2. `ltx2_video.cpp` asks `supports_model_architecture` alongside the two + questions it already asks, and refuses **by name**, naming the platform and + the architecture, when a registered backend declines the model. +3. `check-device-leakage.py` gains a bucket that sees an integer cast to + `vt::DeviceType`. The bucket must be **derived from the property**, not from + the one spelling we happen to have found — see §3. + +**Out.** + +- Any change to what CUDA does. This row must be a no-op on a CUDA build, and + the gate for that is the existing suite passing unchanged on this box. +- Widening or rewriting `device-leakage-baseline.json` beyond the new bucket's + own entries. **The baseline is a ratchet**: a peer measured that + `--write-baseline` refuses to raise it (`REFUSING to write a HIGHER baseline + (32 → 35)`) but that **hand-editing the JSON to a higher number makes the + checker PASS**. So the only real defence is that the diff is visible and + reviewed. Any baseline line this row touches is called out in the PR body, + with its reason, and the reviewer checks the file by md5 against the base for + every key this row does not claim. +- The H3 video engine's model behaviour. This is a device-resolution change. + +## 2. Upstream anchors + +There is no upstream for this: vLLM has no LTX-2.5 or MiniMax-H3 video engine, +and the device seam is ours. The mirror source is therefore **our own +`SelectQueueForModel`** (`src/vllm/entrypoints/model_loader.cpp:59-104`), which +is the shape every other model path already uses, and +`include/vllm/platforms/interface.h:263`, which defines the capability question. +Mirroring an internal seam is what "route it through the shared surface" means; +a second, parallel device resolution in the diffusion lane is exactly the +hand-rolled path AGENTS.md forbids. + +## 3. Design + +**The capability question, at the same site as the existing two.** One added +clause, one refusal message that names the platform, the architecture and the +fact that the backend declined — not a shape error. It must be possible to read +the refusal and know that the build is partial rather than broken. + +**The sibling's resolution.** `static_cast(device)` is replaced +by an explicit mapping: `0` → `kCPU`, non-zero → the platform's +`device_type()`, refused when that is `kCPU` or its backend is absent or it +declines the architecture. That makes the two diffusion lanes answer the device +question the same way, which is the point of the seam. + +**The gate's new bucket is the hard part, and it is where this row can go +wrong.** A bucket that greps `static_cast` closes the one site +we found and nothing else — the same defect as the token grep, one spelling +later. The bucket is defined by the property: *an integer literal or integer +variable becoming a `vt::DeviceType` without passing through the platform +seam*. Whatever the implementation, the acceptance test is adversarial and +stated up front: + +> The bucket must go RED for **at least three spellings** of the same defect +> that are not the one already in the tree — e.g. a C-style cast, a +> `vt::DeviceType(x)` functional cast, and an assignment through an +> intermediate `int`. If it only catches the one we knew about, it is a +> regression test wearing a gate's clothes, and the row says so rather than +> claiming coverage. + +If the property cannot be expressed at the granularity a text checker allows, +that is a finding to record, not to paper over: the row then states the residual +blind spot in the checker's own message, because **a checker's message is the +authority on what it enforces**. + +## 4. Tests + +RED-first for each of the three. + +1. A build with a platform that declines the architecture must refuse the LTX-2.5 + `device = 1` load **by name**. The existing test fixture pattern for a + partial backend is `metal.cpp` / `tenstorrent.cpp`; if neither is + constructible in the CPU test build, the test injects a stub platform rather + than skipping — a skipped test here is the whole finding. +2. `MiniMaxH3VideoDeviceType` keeps its contract: `0` → `kCPU`, `-1` and `2` + throw (`test_minimax_h3_video_fold.cpp:161-164` already assert this and must + stay green **unchanged**), and `1` resolves through the seam rather than by + cast. The new assertion is that on a CPU-only build `1` is **refused**, which + the cast could never do. +3. The checker's three-spelling adversarial test above, each spelling asserted + RED individually, not as a batch. + +**Mutations that must be run and recorded:** revert each of the three changes +independently and confirm the corresponding test goes RED; and confirm the +existing suite is byte-identical in count on CUDA-absent builds, since a changed +count is RED even when it reads green. + +## 5. Risks + +- **The capability guard could refuse a load that works today.** `metal.cpp` and + `tenstorrent.cpp` are the only overriders, so the blast radius is those two + builds — but if either currently *runs* a diffusion model despite a short + list, this row breaks it. Check before assuming; if it does run, the finding + is in the list, not in the guard. +- **The new bucket could red other lanes.** An integer-to-`DeviceType` cast + elsewhere in the tree is either the same defect (fix it or record it) or a + legitimate deserialization boundary (which needs a stated, per-entry reason, + never a blanket directory exemption). +- **Baseline churn.** See §1 Out. + +## 6. Stop conditions + +- If the capability guard turns out to refuse a currently-working configuration, + stop and return `NEEDS_DECISION` rather than either shipping the refusal or + dropping the guard. +- If the new bucket cannot reach three independent spellings, stop and report + the residual blind spot; do not ship a one-spelling bucket described as + coverage. +- `dgx.casa` is not required for any of this. Nothing here is a GPU measurement. + +## Now + +`READY`. Spec committed ahead of implementation; a fresh implementer works from +this file, and a fresh reviewer — not the implementer — reviews the head. From 3a697bbfe376ade4499e651f41bd8535f85f6e01 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Fri, 14 Aug 2026 11:55:43 +0000 Subject: [PATCH 02/12] fix(LTX25-DEVICE-SEAM-SIBLING): the third question, and a leakage bucket that is a property rather than a spelling (#659, #660) Implements the committed spec .agents/specs/ltx25-device-seam-sibling.md. #659. 11cc1d589 routed LTX-2.5's device question through the platform seam and asked two of the three questions its own cited precedent asks. SelectQueueForModel (model_loader.cpp:97) also asks plat.supports_model_architecture(architecture), which exists so a PARTIAL backend can decline BY NAME. metal.cpp:70 and tenstorrent.cpp:52 are the only overriders and both declare exactly {OPTForCausalLM, Qwen3ForCausalLM}. On those builds a device = 1 diffusion load WAS refused by name and had become a queue bind that dies later inside a kernel. CUDA is unaffected - supports_model_architecture defaults to true - which is exactly why it is invisible on the box that runs the gates. The clause is added at the same site in ltx2_video.cpp, and minimax_h3_video.cpp now asks all three. #660. The kcuda bucket is the token grep \bkCUDA\b. minimax_h3_video.cpp:225 (pre-change) wrote static_cast(device) against kCUDA = 1 and scored ZERO, while test_minimax_h3_video_fold.cpp:162 spelled the token honestly and WAS counted: the gate read the confession and missed the act. It is also an enum-ordering hazard - reorder include/vt/device.h and every H3 device-1 load silently re-points. THE LOAD-BEARING CLAIM, which is why this row is worth landing. A bucket that grepped static_cast would close the one site we found and be the same defect one spelling later. dev_cast is anchored on the TARGET TYPE, matched over the whole comment-stripped text, and is RED for SIX spellings that are not in the tree, each asserted individually and each proved by removing the alternative that catches it: M20 C-style (vt::DeviceType)d | alt2 removed -> RED M21 functional vt::DeviceType(d) | alt3 removed -> RED M22 intermediate int + unqualified static_cast<> | alt1 removed -> RED M23 brace vt::DeviceType{d} | alt3 removed -> RED M24 cast split across two lines by clang-format | alt1/join -> RED M29 C-style past the parameter-declaration guards | alt2 removed -> RED Four independent mutations of the pattern, each restored and verified by sha256: alt1 -> 12 RED, alt2 -> 4 RED, alt3 -> 2 RED, join -> 15 RED. Removing any one alternative REDs its own mutants and no others among M20-M24/M29, so no branch is carrying another's weight. NEGATIVES, because a bucket that cannot be lived with gets deleted: M25 (static_cast/ of a DeviceType is the SAFE direction and is how the seam indexes its own registry), M26 (prose and string literals), M27 (the platform registry walk is allowlisted at EXACTLY one - a second cast in platform.cpp still fails), M28 (DSR-ALLOW exempts and is printed loudly), M29 (a parameter declaration is not a cast). M29 is a real false positive the bucket produced on this tree, kept as a mutant: kv_connector.h:225's supports_worker_transfer_on(vt::DeviceType /*device*/) const strips to (vt::DeviceType ) followed by const, which is textually a C-style cast. Two discriminators fixed it and M29 pins both, together with the proof that they did not cost the real detection. RESIDUAL BLIND SPOTS ARE IN THE CHECKER'S OWN DOCSTRING, per the spec's instruction that a checker's message is the authority on what it enforces: casts through a type alias or a template parameter, bit_cast/memcpy/union punning, conversions inside the unscanned src/vt/ device leg, and the fact that nothing type-checks the operand. Those are gaps in a TEXT checker, stated rather than traded away. BASELINE: ONE ADDED LINE, NO CHANGED VALUE. "dev_cast": 0. total stays 32 and every other key is byte-identical. The key is required because load_baseline() reads every entry in BUCKETS; the value is 0 because the H3 site is REPAIRED rather than absorbed and the registry walk is allowlisted with a stated reason. A peer measured that --write-baseline refuses to raise the baseline but that hand-editing the JSON higher makes the checker PASS, so a two-line visible diff is the only real defence and it is called out here for the reviewer to check by md5 against the base for every key this row does not claim. metal.cpp / tenstorrent.cpp RUN NO DIFFUSION MODEL, checked before assuming per spec section 5, with OPTForCausalLM as a positive control in the same command: the control hit 3 times, ltx|minimax|h3|diffus|video hit 0 across platforms/{metal,tenstorrent}.cpp and src/vt/{metal,tenstorrent}/. The guard refuses nothing that works today, so the NEEDS_DECISION stop condition is not triggered. WHERE THE BRIEF AND THE SPEC DISAGREED, resolved in the spec's favour and recorded in the spec. The brief said test_minimax_h3_video_fold.cpp:161-164 must stay green UNCHANGED. Spec section 4.2 enumerates the contract as 0 -> kCPU, -1 and 2 throw, and SEPARATELY requires that on a CPU-only build 1 is REFUSED. Line 162 asserted MiniMaxH3VideoDeviceType(1) == kCUDA - precisely the cast's answer - and cannot survive the change. It is now build-conditional and asserts BOTH arms rather than skipping either. 161/163/164 are untouched. That file's CUDA-load case also registered a BACKEND and no PLATFORM; it could, because the cast never asked whether the build had an accelerator. It supplies both halves now. THE ONE JUDGEMENT CALL: supports_model_architecture is asked with the FAMILY string (ltx-2.5, minimax-h3), not an HF architectures[0] class name. The diffusion lanes are reached through LoadVideoEngine/VideoModelParams::family and never read an architectures entry, so the family slug is the only stable identifier they have, and it is the string the user actually typed. It does mean the seam's key space now mixes HF class names with family slugs; they cannot collide. Flagged rather than buried. GATE. CPU build, no CUDA. CMAKE_EXIT=0 BUILD_EXIT=0, zero compiler errors, zero warnings, zero "No space left"/"BFD assertion" in the build log. ctest -N = 445 = main-at-merge 444 + this row's one new executable; origin/main has since added three more (indextts2 #738/#739/#741), which is the whole of the drift from the 443 measured earlier. Full ctest -j4: 444/445 passed, 2 skipped, 1 FAILED - test_op_parity, which is #737 (a null where a string is required in the MiniMax-Music3 golden manifest added to main today) and reproduces on clean main; this row touches no parity golden. Note it printed "70 passed | 0 failed" while the case THREW: the exit code (CTEST_EXIT=8) is the authority, not the summary. Focused, before -> after: test_ltx2_video 30 cases / 502 assertions UNCHANGED; test_minimax_h3_video_fold 6 / 137 UNCHANGED (the one case I touched executes 4 assertions on either arm, exactly as before - the FAIL is on the untaken path); test_diffusion_device_seam 4 / 23, new. SOURCE MUTATIONS, each reverted independently, rebuilt, and restored by sha256: * revert the h3 seam to the cast -> test_diffusion_device_seam 2/4 cases FAIL (kXPU assertion and the DECLINES refusal), test_minimax_h3_video_fold FATAL at :214 "device 1 must be refused when no accelerator backend is registered", and check-device-leakage RED at dev_cast 1 > baseline 0. Three independent instruments, one defect. * drop the ltx2 capability clause -> test_diffusion_device_seam 1/4 cases FAIL, 5 assertions, on platform/architecture/DECLINES. Restored and re-green at 4/23 and 6/137. preflight --staged: 2 gates fail, audit-live-rows and test_audit_live_rows, both the stale ACTIVE MODEL-MUSIC-minimax-music3 row from a3aa02e19. PROVEN not mine: the same single assertion fails in a clean origin/main worktree, and this diff touches no music3 path. Already filed as #731 and #733. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [ClaudeCode] --- .agents/roadmap_v1.md | 4 +- .agents/specs/ltx25-device-seam-sibling.md | 61 +++++- docs/USAGE.md | 9 +- include/vllm/multimodal/minimax_h3_video.h | 10 +- scripts/check-device-leakage.py | 86 +++++++- scripts/device-leakage-baseline.json | 1 + src/vllm/multimodal/ltx2_video.cpp | 34 ++- src/vllm/multimodal/minimax_h3_video.cpp | 59 ++++- tests/CMakeLists.txt | 7 + tests/scripts/test_device_leakage.py | 190 +++++++++++++++- .../models/test_minimax_h3_video_fold.cpp | 60 +++++- .../multimodal/test_diffusion_device_seam.cpp | 204 ++++++++++++++++++ 12 files changed, 708 insertions(+), 17 deletions(-) create mode 100644 tests/vllm/multimodal/test_diffusion_device_seam.cpp diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index 1f5e11ed2..eb820f054 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -194,8 +194,8 @@ issue is not yet placed. Keyed record: update in place, never append. | [#607](https://github.com/mudler/vllm.cpp/issues/607) | `ENG-MM-INPUT-PIPELINE` | **Premise corrected 2026-08-13, see `specs/multimodal-track.md` §1.5.** Not "skip the vision encoder": `--language-model-only` sets every modality limit to **0** (`multimodal.py:78,321-327`) and is sugar over `--limit-mm-per-prompt`. Two consequences follow, and we have NEITHER — upstream then **refuses every multimodal request** (`processing/context.py:409-428` raises "At most 0 image(s) may be provided in one prompt", from `:461` and `chat_utils.py:662`), and builds the tower uninitialised (`interfaces.py:293`). This is a PORT of the limits mechanism (L1-L4), not the exposure of a boolean. **L1 LANDED 2026-08-13:** `vllm::MultiModalConfig` + `GetLimitPerPrompt` (`include/vllm/config/multimodal.h`) and the refusal it carries (`include/vllm/multimodal/processing/context.h`) are in, unit-gated, with NO serve surface — nothing constructs the config on a live request yet, so the two call sites still validate nothing. L2 (the flags + C-ABI field + wiring those call sites), L3 (tower skip) and L4 (kernel gate) are still owed. 43 of 157 recipes pass the flag and we abort on it. The flag appears in this repo only in `tools/bench/run_serve_low.py`, which passes it to the ORACLE — a grep reads as coverage and is not | feature | | [#651](https://github.com/mudler/vllm.cpp/issues/651) | — | `test_agent_record`'s MODEL-ratchet docstring is two contradictory paragraphs spliced together, and the surviving half records a pin transition that never happened | bug | | [#652](https://github.com/mudler/vllm.cpp/issues/652) | — | `model-matrix.md` prose counters drifted: LTX-2.5 reached the rows and the CI-enforced rollup but none of the five sentences that count them | bug | -| [#659](https://github.com/mudler/vllm.cpp/issues/659) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing) | bug | -| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553) | bug | +| [#659](https://github.com/mudler/vllm.cpp/issues/659) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing). Guard added at the same site; `tests/vllm/multimodal/test_diffusion_device_seam.cpp` holds it against an injected declining platform | bug | +| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553). New `dev_cast` bucket, target-type anchored, RED for 5 spellings not in the tree (M20-M24 + M29); H3 resolves through the seam, so `dev_cast` is 0 and the baseline total is unchanged at 32 | bug | | [#608](https://github.com/mudler/vllm.cpp/issues/608) | `TOOLS-PARSER-BREADTH` | **W0 (record backfill) landed 2026-08-13; row `INVENTORIED` → `PARTIAL`. W1 then shipped `inkling`, taking the registry to 42 names / 38 families.** Re-derived from the two REGISTRIES rather than from recipe usage: **five** `--tool-call-parser` names are upstream-only at the pin — `openai`, `inkling`, `minimax_m3` (W1, recipe demand) and `cohere_command3`, `cohere_command4` (W2, ZERO recipe uses, so usage-driven audits miss them). **Only `inkling` was PORTABLE from vLLM source, and it LANDED** (a `ParserEngineToolAdapter` over the already-ported Inkling engine; the gap was the registry face, not the grammar). Of the four left: `minimax_m3` is backed by the Rust crate; `openai` is a stub delegating to `vllm/parser/harmony.py`, which IS vLLM source but wraps the out-of-tree `openai_harmony` package (the SGLang secondary-oracle check was run and REFUSED — vLLM implements this path, so the rule does not admit a secondary); both Cohere names are shims over the out-of-tree `cohere_melody` package. So W1-remaining/W2 each owe a recorded decision before code rather than a text port. `nemotron_json`, `kimi_k3` and `ling3` are in NEITHER registry and arrive with the pin advance, not here. W3 ports upstream's shared `ToolParserTestConfig` harness. The earlier "six missing" framing was usage-derived: it listed `nemotron_json` as portable (it is not registered at the pin) and missed both Cohere entries | feature | | [#647](https://github.com/mudler/vllm.cpp/issues/647) | — | Oracle policy had no fallback and no pin concept: five upstreams beyond vLLM are already compared against (vLLM-Omni, SGLang, llama.cpp, `transformers`, tt-forge) with their pins scattered across individual specs or absent entirely. AGENTS.md now admits a named secondary oracle where vLLM implements nothing, `.agents/oracles/.md` pins each one file-per-oracle, and `check-oracle-pins.py` enforces both directions. The gateability debts for `sglang`, `diffusers` and `tt-forge` stay open on this issue | feature | | [#672](https://github.com/mudler/vllm.cpp/issues/672) | `MODEL-MUSIC-minimax-music3-mini-max-music3-for-conditional-generation` | MiniMax-Music3: port the text-to-music lane — ~8.6B Qwen3 global LLM (our LANDED architecture at vocab 200000) + 8-codebook RVQ depth decoder + 2.4B fp32 flow-matching DiT + DAC Flow-VAE. Our first music-generating model, and the first row whose primary oracle is not vLLM: absent from the pin, from vLLM `main` and from `vllm-omni`, so it gates against the OPEN diffusers PR #14456 with SGLang-Omni as the e2e cross-check, under the fallback rule added in #647 | feature | diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md index e083f3554..80b8615be 100644 --- a/.agents/specs/ltx25-device-seam-sibling.md +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -192,7 +192,64 @@ count is RED even when it reads green. coverage. - `dgx.casa` is not required for any of this. Nothing here is a GPU measurement. +## Findings from implementation + +**The brief and this spec disagreed on one line, and this spec won.** The +dispatch brief said `test_minimax_h3_video_fold.cpp:161-164` must stay green +*unchanged*, but §4.2 above enumerates what "the contract" means — `0 → kCPU`, +`-1` and `2` throw — and separately requires that on a CPU-only build `1` is +**refused**. Line 162 asserted `MiniMaxH3VideoDeviceType(1) == kCUDA`, which is +precisely the cast's answer and cannot survive the change. It is now +build-conditional and asserts BOTH arms: `== accelerator` where one is +registered, refused-by-name where none is. 161/163/164 are untouched. + +**`test_minimax_h3_video_fold.cpp`'s CUDA-load case registered a BACKEND and no +PLATFORM.** It could, because the cast never asked whether the build had an +accelerator. It does now, so the fixture supplies both halves. That is the +defect being visible rather than a harness concession: a build with a CUDA +backend registered and no CUDA platform is not a build that runs on CUDA. + +**The architecture key is the FAMILY string** (`ltx-2.5`, `minimax-h3`), not an +HF `architectures[0]` class name. The diffusion lanes are reached through +`LoadVideoEngine`/`VideoModelParams::family` and never read an `architectures` +entry, so the family slug is the only stable identifier they have. It does mean +`supports_model_architecture`'s key space now mixes HF class names +(`OPTForCausalLM`) with family slugs; the two cannot collide, and the refusal +names the string the user actually typed. Flagged for the reviewer as the one +judgement call in the row. + +**`metal.cpp` and `tenstorrent.cpp` run no diffusion model** — searched in their +own vocabulary over `src/vllm/platforms/{metal,tenstorrent}.cpp` and +`src/vt/{metal,tenstorrent}/` with `OPTForCausalLM` as a positive control in the +same command: the control hit three times, `ltx|minimax|h3|diffus|video` hit +zero. So §5's stop condition is not triggered: the guard refuses nothing that +works today. + +**The `dev_cast` bucket produced one false positive on the real tree**, and it is +the interesting kind. `kv_connector.h:225`'s +`supports_worker_transfer_on(vt::DeviceType /*device*/) const` strips to +`(vt::DeviceType )` followed by `const` — textually a C-style cast. Two +discriminators fixed it (the `(` must not be glued to an identifier; the `)` must +not be followed by a declarator suffix) and M29 pins both, together with the +proof that the discriminators did not cost the real detection. + +**Residual blind spots are recorded in the checker's own docstring**, per §3's +instruction: type aliases and template parameters that resolve to `DeviceType`, +`bit_cast`/`memcpy`/union punning, conversions inside the unscanned `src/vt/` +leg, and the fact that nothing type-checks the operand. The bucket flags every +cast *to* `DeviceType` and relies on `DSR-ALLOW` for the legitimate ones. + ## Now -`READY`. Spec committed ahead of implementation; a fresh implementer works from -this file, and a fresh reviewer — not the implementer — reviews the head. +`READY`, implemented and awaiting review on `row/LTX25-DEVICE-SEAM-SIBLING` +(PR #671). All three changes are on the branch with their RED, GREEN and +mutation evidence in the PR body; next is a fresh reviewer — not the implementer +— on the immutable head, then the operator's own gate rerun. + +The row stays `READY` in `roadmap_v1.md` deliberately. A lifecycle move to +`ACTIVE` owes `docs/STATUS.md` and `docs/BENCHMARKS.md` in the same change +(scripts/check-doc-checkpoint.py), and those are projections of what the project +CLAIMS — which this row does not change until it lands. Writing them from an +unmerged PR would also put two shared files under a lock for the length of a +review. The operator moves the state, and writes those two surfaces, when it +merges. diff --git a/docs/USAGE.md b/docs/USAGE.md index 6d174430a..72fb7af19 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -1711,7 +1711,14 @@ The LTX-2.5 arm runs on the CPU in f32 and on CUDA in bf16. `device = 0` takes the f32 parity forward; `device = 1` stages the DiT to the GPU one tensor at a time and runs the device-resident forward, so a CUDA handle means a CUDA forward. On a build with no CUDA backend, `device = 1` is refused by name rather than -served the CPU forward behind a CUDA handle. `encoder_path` loads the Gemma-4 +served the CPU forward behind a CUDA handle. It is also refused when the build's +accelerator is a PARTIAL backend that declines this architecture — Metal and +Tenstorrent each register the kernels for a named short list of models, and a +backend that has not registered this one now says so by name instead of binding +a queue and failing later inside a kernel. The same three questions decide +`minimax-h3`'s `device = 1`, which resolves through the platform seam rather +than reading the ABI selector as an enum value, so on a CPU-only build it throws +instead of naming CUDA. `encoder_path` loads the Gemma-4 text tower, and the request's own `prompt` then conditions the render; the tower itself runs on the CPU in f32 whichever device the DiT is on. Without one, conditioning comes from the two prompt-embeds files, which must agree on their diff --git a/include/vllm/multimodal/minimax_h3_video.h b/include/vllm/multimodal/minimax_h3_video.h index 4156d510f..f59a0fdba 100644 --- a/include/vllm/multimodal/minimax_h3_video.h +++ b/include/vllm/multimodal/minimax_h3_video.h @@ -58,8 +58,14 @@ namespace vllm::multimodal { inline constexpr char kMiniMaxH3VideoFamily[] = "minimax-h3"; // Map the stable public video ABI device selector onto the runtime's generic -// backend key. The ABI remains 0=CPU / 1=CUDA; callers below this seam dispatch -// only through the returned DeviceType. +// backend key. The ABI is unchanged — 0 is the CPU, 1 is the accelerator, and +// anything else throws — but 1 is RESOLVED through the platform seam +// (`CurrentPlatform().device_type()` + `TryGetBackend` + +// `supports_model_architecture`), never cast from the integer. It therefore +// THROWS on a build with no accelerator backend, or one whose partial backend +// declines this architecture, instead of naming a device that build cannot run +// (#659, #660). Callers below this seam dispatch only through the returned +// DeviceType. vt::DeviceType MiniMaxH3VideoDeviceType(int32_t device); // ── Load-time parameters (the checkpoint set; the C ABI mirror is diff --git a/scripts/check-device-leakage.py b/scripts/check-device-leakage.py index c8f93352a..27795ac68 100644 --- a/scripts/check-device-leakage.py +++ b/scripts/check-device-leakage.py @@ -19,20 +19,55 @@ attention-registry work each added a device test in passing. Leakage grows silently under well-executed work, so it needs a ratchet, not a cleanup. -WHAT IT COUNTS. Four buckets, over non-comment / non-string-literal source: +WHAT IT COUNTS. Five buckets, over non-comment / non-string-literal source: kcuda textual `DeviceType::kCUDA` (or bare `kCUDA`) references is_cuda `is_cuda()` call sites + dev_cast an integer converted into a `vt::DeviceType` by a cast, in any + spelling — the device named by ENUM VALUE instead of by the + platform seam cuda_inc `#include "vt/cuda/…"` / `` NOT inside a CUDA or VT_* preprocessor guard (i.e. a non-CUDA build cannot compile) vt_ifdef `#ifdef VT_*` / `#if defined(VT_*)` build-time kernel-feature gates -Comments and string literals are stripped before matching in the first two +Comments and string literals are stripped before matching in the first three buckets. That is a DELIBERATE correction to the audit's composition, which excluded comment lines from `is_cuda` but not from `kCUDA` — a prose mention of a device name is not device leakage, and making the rule uniform is what keeps the metric honest in both directions. +WHY `dev_cast` EXISTS (#660). `kcuda` is a TOKEN grep. `DeviceType` is +`enum class DeviceType : uint8_t { kCPU = 0, kCUDA = 1, … }` +(include/vt/device.h), so `static_cast(device)` on the public +0/1 ABI selector hardcodes CUDA *by enum value* and never writes the token. That +is exactly what `src/vllm/multimodal/minimax_h3_video.cpp` did: it scored ZERO in +`kcuda` while the test asserting the same mapping spelled `kCUDA` honestly and +WAS counted. The gate read the confession and missed the act. It is also an +enum-ordering hazard — reordering the enum silently re-points every such site. + +`dev_cast` is anchored on the TARGET TYPE, not on the operand and not on one +cast keyword, because the property is "an integer becomes a DeviceType outside +the platform seam" and the property is what has to be caught. The reverse +direction (`static_cast(type)`) is the SAFE one and is deliberately not +counted: it is how the seam itself indexes its registry. + +WHAT `dev_cast` STILL CANNOT SEE, stated here because a checker's message is the +authority on what it enforces and an instrument that hides its blind spot +returns a false pass: + + * a cast whose target is a type ALIAS (`using DT = vt::DeviceType;` then + `static_cast
(x)`), or a template parameter that resolves to DeviceType; + * `std::bit_cast`, `memcpy` or a union punning an integer onto a DeviceType; + * a conversion that happens inside `src/vt/` and is merely CALLED from the + shared layer — `src/vt/` is a device leg and is not scanned at all; + * whether the operand really is an integer. Nothing here type-checks; the + bucket flags every cast TO DeviceType and relies on `// DSR-ALLOW()` + for the legitimate ones (a wire-format decode is the expected case). + +Those are gaps in a TEXT checker, not gaps that were traded away for +convenience. tests/scripts/test_device_leakage.py M20-M28 pin what it does +catch, each spelling asserted on its own. + HOW THE RATCHET WORKS. `scripts/device-leakage-baseline.json` holds the accepted per-bucket counts. Any bucket ABOVE its baseline fails. Any bucket BELOW its baseline also fails, with the instruction to re-run `--write-baseline` and commit @@ -73,10 +108,40 @@ SCAN_ROOTS = ("src/vllm", "include/vllm") SOURCE_SUFFIXES = {".h", ".hpp", ".cpp", ".cc", ".cu", ".cuh"} -BUCKETS = ("kcuda", "is_cuda", "cuda_inc", "vt_ifdef") +BUCKETS = ("kcuda", "is_cuda", "dev_cast", "cuda_inc", "vt_ifdef") RE_KCUDA = re.compile(r"\bkCUDA\b") RE_IS_CUDA = re.compile(r"\bis_cuda\s*\(\s*\)") + +# `dev_cast`: an integer becoming a DeviceType. Three alternatives, one per C++ +# spelling of the same conversion, all anchored on the TARGET TYPE: +# +# 1. a named cast static_cast(x) / / … +# 2. a C-style cast (vt::DeviceType)x +# 3. a functional cast vt::DeviceType(x) and vt::DeviceType{x} +# +# `\b` after the type name keeps `DeviceTypeName(t)` out. The lookbehind on (3) +# keeps `X::DeviceType(` and `foo.DeviceType(` and `vector(` out, so +# a qualified spelling is matched once at its `vt::`, not twice. The negative +# lookahead keeps a zero-argument `DeviceType()` value-initialisation out — that +# converts nothing. Matched over the WHOLE comment-stripped text rather than +# line by line, so a clang-format-wrapped cast cannot slip between two lines. +# +# (2) needs two guards, because `(Type)ident` and a PARAMETER LIST are textually +# the same thing. `supports_worker_transfer_on(vt::DeviceType /*device*/) const` +# in kv_connector.h is a declaration whose parameter name is commented out; after +# stripping it reads exactly like a cast applied to `const`. So the `(` must not +# be glued to an identifier (that is a call or a declarator), except after the +# keywords that legitimately precede a parenthesised expression; and what follows +# must not be a declarator suffix. +RE_DEVTYPE_CAST = re.compile( + r"(?:static_cast|reinterpret_cast|const_cast|dynamic_cast)\s*<\s*" + r"(?:const\s+)?(?:vt\s*::\s*)?DeviceType\b\s*>" + r"|(?:(?])(?:vt\s*::\s*)?DeviceType\b\s*[({]\s*(?![)}])" +) RE_CUDA_INCLUDE = re.compile(r'^\s*#\s*include\s*[<"](?:vt/cuda/|cuda_runtime)') RE_PP_IF = re.compile(r"^\s*#\s*(ifdef|ifndef|if)\b(.*)$") RE_PP_ELIF = re.compile(r"^\s*#\s*elif\b(.*)$") @@ -114,6 +179,12 @@ "`{kCUDA, kXPU, kVULKAN, kMETAL, kCPU}` — a data list that " "names every platform equally, mirroring upstream's " "`platforms/__init__.py` import probe."), + "dev_cast": (1, "`FindPlatformByName`'s `static_cast(i)`: the " + "registry is an array indexed BY DeviceType, so turning a " + "slot index back into its type is the seam's own inverse, " + "not a model file naming a device by enum value. Budgeted " + "at exactly one — a second cast in this file is not the " + "registry walk and fails."), }, "include/vllm/platforms/interface.h": { "kcuda": (1, "the `is_cuda()` DEFINITION itself " @@ -334,6 +405,15 @@ def scan(root: Path) -> Result: for m in RE_IS_CUDA.finditer(code): del m found["is_cuda"].append(Hit(rel, lineno, "is_cuda", raw_lines[lineno - 1].strip())) + # `dev_cast` matches over the WHOLE stripped text, not line by line: a + # cast wrapped across two lines is the same conversion, and a + # line-oriented matcher would read the halves as two innocent lines. + # The match offset is mapped back to a 1-based line for reporting. + joined = "\n".join(code_lines) + for m in RE_DEVTYPE_CAST.finditer(joined): + lineno = joined.count("\n", 0, m.start()) + 1 + text = raw_lines[lineno - 1].strip() if lineno <= len(raw_lines) else m.group(0) + found["dev_cast"].append(Hit(rel, lineno, "dev_cast", text)) for lineno, line in enumerate(raw_lines, start=1): if RE_CUDA_INCLUDE.match(line) and not guarded[lineno - 1]: found["cuda_inc"].append(Hit(rel, lineno, "cuda_inc", line.strip())) diff --git a/scripts/device-leakage-baseline.json b/scripts/device-leakage-baseline.json index 42eec26f5..1e00af952 100644 --- a/scripts/device-leakage-baseline.json +++ b/scripts/device-leakage-baseline.json @@ -13,6 +13,7 @@ "buckets": { "kcuda": 0, "is_cuda": 0, + "dev_cast": 0, "cuda_inc": 0, "vt_ifdef": 32 } diff --git a/src/vllm/multimodal/ltx2_video.cpp b/src/vllm/multimodal/ltx2_video.cpp index c0eaf389f..127fe4fc8 100644 --- a/src/vllm/multimodal/ltx2_video.cpp +++ b/src/vllm/multimodal/ltx2_video.cpp @@ -548,8 +548,8 @@ std::unique_ptr Ltx2VideoEngine::Load(const VideoModelParams& p // what would make every later timing and every "it ran on the GPU" claim false. im.on_device = params.device != 0; if (im.on_device) { - const vt::DeviceType accelerator = - vllm::platforms::CurrentPlatform().device_type(); + const vllm::platforms::Platform& platform = vllm::platforms::CurrentPlatform(); + const vt::DeviceType accelerator = platform.device_type(); if (accelerator == vt::DeviceType::kCPU || vt::TryGetBackend(accelerator) == nullptr) { Fail("device " + std::to_string(params.device) + @@ -560,6 +560,36 @@ std::unique_ptr Ltx2VideoEngine::Load(const VideoModelParams& p "what is missing is the backend. Refusing rather than running the CPU forward " "behind an accelerator handle."); } + // The THIRD question, which the seam's own precedent asks and this file did + // not (#659). "Is there an accelerator" and "is a backend registered" are + // both true on a PARTIAL backend — Metal registers 15 of 75 ops, Tenstorrent + // a comparable slice — and both name exactly two text architectures in their + // `supports_model_architecture` allow-lists (src/vllm/platforms/metal.cpp:70, + // src/vllm/platforms/tenstorrent.cpp:52). Before the seam landed, such a + // build asked `TryGetBackend(kCUDA)`, got nullptr, and REFUSED BY NAME; after + // it, it is handed a queue and dies later inside a kernel bind with a shape + // error that says nothing about what is missing. CUDA and CPU are unaffected: + // `supports_model_architecture` defaults to true (interface.h:263) and is a + // claim only a partial backend ever narrows. + // + // The refusal above argues that serving the CPU forward behind an accelerator + // handle "would make every later timing and every 'it ran on the GPU' claim + // false". A partial backend that binds and dies is the same thing one level + // down: a device claim this build cannot honour. + // + // The key is the FAMILY string — this lane's stable registry name + // (`VideoModelParams::family`) — because the diffusion engines are reached + // through `LoadVideoEngine`, not through ModelRegistry's HF `architectures`. + if (!platform.supports_model_architecture(kLtx2VideoFamily)) { + Fail("device " + std::to_string(params.device) + " resolves to platform '" + + std::string(vt::DeviceTypeName(accelerator)) + + "', and that platform DECLINES the architecture '" + + std::string(kLtx2VideoFamily) + + "' (Platform::supports_model_architecture): it is a PARTIAL backend that has " + "not registered the kernels this model needs. The build is partial, not " + "broken. Refusing by name rather than binding a queue that would die inside a " + "kernel bind with an error that names none of this."); + } // `vt::CreateQueue(Device)`, NOT `Backend::CreateQueue()`. backend.h:212-217 // records the method as a "temporary index-0 migration shim" and says new // adapter code must use the free function, and the difference is not diff --git a/src/vllm/multimodal/minimax_h3_video.cpp b/src/vllm/multimodal/minimax_h3_video.cpp index c5f1b256b..375a0e882 100644 --- a/src/vllm/multimodal/minimax_h3_video.cpp +++ b/src/vllm/multimodal/minimax_h3_video.cpp @@ -32,6 +32,7 @@ #include "vllm/model_executor/model_loader/gguf_reader.h" #include "vllm/model_executor/model_loader/safetensors_reader.h" #include "vllm/model_executor/models/minimax_h3.h" +#include "vllm/platforms/interface.h" // CurrentPlatform() — which accelerator, if any #include "vllm/tokenizer/tokenizer.h" #include "vt/backend.h" #include "vt/dtype.h" @@ -218,11 +219,65 @@ void FillNoise(std::vector& out, uint64_t seed) { // ── the engine ─────────────────────────────────────────────────────────────── +// ── where this engine runs (#659, #660) ────────────────────────────────────── +// +// `device` is the public video ABI's selector: 0 is the CPU, 1 is "the +// accelerator". WHICH accelerator is the PLATFORM's question, not this model +// file's — the same question `ltx2_video.cpp:549-580` and `SelectQueueForModel` +// (src/vllm/entrypoints/model_loader.cpp:75-104) ask. +// +// This used to be `static_cast(device)`, which is not a mapping +// at all. It reads the ABI selector AS AN ENUM VALUE and is correct only for as +// long as `kCUDA` stays 1 in include/vt/device.h — reorder that enum and every +// H3 device-1 load silently re-points at a different backend. It also named CUDA +// without writing `kCUDA`, so the DSR ratchet's token grep scored it ZERO while +// the test asserting the very same mapping spelled the token honestly and WAS +// counted: the gate read the confession and missed the act (#660). The +// `dev_cast` bucket in scripts/check-device-leakage.py now sees it. +// +// Three questions, the same three the seam answers everywhere else: +// 1. is there an accelerator at all (`CurrentPlatform().device_type()`), +// 2. is a backend registered for it (`vt::TryGetBackend`), +// 3. can that backend actually run THIS model +// (`Platform::supports_model_architecture`, interface.h:263) — a PARTIAL +// backend (Metal 15/75 ops, Tenstorrent) must be able to decline by name +// rather than be handed a queue and die inside a kernel bind (#659). +// +// The architecture key is the family string, because that is this lane's stable +// registry name (`VideoModelParams::family`); the diffusion engines are not +// reached through ModelRegistry's HF `architectures` entry. +// +// On a CUDA box all three pass and this resolves EXACTLY the device the cast +// did. On a CPU-only build device 1 is now REFUSED here instead of returning +// kCUDA and failing one step later in `vt::GetBackend(kCUDA)`, and the refusal +// says which piece is missing. vt::DeviceType MiniMaxH3VideoDeviceType(int32_t device) { if (device != 0 && device != 1) { - throw std::runtime_error("minimax_h3 video: device must be 0 (cpu) or 1 (cuda)"); + throw std::runtime_error( + "minimax_h3 video: device must be 0 (cpu) or 1 (the accelerator this build " + "resolves)"); } - return static_cast(device); + if (device == 0) return vt::DeviceType::kCPU; + + const vllm::platforms::Platform& platform = vllm::platforms::CurrentPlatform(); + const vt::DeviceType accelerator = platform.device_type(); + if (accelerator == vt::DeviceType::kCPU || vt::TryGetBackend(accelerator) == nullptr) { + throw std::runtime_error( + "minimax_h3 video: device 1 asks for an accelerator, but no accelerator backend " + "is registered in this build (the platform seam resolves to '" + + std::string(vt::DeviceTypeName(accelerator)) + + "'). Refusing rather than naming a device this build cannot run on."); + } + if (!platform.supports_model_architecture(kMiniMaxH3VideoFamily)) { + throw std::runtime_error( + "minimax_h3 video: device 1 resolves to platform '" + + std::string(vt::DeviceTypeName(accelerator)) + "', and that platform DECLINES the " + "architecture '" + std::string(kMiniMaxH3VideoFamily) + + "' (Platform::supports_model_architecture): it is a PARTIAL backend that has not " + "registered the kernels this model needs. The build is partial, not broken. " + "Refusing by name rather than binding a queue that would die inside a kernel bind."); + } + return accelerator; } struct MiniMaxH3VideoEngine::Impl { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d90527b62..d4225561f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -156,6 +156,13 @@ target_compile_definitions(test_video_engine PRIVATE # LTX-2.5 L7: the family behind the VideoEngine seam and the driving loop, run # over a reduced-dimension checkpoint set written in the SHIPPED file format. vllm_cpp_add_test(test_ltx2_video vllm/multimodal/test_ltx2_video.cpp) +# #659/#660: both diffusion lanes' device resolution held against a PARTIAL +# backend that DECLINES the architecture. Its own executable for the same reason +# test_device_selection is one — it registers a fake platform and backend in the +# global registries (XPU slot, and over the CUDA slot so CurrentPlatform() +# resolves to it on a CUDA build too), and process isolation keeps that out of +# every other suite. +vllm_cpp_add_test(test_diffusion_device_seam vllm/multimodal/test_diffusion_device_seam.cpp) # LTX-2.5 DiT parity gate (row MODEL-DIFFUSION-LTX25, spec phase L2). The goldens # live next to the test source, like every other generated .inc. vllm_cpp_add_test(test_ltx2 vllm/models/test_ltx2.cpp) diff --git a/tests/scripts/test_device_leakage.py b/tests/scripts/test_device_leakage.py index a236b6de1..232b6ff1c 100644 --- a/tests/scripts/test_device_leakage.py +++ b/tests/scripts/test_device_leakage.py @@ -18,8 +18,21 @@ 3. A REDUCTION also fails until the baseline is lowered in the same commit, which is what makes it a ratchet rather than a threshold (M10-M12). 4. The escape hatch is real, bounded and LOUD (M13-M15). + 5. The `dev_cast` bucket is derived from the PROPERTY — an integer becoming a + `vt::DeviceType` outside the platform seam — and not from the one spelling + that happened to be in the tree when it was written (M20-M28, #660). python3 tests/scripts/test_device_leakage.py + +WHY (5) IS ASSERTED SO HARD. `kcuda` is the token grep `\bkCUDA\b`. +`minimax_h3_video.cpp` wrote `static_cast(device)` against +`kCUDA = 1` and scored ZERO in that bucket while the TEST that asserted the same +mapping spelled the token honestly and was counted — the gate read the confession +and missed the act (#660). A bucket that closed only `static_cast` +would be that same defect one spelling later: a regression test wearing a gate's +clothes. So every mutant below is a spelling that is NOT in the tree, and each is +asserted on its own rather than in a batch, because a batch passes when one +alternative in the pattern works. """ from __future__ import annotations @@ -64,6 +77,14 @@ ), "src/vllm/platforms/platform.cpp": ( "static const DeviceType kOrder[] = {DeviceType::kCUDA, DeviceType::kCPU};\n" + # The registry-walk inverse: an array INDEXED by DeviceType turned back + # into its type. Allowlisted at exactly one `dev_cast`, so the fixture + # has to carry it or the two-way allowlist ratchet reads the synthetic + # tree as a platform leg that lost its cast. + "Platform* Find(std::string_view n) {\n" + " const DeviceType type = static_cast(i);\n" + " return nullptr;\n" + "}\n" ), "include/vllm/platforms/interface.h": ( "bool is_cuda() const { return device_type() == DeviceType::kCUDA; }\n" @@ -86,7 +107,7 @@ # The synthetic tree's own DSR: one `kCUDA` in toy.cpp; everything else is either # allowlisted platform leg or outside the scanned roots. -BASE_BUCKETS = {"kcuda": 1, "is_cuda": 0, "cuda_inc": 0, "vt_ifdef": 0} +BASE_BUCKETS = {"kcuda": 1, "is_cuda": 0, "dev_cast": 0, "cuda_inc": 0, "vt_ifdef": 0} class Tree: @@ -356,6 +377,152 @@ def test_M19_device_legs_under_src_vt_are_not_scanned(self) -> None: rc, _out, err = self.tree.run() self.assertEqual(rc, 0, err) + # --- 5. `dev_cast` is a PROPERTY, not a spelling (#660) ------------------ + # + # The spelling already in the tree when this bucket was written was + # `static_cast(device)`. NONE of the mutants below use it. + # Each is asserted individually: a single test that planted all of them at + # once would pass while four of the five alternatives were dead. + + def plant(self, body: str) -> tuple[int, str, str]: + self.tree.append("src/vllm/model_executor/models/toy.cpp", body) + return self.tree.run() + + def test_M20_c_style_cast_to_devicetype_fails(self) -> None: + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) { return (vt::DeviceType)d; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M21_functional_cast_to_devicetype_fails(self) -> None: + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) { return vt::DeviceType(d); }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M22_cast_of_an_intermediate_int_variable_fails(self) -> None: + # The operand is neither a literal nor the parameter, so a pattern + # anchored on WHAT IS CAST rather than on the TARGET TYPE would miss it. + # The qualifier is dropped too: `DeviceType`, not `vt::DeviceType`. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(const Params& p) {\n" + " int raw = p.device;\n" + " raw = raw ? 1 : 0;\n" + " return static_cast(raw);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M23_brace_initialized_cast_to_devicetype_fails(self) -> None: + # C++17 permits list-initialising a scoped enum with a fixed underlying + # type from an integer, so this is a real, compiling laundering route. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) { return vt::DeviceType{d}; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M24_cast_split_across_lines_fails(self) -> None: + # clang-format wraps long casts. A line-oriented matcher reads the two + # halves as two innocent lines; the bucket matches over the whole + # comment-stripped text and maps the offset back to a line. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t device_selector_from_the_public_abi) {\n" + " return static_cast<\n" + " vt::DeviceType>(device_selector_from_the_public_abi);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + # --- and the negatives, which are what keep the bucket usable ------------ + + def test_M25_casting_a_devicetype_TO_an_integer_does_not_count(self) -> None: + # The safe direction. `std::to_string(static_cast(type))` is in the + # platform seam itself; a bucket that counted it would be unlivable. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "int Index(vt::DeviceType t) { return static_cast(t); }\n" + "size_t Slot(vt::DeviceType t) { return static_cast(t); }\n" + "const char* N(vt::DeviceType t) { return vt::DeviceTypeName(t); }\n" + "std::vector All() { return kOrder; }\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + + def test_M26_a_devicetype_cast_in_prose_or_a_string_does_not_count(self) -> None: + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "// history: this was (vt::DeviceType)device before the seam landed\n" + "/* static_cast(1) is what #660 was about */\n" + 'void D() { VT_CHECK(ok, "never write vt::DeviceType(raw) here"); }\n', + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + + def test_M27_the_platform_registry_walk_is_allowlisted_exactly(self) -> None: + # platform.cpp turns a REGISTRY INDEX back into a DeviceType. That is the + # seam's own inverse, not a model file hardcoding a device — but it is + # budgeted at exactly one, so a second cast in that file still fails. + self.tree.write( + "src/vllm/platforms/platform.cpp", + "static const DeviceType kOrder[] = {DeviceType::kCUDA, DeviceType::kCPU};\n" + "Platform* Find(std::string_view n) {\n" + " const DeviceType type = static_cast(i);\n" + " return nullptr;\n" + "}\n", + ) + rc, _out, err = self.tree.run() + self.assertEqual(rc, 0, err) + self.tree.append( + "src/vllm/platforms/platform.cpp", + "DeviceType Sneak(int d) { return (DeviceType)d; }\n", + ) + rc, _out, err = self.tree.run() + self.assertEqual(rc, 1) + self.require(err, "ALLOWLIST STALE: src/vllm/platforms/platform.cpp") + self.require(err, "bucket 'dev_cast' has 2 reference(s)") + + def test_M29_a_parameter_declaration_is_not_a_c_style_cast(self) -> None: + # The false positive this bucket actually produced on the real tree, kept + # as a mutant so it cannot come back. `(vt::DeviceType /*device*/)` strips + # to `(vt::DeviceType )` and is then textually a cast applied to `const`. + # Both discriminators are exercised: the `(` glued to the function name, + # and the declarator suffix after the `)`. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "struct Sink {\n" + " virtual bool supports_on(vt::DeviceType /*device*/) const { return true; }\n" + " virtual void note (vt::DeviceType) const noexcept;\n" + "};\n" + "using Fn = void(vt::DeviceType);\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + # …and the discriminator must not have cost the real thing: the same file + # with an actual C-style cast still fails. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType R(int d) { return (vt::DeviceType)d; }\n", + ) + rc, _out, err = self.tree.run() + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M28_dsr_allow_exempts_a_dev_cast_and_says_so_loudly(self) -> None: + # The legitimate case the risk register names: a deserialization boundary + # that reads a device off the wire. It buys an exemption only with a row + # id and a reason, and it is printed on every run. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "// DSR-ALLOW(S9): wire-format decode, validated against kNumDeviceTypes\n" + "vt::DeviceType Decode(uint8_t b) { return (vt::DeviceType)b; }\n", + ) + rc, out, err = self.tree.run() + self.assertEqual(rc, 0, err) + self.require(out, "DSR-ALLOW exemptions in force: 1") + self.require(out, "[dev_cast]") + class RealTreeTests(unittest.TestCase): """Hard expectations about THIS repository, not a synthetic mutant.""" @@ -406,6 +573,27 @@ def test_server_main_dsr_allows_cover_only_the_profiler_guard(self) -> None: f"line {index + 1} has no DSR-ALLOW on it or directly above it", ) + def test_the_diffusion_lanes_resolve_device_through_the_seam(self) -> None: + # #660's actual site, pinned as a fact about THIS tree rather than as a + # number in a baseline. The positive control is in the same assertion: + # the pattern must still fire on the defect's own text, or a null result + # below would prove only that the regex is broken. + defect = "return static_cast(device);" + self.assertTrue( + dl.RE_DEVTYPE_CAST.search(defect), + "positive control failed: the dev_cast pattern no longer matches the " + "very line #660 was filed about, so its absence below proves nothing", + ) + for rel in ( + "src/vllm/multimodal/minimax_h3_video.cpp", + "src/vllm/multimodal/ltx2_video.cpp", + ): + code = "\n".join( + dl.strip_comments_and_strings((ROOT / rel).read_text(encoding="utf-8")) + ) + found = [m.group(0) for m in dl.RE_DEVTYPE_CAST.finditer(code)] + self.assertEqual(found, [], f"{rel} casts an integer into a DeviceType") + def test_every_allowlisted_path_exists(self) -> None: # A stale allowlist entry is a silent exemption for a file that may later # be recreated with different contents. diff --git a/tests/vllm/models/test_minimax_h3_video_fold.cpp b/tests/vllm/models/test_minimax_h3_video_fold.cpp index afad82be7..ef8db37c0 100644 --- a/tests/vllm/models/test_minimax_h3_video_fold.cpp +++ b/tests/vllm/models/test_minimax_h3_video_fold.cpp @@ -39,6 +39,7 @@ #include "vllm/entrypoints/openai/video_api.h" #include "vllm/model_executor/model_loader/gguf_reader.h" #include "vllm/model_executor/models/minimax_h3.h" +#include "vllm/platforms/interface.h" // CurrentPlatform() — the seam device 1 resolves through #include "minimax_h3_video_fold_fixture.h" #include "vt/backend.h" @@ -116,14 +117,42 @@ class CountingCudaBackend final : public vt::Backend { int create_queue_calls = 0; }; +// The PLATFORM half of the same fake. #660: device 1 used to be +// `static_cast(1)`, so registering a backend alone was enough to +// fake an accelerator — the engine never asked whether this build HAS one. It +// asks now, through `CurrentPlatform()`, so a test that wants the accelerator +// path has to supply a platform as well as a backend. That is not a harness +// concession; it is the defect being visible: a build with a CUDA backend +// registered and no CUDA platform is not a build that runs on CUDA. +class FakeCudaPlatform final : public vllm::platforms::Platform { + public: + explicit FakeCudaPlatform(vt::Backend& backend) : backend_(backend) {} + vt::DeviceType device_type() const override { return vt::DeviceType::kCUDA; } + vt::Backend& backend() const override { return backend_; } + vllm::platforms::DeviceCapability get_device_capability() const override { return {}; } + std::vector supported_dtypes() const override { return {vt::DType::kBF16}; } + vllm::platforms::ResidencyPolicy residency_policy() const override { return {}; } + + private: + vt::Backend& backend_; +}; + class ScopedCudaBackendRegistration { public: explicit ScopedCudaBackendRegistration(vt::Backend* replacement) - : previous_(vt::TryGetBackend(vt::DeviceType::kCUDA)) { + : previous_(vt::TryGetBackend(vt::DeviceType::kCUDA)), + previous_platform_(vllm::platforms::HasPlatform(vt::DeviceType::kCUDA) + ? &vllm::platforms::GetPlatform(vt::DeviceType::kCUDA) + : nullptr), + platform_(*replacement) { vt::RegisterBackend(vt::DeviceType::kCUDA, replacement); + vllm::platforms::RegisterPlatform(vt::DeviceType::kCUDA, &platform_); } ~ScopedCudaBackendRegistration() { if (previous_ != nullptr) vt::RegisterBackend(vt::DeviceType::kCUDA, previous_); + if (previous_platform_ != nullptr) { + vllm::platforms::RegisterPlatform(vt::DeviceType::kCUDA, previous_platform_); + } } ScopedCudaBackendRegistration(const ScopedCudaBackendRegistration&) = delete; @@ -131,6 +160,8 @@ class ScopedCudaBackendRegistration { private: vt::Backend* previous_; + vllm::platforms::Platform* previous_platform_; + FakeCudaPlatform platform_; }; void CheckAgainstGoldens(const std::string& out_dir) { @@ -159,9 +190,34 @@ void CheckAgainstGoldens(const std::string& out_dir) { TEST_CASE("minimax_h3 video fold: ABI device selectors map through DeviceType") { CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(0) == vt::DeviceType::kCPU); - CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(1) == vt::DeviceType::kCUDA); CHECK_THROWS(vllm::multimodal::MiniMaxH3VideoDeviceType(-1)); CHECK_THROWS(vllm::multimodal::MiniMaxH3VideoDeviceType(2)); + + // Selector 1 is RESOLVED through the platform seam, not cast from the integer + // (#660: `static_cast(1)` was kCUDA only because kCUDA happens + // to be enum value 1). So the answer depends on what this build registered, + // and this case asserts BOTH arms rather than skipping either — a skip here + // would leave the CPU-only build, which is the one the defect hid on, + // unmeasured. + const vt::DeviceType accelerator = vllm::platforms::CurrentPlatform().device_type(); + const bool have_accelerator = + accelerator != vt::DeviceType::kCPU && vt::TryGetBackend(accelerator) != nullptr; + if (have_accelerator) { + // On the CUDA box this is byte-for-byte the old answer. + CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(1) == accelerator); + } else { + // The assertion the cast could never make: on a CPU-only build, device 1 is + // REFUSED by name instead of returning kCUDA and failing one step later + // inside `vt::GetBackend(kCUDA)`. + try { + (void)vllm::multimodal::MiniMaxH3VideoDeviceType(1); + FAIL("device 1 must be refused when no accelerator backend is registered"); + } catch (const std::exception& e) { + const std::string msg = e.what(); + INFO(msg); + CHECK(msg.find("no accelerator backend is registered") != std::string::npos); + } + } } TEST_CASE("minimax_h3 video fold: CUDA load creates exactly one queue") { diff --git a/tests/vllm/multimodal/test_diffusion_device_seam.cpp b/tests/vllm/multimodal/test_diffusion_device_seam.cpp new file mode 100644 index 000000000..70be8d386 --- /dev/null +++ b/tests/vllm/multimodal/test_diffusion_device_seam.cpp @@ -0,0 +1,204 @@ +// #659 / #660 — the diffusion lane's device seam, held against a PARTIAL +// backend. +// +// The two lanes ask "where does this engine run" and the answer has three parts: +// is there an accelerator, is a backend registered for it, and can that backend +// actually run THIS model. Only the first two were being asked. The third, +// `Platform::supports_model_architecture` (include/vllm/platforms/interface.h), +// exists precisely so a partial backend can decline BY NAME — Metal registers 15 +// of 75 ops and Tenstorrent a comparable slice, and both name exactly two +// architectures (src/vllm/platforms/metal.cpp:70, +// src/vllm/platforms/tenstorrent.cpp:52). On such a build a `device = 1` +// diffusion load WAS refused by name and became a queue bind that dies later +// inside a kernel with a shape error. +// +// CUDA cannot see any of this: `supports_model_architecture` defaults to true, +// so on the box that runs the gates all three questions have always passed. That +// is exactly why the guard needs a platform this test brings with it. +// +// A SEPARATE EXECUTABLE, for the reason tests/vllm/entrypoints/ +// test_device_selection.cpp is one: it registers a fake platform and backend in +// the otherwise-unused XPU slot AND over the CUDA slot, so `CurrentPlatform()` +// resolves to the fake whether or not this build has a real CUDA platform. +// Process isolation keeps those global registries out of every other suite. +// +// WHY THE FAKE REPORTS kXPU. `static_cast(1)` is `kCUDA` +// (include/vt/device.h: kCPU = 0, kCUDA = 1). `kXPU` is 4. So an assertion that +// device 1 resolves to kXPU is one the old integer cast could not satisfy under +// any circumstances — it is the seam being asked, not a constant being returned. +#include + +#include +#include +#include +#include +#include +#include + +#include "vllm/multimodal/ltx2_video.h" +#include "vllm/multimodal/minimax_h3_video.h" +#include "vllm/platforms/interface.h" +#include "vt/backend.h" +#include "vt/device.h" + +namespace { + +class FakeXpuBackend final : public vt::Backend { + public: + void* Alloc(size_t bytes) override { return std::malloc(bytes == 0 ? 1 : bytes); } + void Free(void* p) override { std::free(p); } + void Memset(vt::Queue&, void* p, int value, size_t bytes) override { + std::memset(p, value, bytes); + } + void Copy(vt::Queue&, void* dst, const void* src, size_t bytes) override { + std::memcpy(dst, src, bytes); + } + vt::Queue CreateQueue() override { + return vt::Queue{vt::Device{vt::DeviceType::kXPU, 0}, nullptr}; + } + bool UnifiedMemory() const override { return true; } +}; + +// A PARTIAL backend, shaped exactly like MetalPlatform and TenstorrentPlatform: +// a real accelerator with a registered backend that names the architectures +// whose kernels it has, and declines everything else. `accepts_everything` flips +// it into a COMPLETE backend so the same process can prove the guard does not +// refuse a configuration that works. +class PartialXpuPlatform final : public vllm::platforms::Platform { + public: + explicit PartialXpuPlatform(FakeXpuBackend& backend) : backend_(backend) {} + + vt::DeviceType device_type() const override { return vt::DeviceType::kXPU; } + vt::Backend& backend() const override { return backend_; } + vllm::platforms::DeviceCapability get_device_capability() const override { return {}; } + std::vector supported_dtypes() const override { return {vt::DType::kBF16}; } + vllm::platforms::ResidencyPolicy residency_policy() const override { return {}; } + + bool supports_model_architecture(std::string_view architecture) const override { + if (accepts_everything) return true; + return architecture == "OPTForCausalLM"; + } + + bool accepts_everything = false; + + private: + FakeXpuBackend& backend_; +}; + +FakeXpuBackend& Backend() { + static FakeXpuBackend backend; + return backend; +} + +PartialXpuPlatform& Platform() { + static PartialXpuPlatform platform(Backend()); + return platform; +} + +// Registered into the CUDA slot as well as its own: `CurrentPlatform()` walks +// {kCUDA, kROCM, kXPU, …} and returns the first REGISTERED entry +// (src/vllm/platforms/platform.cpp), so without this a CUDA build would resolve +// to the real CUDA platform and the guard would never be exercised. +void RegisterPartialAccelerator(bool accepts_everything) { + vt::RegisterBackend(vt::DeviceType::kXPU, &Backend()); + vllm::platforms::RegisterPlatform(vt::DeviceType::kXPU, &Platform()); + vllm::platforms::RegisterPlatform(vt::DeviceType::kCUDA, &Platform()); + Platform().accepts_everything = accepts_everything; +} + +// A path that does not exist. If the capability guard were reverted, the LTX-2.5 +// load would sail past it and die opening this instead — a DIFFERENT message, +// which is what makes the assertions below fail for the intended reason rather +// than by accident. +vllm::multimodal::VideoModelParams DeviceOneParams() { + vllm::multimodal::VideoModelParams mp; + mp.family = vllm::multimodal::kLtx2VideoFamily; + mp.dit_path = "/nonexistent/ltx2-dit-that-is-never-opened.safetensors"; + mp.device = 1; + return mp; +} + +std::string LoadError(const vllm::multimodal::VideoModelParams& mp) { + try { + (void)vllm::multimodal::Ltx2VideoEngine::Load(mp); + } catch (const std::exception& e) { + return e.what(); + } + return ""; +} + +} // namespace + +TEST_CASE("ltx2 video: a platform that DECLINES the architecture refuses device 1 by name") { + RegisterPartialAccelerator(/*accepts_everything=*/false); + + // Both of the questions this file already asked PASS here: the platform is an + // accelerator, and a backend is registered for it. Without the third question + // the load proceeds into the DiT. + REQUIRE(vllm::platforms::CurrentPlatform().device_type() == vt::DeviceType::kXPU); + REQUIRE(vt::TryGetBackend(vt::DeviceType::kXPU) != nullptr); + REQUIRE_FALSE(vllm::platforms::CurrentPlatform().supports_model_architecture( + vllm::multimodal::kLtx2VideoFamily)); + + const std::string msg = LoadError(DeviceOneParams()); + INFO(msg); + REQUIRE_FALSE(msg.empty()); + // BY NAME: the platform, the architecture, and the fact that the backend + // declined rather than that something was malformed. + CHECK(msg.find("xpu") != std::string::npos); + CHECK(msg.find(vllm::multimodal::kLtx2VideoFamily) != std::string::npos); + CHECK(msg.find("DECLINES") != std::string::npos); + CHECK(msg.find("supports_model_architecture") != std::string::npos); + // And it must not be the OTHER refusal: a backend IS registered here, so + // blaming a missing one would be a wrong diagnosis that reads as a right one. + CHECK(msg.find("no accelerator backend is registered") == std::string::npos); + // Nor the shape of failure the guard exists to prevent: opening the DiT. + CHECK(msg.find("/nonexistent/") == std::string::npos); +} + +TEST_CASE("ltx2 video: a COMPLETE backend is not refused by the capability guard") { + // The risk the guard carries is refusing a configuration that works. A + // platform that accepts the architecture must get past this clause and fail + // only on the thing that is genuinely wrong — the missing checkpoint. + RegisterPartialAccelerator(/*accepts_everything=*/true); + + const std::string msg = LoadError(DeviceOneParams()); + INFO(msg); + REQUIRE_FALSE(msg.empty()); + CHECK(msg.find("DECLINES") == std::string::npos); + CHECK(msg.find("supports_model_architecture") == std::string::npos); +} + +TEST_CASE("minimax_h3 video: device 1 RESOLVES through the seam, it is not enum value 1") { + RegisterPartialAccelerator(/*accepts_everything=*/true); + + // kXPU is 4. The integer cast this replaced returned kCUDA (1) for any build, + // any platform, forever; it could not produce this answer. + CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(1) == vt::DeviceType::kXPU); + CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(1) != vt::DeviceType::kCUDA); + + // The rest of the public contract is untouched by the change. + CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(0) == vt::DeviceType::kCPU); + CHECK_THROWS(vllm::multimodal::MiniMaxH3VideoDeviceType(-1)); + CHECK_THROWS(vllm::multimodal::MiniMaxH3VideoDeviceType(2)); +} + +TEST_CASE("minimax_h3 video: a platform that DECLINES the architecture refuses device 1") { + RegisterPartialAccelerator(/*accepts_everything=*/false); + + std::string msg; + try { + (void)vllm::multimodal::MiniMaxH3VideoDeviceType(1); + FAIL("device 1 must be refused when the resolved platform declines the architecture"); + } catch (const std::exception& e) { + msg = e.what(); + } + INFO(msg); + CHECK(msg.find("xpu") != std::string::npos); + CHECK(msg.find(vllm::multimodal::kMiniMaxH3VideoFamily) != std::string::npos); + CHECK(msg.find("DECLINES") != std::string::npos); + CHECK(msg.find("no accelerator backend is registered") == std::string::npos); + + // 0 still resolves, because the CPU asks none of these questions. + CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(0) == vt::DeviceType::kCPU); +} From d7d64804d6518936533258500235741c613f5ce8 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Fri, 14 Aug 2026 13:07:57 +0000 Subject: [PATCH 03/12] fix(LTX25-DEVICE-SEAM-SIBLING): the bucket missed the purest spelling, and the test that ruled it out could not see it (#659, #660) Review repair for PR #671 at 094ac9e4. Four findings; the core work the review reproduced (branch orthogonality, the by-hash baseline, both source mutations) is untouched. F1 (High). THE BUCKET MISSED THE PUREST FORM OF THE DEFECT. (vt::DeviceType)d -> dev_cast 1, RED (what M29 asserted) (vt::DeviceType)1 -> dev_cast 0, GREEN <-- MISSED (vt::DeviceType)-1 -> dev_cast 0, GREEN <-- MISSED The C-style alternative's trailing lookahead admitted only [A-Za-z_(], and `1` is a digit. Naming a device by its literal enum value is exactly what this bucket exists to police, so the one form it could not see was the one form it was for. What makes it High rather than a nit: the spec and the PR body both assert the M29 discriminators cost nothing, and they cost this - while M29's own "did not cost the real thing" assertion used an IDENTIFIER operand, so the test could not detect the gap it was written to rule out. A guard that certifies itself is the exact disease this row exists to fix, reproduced inside the row's own instrument. M29 now pins the literal FIRST, then the signed literal, then the identifier, all in the same file as the false-positive fixture. Re-derived rather than taken from the review: over src/vllm + include/vllm, 720 files, positive control \bDeviceType\b = 162 matches in the same command, admitting `0-9+-` adds ZERO hits and loses none. The only raw match under either pattern is platform.cpp:85's allowlisted registry-walk inverse. F3 (Medium). THE DOCSTRING UNDER-REPORTED, AND THREE CAST KEYWORDS WERE DEAD. Four more plain spellings read GREEN in SCANNED files, each compile-checked as legal C++ before being called a blind spot, and each now closed: static_cast<::vt::DeviceType>(raw) global-scope qualification static_cast(raw) elaborated-type-specifier vt::DeviceType dt{raw}; list-init in a DECLARATION (M23 caught only the unnamed temporary) *reinterpret_cast(&raw) pointer punning The pointer target is what makes the keyword list honest. reinterpret_cast, const_cast and dynamic_cast TO a scoped enum are ill-formed - compile-checked, all three rejected, with static_cast as a live control that compiles - so before this change those three keywords could only ever have matched code that does not build. They created an appearance of coverage the pattern did not have. Matching a pointer target makes them live instead of dropping them. MEASURED, NOT ASSUMED. My first cut of the declaration form accepted `(` as well as `{` and introduced THREE false positives on the real tree - every function DEFINITION returning DeviceType, including MiniMaxH3VideoDeviceType and ResolveExplicitDeviceType. The declarator form takes `{` only, and that costs nothing real: `vt::DeviceType dt(raw)` is ill-formed, there being no implicit int -> scoped-enum conversion. M34 pins that negative. Final pattern: zero new hits, zero lost hits. Residual blind spots STILL NAMED in the docstring, per the row's own standard: type aliases, MACROS and template parameters resolving to DeviceType; bit_cast/memcpy/union punning (the pointer-cast spelling is now caught, these three are not, because no spelling of the target type appears at the site); the unscanned src/vt/ leg; and that nothing type-checks the operand - which is also why a pointer target counts, since const_cast(p) removes const rather than converting, and buys its exemption the same way. F2 (Medium). THE ROW ADDED A TEST THAT FAILS ON THE BUILD CLASS #659 SERVES. test_minimax_h3_video_fold.cpp's `have_accelerator` asked two of the three questions (device_type() != kCPU && TryGetBackend() != nullptr). On Metal or Tenstorrent both are true, so the test took the `== accelerator` arm while the source CORRECTLY refused, and the refusal surfaced as an uncaught exception. Proven by mutation, not deduced - a DecliningPlatform registered in the XPU slot, production code untouched: before :247 ERROR: CHECK( MiniMaxH3VideoDeviceType(1) == accelerator ) THREW "... DECLINES the architecture 'minimax-h3' ..." 6 cases | 5 passed | 1 failed | 137 assertions | FOLD_EXIT=1 after 6 cases | 6 passed | 0 failed | 137 assertions | FOLD_EXIT=0 So the row shipped a false RED on precisely the partial-backend build it was written to protect, invisible on the CPU and CUDA boxes that run the gates - which is this row's own thesis about #659, turned against its own test. The predicate is now three-way and asserts WHICH refusal, because a right refusal for a wrong reason is a wrong diagnosis that reads as a right one. The review also ruled on the earlier brief-vs-spec disagreement, and the spec won: keeping fold:162's `== kCUDA` would have required the source to still return kCUDA on a CPU-only build - the defect itself. The build-conditional form stays; F2 was a bug in the predicate, not in that decision. F4 (Low). AN ASSERTION THAT ONLY CHECKED ABSENCES. The "COMPLETE backend is not refused" case checked that DECLINES and supports_model_architecture were absent and never positively asserted the load reached the missing checkpoint, so it would pass on any OTHER wrong failure. Mutation (an unrelated Fail() planted after the capability clause): the two absence checks BOTH passed and only the new assertion caught it - 4 cases | 3 passed | 1 failed, 24 | 23 passed | 1 failed, the failure at :178 on the /nonexistent/ path. Restored by sha256. F6. THE DECLINE CONSEQUENCE INVERTS THE CITED PRECEDENT, recorded in the spec, no code change. model_loader.cpp:97's capability question lives on the kAuto path, whose answer to a decline is to fall through to :103 and serve on CPU; metal.cpp:65-69 states that policy in as many words. Both diffusion lanes THROW. That is correct - device = 1 is an explicit accelerator request, and :71-72 already says an explicit accelerator must fail loudly rather than silently serve on CPU - but the PR described the change as mirroring the precedent, and it mirrors the QUESTION while inverting the CONSEQUENCE. Both halves are the seam's own, from two different paths of it. Also recorded: vulkan.cpp does NOT override supports_model_architecture, so a partial Vulkan build still binds and dies. Residual, not this row's to fix. GATE. CPU build, no CUDA. BUILD_EXIT=0 throughout, zero "No space left"/"BFD assertion" in any build log. tests/scripts/test_device_leakage.py 37 -> 42 tests, OK, exit 0. RED-first against the HEAD checker: 5 failures (M29 strengthened, M30-M33). M34 is green both before and after by construction - it guards a false positive the change could have introduced, not one it fixes. check-device-leakage on the real tree: dev_cast=0, total 32 == baseline 32. scripts/device-leakage-baseline.json is UNTOUCHED by this commit. test_minimax_h3_video_fold 6 / 137 -> 6 / 137, UNCHANGED. test_diffusion_device_seam 4 / 23 -> 4 / 24, +1 = the F4 assertion, exactly. Both source mutations restored and verified by sha256, not by git status. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [ClaudeCode] --- .agents/roadmap_v1.md | 4 +- .agents/specs/ltx25-device-seam-sibling.md | 73 +++++++++++++- scripts/check-device-leakage.py | 59 +++++++++--- tests/scripts/test_device_leakage.py | 94 ++++++++++++++++++- .../models/test_minimax_h3_video_fold.cpp | 34 +++++-- .../multimodal/test_diffusion_device_seam.cpp | 10 ++ 6 files changed, 245 insertions(+), 29 deletions(-) diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index eb820f054..762342795 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -194,8 +194,8 @@ issue is not yet placed. Keyed record: update in place, never append. | [#607](https://github.com/mudler/vllm.cpp/issues/607) | `ENG-MM-INPUT-PIPELINE` | **Premise corrected 2026-08-13, see `specs/multimodal-track.md` §1.5.** Not "skip the vision encoder": `--language-model-only` sets every modality limit to **0** (`multimodal.py:78,321-327`) and is sugar over `--limit-mm-per-prompt`. Two consequences follow, and we have NEITHER — upstream then **refuses every multimodal request** (`processing/context.py:409-428` raises "At most 0 image(s) may be provided in one prompt", from `:461` and `chat_utils.py:662`), and builds the tower uninitialised (`interfaces.py:293`). This is a PORT of the limits mechanism (L1-L4), not the exposure of a boolean. **L1 LANDED 2026-08-13:** `vllm::MultiModalConfig` + `GetLimitPerPrompt` (`include/vllm/config/multimodal.h`) and the refusal it carries (`include/vllm/multimodal/processing/context.h`) are in, unit-gated, with NO serve surface — nothing constructs the config on a live request yet, so the two call sites still validate nothing. L2 (the flags + C-ABI field + wiring those call sites), L3 (tower skip) and L4 (kernel gate) are still owed. 43 of 157 recipes pass the flag and we abort on it. The flag appears in this repo only in `tools/bench/run_serve_low.py`, which passes it to the ORACLE — a grep reads as coverage and is not | feature | | [#651](https://github.com/mudler/vllm.cpp/issues/651) | — | `test_agent_record`'s MODEL-ratchet docstring is two contradictory paragraphs spliced together, and the surviving half records a pin transition that never happened | bug | | [#652](https://github.com/mudler/vllm.cpp/issues/652) | — | `model-matrix.md` prose counters drifted: LTX-2.5 reached the rows and the CI-enforced rollup but none of the five sentences that count them | bug | -| [#659](https://github.com/mudler/vllm.cpp/issues/659) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing). Guard added at the same site; `tests/vllm/multimodal/test_diffusion_device_seam.cpp` holds it against an injected declining platform | bug | -| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553). New `dev_cast` bucket, target-type anchored, RED for 5 spellings not in the tree (M20-M24 + M29); H3 resolves through the seam, so `dev_cast` is 0 and the baseline total is unchanged at 32 | bug | +| [#659](https://github.com/mudler/vllm.cpp/issues/659) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing). Guard added at the same site; `tests/vllm/multimodal/test_diffusion_device_seam.cpp` holds it against an injected declining platform, and `test_minimax_h3_video_fold.cpp`'s own predicate asks all THREE questions — asking two made it red on a correct refusal, i.e. on exactly the partial-backend build this issue exists to serve | bug | +| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553). New `dev_cast` bucket, target-type anchored, RED for TEN spellings not in the tree (M20-M24, M29-M33) — including the literal operand `(vt::DeviceType)1`, which the first cut MISSED while catching `(vt::DeviceType)d`, and whose own discriminator test used an identifier and so could not see the gap it was written to rule out. `::`-qualified, `enum`-elaborated, named list-init and pointer-pun targets are closed too, each measured at zero new hits over the real tree; M34 pins the function-definition negative | bug | | [#608](https://github.com/mudler/vllm.cpp/issues/608) | `TOOLS-PARSER-BREADTH` | **W0 (record backfill) landed 2026-08-13; row `INVENTORIED` → `PARTIAL`. W1 then shipped `inkling`, taking the registry to 42 names / 38 families.** Re-derived from the two REGISTRIES rather than from recipe usage: **five** `--tool-call-parser` names are upstream-only at the pin — `openai`, `inkling`, `minimax_m3` (W1, recipe demand) and `cohere_command3`, `cohere_command4` (W2, ZERO recipe uses, so usage-driven audits miss them). **Only `inkling` was PORTABLE from vLLM source, and it LANDED** (a `ParserEngineToolAdapter` over the already-ported Inkling engine; the gap was the registry face, not the grammar). Of the four left: `minimax_m3` is backed by the Rust crate; `openai` is a stub delegating to `vllm/parser/harmony.py`, which IS vLLM source but wraps the out-of-tree `openai_harmony` package (the SGLang secondary-oracle check was run and REFUSED — vLLM implements this path, so the rule does not admit a secondary); both Cohere names are shims over the out-of-tree `cohere_melody` package. So W1-remaining/W2 each owe a recorded decision before code rather than a text port. `nemotron_json`, `kimi_k3` and `ling3` are in NEITHER registry and arrive with the pin advance, not here. W3 ports upstream's shared `ToolParserTestConfig` harness. The earlier "six missing" framing was usage-derived: it listed `nemotron_json` as portable (it is not registered at the pin) and missed both Cohere entries | feature | | [#647](https://github.com/mudler/vllm.cpp/issues/647) | — | Oracle policy had no fallback and no pin concept: five upstreams beyond vLLM are already compared against (vLLM-Omni, SGLang, llama.cpp, `transformers`, tt-forge) with their pins scattered across individual specs or absent entirely. AGENTS.md now admits a named secondary oracle where vLLM implements nothing, `.agents/oracles/.md` pins each one file-per-oracle, and `check-oracle-pins.py` enforces both directions. The gateability debts for `sglang`, `diffusers` and `tt-forge` stay open on this issue | feature | | [#672](https://github.com/mudler/vllm.cpp/issues/672) | `MODEL-MUSIC-minimax-music3-mini-max-music3-for-conditional-generation` | MiniMax-Music3: port the text-to-music lane — ~8.6B Qwen3 global LLM (our LANDED architecture at vocab 200000) + 8-codebook RVQ depth decoder + 2.4B fp32 flow-matching DiT + DAC Flow-VAE. Our first music-generating model, and the first row whose primary oracle is not vLLM: absent from the pin, from vLLM `main` and from `vllm-omni`, so it gates against the OPEN diffusers PR #14456 with SGLang-Omni as the e2e cross-check, under the fallback rule added in #647 | feature | diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md index 80b8615be..3b66a14e1 100644 --- a/.agents/specs/ltx25-device-seam-sibling.md +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -234,10 +234,75 @@ not be followed by a declarator suffix) and M29 pins both, together with the proof that the discriminators did not cost the real detection. **Residual blind spots are recorded in the checker's own docstring**, per §3's -instruction: type aliases and template parameters that resolve to `DeviceType`, -`bit_cast`/`memcpy`/union punning, conversions inside the unscanned `src/vt/` -leg, and the fact that nothing type-checks the operand. The bucket flags every -cast *to* `DeviceType` and relies on `DSR-ALLOW` for the legitimate ones. +instruction: type aliases, macros and template parameters that resolve to +`DeviceType`, `bit_cast`/`memcpy`/union punning, conversions inside the unscanned +`src/vt/` leg, and the fact that nothing type-checks the operand. The bucket +flags every cast *to* `DeviceType` and relies on `DSR-ALLOW` for the legitimate +ones. + +## Findings from review (PR #671, head `094ac9e4`) + +**The bucket missed the purest form of the defect, and the test that ruled that +out could not see it.** `(vt::DeviceType)d` scored 1; `(vt::DeviceType)1` scored +**0**, because the C-style alternative's trailing lookahead admitted only +`[A-Za-z_(]` and `1` is a digit. Naming a device by its literal enum value is +precisely what this bucket exists to police. Worse, M29's "the discriminators did +not cost the real detection" assertion used an *identifier* operand, so the test +could not detect the gap it was written to rule out — a guard that certifies +itself, which is the disease this row exists to fix, reproduced inside the row's +own instrument. M29 now pins the literal and signed-literal operands first. + +**Four more plain spellings read GREEN in scanned files** and are closed rather +than documented, each measured at zero new hits over `src/vllm` + `include/vllm`: +global-scope qualification `static_cast<::vt::DeviceType>`, the +elaborated-type-specifier `static_cast`, direct-list-init in +a *declaration* (`vt::DeviceType dt{raw}` — M23 caught only the unnamed +temporary), and pointer punning `*reinterpret_cast(&raw)`. + +**The pointer form is what makes three of the four cast keywords real.** +`reinterpret_cast`, `const_cast` and `dynamic_cast` to a scoped enum are +ill-formed — compile-checked, all three rejected, with `static_cast` as a live +control that compiles — so before this change those keywords could only ever +match code that does not build, creating an appearance of coverage the pattern +did not have. Matching a pointer target makes them live. + +**The declaration form takes `{` and never `(`, and that costs nothing.** +`vt::DeviceType dt(raw)` is ill-formed (no implicit int→scoped-enum conversion), +so excluding it loses no real spelling, whereas admitting `(` matched every +function *definition* whose return type is `DeviceType` — measured, 3 false +positives including `MiniMaxH3VideoDeviceType` and `ResolveExplicitDeviceType`. +M34 pins that negative. + +**The fold test asked two of the three questions and so red on the build class +#659 exists to serve.** `test_minimax_h3_video_fold.cpp`'s `have_accelerator` +predicate tested only `device_type() != kCPU && TryGetBackend() != nullptr`. On +Metal or Tenstorrent both are true, so the test took the `== accelerator` arm +while the source *correctly* refused, and the refusal surfaced as an uncaught +exception — a false RED, invisible on the CPU and CUDA boxes that run the gates, +which is this row's own thesis about #659 turned against its own test. The +predicate is now three-way and asserts *which* refusal, since a right refusal for +a wrong reason is a wrong diagnosis that reads as a right one. + +**The decline CONSEQUENCE inverts the cited precedent, deliberately.** The PR +described the change as mirroring `model_loader.cpp:97`, and it mirrors that +site's *question* while inverting its *answer*. `:97`'s capability test lives on +the `kAuto` path, whose response to a decline is to fall through to `:103` and +**serve on CPU**; `metal.cpp:65-69` states that policy in as many words ("falls +back to the CPU reference … and runs correctly, just slowly — which is strictly +better than dying inside a kernel bind"). Both diffusion lanes instead **throw**. +That is correct, but it is the *explicit-device* path's polarity, not the +`kAuto` path's: `device = 1` is an explicit accelerator request, and +`model_loader.cpp:71-72` already says of that path "an explicit accelerator whose +queue cannot be created must FAIL the load loudly, never silently serve on CPU" — +the same argument `ltx2_video.cpp:545-548` makes for refusing rather than serving +the CPU forward behind an accelerator handle. So the lanes mirror the capability +question from one path and the failure polarity from the other, and both halves +are the seam's own. + +**Residual, not this row's to fix:** `vulkan.cpp` does not override +`supports_model_architecture`, so it inherits `interface.h:263`'s default `true` +and a partial Vulkan build still binds and dies inside a kernel. Only +`metal.cpp:70` and `tenstorrent.cpp:52` narrow the claim. ## Now diff --git a/scripts/check-device-leakage.py b/scripts/check-device-leakage.py index 27795ac68..51c256e1c 100644 --- a/scripts/check-device-leakage.py +++ b/scripts/check-device-leakage.py @@ -51,22 +51,37 @@ direction (`static_cast(type)`) is the SAFE one and is deliberately not counted: it is how the seam itself indexes its registry. +WHAT `dev_cast` DOES SEE, in every spelling of the one conversion: the four cast +keywords and the C-style, functional, brace and DECLARATION forms; the type +written as `DeviceType`, `vt::DeviceType`, `::vt::DeviceType` or +`enum vt::DeviceType`; an operand that is an identifier, a literal (`)1`), a +signed literal (`)-1`) or a parenthesised expression; a target that is a POINTER, +which is how `reinterpret_cast` puns an integer's bytes onto a DeviceType; and a +cast clang-format has wrapped across two lines. + WHAT `dev_cast` STILL CANNOT SEE, stated here because a checker's message is the authority on what it enforces and an instrument that hides its blind spot returns a false pass: * a cast whose target is a type ALIAS (`using DT = vt::DeviceType;` then - `static_cast
(x)`), or a template parameter that resolves to DeviceType; - * `std::bit_cast`, `memcpy` or a union punning an integer onto a DeviceType; + `static_cast
(x)`), a macro that expands to the type name, or a template + parameter that resolves to DeviceType; + * `std::bit_cast`, `memcpy` or a union punning an integer onto a DeviceType. + The POINTER-cast spelling of the same pun IS caught; these three are not, + because no spelling of the target type appears at the conversion site; * a conversion that happens inside `src/vt/` and is merely CALLED from the shared layer — `src/vt/` is a device leg and is not scanned at all; * whether the operand really is an integer. Nothing here type-checks; the bucket flags every cast TO DeviceType and relies on `// DSR-ALLOW()` - for the legitimate ones (a wire-format decode is the expected case). + for the legitimate ones (a wire-format decode is the expected case). This is + also why a pointer target counts: `const_cast(p)` removes const + rather than converting an integer, and buys its exemption the same way. Those are gaps in a TEXT checker, not gaps that were traded away for -convenience. tests/scripts/test_device_leakage.py M20-M28 pin what it does -catch, each spelling asserted on its own. +convenience. tests/scripts/test_device_leakage.py M20-M33 pin what it does +catch, each spelling asserted on its own — including, in M29, the literal +operand, because a discriminator tested only on the case it was tuned for is a +guard that certifies itself. HOW THE RATCHET WORKS. `scripts/device-leakage-baseline.json` holds the accepted per-bucket counts. Any bucket ABOVE its baseline fails. Any bucket BELOW its @@ -117,8 +132,16 @@ # spelling of the same conversion, all anchored on the TARGET TYPE: # # 1. a named cast static_cast(x) / / … -# 2. a C-style cast (vt::DeviceType)x -# 3. a functional cast vt::DeviceType(x) and vt::DeviceType{x} +# and the POINTER form, *reinterpret_cast(&x) +# 2. a C-style cast (vt::DeviceType)x and (vt::DeviceType)1 +# 3. a functional cast vt::DeviceType(x) / vt::DeviceType{x}, and the same +# conversion spelled as a DECLARATION, `DeviceType d{x}` +# +# `_DEVTYPE_QUAL` absorbs the ways the SAME type can be written before its name: +# `vt::DeviceType`, a global-scope-qualified `::vt::DeviceType`, and an +# elaborated-type-specifier `enum vt::DeviceType`. All three name one type, so a +# pattern that recognised only the first would be a spelling grep again — which +# is the exact defect (#660) this bucket exists to answer. # # `\b` after the type name keeps `DeviceTypeName(t)` out. The lookbehind on (3) # keeps `X::DeviceType(` and `foo.DeviceType(` and `vector(` out, so @@ -134,13 +157,27 @@ # be glued to an identifier (that is a call or a declarator), except after the # keywords that legitimately precede a parenthesised expression; and what follows # must not be a declarator suffix. +# +# (2)'s trailing class admits DIGITS and a leading SIGN, not just identifiers. +# `(vt::DeviceType)1` — the device named by its literal enum value — is the +# PUREST form of what this bucket polices, and an identifier-only lookahead +# missed exactly it while catching `(vt::DeviceType)d`. Measured over +# `src/vllm` + `include/vllm`: admitting `0-9+-` adds ZERO hits. +# +# (3)'s DECLARATION form takes `{` only. `DeviceType d{x}` is a real conversion +# (C++17 permits list-initialising a scoped enum with a fixed underlying type), +# but `DeviceType d(x)` is ILL-FORMED — there is no implicit int→scoped-enum +# conversion — so nothing is lost by excluding it, while allowing `(` would match +# every FUNCTION DEFINITION whose return type is DeviceType (measured: 3 false +# positives, e.g. `vt::DeviceType MiniMaxH3VideoDeviceType(`). +_DEVTYPE_QUAL = r"(?:enum\s+)?(?:::\s*)?(?:vt\s*::\s*)?" RE_DEVTYPE_CAST = re.compile( r"(?:static_cast|reinterpret_cast|const_cast|dynamic_cast)\s*<\s*" - r"(?:const\s+)?(?:vt\s*::\s*)?DeviceType\b\s*>" + r"(?:const\s+)?" + _DEVTYPE_QUAL + r"DeviceType\b\s*\*?\s*>" r"|(?:(?])(?:vt\s*::\s*)?DeviceType\b\s*[({]\s*(?![)}])" + r"\(\s*(?:const\s+)?" + _DEVTYPE_QUAL + r"DeviceType\b\s*\)\s*" + r"(?!(?:const|volatile|noexcept|override|final|try)\b)(?=[A-Za-z_(0-9+\-])" + r"|(?])" + _DEVTYPE_QUAL + r"DeviceType\b\s*(?:\w+\s*\{|[({])\s*(?![)}])" ) RE_CUDA_INCLUDE = re.compile(r'^\s*#\s*include\s*[<"](?:vt/cuda/|cuda_runtime)') RE_PP_IF = re.compile(r"^\s*#\s*(ifdef|ifndef|if)\b(.*)$") diff --git a/tests/scripts/test_device_leakage.py b/tests/scripts/test_device_leakage.py index 232b6ff1c..f6ad4b8a1 100644 --- a/tests/scripts/test_device_leakage.py +++ b/tests/scripts/test_device_leakage.py @@ -20,7 +20,7 @@ 4. The escape hatch is real, bounded and LOUD (M13-M15). 5. The `dev_cast` bucket is derived from the PROPERTY — an integer becoming a `vt::DeviceType` outside the platform seam — and not from the one spelling - that happened to be in the tree when it was written (M20-M28, #660). + that happened to be in the tree when it was written (M20-M34, #660). python3 tests/scripts/test_device_leakage.py @@ -499,15 +499,101 @@ def test_M29_a_parameter_declaration_is_not_a_c_style_cast(self) -> None: "using Fn = void(vt::DeviceType);\n", ) self.assertEqual(self.tree.scan().counts["dev_cast"], 0) - # …and the discriminator must not have cost the real thing: the same file - # with an actual C-style cast still fails. + # …and the discriminator must not have cost the real thing. The operand + # asserted FIRST is a LITERAL, because that is the form the discriminator + # actually did cost: `(vt::DeviceType)1` — the device named by its raw + # enum value — is the PUREST case this bucket exists to police, and an + # identifier-only trailing class let it through while still catching + # `(vt::DeviceType)d`. A discriminator exercised only on the case it was + # tuned for is a guard that certifies itself, which is the disease this + # row is fixing; so both operand kinds are pinned here. self.tree.append( "src/vllm/model_executor/models/toy.cpp", - "vt::DeviceType R(int d) { return (vt::DeviceType)d; }\n", + "vt::DeviceType RLit(int d) { return (vt::DeviceType)1; }\n", ) rc, _out, err = self.tree.run() self.assertEqual(rc, 1) self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + # A SIGNED literal is the same conversion with one more character. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType RNeg(int d) { return (vt::DeviceType)-1; }\n", + ) + rc, _out, err = self.tree.run() + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 2 > baseline 0") + # And the identifier operand, in the SAME file as the false-positive + # fixture, so the two discriminators are shown not to have cost it. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType RId(int d) { return (vt::DeviceType)d; }\n", + ) + rc, _out, err = self.tree.run() + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 3 > baseline 0") + + def test_M30_a_global_scope_qualified_target_fails(self) -> None: + # `::vt::DeviceType` and `vt::DeviceType` name ONE type. A pattern that + # recognised only the second would be the token grep again, one + # qualification later. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) {\n" + " return static_cast<::vt::DeviceType>(d);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M31_an_elaborated_type_specifier_target_fails(self) -> None: + # `enum vt::DeviceType` is the same type spelled the C way. It compiles. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) {\n" + " return static_cast(d);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M32_list_initialising_a_NAMED_declaration_fails(self) -> None: + # M23 catches the UNNAMED temporary `vt::DeviceType{d}`. Giving the same + # conversion a name is the spelling a person would actually write, and it + # is the one M23 alone let through. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) {\n" + " vt::DeviceType dt{d};\n" + " return dt;\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M33_pointer_punning_onto_a_devicetype_fails(self) -> None: + # The ONE form `reinterpret_cast` can legally take: casting to a scoped + # enum directly is ill-formed, so without the pointer target the three + # non-`static_cast` keywords in the pattern are decoration — they could + # only ever match code that does not compile. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(uint8_t r) {\n" + " return *reinterpret_cast(&r);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M34_a_function_RETURNING_DeviceType_is_not_a_conversion(self) -> None: + # The false positive M32's declaration form produces if it accepts `(` as + # well as `{`. These three shapes are all real sites in this repository + # (`MiniMaxH3VideoDeviceType`, `ResolveExplicitDeviceType`), and reading a + # function definition as a cast would red the tree for declaring the very + # seam this bucket wants people to use. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType MiniMaxH3VideoDeviceType(int32_t device);\n" + "vt::DeviceType ResolveExplicitDeviceType(const Params& p) { return kCpu; }\n" + "enum vt::DeviceType Named(int d);\n" + "::vt::DeviceType AlsoNamed(int d) { return kCpu; }\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) def test_M28_dsr_allow_exempts_a_dev_cast_and_says_so_loudly(self) -> None: # The legitimate case the risk register names: a deserialization boundary diff --git a/tests/vllm/models/test_minimax_h3_video_fold.cpp b/tests/vllm/models/test_minimax_h3_video_fold.cpp index ef8db37c0..fada8d9fc 100644 --- a/tests/vllm/models/test_minimax_h3_video_fold.cpp +++ b/tests/vllm/models/test_minimax_h3_video_fold.cpp @@ -199,23 +199,41 @@ TEST_CASE("minimax_h3 video fold: ABI device selectors map through DeviceType") // and this case asserts BOTH arms rather than skipping either — a skip here // would leave the CPU-only build, which is the one the defect hid on, // unmeasured. - const vt::DeviceType accelerator = vllm::platforms::CurrentPlatform().device_type(); - const bool have_accelerator = + const vllm::platforms::Platform& platform = vllm::platforms::CurrentPlatform(); + const vt::DeviceType accelerator = platform.device_type(); + const bool have_backend = accelerator != vt::DeviceType::kCPU && vt::TryGetBackend(accelerator) != nullptr; - if (have_accelerator) { + // The THIRD question, asked here because the SOURCE asks it. A predicate that + // asks only the first two describes a different function than the one under + // test: on a PARTIAL backend (Metal, Tenstorrent) both of the first two are + // true, the source correctly refuses BY NAME, and a two-question predicate + // routes that correct refusal into the `== accelerator` arm — where it + // surfaces as an uncaught exception. That is a false RED on precisely the + // build class #659 exists to serve, and it is invisible on the CPU and CUDA + // boxes that run the gates, which is this row's own thesis about #659. + const bool accepts_architecture = + have_backend && + platform.supports_model_architecture(vllm::multimodal::kMiniMaxH3VideoFamily); + + if (accepts_architecture) { // On the CUDA box this is byte-for-byte the old answer. CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(1) == accelerator); } else { - // The assertion the cast could never make: on a CPU-only build, device 1 is - // REFUSED by name instead of returning kCUDA and failing one step later - // inside `vt::GetBackend(kCUDA)`. + // The assertion the cast could never make: device 1 is REFUSED by name + // instead of returning kCUDA and failing one step later inside + // `vt::GetBackend(kCUDA)`. WHICH refusal is itself asserted — a partial + // backend must be told it is partial, not told its backend is missing, and + // a right refusal for a wrong reason is a wrong diagnosis that reads as a + // right one. + const std::string want = + have_backend ? "DECLINES" : "no accelerator backend is registered"; try { (void)vllm::multimodal::MiniMaxH3VideoDeviceType(1); - FAIL("device 1 must be refused when no accelerator backend is registered"); + FAIL("device 1 must be refused when this build cannot honour it"); } catch (const std::exception& e) { const std::string msg = e.what(); INFO(msg); - CHECK(msg.find("no accelerator backend is registered") != std::string::npos); + CHECK(msg.find(want) != std::string::npos); } } } diff --git a/tests/vllm/multimodal/test_diffusion_device_seam.cpp b/tests/vllm/multimodal/test_diffusion_device_seam.cpp index 70be8d386..6a2446e66 100644 --- a/tests/vllm/multimodal/test_diffusion_device_seam.cpp +++ b/tests/vllm/multimodal/test_diffusion_device_seam.cpp @@ -167,6 +167,16 @@ TEST_CASE("ltx2 video: a COMPLETE backend is not refused by the capability guard REQUIRE_FALSE(msg.empty()); CHECK(msg.find("DECLINES") == std::string::npos); CHECK(msg.find("supports_model_architecture") == std::string::npos); + // POSITIVELY: the load got PAST the capability clause and failed on the thing + // that is genuinely wrong — the checkpoint that does not exist. Asserting only + // the two absences above would pass on any OTHER wrong failure (a refusal + // reworded, a throw from an earlier clause, a message that never mentions the + // path), which would leave "the guard does not refuse a working configuration" + // unproven while reading green. Case 1 at the top of this file asserts the + // mirror image of this line; the pair is what pins the guard to exactly one + // arm each. + CHECK(msg.find("/nonexistent/ltx2-dit-that-is-never-opened.safetensors") != + std::string::npos); } TEST_CASE("minimax_h3 video: device 1 RESOLVES through the seam, it is not enum value 1") { From 70d11c6b4a7a48e1fd2afb7e94f1fe6f3b3561f3 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Fri, 14 Aug 2026 18:30:22 +0000 Subject: [PATCH 04/12] fix(LTX25-DEVICE-SEAM-SIBLING): the docstring named the target type, the pattern named one sigil (#659, #660) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FOLLOWING_AGENTS_PROTOCOL Review finding F5, and the third time this row's own thesis has come back for its instrument. `dev_cast`'s docstring says the bucket is "anchored on the TARGET TYPE, not on the operand and not on one cast keyword", and lists `std::bit_cast` as unreachable "because no spelling of the target type appears at the conversion site". Both claims were false for spellings that DO write the target type at the conversion site and scored ZERO. A checker's own message is the authority on what it enforces (AGENTS.md), so an over-claiming message is a defect in the gate, not in the prose. Nine spellings, every one compile-verified legal (g++ -std=c++20 -Wall -Wextra, exit 0 — read, not assumed), each 0 before and 1 after: reinterpret_cast(raw) reference target; the standard DEFINES it as M33's pointer pun static_cast(d) east const static_cast(t) east const + reference, the form that compiles with NO warning and so survives a -Werror build (vt::DeviceType const)d east const, C-style (vt::DeviceType)*cursor the WIRE-DECODE spelling the docstring itself names as the expected DSR-ALLOW case (vt::DeviceType)~mask / )!flag the same trailing-class hole reinterpret_cast(p) `\*?` is ONE star std::bit_cast(raw) spells the target in full Three edits: the named-cast target takes cv on either side of the name and a RUN of `*`/`&`; the C-style trailing class admits `*~!`; `bit_cast` joins the cast keywords. `&` is deliberately NOT admitted after a C-style `)` — `) &` and `) &&` are ref-qualifiers on a member declarator, which is the false positive the trailing guard exists to reject, so the C-STYLE pointer pun `*(vt::DeviceType*)&x` stays blind and is now NAMED as blind with the reason that is true of it. The blind-spot list no longer shares one reason across entries it is false for. Measured, not asserted. Over 740 files in `src/vllm` + `include/vllm`, with `\bDeviceType\b` = 162 matches as a POSITIVE CONTROL in the same command: 0 new, 0 lost. Shipped hits 1, widened hits 1 — the same allowlisted `platform.cpp:85` registry-walk inverse. dev_cast stays 0, total stays 32, the baseline is untouched. M35-M40 pin it, one mutant per newly-closed spelling, each sub-spelling asserted INDIVIDUALLY (the M29 shape) because a mutant that only exercises the case the pattern was tuned for is a guard that certifies itself. RED-before is real: with the pattern reverted to its pre-repair value in-process, M35-M39 all go RED while M20, M33 and M40 stay GREEN, so a blanket failure cannot pass for five findings. M40 is the negative — pointer- and reference-returning declarations, ref- and rvalue-ref-qualified members with a SPACE before the paren, volatile members, `std::vector` — all still 0. Issues: #659, #660. Campaign: #644. Spec: .agents/specs/ltx25-device-seam-sibling.md Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code] --- scripts/check-device-leakage.py | 84 +++++++++++++------ tests/scripts/test_device_leakage.py | 121 ++++++++++++++++++++++++++- 2 files changed, 180 insertions(+), 25 deletions(-) diff --git a/scripts/check-device-leakage.py b/scripts/check-device-leakage.py index 51c256e1c..77519a72a 100644 --- a/scripts/check-device-leakage.py +++ b/scripts/check-device-leakage.py @@ -52,33 +52,46 @@ counted: it is how the seam itself indexes its registry. WHAT `dev_cast` DOES SEE, in every spelling of the one conversion: the four cast -keywords and the C-style, functional, brace and DECLARATION forms; the type -written as `DeviceType`, `vt::DeviceType`, `::vt::DeviceType` or -`enum vt::DeviceType`; an operand that is an identifier, a literal (`)1`), a -signed literal (`)-1`) or a parenthesised expression; a target that is a POINTER, -which is how `reinterpret_cast` puns an integer's bytes onto a DeviceType; and a -cast clang-format has wrapped across two lines. +keywords, `bit_cast`, and the C-style, functional, brace and DECLARATION forms; +the type written as `DeviceType`, `vt::DeviceType`, `::vt::DeviceType` or +`enum vt::DeviceType`, with `const`/`volatile` on EITHER side of the name; an +operand that is an identifier, a literal (`)1`), a signed literal (`)-1`), a +parenthesised expression or a unary expression (`)*cursor`, `)~mask`, `)!flag`); +a target that is a POINTER, a POINTER-TO-POINTER or a REFERENCE, which is how +`reinterpret_cast` puns an integer's bytes onto a DeviceType; and a cast +clang-format has wrapped across two lines. WHAT `dev_cast` STILL CANNOT SEE, stated here because a checker's message is the authority on what it enforces and an instrument that hides its blind spot -returns a false pass: +returns a false pass. Each entry states the reason that is true OF THAT ENTRY, +because a list whose shared reason is false for one member is the same false +claim in a smaller font: * a cast whose target is a type ALIAS (`using DT = vt::DeviceType;` then `static_cast
(x)`), a macro that expands to the type name, or a template - parameter that resolves to DeviceType; - * `std::bit_cast`, `memcpy` or a union punning an integer onto a DeviceType. - The POINTER-cast spelling of the same pun IS caught; these three are not, - because no spelling of the target type appears at the conversion site; + parameter that resolves to DeviceType. Reason: no spelling of the target + type appears at the conversion site; + * `memcpy` or a union punning an integer onto a DeviceType. Same reason: the + type is named at the declaration, never at the conversion. (`std::bit_cast` + DOES name it there, so it is a cast keyword above rather than an entry here, + and the `reinterpret_cast` pointer/reference puns are caught too.); + * a C-STYLE cast whose target is a pointer or reference, `*(vt::DeviceType*)&x`. + Reason: the C-style alternative must not swallow a parameter list, and its + two discriminators — the `(` not glued to an identifier, and no declarator + suffix after the `)` — cannot tell `void note (vt::DeviceType*) &` from a + pun, because both are `)` followed by `&`. The NAMED-cast spelling of the + same pun IS caught, which is why this is a narrow gap rather than the class; * a conversion that happens inside `src/vt/` and is merely CALLED from the shared layer — `src/vt/` is a device leg and is not scanned at all; * whether the operand really is an integer. Nothing here type-checks; the bucket flags every cast TO DeviceType and relies on `// DSR-ALLOW()` for the legitimate ones (a wire-format decode is the expected case). This is - also why a pointer target counts: `const_cast(p)` removes const - rather than converting an integer, and buys its exemption the same way. + also why a pointer or reference target counts: `const_cast(t)` + removes const rather than converting an integer, and buys its exemption the + same way. Those are gaps in a TEXT checker, not gaps that were traded away for -convenience. tests/scripts/test_device_leakage.py M20-M33 pin what it does +convenience. tests/scripts/test_device_leakage.py M20-M40 pin what it does catch, each spelling asserted on its own — including, in M29, the literal operand, because a discriminator tested only on the case it was tuned for is a guard that certifies itself. @@ -132,7 +145,8 @@ # spelling of the same conversion, all anchored on the TARGET TYPE: # # 1. a named cast static_cast(x) / / … -# and the POINTER form, *reinterpret_cast(&x) +# and the INDIRECT forms, *reinterpret_cast(&x) +# and reinterpret_cast(x) # 2. a C-style cast (vt::DeviceType)x and (vt::DeviceType)1 # 3. a functional cast vt::DeviceType(x) / vt::DeviceType{x}, and the same # conversion spelled as a DECLARATION, `DeviceType d{x}` @@ -158,11 +172,30 @@ # keywords that legitimately precede a parenthesised expression; and what follows # must not be a declarator suffix. # -# (2)'s trailing class admits DIGITS and a leading SIGN, not just identifiers. -# `(vt::DeviceType)1` — the device named by its literal enum value — is the -# PUREST form of what this bucket polices, and an identifier-only lookahead -# missed exactly it while catching `(vt::DeviceType)d`. Measured over -# `src/vllm` + `include/vllm`: admitting `0-9+-` adds ZERO hits. +# (2)'s trailing class admits DIGITS, a leading SIGN and the UNARY OPERATORS +# `*`, `~` and `!`, not just identifiers. `(vt::DeviceType)1` — the device named +# by its literal enum value — is the PUREST form of what this bucket polices, and +# an identifier-only lookahead missed exactly it while catching +# `(vt::DeviceType)d`; `(vt::DeviceType)*cursor` is the WIRE-DECODE spelling this +# docstring itself names as the expected `DSR-ALLOW` case, and a class that could +# not see it was refusing to police the one site it predicted. `&` is deliberately +# NOT admitted: `) &` and `) &&` are ref-qualifiers on a member declarator, which +# is the false positive the trailing guard exists to reject. Measured over +# `src/vllm` + `include/vllm`: admitting `0-9+-*~!` adds ZERO hits. +# +# (1) and (2) both take the cv-qualifiers on EITHER SIDE of the type name, and (1) +# takes any run of `*` and `&`. `vt::DeviceType const` is the same type as +# `const vt::DeviceType`, and `reinterpret_cast(raw)` is the same +# pun as `*reinterpret_cast(&raw)` — both COMPILE (checked, not +# read), and a pattern that saw west const and one star while the docstring +# claimed the target type is what it is anchored on would be asserting coverage it +# did not have. That is #660's own defect turned on the instrument, which is why +# the run of `[*&]` is a run and the cv-qualifier group is two-sided. +# +# `bit_cast` joins the four cast keywords for the same reason: `bit_cast` spells +# the target type at the conversion site, so the blind-spot note below could not +# honestly claim it as unreachable — `memcpy` and union punning still are, and +# they are the cases where no spelling of the type appears at all. # # (3)'s DECLARATION form takes `{` only. `DeviceType d{x}` is a real conversion # (C++17 permits list-initialising a scoped enum with a fixed underlying type), @@ -171,12 +204,15 @@ # every FUNCTION DEFINITION whose return type is DeviceType (measured: 3 false # positives, e.g. `vt::DeviceType MiniMaxH3VideoDeviceType(`). _DEVTYPE_QUAL = r"(?:enum\s+)?(?:::\s*)?(?:vt\s*::\s*)?" +# cv-qualifiers, which C++ permits on either side of the type name. +_CV_WEST = r"(?:(?:const|volatile)\s+)*" +_CV_EAST = r"(?:\s*(?:const|volatile)\b)*" RE_DEVTYPE_CAST = re.compile( - r"(?:static_cast|reinterpret_cast|const_cast|dynamic_cast)\s*<\s*" - r"(?:const\s+)?" + _DEVTYPE_QUAL + r"DeviceType\b\s*\*?\s*>" + r"(?:static_cast|reinterpret_cast|const_cast|dynamic_cast|bit_cast)\s*<\s*" + + _CV_WEST + _DEVTYPE_QUAL + r"DeviceType\b" + _CV_EAST + r"(?:\s*[*&])*\s*>" r"|(?:(?])" + _DEVTYPE_QUAL + r"DeviceType\b\s*(?:\w+\s*\{|[({])\s*(?![)}])" ) RE_CUDA_INCLUDE = re.compile(r'^\s*#\s*include\s*[<"](?:vt/cuda/|cuda_runtime)') diff --git a/tests/scripts/test_device_leakage.py b/tests/scripts/test_device_leakage.py index f6ad4b8a1..fe71293df 100644 --- a/tests/scripts/test_device_leakage.py +++ b/tests/scripts/test_device_leakage.py @@ -20,7 +20,7 @@ 4. The escape hatch is real, bounded and LOUD (M13-M15). 5. The `dev_cast` bucket is derived from the PROPERTY — an integer becoming a `vt::DeviceType` outside the platform seam — and not from the one spelling - that happened to be in the tree when it was written (M20-M34, #660). + that happened to be in the tree when it was written (M20-M40, #660). python3 tests/scripts/test_device_leakage.py @@ -595,6 +595,125 @@ def test_M34_a_function_RETURNING_DeviceType_is_not_a_conversion(self) -> None: ) self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + # --- M35-M40: the row's own thesis, turned on the instrument a THIRD time -- + # + # M33 made the pointer target real, and the docstring then claimed the bucket + # is anchored on "the TARGET TYPE, not on the operand and not on one cast + # keyword". Four spellings that DO write the target type at the conversion + # site still scored zero, so the message asserted coverage the pattern did not + # have — which is #660 exactly, one sigil later. Every mutant below is + # compile-verified legal C++ (g++ -std=c++20 -Wall -Wextra, exit 0), because a + # "miss" that does not compile is not a miss. + + def test_M35_a_REFERENCE_target_fails(self) -> None: + # `reinterpret_cast(raw)` is the same pun as M33's + # `*reinterpret_cast(&raw)` — the standard defines the + # reference form in terms of the pointer form — and it is one character + # shorter. A `\*?` that admits a star but not an ampersand is a spelling + # grep wearing the target type's clothes. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(uint8_t& raw) {\n" + " return reinterpret_cast(raw);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M36_an_EAST_const_target_fails(self) -> None: + # `vt::DeviceType const` and `const vt::DeviceType` are ONE type. The + # pattern took the west spelling only, so the east one laundered the same + # conversion. Asserted in all three places a cv-qualifier can sit: the + # named cast, the named cast with a reference (the form that compiles + # WITHOUT -Wignored-qualifiers, so it survives a -Werror build), and the + # C-style cast. Each is asserted on its own, so one working alternative + # cannot certify the other two. + rc, _out, err = self.plant( + "vt::DeviceType Resolve(int32_t d) { return static_cast(d); }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + rc, _out, err = self.plant( + "const vt::DeviceType& Ref(const vt::DeviceType& t) {\n" + " return static_cast(t);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 2 > baseline 0") + rc, _out, err = self.plant( + "vt::DeviceType CStyle(int32_t d) { return (vt::DeviceType const)d; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 3 > baseline 0") + + def test_M37_a_UNARY_operand_after_a_c_style_cast_fails(self) -> None: + # `(vt::DeviceType)*cursor` is the WIRE-FORMAT DECODE the checker's own + # docstring names as the expected `DSR-ALLOW` case — so the one site the + # bucket predicted it would meet was the one its trailing class could not + # see. The class admitted a leading SIGN but not a dereference. `~` and + # `!` are the same hole. Each asserted individually. + rc, _out, err = self.plant( + "vt::DeviceType Decode(const uint8_t* cursor) { return (vt::DeviceType)*cursor; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + rc, _out, err = self.plant( + "vt::DeviceType Inv(uint8_t m) { return (vt::DeviceType)~m; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 2 > baseline 0") + rc, _out, err = self.plant( + "vt::DeviceType Not(bool f) { return (vt::DeviceType)!f; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 3 > baseline 0") + + def test_M38_a_POINTER_TO_POINTER_target_fails(self) -> None: + # `\*?` is ONE star. Two stars is the same pun through one more level of + # indirection, and it compiles. + rc, _out, err = self.plant( + "vt::DeviceType** Table(uint8_t** p) { return reinterpret_cast(p); }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M39_bit_cast_to_devicetype_fails(self) -> None: + # The docstring used to list `std::bit_cast` as unreachable "because no + # spelling of the target type appears at the conversion site". It does + # appear — `std::bit_cast(raw)` writes it in full — so the + # entry gave a REASON that was false for the case it listed. `bit_cast` + # is now a cast keyword; `memcpy` and union punning keep the entry, and + # keep the reason, because for them it is true. + rc, _out, err = self.plant( + "vt::DeviceType Pun(uint8_t raw) { return std::bit_cast(raw); }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M40_the_widening_did_not_swallow_declarators(self) -> None: + # What M35-M38 could have cost, pinned as a negative. A run of `*`/`&` in + # a named cast's target and a `*` in the C-style trailing class are both + # reachable only after a cast token, but a POINTER- or REFERENCE-returning + # declaration and a ref-qualified member function are the shapes that look + # closest to them. The last two carry a SPACE before the `(`, which is + # what defeats the glued-identifier discriminator and leaves only the + # declarator-suffix guard — the reason `&` is deliberately NOT in the + # C-style trailing class, and the reason the C-style POINTER pun is + # declared blind in the checker's docstring instead of being closed here. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType* Slot(int i);\n" + "vt::DeviceType& SlotRef(int i);\n" + "const vt::DeviceType& Peek();\n" + "vt::DeviceType** Table();\n" + "struct Sink {\n" + " void note (vt::DeviceType*) &;\n" + " void note2 (vt::DeviceType*) &&;\n" + " void note3 (vt::DeviceType) volatile;\n" + "};\n" + "std::vector AllPtrs();\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + def test_M28_dsr_allow_exempts_a_dev_cast_and_says_so_loudly(self) -> None: # The legitimate case the risk register names: a deserialization boundary # that reads a device off the wire. It buys an exemption only with a row From 841521406de02573cc783aacc6113592d2923bf0 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Fri, 14 Aug 2026 18:36:15 +0000 Subject: [PATCH 05/12] fix(LTX25-DEVICE-SEAM-SIBLING): re-derive the tenstorrent anchor at the merge, and record F5 (#659, #660) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FOLLOWING_AGENTS_PROTOCOL `9f2b9bb9a` moved `tenstorrent.cpp`'s `supports_model_architecture` from `:52` to `:55`. This branch cited `:52` in four places, all of which became wrong the moment the re-merge landed: src/vllm/multimodal/ltx2_video.cpp tests/vllm/multimodal/test_diffusion_device_seam.cpp .agents/specs/ltx25-device-seam-sibling.md (x2) Re-derived on the MERGED tree, not copied from the review: `tenstorrent.cpp:55`, and `metal.cpp:70`, `interface.h:263`, `model_loader.cpp:97` all still hold. Every `path:NN` anchor in this row's files was then resolved mechanically rather than spot-checked — 103 distinct anchors — and the only ones that do not land on live code are upstream Python paths (not in this tree) and the two `examples/*/main.cpp` citations in `minimax_h3_video.cpp:3-4`, which carry their own `@ fc636c76` and are therefore provenance rather than drift. `#794` records the rest of this lane's mis-cited anchors and is not widened here. The one anchor this row OWNS that was drifting the same way is `minimax_h3_video.cpp:221-226` in §0 and in the `#660` roadmap key: it describes the defect this row REMOVES, so an unanchored line number there points at a blank line the moment the row lands. Anchored on the spec's base SHA `11cc1d589`, where `git show` resolves it to exactly the `static_cast(device)` the issue is about. The spec gains a `## Findings from review round 2` section carrying F5's nine spellings, their compile proofs, the 0-new/0-lost measurement with its positive control, and — stated as a finding rather than buried — the ONE spelling that stays blind and the reason that is true of it. `## Now` is refreshed; the row stays `READY`, so no lifecycle surface is owed. The `#660` roadmap key is corrected from "TEN spellings" to nineteen, since a record that under-states its own gate is the same class of defect this row keeps finding. Verified keyed: 264 keys on `33f570ea9` and 264 here, 0 added, 0 removed, exactly `#659` and `#660` changed, 0 differing non-table lines. Issues: #659, #660. Campaign: #644. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code] --- .agents/roadmap_v1.md | 2 +- .agents/specs/ltx25-device-seam-sibling.md | 86 +++++++++++++++++-- src/vllm/multimodal/ltx2_video.cpp | 2 +- .../multimodal/test_diffusion_device_seam.cpp | 2 +- 4 files changed, 82 insertions(+), 10 deletions(-) diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index 578efbc0b..32359cdae 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -199,7 +199,7 @@ issue is not yet placed. Keyed record: update in place, never append. | [#651](https://github.com/mudler/vllm.cpp/issues/651) | — | `test_agent_record`'s MODEL-ratchet docstring is two contradictory paragraphs spliced together, and the surviving half records a pin transition that never happened | bug | | [#652](https://github.com/mudler/vllm.cpp/issues/652) | — | `model-matrix.md` prose counters drifted: LTX-2.5 reached the rows and the CI-enforced rollup but none of the five sentences that count them | bug | | [#659](https://github.com/mudler/vllm.cpp/issues/659) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing). Guard added at the same site; `tests/vllm/multimodal/test_diffusion_device_seam.cpp` holds it against an injected declining platform, and `test_minimax_h3_video_fold.cpp`'s own predicate asks all THREE questions — asking two made it red on a correct refusal, i.e. on exactly the partial-backend build this issue exists to serve | bug | -| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553). New `dev_cast` bucket, target-type anchored, RED for TEN spellings not in the tree (M20-M24, M29-M33) — including the literal operand `(vt::DeviceType)1`, which the first cut MISSED while catching `(vt::DeviceType)d`, and whose own discriminator test used an identifier and so could not see the gap it was written to rule out. `::`-qualified, `enum`-elaborated, named list-init and pointer-pun targets are closed too, each measured at zero new hits over the real tree; M34 pins the function-definition negative | bug | +| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226 @ 11cc1d589`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553). New `dev_cast` bucket, target-type anchored, RED for NINETEEN spellings not in the tree (M20-M24, M29-M33, M35-M39) — including the literal operand `(vt::DeviceType)1`, which the first cut MISSED while catching `(vt::DeviceType)d`, and whose own discriminator test used an identifier and so could not see the gap it was written to rule out. Review round 2 found the same shape once more IN THE DOCSTRING: it claimed anchoring on the target type while a REFERENCE target, east const, a dereferenced operand, a second star and `bit_cast` all scored zero — nine compile-verified-legal spellings, all now RED, measured 0 new / 0 lost over 740 files with `\bDeviceType\b`=162 as the positive control. The one gap left, the C-STYLE pointer pun, is named in the checker's message with the reason that is true of it, because `) &` is a ref-qualifier the trailing guard must keep rejecting. M34/M40 pin the negatives | bug | | [#674](https://github.com/mudler/vllm.cpp/issues/674) | — | `main` is RED on `sanitize-cpu (address,undefined)` since `cefacd2d0`: `Ltx2LoadVaeWeights` (`ltx2_loader.cpp:1325`) reinterpret_casts the safetensors mmap to `const uint16_t*`, and that payload offset carries NO alignment guarantee — UB everywhere, a real fault on `build-test-cpu-arm64` and Jetson/Orin sm_110. THIRD recurrence of one class after [#301](https://github.com/mudler/vllm.cpp/issues/301) (closed; it left the `vt::LoadUnaligned` seam) and [#627](https://github.com/mudler/vllm.cpp/issues/627) (`qwen3_5_weights.cpp`, still open). The coverage that caught it was ACCIDENTAL — the fixture's JSON header happens to land that tensor odd — so the fix owes a case that FORCES the odd offset and asserts the parity | bug | | [#664](https://github.com/mudler/vllm.cpp/issues/664) | — | **FIXED 2026-08-13, `row/FIX-WINDOWS-POSIX-VIDEO-ENGINE`.** `windows-msvc-cpu` / `windows-msvc-vulkan` were RED on EVERY open PR (9 sampled across 5 unrelated lanes) from ONE file: `video_engine.cpp` reached Windows with ``, `::stat` and `S_ISDIR` (landed `cefacd2d0`), which `check-windows-portability.py:1675-1688` flags under `full_source_posix` — every scanned source, not only the platform-boundary set. `main` was never a denominator because the Windows jobs are PR-only and `skipped` on push (#584). Repaired at the SOURCE, not the checker: `IsDir`/`Exists` now take the `std::error_code` overloads through a file-local `NativePath`, which preserves `::stat`'s return-false-for-an-uninspectable-path behaviour that the THROWING overloads would have turned into a `filesystem_error` escaping a registry query | bug | | [#608](https://github.com/mudler/vllm.cpp/issues/608) | `TOOLS-PARSER-BREADTH` | **W0 (record backfill) landed 2026-08-13; row `INVENTORIED` → `PARTIAL`. W1 then shipped `inkling`, taking the registry to 42 names / 38 families.** Re-derived from the two REGISTRIES rather than from recipe usage: **five** `--tool-call-parser` names are upstream-only at the pin — `openai`, `inkling`, `minimax_m3` (W1, recipe demand) and `cohere_command3`, `cohere_command4` (W2, ZERO recipe uses, so usage-driven audits miss them). **Only `inkling` was PORTABLE from vLLM source, and it LANDED** (a `ParserEngineToolAdapter` over the already-ported Inkling engine; the gap was the registry face, not the grammar). Of the four left: `minimax_m3` is backed by the Rust crate; `openai` is a stub delegating to `vllm/parser/harmony.py`, which IS vLLM source but wraps the out-of-tree `openai_harmony` package (the SGLang secondary-oracle check was run and REFUSED — vLLM implements this path, so the rule does not admit a secondary); both Cohere names are shims over the out-of-tree `cohere_melody` package. So W1-remaining/W2 each owe a recorded decision before code rather than a text port. `nemotron_json`, `kimi_k3` and `ling3` are in NEITHER registry and arrive with the pin advance, not here. W3 ports upstream's shared `ToolParserTestConfig` harness. The earlier "six missing" framing was usage-derived: it listed `nemotron_json` as portable (it is not registered at the pin) and missed both Cohere entries | feature | diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md index 3b66a14e1..97e3c846f 100644 --- a/.agents/specs/ltx25-device-seam-sibling.md +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -33,7 +33,7 @@ precedent it cites, `SelectQueueForModel`, asks a third question — `supports_model_architecture` exists precisely so a **partial** backend can decline by name. Two platforms override it and both declare a short list: -`src/vllm/platforms/metal.cpp:70` and `src/vllm/platforms/tenstorrent.cpp:52`. +`src/vllm/platforms/metal.cpp:70` and `src/vllm/platforms/tenstorrent.cpp:55`. On such a build a `device = 1` LTX-2.5 load **was refused by name** and is now accepted, so the failure moves from a refusal that says which piece is missing into a kernel bind that says nothing. CUDA is unaffected, which is exactly why @@ -51,7 +51,9 @@ spells its way past it.** `scripts/check-device-leakage.py:78` is RE_KCUDA = re.compile(r"\bkCUDA\b") ``` -`src/vllm/multimodal/minimax_h3_video.cpp:221-226` never writes that token: +`src/vllm/multimodal/minimax_h3_video.cpp:221-226 @ 11cc1d589` — anchored on the +base SHA, because this row is what removes it and an unanchored line number here +would point at a blank line the moment it lands — never writes that token: ```cpp vt::DeviceType MiniMaxH3VideoDeviceType(int32_t device) { @@ -302,14 +304,84 @@ are the seam's own. **Residual, not this row's to fix:** `vulkan.cpp` does not override `supports_model_architecture`, so it inherits `interface.h:263`'s default `true` and a partial Vulkan build still binds and dies inside a kernel. Only -`metal.cpp:70` and `tenstorrent.cpp:52` narrow the claim. +`metal.cpp:70` and `tenstorrent.cpp:55` narrow the claim. + +## Findings from review round 2 (PR #671, head `074ef1420`) + +**F5 — the row's own thesis came back for its instrument a THIRD time, and this +one was in the checker's MESSAGE.** M33 had made the pointer target real, and the +docstring then said the bucket is "anchored on the TARGET TYPE, not on the +operand and not on one cast keyword", and listed `std::bit_cast` as unreachable +"because no spelling of the target type appears at the conversion site". Nine +spellings that DO write the target type at the conversion site scored **zero**. +AGENTS.md makes a checker's own message the authority on what it enforces, so an +over-claiming message is a defect in the gate, not a wording nit — and it is #660 +exactly, one sigil later. + +Each spelling was **compile-verified legal** (`g++ -std=c++20 -Wall -Wextra`, +exit 0) before being called a miss, because a "miss" that does not compile is +not a miss; and each measured **0 before / 1 after**, individually: + +| spelling | why it slipped | +|---|---| +| `reinterpret_cast(raw)` | reference target; the standard *defines* it as M33's pointer pun, and `\*?` admits a star but not an ampersand | +| `static_cast(d)` | east const | +| `static_cast(t)` | east const + reference — the form that compiles with **no** warning, so it survives a `-Werror` build where the plain east-const prvalue trips `-Wignored-qualifiers` | +| `(vt::DeviceType const)d` | east const, C-style | +| `(vt::DeviceType)*cursor` | the trailing class admitted a sign but not a dereference — **and this is the wire-decode spelling the docstring itself names as the expected `DSR-ALLOW` case**, so the one site the bucket predicted meeting was the one it could not see | +| `(vt::DeviceType)~mask`, `(vt::DeviceType)!flag` | the same hole | +| `reinterpret_cast(p)` | `\*?` is one star | +| `std::bit_cast(raw)` | the docstring's own reason was false for it: it spells the target in full | + +Three edits close them: the named-cast target takes cv-qualifiers on **either** +side of the name and a **run** of `*`/`&`; the C-style trailing class admits +`*~!`; `bit_cast` joins the cast keywords. + +**What was NOT closed, and why the docstring now says so per-entry.** `&` is +deliberately absent from the C-style trailing class: `) &` and `) &&` are +ref-qualifiers on a member declarator, which is precisely the false positive the +trailing guard exists to reject, and the guard cannot tell +`void note (vt::DeviceType*) &` from a pun. So the **C-style** pointer pun +`*(vt::DeviceType*)&x` stays blind, and is named as blind with the reason that is +true *of it* — the named-cast spelling of the same pun is caught, which is what +makes it a narrow gap rather than the class. The old blind-spot list shared one +reason across three entries and that reason was false for one of them; each entry +now carries its own. + +**Measured, not asserted.** Over **740** files in `src/vllm` + `include/vllm`, +with `\bDeviceType\b` = **162** matches as a positive control **in the same +command**: **0 new, 0 lost**. Shipped hits 1, widened hits 1 — the same +allowlisted `platform.cpp:85` registry-walk inverse. `dev_cast` stays 0, `total` +stays 32, `scripts/device-leakage-baseline.json` is untouched by this round. + +**M35-M40**, one mutant per newly-closed spelling, each sub-spelling asserted +**individually** (the M29 shape), because a mutant that only exercises the case +the pattern was tuned for is a guard that certifies itself. RED-before is real +rather than narrated: with the pattern reverted in-process to its pre-repair +value, M35-M39 all go RED while M20, M33 and M40 stay GREEN — so a blanket +failure cannot pass for five findings. M40 is the negative the widening could +have cost: pointer- and reference-returning declarations, ref- and +rvalue-ref-qualified members **with a space before the paren**, volatile +members, and `std::vector`. + +**Anchor drift, re-derived at the merge.** `9f2b9bb9a` moved +`tenstorrent.cpp`'s `supports_model_architecture` from `:52` to `:55`; the four +citations on this branch (`ltx2_video.cpp`, `test_diffusion_device_seam.cpp`, and +§0 and the residual note above) are corrected. `metal.cpp:70`, +`interface.h:263` and `model_loader.cpp:97` were re-derived on the merged tree +and all three still hold. Every `path:NN` anchor in this row's files was +resolved mechanically; the only remaining unresolvable ones are upstream Python +paths and two `examples/*/main.cpp` citations that carry their own `@ fc636c76` +and so are provenance rather than drift. The `minimax_h3_video.cpp:221-226` +citation in §0 is now anchored on the base SHA for the same reason. ## Now -`READY`, implemented and awaiting review on `row/LTX25-DEVICE-SEAM-SIBLING` -(PR #671). All three changes are on the branch with their RED, GREEN and -mutation evidence in the PR body; next is a fresh reviewer — not the implementer -— on the immutable head, then the operator's own gate rerun. +`READY`, implemented and awaiting a fresh review on +`row/LTX25-DEVICE-SEAM-SIBLING` (PR #671). All three changes plus the F5 repair +are on the branch with their RED, GREEN and mutation evidence in the PR body; +next is a fresh reviewer — not the implementer — on the immutable head, then the +operator's own gate rerun. The row stays `READY` in `roadmap_v1.md` deliberately. A lifecycle move to `ACTIVE` owes `docs/STATUS.md` and `docs/BENCHMARKS.md` in the same change diff --git a/src/vllm/multimodal/ltx2_video.cpp b/src/vllm/multimodal/ltx2_video.cpp index a32fa53c5..8b580a853 100644 --- a/src/vllm/multimodal/ltx2_video.cpp +++ b/src/vllm/multimodal/ltx2_video.cpp @@ -580,7 +580,7 @@ std::unique_ptr Ltx2VideoEngine::Load(const VideoModelParams& p // both true on a PARTIAL backend — Metal registers 15 of 75 ops, Tenstorrent // a comparable slice — and both name exactly two text architectures in their // `supports_model_architecture` allow-lists (src/vllm/platforms/metal.cpp:70, - // src/vllm/platforms/tenstorrent.cpp:52). Before the seam landed, such a + // src/vllm/platforms/tenstorrent.cpp:55). Before the seam landed, such a // build asked `TryGetBackend(kCUDA)`, got nullptr, and REFUSED BY NAME; after // it, it is handed a queue and dies later inside a kernel bind with a shape // error that says nothing about what is missing. CUDA and CPU are unaffected: diff --git a/tests/vllm/multimodal/test_diffusion_device_seam.cpp b/tests/vllm/multimodal/test_diffusion_device_seam.cpp index 6a2446e66..f554e9230 100644 --- a/tests/vllm/multimodal/test_diffusion_device_seam.cpp +++ b/tests/vllm/multimodal/test_diffusion_device_seam.cpp @@ -8,7 +8,7 @@ // exists precisely so a partial backend can decline BY NAME — Metal registers 15 // of 75 ops and Tenstorrent a comparable slice, and both name exactly two // architectures (src/vllm/platforms/metal.cpp:70, -// src/vllm/platforms/tenstorrent.cpp:52). On such a build a `device = 1` +// src/vllm/platforms/tenstorrent.cpp:55). On such a build a `device = 1` // diffusion load WAS refused by name and became a queue bind that dies later // inside a kernel with a shape error. // From cbc2ee1d5cdd40102cd443bce7fd448c833e929b Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Fri, 14 Aug 2026 22:49:00 +0000 Subject: [PATCH 06/12] fix(LTX25-DEVICE-SEAM-SIBLING): one reason per blind spot (#659, #660) Review round 3 of #671 found this row's own thesis in the checker's message for a fourth time. This time it sat inside a blind-spot entry. `check-device-leakage.py:78-83` covered "a C-style cast to a pointer or reference" under one reason: "both are `)` followed by `&`". That reason is true of `*(vt::DeviceType*)&x`. It is false of `(vt::DeviceType&)raw`, whose next character is an identifier. The reviewer built the feared false positive instead of accepting the argument, and the entry split cleanly. Widening only inside the parens catches the reference form and `(vt::DeviceType*)vp` at zero new hits. The negatives `void note (vt::DeviceType*) &;` and `... &&;` stay at zero, because `&` remains out of the trailing class. So the trailing exclusion earns its place, the C-style pointer pun stays blind for a reason of its own, and the one entry becomes two. Eight spellings close here, counted rather than rounded. A compiler proved each one legal (g++ 13.3, `-std=c++20 -Wall -Wextra -fsyntax-only`, exit 0), and each measured 0 before and 1 after on its own. The C-style targets `(vt::DeviceType&)raw` and `(vt::DeviceType*)vp` needed the run inside the parens. The declaration form carried no cv-group at all, so `vt::DeviceType const d{raw}`, `vt::DeviceType volatile d{raw}` and `struct Cfg { vt::DeviceType const kD{1}; }` all scored 0. That is a fourth place a cv-qualifier sits, while M36 asserted "all three places". That is M29's self-certifying enumeration, one round later. `std::bit_cast(raw)` slipped because `bit_cast` is a function template with two parameters, and terminating the target at `>` assumed the one-argument shape of the four real casts. `reinterpret_cast(p)` slipped because the `[*&]` run stopped at the first cv-qualifier. `__builtin_bit_cast(vt::DeviceType, raw)` slipped because it is not a template-id at all. One pre-existing false positive closes rather than grows. `sizeof (vt::DeviceType) + 1` already scored 1. `sizeof` is not glued to its paren, so the identifier discriminator passes, and `+` sits in the trailing class. Admitting `*` inside the parens would extend that miss to the pointer spelling. A `sizeof` or an `alignof` converts nothing, so the pattern excludes both. M45 pins it and goes RED against the shipped pattern. The rest of this change stops widening and makes the claim honest. Four rounds each found a spelling the previous round's message already claimed, and every one closed at zero hits, so none was a coverage trade. There was simply another spelling each time. The docstring now claims only measured behavior. It enumerates the forms it matches. It gives every blind spot a reason true of that entry alone. The list holds the C-style pointer pun, a character-literal operand, which the comment stripper blanks to whitespace before the pattern runs, and `memcpy` or a union, whose reason becomes "no cast expression to anchor on" rather than the `bit_cast` reason they wrongly carried. It then states the residual as a property. A text checker enforces a set of spellings, so a green `dev_cast` reports that none of the listed spellings is present, and never that no integer becomes a DeviceType here. The structural answer is an AST-level check, filed as #828 with all four rounds as its evidence, and deliberately not built here. M41-M46 add one mutant per newly closed spelling, each asserted on its own. M46 is a new shape. It pins that the declared blind spots stay blind, with `(vt::DeviceType)buf[0]` as a positive control in the same test. A later widening that closes one of them turns M46 RED, which forces the message to change in the same commit. RED-before is measured, not narrated. With `RE_DEVTYPE_CAST` reverted in process to `79ebbce42`, exactly six mutants go RED (M36, M41-M45), and M20, M33, M40 and M46 stay GREEN, so one blanket failure cannot pass for six findings. The applied pattern reds none of them. The suite grows 48 -> 54. The tree measurement is re-derived, not quoted. The scanned set is 760 files across `src/vllm` and `include/vllm`, not the 740 that the roadmap row and this spec both claimed. The same pass measured `\bDeviceType\b` = 162 and `\bkCUDA\b` = 18 as positive controls, and `bit_cast` = 0 against `static_cast` = 9950. The result is 0 new and 0 lost. `dev_cast` stays at its single allowlisted `platform.cpp:85` registry-walk inverse, the total stays 32 against a baseline of 32, and this commit does not touch `scripts/device-leakage-baseline.json`. This commit's body follows `.agents/style/commits.md`, which lands in the merge below it. The prose inside `check-device-leakage.py` and the spec keeps the em dashes and semicolons of the surrounding text, which `.agents/style/prose.md` now discourages. Rewriting that file's voice is not this row's scope, and a dash-free island inside a docstring written in the older voice would read worse than either choice alone. The deviation is stated here so a reviewer can rule on it rather than discover it. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- .agents/roadmap_v1.md | 2 +- .agents/specs/ltx25-device-seam-sibling.md | 87 ++++++++- scripts/check-device-leakage.py | 209 +++++++++++++++------ tests/scripts/test_device_leakage.py | 188 +++++++++++++++++- 4 files changed, 417 insertions(+), 69 deletions(-) diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index 32359cdae..c8aa5bf70 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -199,7 +199,7 @@ issue is not yet placed. Keyed record: update in place, never append. | [#651](https://github.com/mudler/vllm.cpp/issues/651) | — | `test_agent_record`'s MODEL-ratchet docstring is two contradictory paragraphs spliced together, and the surviving half records a pin transition that never happened | bug | | [#652](https://github.com/mudler/vllm.cpp/issues/652) | — | `model-matrix.md` prose counters drifted: LTX-2.5 reached the rows and the CI-enforced rollup but none of the five sentences that count them | bug | | [#659](https://github.com/mudler/vllm.cpp/issues/659) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing). Guard added at the same site; `tests/vllm/multimodal/test_diffusion_device_seam.cpp` holds it against an injected declining platform, and `test_minimax_h3_video_fold.cpp`'s own predicate asks all THREE questions — asking two made it red on a correct refusal, i.e. on exactly the partial-backend build this issue exists to serve | bug | -| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226 @ 11cc1d589`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553). New `dev_cast` bucket, target-type anchored, RED for NINETEEN spellings not in the tree (M20-M24, M29-M33, M35-M39) — including the literal operand `(vt::DeviceType)1`, which the first cut MISSED while catching `(vt::DeviceType)d`, and whose own discriminator test used an identifier and so could not see the gap it was written to rule out. Review round 2 found the same shape once more IN THE DOCSTRING: it claimed anchoring on the target type while a REFERENCE target, east const, a dereferenced operand, a second star and `bit_cast` all scored zero — nine compile-verified-legal spellings, all now RED, measured 0 new / 0 lost over 740 files with `\bDeviceType\b`=162 as the positive control. The one gap left, the C-STYLE pointer pun, is named in the checker's message with the reason that is true of it, because `) &` is a ref-qualifier the trailing guard must keep rejecting. M34/M40 pin the negatives | bug | +| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226 @ 11cc1d589`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553). New `dev_cast` bucket, with **29** individually-asserted RED spellings that are not in the tree (counted, not quoted: M20-M24, M29-M33, M35-M39, M41-M44) — including the literal operand `(vt::DeviceType)1`, which the first cut MISSED while catching `(vt::DeviceType)d`, and whose own discriminator test used an identifier and so could not see the gap it was written to rule out. Rounds 2-4 each found the same shape once more IN THE MESSAGE: round 2, nine spellings that wrote the target type the docstring named as its anchor; round 4, eight more — a C-style REFERENCE and POINTER target, the DECLARATION form's cv-qualifiers in three spellings (a FOURTH cv-position, while M36 asserted "all three"), `bit_cast`'s two-argument form, a cv-qualifier inside the pointer run, and `__builtin_bit_cast`. All compile-verified legal, all now RED, measured 0 new / 0 lost over **760** files with `\bDeviceType\b`=162 and `\bkCUDA\b`=18 as positive controls in the same pass; `sizeof (vt::DeviceType) + 1` was a pre-existing FALSE positive and is now excluded. The docstring no longer claims the property: it enumerates the spellings it matches, states each blind spot with a reason true of that entry alone (`*(vt::DeviceType*)&x`, a character-literal operand, `memcpy`), and names the residual as a property a text checker cannot decide — [#828](https://github.com/mudler/vllm.cpp/issues/828) tracks the AST-level check. M34/M40/M45 pin the negatives; M46 pins that the declared blind spots are still blind, so message and pattern cannot drift apart silently | bug | | [#674](https://github.com/mudler/vllm.cpp/issues/674) | — | `main` is RED on `sanitize-cpu (address,undefined)` since `cefacd2d0`: `Ltx2LoadVaeWeights` (`ltx2_loader.cpp:1325`) reinterpret_casts the safetensors mmap to `const uint16_t*`, and that payload offset carries NO alignment guarantee — UB everywhere, a real fault on `build-test-cpu-arm64` and Jetson/Orin sm_110. THIRD recurrence of one class after [#301](https://github.com/mudler/vllm.cpp/issues/301) (closed; it left the `vt::LoadUnaligned` seam) and [#627](https://github.com/mudler/vllm.cpp/issues/627) (`qwen3_5_weights.cpp`, still open). The coverage that caught it was ACCIDENTAL — the fixture's JSON header happens to land that tensor odd — so the fix owes a case that FORCES the odd offset and asserts the parity | bug | | [#664](https://github.com/mudler/vllm.cpp/issues/664) | — | **FIXED 2026-08-13, `row/FIX-WINDOWS-POSIX-VIDEO-ENGINE`.** `windows-msvc-cpu` / `windows-msvc-vulkan` were RED on EVERY open PR (9 sampled across 5 unrelated lanes) from ONE file: `video_engine.cpp` reached Windows with ``, `::stat` and `S_ISDIR` (landed `cefacd2d0`), which `check-windows-portability.py:1675-1688` flags under `full_source_posix` — every scanned source, not only the platform-boundary set. `main` was never a denominator because the Windows jobs are PR-only and `skipped` on push (#584). Repaired at the SOURCE, not the checker: `IsDir`/`Exists` now take the `std::error_code` overloads through a file-local `NativePath`, which preserves `::stat`'s return-false-for-an-uninspectable-path behaviour that the THROWING overloads would have turned into a `filesystem_error` escaping a registry query | bug | | [#608](https://github.com/mudler/vllm.cpp/issues/608) | `TOOLS-PARSER-BREADTH` | **W0 (record backfill) landed 2026-08-13; row `INVENTORIED` → `PARTIAL`. W1 then shipped `inkling`, taking the registry to 42 names / 38 families.** Re-derived from the two REGISTRIES rather than from recipe usage: **five** `--tool-call-parser` names are upstream-only at the pin — `openai`, `inkling`, `minimax_m3` (W1, recipe demand) and `cohere_command3`, `cohere_command4` (W2, ZERO recipe uses, so usage-driven audits miss them). **Only `inkling` was PORTABLE from vLLM source, and it LANDED** (a `ParserEngineToolAdapter` over the already-ported Inkling engine; the gap was the registry face, not the grammar). Of the four left: `minimax_m3` is backed by the Rust crate; `openai` is a stub delegating to `vllm/parser/harmony.py`, which IS vLLM source but wraps the out-of-tree `openai_harmony` package (the SGLang secondary-oracle check was run and REFUSED — vLLM implements this path, so the rule does not admit a secondary); both Cohere names are shims over the out-of-tree `cohere_melody` package. So W1-remaining/W2 each owe a recorded decision before code rather than a text port. `nemotron_json`, `kimi_k3` and `ling3` are in NEITHER registry and arrive with the pin advance, not here. W3 ports upstream's shared `ToolParserTestConfig` harness. The earlier "six missing" framing was usage-derived: it listed `nemotron_json` as portable (it is not registered at the pin) and missed both Cohere entries | feature | diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md index 97e3c846f..9ac82eaf8 100644 --- a/.agents/specs/ltx25-device-seam-sibling.md +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -348,9 +348,10 @@ makes it a narrow gap rather than the class. The old blind-spot list shared one reason across three entries and that reason was false for one of them; each entry now carries its own. -**Measured, not asserted.** Over **740** files in `src/vllm` + `include/vllm`, -with `\bDeviceType\b` = **162** matches as a positive control **in the same -command**: **0 new, 0 lost**. Shipped hits 1, widened hits 1 — the same +**Measured, not asserted.** Over the files in `src/vllm` + `include/vllm`, with +`\bDeviceType\b` = **162** matches as a positive control **in the same +command**: **0 new, 0 lost**. (This round said "740 files"; the scanned set is +**760** — see round 3 below, which re-derived it.) Shipped hits 1, widened hits 1 — the same allowlisted `platform.cpp:85` registry-walk inverse. `dev_cast` stays 0, `total` stays 32, `scripts/device-leakage-baseline.json` is untouched by this round. @@ -375,6 +376,86 @@ paths and two `examples/*/main.cpp` citations that carry their own `@ fc636c76` and so are provenance rather than drift. The `minimax_h3_video.cpp:221-226` citation in §0 is now anchored on the base SHA for the same reason. +## Findings from review round 3 (PR #671, head `79ebbce42`) + +**The blind-spot ENTRY bound two members under one reason, and the reason was +true of only one of them.** The note covered "a C-STYLE cast whose target is a +pointer **or reference**" with the single reason "both are `)` followed by `&`". +That is true of `*(vt::DeviceType*)&x` and **false** of `(vt::DeviceType&)raw`, +whose next character is `r`. The reviewer **constructed the feared false +positive** instead of accepting the argument, which split the entry cleanly: +widening only **inside the parens** catches the reference form *and* +`(vt::DeviceType*)vp` at **zero** new hits, while the ref-qualifier negatives +`void note (vt::DeviceType*) &;` and `… &&;` stay at zero because `&` is still +out of the **trailing** class. So the trailing-class exclusion is justified, the +C-style **pointer pun** legitimately stays blind, and the entry is now two +entries with a reason each. Round 2 closed with "the old blind-spot list shared +one reason across three entries and that reason was false for one of them; each +entry now carries its own" — and the entry it wrote to replace them did the same +thing to two. The spec paragraph was accurate about the pointer pun; the +**checker's own message**, which is the authority, said "pointer or reference". + +**Eight more spellings — counted, not rounded; the table groups two of them on +one row because they share a reason — each compile-verified legal (`g++ 13.3 +-std=c++20 -Wall -Wextra -fsyntax-only`, exit 0) and each measured 0 before / 1 +after, individually:** + +| spelling | why it slipped | +|---|---| +| `(vt::DeviceType&)raw` | the C-style target admitted no `*`/`&` run at all | +| `(vt::DeviceType*)vp` | same | +| `vt::DeviceType const d{raw}` | alternative (3), the DECLARATION form, had **no cv-group** — a **fourth** place a cv-qualifier can sit, while M36 asserted "all three places". An enumeration certifying its own completeness, which is M29's defect returning | +| `vt::DeviceType volatile d{raw}`, `struct Cfg { vt::DeviceType const kD{1}; }` | same, and the member form is the one a person actually writes | +| `std::bit_cast(raw)` | round 2 added `bit_cast` *because* it spells the target at the site, then terminated the target at `>` on the assumption every cast takes one template argument. `bit_cast` is a **function template with two parameters** | +| `reinterpret_cast(p)` | the `[*&]` run stopped at the first cv-qualifier, so the reference-to-const-pointer the docstring claimed was not reachable | +| `__builtin_bit_cast(vt::DeviceType, raw)` | not a template-id at all, so (1)'s `<…>` anchor cannot reach it and (2)'s glued-identifier discriminator rejects it | + +**One pre-existing FALSE positive, closed rather than widened.** +`sizeof (vt::DeviceType) + 1` already scored **1** before this round — `sizeof` +is not glued to its paren, so the identifier discriminator passes, and `+` is in +the trailing class — and admitting `*` inside the parens would have extended it +to `sizeof (vt::DeviceType*) + 1`. `sizeof` and `alignof` convert nothing, so +both are excluded outright. M45 pins it, and is RED against the shipped pattern. + +**What is NOT closed, and why the checker now says so as a PROPERTY.** Four +rounds have each found a spelling the previous round's message already claimed, +and every one closed at **zero** hits — so none was a coverage/false-positive +trade; there was simply always another spelling. Widening again is not the +answer to that, and the docstring no longer implies it is. It now: (a) +enumerates the forms it **measured** itself matching, (b) gives every blind spot +a reason true of that entry alone — the C-style pointer pun `*(vt::DeviceType*)&x` +(closing it needs `&` in the trailing class, which is the ref-qualifier false +positive), a **character-literal** operand `(vt::DeviceType)'\x01'` (character +literals are blanked to whitespace by `strip_comments_and_strings` before the +pattern ever runs, so the operand is gone by match time), `memcpy`/union (no cast +**expression** to anchor on — *not* "the type is not named at the site", which is +false for the idiomatic `std::memcpy(&dt, &raw, sizeof(vt::DeviceType))`), the +unscanned `src/vt/` leg, and the absence of any type check — and (c) states the +residual as a property: **a text checker enforces a set of SPELLINGS, not "an +integer becomes a `vt::DeviceType`"**, so a green `dev_cast` means "none of the +listed spellings is present" and never "no integer becomes a DeviceType here". +The structural answer is an **AST-level check** (clang tooling, where the +destination type canonicalises and the source type is known), filed as +[#828](https://github.com/mudler/vllm.cpp/issues/828) with all four rounds' +evidence, since "we kept finding more spellings" *is* the argument for it. Not +built here. + +**M41-M46 and the M36 repair**, one mutant per newly-closed spelling, each +asserted individually. **M46 is the new shape**: it pins that the **declared +blind spots are still blind**, with `(vt::DeviceType)buf[0]` as a positive +control in the same test — so if a later widening closes one, M46 goes RED and +the message must be corrected in the same change. That is the only mechanism +that keeps a checker's message the authority on what it enforces. RED-before is +real, not narrated: with `RE_DEVTYPE_CAST` reverted in-process to `79ebbce42`, +exactly **6** mutants go RED (M36, M41-M45) and M20/M33/M40/M46 stay GREEN, so a +blanket failure cannot pass for six findings; applied, 0 RED. Suite 48 → **54** +tests, the +6 fully attributed to M41-M46. + +**File count re-derived.** #660's roadmap row and this spec both said "740 +files"; the scanned set is **760**, measured with `\bDeviceType\b` = **162** and +`\bkCUDA\b` = **18** as positive controls in the same pass (the 162 is why the +old number's other figures still hold). Both records are corrected. + ## Now `READY`, implemented and awaiting a fresh review on diff --git a/scripts/check-device-leakage.py b/scripts/check-device-leakage.py index 77519a72a..7b8e2f866 100644 --- a/scripts/check-device-leakage.py +++ b/scripts/check-device-leakage.py @@ -45,56 +45,103 @@ WAS counted. The gate read the confession and missed the act. It is also an enum-ordering hazard — reordering the enum silently re-points every such site. -`dev_cast` is anchored on the TARGET TYPE, not on the operand and not on one -cast keyword, because the property is "an integer becomes a DeviceType outside -the platform seam" and the property is what has to be caught. The reverse -direction (`static_cast(type)`) is the SAFE one and is deliberately not -counted: it is how the seam itself indexes its registry. - -WHAT `dev_cast` DOES SEE, in every spelling of the one conversion: the four cast -keywords, `bit_cast`, and the C-style, functional, brace and DECLARATION forms; -the type written as `DeviceType`, `vt::DeviceType`, `::vt::DeviceType` or -`enum vt::DeviceType`, with `const`/`volatile` on EITHER side of the name; an -operand that is an identifier, a literal (`)1`), a signed literal (`)-1`), a -parenthesised expression or a unary expression (`)*cursor`, `)~mask`, `)!flag`); -a target that is a POINTER, a POINTER-TO-POINTER or a REFERENCE, which is how -`reinterpret_cast` puns an integer's bytes onto a DeviceType; and a cast -clang-format has wrapped across two lines. +The reverse direction (`static_cast(type)`) is the SAFE one and is +deliberately not counted: it is how the seam itself indexes its registry. + +WHAT `dev_cast` ENFORCES, EXACTLY. It is a regular expression over +comment-stripped text, so what it enforces is a SET OF SPELLINGS. It does NOT +enforce the property it was written for — "an integer becomes a DeviceType +outside the platform seam" — and it cannot: it cannot resolve a name to a type, +cannot tell an integer operand from a DeviceType one, and cannot see a conversion +whose target type is not written at the conversion site. Read a green `dev_cast` +as "none of the spellings enumerated below is present", never as "no integer +becomes a DeviceType here". #828 tracks the AST-level check (clang tooling), +where the destination type canonicalises and the source type is known, and which +is what would enforce the property. This pattern is the INTERIM, and says so. + +That distinction is measured, not defensive. FOUR review rounds of this one +bucket each found a spelling the previous round's message already claimed to +cover. #660 shipped it; round 1 found the literal operand `(vt::DeviceType)1`, +and the mutant written to rule that out used an IDENTIFIER operand, so it could +not see the gap it existed to close; round 2 found nine more that all write the +target type the docstring named as its anchor; round 3 found eight more (counted, +not rounded), including a FOURTH place a cv-qualifier can sit while the mutant +asserted "all three". +Every one of them closed at ZERO hits on the scanned tree, so none was a +coverage-versus-false-positive trade — there was simply always another spelling. +So the list below is an enumeration of what was MEASURED, not a claim about a +class. + +WHAT `dev_cast` DOES SEE. Each entry is pinned by its own mutant in +tests/scripts/test_device_leakage.py, asserted individually: + + * the named casts `static_cast`, `reinterpret_cast`, `const_cast`, + `dynamic_cast` and `bit_cast`, plus the builtin spelling + `__builtin_bit_cast(vt::DeviceType, x)`. `bit_cast` is a FUNCTION template + with two parameters, so its second template argument (`From`) may be + written — as long as it is on one line and contains no nested angle + brackets; + * the C-style cast `(vt::DeviceType)x`, the functional casts + `vt::DeviceType(x)` and `vt::DeviceType{x}`, and the same conversion spelled + as a DECLARATION, `vt::DeviceType d{x}`; + * the type written `DeviceType`, `vt::DeviceType`, `::vt::DeviceType` or + `enum vt::DeviceType`, with `const`/`volatile` on EITHER side of the name in + ALL of those forms — the declaration form included, which is the fourth + cv-position and the one round 3 found (`vt::DeviceType const kD{1}`); + * an operand that is an identifier, a literal (`)1`), a signed literal (`)-1`), + a parenthesised expression, or a unary expression (`)*cursor`, `)~mask`, + `)!flag`); + * a target that is a POINTER, a POINTER-TO-POINTER or a REFERENCE — the pun + `reinterpret_cast` performs on an integer's bytes — in the named-cast form + AND the C-style form, with cv-qualifiers anywhere inside the `*`/`&` run + (`reinterpret_cast(p)`); + * a cast clang-format has wrapped across two lines. WHAT `dev_cast` STILL CANNOT SEE, stated here because a checker's message is the authority on what it enforces and an instrument that hides its blind spot -returns a false pass. Each entry states the reason that is true OF THAT ENTRY, -because a list whose shared reason is false for one member is the same false -claim in a smaller font: +returns a false pass. Every entry states the reason that is true OF THAT ENTRY +ALONE, and no entry binds two cases whose reasons differ — a list whose shared +reason is false for one member is the same false claim in a smaller font, and +that is precisely what rounds 2 and 3 each found here: * a cast whose target is a type ALIAS (`using DT = vt::DeviceType;` then - `static_cast
(x)`), a macro that expands to the type name, or a template - parameter that resolves to DeviceType. Reason: no spelling of the target - type appears at the conversion site; - * `memcpy` or a union punning an integer onto a DeviceType. Same reason: the - type is named at the declaration, never at the conversion. (`std::bit_cast` - DOES name it there, so it is a cast keyword above rather than an entry here, - and the `reinterpret_cast` pointer/reference puns are caught too.); - * a C-STYLE cast whose target is a pointer or reference, `*(vt::DeviceType*)&x`. - Reason: the C-style alternative must not swallow a parameter list, and its - two discriminators — the `(` not glued to an identifier, and no declarator - suffix after the `)` — cannot tell `void note (vt::DeviceType*) &` from a - pun, because both are `)` followed by `&`. The NAMED-cast spelling of the - same pun IS caught, which is why this is a narrow gap rather than the class; + `static_cast
(x)`), a MACRO that expands to the type name, or a TEMPLATE + PARAMETER that resolves to DeviceType. Reason: no spelling of the target + type appears at the conversion site, so there is nothing to anchor on; + * `memcpy` or a UNION punning an integer onto a DeviceType. Reason: there is + no cast EXPRESSION to anchor on. (Not "the type is not named at the site": + the idiomatic `std::memcpy(&dt, &raw, sizeof(vt::DeviceType))` names it right + there, and matching that would mean matching a bare `sizeof(vt::DeviceType)`, + which converts nothing. The old entry gave `bit_cast`'s reason to `memcpy`, + and `bit_cast` is now a keyword above.); + * the C-style POINTER PUN whose operand begins with `&`, `*(vt::DeviceType*)&x`. + Reason: closing it needs `&` in the trailing class, and `) &` / `) &&` are + ref-qualifiers on a member declarator — `void note (vt::DeviceType*) &;` + compiles, and admitting `&` turns it into a false positive (measured: 2). + Every OTHER C-style pointer or reference cast IS caught — `(vt::DeviceType&)` + and `(vt::DeviceType*)` both — as is the named-cast spelling of this same + pun, so this is one operand shape rather than the class; + * a CHARACTER-LITERAL operand, `(vt::DeviceType)'\x01'`. Reason: character + literals are blanked to whitespace by `strip_comments_and_strings` before the + pattern ever runs, so the operand is gone by match time and no trailing class + could see it. `(vt::DeviceType)buf[0]` is caught, so this is that one literal + kind, not the wire-decode shape; * a conversion that happens inside `src/vt/` and is merely CALLED from the - shared layer — `src/vt/` is a device leg and is not scanned at all; - * whether the operand really is an integer. Nothing here type-checks; the - bucket flags every cast TO DeviceType and relies on `// DSR-ALLOW()` + shared layer. Reason: `src/vt/` is a device leg and is not scanned at all; + * whether the operand really is an integer. Reason: nothing here type-checks. + The bucket flags every cast TO DeviceType and relies on `// DSR-ALLOW()` for the legitimate ones (a wire-format decode is the expected case). This is also why a pointer or reference target counts: `const_cast(t)` removes const rather than converting an integer, and buys its exemption the - same way. + same way; + * any spelling that is not on the list above. Reason: this is a spelling list, + and four rounds of finding a new one is the evidence that a spelling list is + not the class. That gap does not close by widening; it closes at #828. -Those are gaps in a TEXT checker, not gaps that were traded away for -convenience. tests/scripts/test_device_leakage.py M20-M40 pin what it does -catch, each spelling asserted on its own — including, in M29, the literal -operand, because a discriminator tested only on the case it was tuned for is a -guard that certifies itself. +tests/scripts/test_device_leakage.py M20-M46 pin what it does catch, each +spelling asserted on its own — including, in M29, the literal operand, because a +discriminator tested only on the case it was tuned for is a guard that certifies +itself. HOW THE RATCHET WORKS. `scripts/device-leakage-baseline.json` holds the accepted per-bucket counts. Any bucket ABOVE its baseline fails. Any bucket BELOW its @@ -141,13 +188,18 @@ RE_KCUDA = re.compile(r"\bkCUDA\b") RE_IS_CUDA = re.compile(r"\bis_cuda\s*\(\s*\)") -# `dev_cast`: an integer becoming a DeviceType. Three alternatives, one per C++ +# `dev_cast`: an integer becoming a DeviceType. Four alternatives, one per C++ # spelling of the same conversion, all anchored on the TARGET TYPE: # # 1. a named cast static_cast(x) / / … # and the INDIRECT forms, *reinterpret_cast(&x) # and reinterpret_cast(x) -# 2. a C-style cast (vt::DeviceType)x and (vt::DeviceType)1 +# 1b. __builtin_bit_cast __builtin_bit_cast(vt::DeviceType, x) — the same +# conversion as (1)'s `bit_cast`, but a two-argument +# BUILTIN rather than a template-id, so (1)'s `<…>` +# anchor cannot reach it +# 2. a C-style cast (vt::DeviceType)x, (vt::DeviceType)1, +# (vt::DeviceType&)raw and (vt::DeviceType*)vp # 3. a functional cast vt::DeviceType(x) / vt::DeviceType{x}, and the same # conversion spelled as a DECLARATION, `DeviceType d{x}` # @@ -183,37 +235,76 @@ # is the false positive the trailing guard exists to reject. Measured over # `src/vllm` + `include/vllm`: admitting `0-9+-*~!` adds ZERO hits. # -# (1) and (2) both take the cv-qualifiers on EITHER SIDE of the type name, and (1) -# takes any run of `*` and `&`. `vt::DeviceType const` is the same type as -# `const vt::DeviceType`, and `reinterpret_cast(raw)` is the same -# pun as `*reinterpret_cast(&raw)` — both COMPILE (checked, not -# read), and a pattern that saw west const and one star while the docstring -# claimed the target type is what it is anchored on would be asserting coverage it -# did not have. That is #660's own defect turned on the instrument, which is why -# the run of `[*&]` is a run and the cv-qualifier group is two-sided. +# (2)'s TARGET, by contrast, does take a run of `*` and `&` — INSIDE the parens, +# where no declarator can be confused with it. That is the split the blind-spot +# entry used to get wrong: the old note bound "pointer or reference" together +# under the single reason "both are `)` followed by `&`", which is true of +# `*(vt::DeviceType*)&x` and FALSE of `(vt::DeviceType&)raw`, whose next character +# is an identifier. Widening only inside the parens catches the reference form and +# `(vt::DeviceType*)vp` at zero cost; the ref-qualifier negatives +# `void note (vt::DeviceType*) &;` and `… &&;` stay at zero because `&` is still +# out of the TRAILING class. Both halves compile-verified, both measured. # -# `bit_cast` joins the four cast keywords for the same reason: `bit_cast` spells -# the target type at the conversion site, so the blind-spot note below could not -# honestly claim it as unreachable — `memcpy` and union punning still are, and -# they are the cases where no spelling of the type appears at all. +# `sizeof (vt::DeviceType) + 1` was ALREADY a false positive before that widening +# — `sizeof` is not glued to its paren there, so the identifier discriminator +# passes, and `+` is in the trailing class — and admitting `*` inside the parens +# would have extended it to `sizeof (vt::DeviceType*) + 1`. A `sizeof` or +# `alignof` converts nothing, so both are excluded outright rather than left as a +# gap the widening made wider. The lookbehinds are fixed-width, so they cover the +# one-space spelling clang-format produces; `sizeof(` with no space was already +# blocked by the glued-identifier discriminator. Measured: removing these two +# costs nothing on the tree (dev_cast stays at its single allowlisted hit). +# +# (1) and (2) and (3) all take the cv-qualifiers on EITHER SIDE of the type name, +# and (1) takes any run of `*`, `&` and cv-qualifiers INTERLEAVED. +# `vt::DeviceType const` is the same type as `const vt::DeviceType`; +# `reinterpret_cast(raw)` is the same pun as +# `*reinterpret_cast(&raw)`; and `vt::DeviceType* const&` puts a +# cv-qualifier one position further right than a `[*&]`-only run can reach — all +# COMPILE (checked, not read). (3) is where the fourth cv-position hides: +# `vt::DeviceType const d{raw}` is a declaration, and a group present on (1) and +# (2) but absent from (3) is a docstring claiming three places while the code +# covers two. +# +# `bit_cast` joins the four cast keywords because it spells the target type at the +# conversion site, so the blind-spot note below could not honestly claim it as +# unreachable. It is a FUNCTION template, not a cast operator, so `From` may also +# be written — `std::bit_cast(raw)` compiles — and +# the optional `,` tail is what lets (1) reach that form. `__builtin_bit_cast` +# gets its own alternative because it is not a template-id at all. `memcpy` and +# union punning stay blind, and their reason is now their own: there is no cast +# expression to anchor on. # # (3)'s DECLARATION form takes `{` only. `DeviceType d{x}` is a real conversion # (C++17 permits list-initialising a scoped enum with a fixed underlying type), # but `DeviceType d(x)` is ILL-FORMED — there is no implicit int→scoped-enum # conversion — so nothing is lost by excluding it, while allowing `(` would match # every FUNCTION DEFINITION whose return type is DeviceType (measured: 3 false -# positives, e.g. `vt::DeviceType MiniMaxH3VideoDeviceType(`). +# positives, e.g. `vt::DeviceType MiniMaxH3VideoDeviceType(`). Adding the east +# cv-group to (3) does not reopen that: `vt::DeviceType const Frozen(int);` still +# scores 0, because `{` is still the only initialiser admitted. _DEVTYPE_QUAL = r"(?:enum\s+)?(?:::\s*)?(?:vt\s*::\s*)?" # cv-qualifiers, which C++ permits on either side of the type name. _CV_WEST = r"(?:(?:const|volatile)\s+)*" _CV_EAST = r"(?:\s*(?:const|volatile)\b)*" +# A named cast's target suffix: `*`, `&` and cv-qualifiers, any number, any order. +# `vt::DeviceType* const&` needs the cv-qualifier INSIDE the run, not before it. +_TARGET_SUFFIX = r"(?:\s*(?:const\b|volatile\b|[*&]))*" +# `bit_cast`'s second template parameter (`From`), which may be written out. +# One line, no nested angle brackets — deliberately narrow, since the scanned +# roots contain zero `bit_cast` occurrences and a greedy tail would be the only +# way this could cost a false positive. +_TEMPLATE_TAIL = r"(?:\s*,[^<>;{}\n]*)?" RE_DEVTYPE_CAST = re.compile( r"(?:static_cast|reinterpret_cast|const_cast|dynamic_cast|bit_cast)\s*<\s*" - + _CV_WEST + _DEVTYPE_QUAL + r"DeviceType\b" + _CV_EAST + r"(?:\s*[*&])*\s*>" - r"|(?:(?" + r"|__builtin_bit_cast\s*\(\s*" + + _CV_WEST + _DEVTYPE_QUAL + r"DeviceType\b" + _CV_EAST + r"\s*," + r"|(?:(?])" + _DEVTYPE_QUAL + r"DeviceType\b\s*(?:\w+\s*\{|[({])\s*(?![)}])" + r"|(?])" + _DEVTYPE_QUAL + r"DeviceType\b" + _CV_EAST + + r"\s*(?:\w+\s*\{|[({])\s*(?![)}])" ) RE_CUDA_INCLUDE = re.compile(r'^\s*#\s*include\s*[<"](?:vt/cuda/|cuda_runtime)') RE_PP_IF = re.compile(r"^\s*#\s*(ifdef|ifndef|if)\b(.*)$") diff --git a/tests/scripts/test_device_leakage.py b/tests/scripts/test_device_leakage.py index fe71293df..9e675c9b6 100644 --- a/tests/scripts/test_device_leakage.py +++ b/tests/scripts/test_device_leakage.py @@ -20,7 +20,12 @@ 4. The escape hatch is real, bounded and LOUD (M13-M15). 5. The `dev_cast` bucket is derived from the PROPERTY — an integer becoming a `vt::DeviceType` outside the platform seam — and not from the one spelling - that happened to be in the tree when it was written (M20-M40, #660). + that happened to be in the tree when it was written (M20-M46, #660) — and + that what it actually enforces is a SET OF SPELLINGS rather than that + property, which is why M46 pins the DECLARED BLIND SPOTS as still blind: + the checker's message is the authority on what it enforces, so the message + and the pattern have to be able to disagree loudly. #828 tracks the + AST-level check that would enforce the property itself. python3 tests/scripts/test_device_leakage.py @@ -622,11 +627,18 @@ def test_M35_a_REFERENCE_target_fails(self) -> None: def test_M36_an_EAST_const_target_fails(self) -> None: # `vt::DeviceType const` and `const vt::DeviceType` are ONE type. The # pattern took the west spelling only, so the east one laundered the same - # conversion. Asserted in all three places a cv-qualifier can sit: the - # named cast, the named cast with a reference (the form that compiles - # WITHOUT -Wignored-qualifiers, so it survives a -Werror build), and the - # C-style cast. Each is asserted on its own, so one working alternative - # cannot certify the other two. + # conversion. + # + # This comment used to say "all three places a cv-qualifier can sit" and + # then list three. There are FOUR: the named cast, the named cast with a + # reference (the form that compiles WITHOUT -Wignored-qualifiers, so it + # survives a -Werror build), the C-style cast — and the DECLARATION form, + # `vt::DeviceType const d{raw}`, which had no cv-group in the pattern at + # all. An enumeration that certifies its own completeness is the exact + # defect this row exists to fix (it is M29's disease, one round later), so + # the count is now the measured one and the fourth case is asserted below. + # Each is asserted on its own, so one working alternative cannot certify + # the others. rc, _out, err = self.plant( "vt::DeviceType Resolve(int32_t d) { return static_cast(d); }\n" ) @@ -644,6 +656,33 @@ def test_M36_an_EAST_const_target_fails(self) -> None: ) self.assertEqual(rc, 1) self.require(err, "DSR REGRESSION in bucket 'dev_cast': 3 > baseline 0") + # The FOURTH place, and the one the "all three" comment above hid: the + # DECLARATION form. `vt::DeviceType const d{raw}` and + # `vt::DeviceType volatile d{raw}` both compile (g++ -std=c++20 -Wall + # -Wextra, exit 0) and both scored 0, because alternative (3) carried no + # cv-group. The member-declaration spelling — a named device CONSTANT + # initialised from the literal `1` — is the one a person actually writes. + rc, _out, err = self.plant( + "vt::DeviceType DeclConst(uint8_t raw) {\n" + " vt::DeviceType const d{raw};\n" + " return d;\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 4 > baseline 0") + rc, _out, err = self.plant( + "uint8_t DeclVol(uint8_t raw) {\n" + " vt::DeviceType volatile d{raw};\n" + " return static_cast(d);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 5 > baseline 0") + rc, _out, err = self.plant( + "struct Cfg { vt::DeviceType const kD{1}; };\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 6 > baseline 0") def test_M37_a_UNARY_operand_after_a_c_style_cast_fails(self) -> None: # `(vt::DeviceType)*cursor` is the WIRE-FORMAT DECODE the checker's own @@ -714,6 +753,143 @@ def test_M40_the_widening_did_not_swallow_declarators(self) -> None: ) self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + # --- M41-M46: round 4. The blind-spot ENTRY bound two members under one + # + # reason, and the reason was true of only one of them. The old note covered + # "a C-STYLE cast whose target is a pointer OR REFERENCE" with the single + # reason "both are `)` followed by `&`" — true of `*(vt::DeviceType*)&x`, + # false of `(vt::DeviceType&)raw`, whose next character is an identifier. So + # the reference form was reachable by widening only INSIDE the parens, at + # zero cost, while the entry declared it blind. That is the same defect as + # rounds 1-3, moved from the pattern into the per-entry REASON, which is why + # every closure below is paired with an assertion that its blind-spot twin is + # STILL blind (M46) rather than with a wider claim. + # + # Every mutant below is compile-verified legal C++ (g++ -std=c++20 -Wall + # -Wextra -fsyntax-only, exit 0) and measured individually at 0 before / 1 + # after, over the same 760-file scan roots that keep `dev_cast` at its single + # allowlisted hit. + + def test_M41_a_C_STYLE_pointer_or_reference_target_fails(self) -> None: + # Widening INSIDE the parens, where no declarator can be confused with a + # target. `(vt::DeviceType&)raw` is the C-style spelling of M35's pun and + # `(vt::DeviceType*)vp` is a plain pointer cast off a `void*`; both + # compile, both scored 0. Asserted separately, because one alternative + # working is not the other one working. + rc, _out, err = self.plant( + "vt::DeviceType Ref(uint8_t& raw) { return (vt::DeviceType&)raw; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + rc, _out, err = self.plant( + "vt::DeviceType* Ptr(void* vp) { return (vt::DeviceType*)vp; }\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 2 > baseline 0") + + def test_M42_bit_cast_may_name_its_SOURCE_type_too(self) -> None: + # M39 added `bit_cast` on the argument that it spells the target type at + # the conversion site. It is a FUNCTION template with TWO parameters, not + # a cast operator with one, so `From` may be written — and terminating the + # target at `>` assumed the one-argument shape the four real casts have. + # `std::bit_cast(raw)` compiles and scored + # 0. Over-matching is not a risk: the scanned roots contain ZERO + # `bit_cast` and ZERO `__builtin_bit_cast` occurrences — measured over the + # comment-stripped text of all 760 files, with `static_cast` = 9950 as the + # positive control in the same pass, so the zero is an absence and not a + # broken grep. + rc, _out, err = self.plant( + "vt::DeviceType Pun(uint8_t raw) {\n" + " return std::bit_cast(raw);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M43_a_cv_qualifier_INSIDE_the_pointer_run_fails(self) -> None: + # M35 made the target suffix a run of `[*&]`, and the docstring then + # claimed a REFERENCE target. `vt::DeviceType* const&` is exactly that — + # a reference to a const pointer — and the run stopped at the first + # cv-qualifier. One position to the right of round 2's own finding. + rc, _out, err = self.plant( + "vt::DeviceType* const& Slot(vt::DeviceType** p) {\n" + " return reinterpret_cast(*p);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M44_the_BUILTIN_spelling_of_bit_cast_fails(self) -> None: + # `__builtin_bit_cast(vt::DeviceType, raw)` compiles under g++ 13.3 and + # names the target type at the conversion site just as fully as + # `std::bit_cast` does — but it is not a template-id, so alternative (1)'s + # `<…>` anchor cannot reach it and alternative (2)'s glued-identifier + # discriminator rejects it. Listing `bit_cast` as caught while this + # spelling scored 0 would be the same overclaim in a smaller font. + rc, _out, err = self.plant( + "vt::DeviceType Pun(uint8_t raw) {\n" + " return __builtin_bit_cast(vt::DeviceType, raw);\n" + "}\n" + ) + self.assertEqual(rc, 1) + self.require(err, "DSR REGRESSION in bucket 'dev_cast': 1 > baseline 0") + + def test_M45_sizeof_and_alignof_convert_nothing(self) -> None: + # A false positive the widening WOULD have grown. `sizeof (vt::DeviceType) + # + 1` already scored 1 before this round — `sizeof` is not glued to its + # paren, so the identifier discriminator passes, and `+` is in the + # trailing class — and admitting `*` inside the parens would have extended + # it to the pointer spelling. `sizeof` and `alignof` convert nothing, so + # both are excluded outright rather than left as a gap made wider. The + # first two of these are RED against the SHIPPED pattern. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "size_t A() { return sizeof (vt::DeviceType) + 1; }\n" + "size_t B() { return alignof (vt::DeviceType) + 1; }\n" + "size_t C() { return sizeof (vt::DeviceType*) + 1; }\n" + "size_t D() { return sizeof(vt::DeviceType) + 1; }\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + + def test_M46_the_declared_blind_spots_are_STILL_blind(self) -> None: + # The other half of a per-entry reason: the docstring says these three + # score 0, so this pins that they do. If a later widening closes one, this + # test goes RED and the message has to be corrected in the same change — + # which is the only mechanism that keeps a checker's message the authority + # on what it enforces. It is not an argument that they SHOULD stay blind. + # + # * `*(vt::DeviceType*)&x` — closing it needs `&` in the TRAILING class, + # which is what makes `void note (vt::DeviceType*) &;` a false + # positive (M40 pins that negative); + # * `(vt::DeviceType)'\x01'` — character literals are blanked to + # whitespace by strip_comments_and_strings before the pattern runs, so + # the operand is gone by match time; + # * `std::memcpy(&dt, &raw, sizeof(vt::DeviceType))` — there is no cast + # EXPRESSION to anchor on. Note it DOES name the type at the site, so + # the old shared reason ("the type is named at the declaration, never + # at the conversion") was false for it. + # + # All three compile (g++ -std=c++20 -Wall -Wextra -fsyntax-only, exit 0). + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType Pun(uint8_t x) { return *(vt::DeviceType*)&x; }\n" + "vt::DeviceType Chr() { return (vt::DeviceType)'\\x01'; }\n" + "vt::DeviceType Mem(uint8_t raw) {\n" + " vt::DeviceType dt{};\n" + " std::memcpy(&dt, &raw, sizeof(vt::DeviceType));\n" + " return dt;\n" + "}\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + # The positive control for that zero, in the same test: the pattern is + # alive, and one character more of operand IS caught. A null result from a + # dead regex would otherwise prove nothing. + self.tree.append( + "src/vllm/model_executor/models/toy.cpp", + "vt::DeviceType Buf(const uint8_t* b) { return (vt::DeviceType)b[0]; }\n", + ) + self.assertEqual(self.tree.scan().counts["dev_cast"], 1) + def test_M28_dsr_allow_exempts_a_dev_cast_and_says_so_loudly(self) -> None: # The legitimate case the risk register names: a deserialization boundary # that reads a device off the wire. It buys an exemption only with a row From 872b27b4ae1fc405ee5f4909d81e7e16570e3afd Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Fri, 14 Aug 2026 22:51:19 +0000 Subject: [PATCH 07/12] record(LTX25-DEVICE-SEAM-SIBLING): anchor the scanned file count to a SHA (#660) The round-3 repair corrected "740 files" to 760. Re-running the same measurement on the merged tree returned 765, because main added five source files under `src/vllm` and `include/vllm` in the 17 commits this branch just merged. That is the interesting part, so this commit records it rather than silently overwriting one number with another. A file count of one tree, stored inside another file, is a measurement that every unrelated pull request invalidates. AGENTS.md names that shape under "never store a measurement of one file inside another file". The 740 was not wrong when someone wrote it. It rotted. Both records now carry the SHA the number was measured at. A reader who finds a mismatch then knows to re-derive the count instead of doubting the rest of the section. The durable statement is the pair of positive controls, which the merge left unchanged: `\bDeviceType\b` = 162 and `\bkCUDA\b` = 18 in the same pass, `dev_cast` still at its single allowlisted `platform.cpp:85` registry-walk inverse, and 0 new and 0 lost against the shipped pattern. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- .agents/roadmap_v1.md | 2 +- .agents/specs/ltx25-device-seam-sibling.md | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index 0407ab2c6..ce9c3209a 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -208,7 +208,7 @@ issue is not yet placed. Keyed record: update in place, never append. | [#651](https://github.com/mudler/vllm.cpp/issues/651) | — | `test_agent_record`'s MODEL-ratchet docstring is two contradictory paragraphs spliced together, and the surviving half records a pin transition that never happened | bug | | [#652](https://github.com/mudler/vllm.cpp/issues/652) | — | `model-matrix.md` prose counters drifted: LTX-2.5 reached the rows and the CI-enforced rollup but none of the five sentences that count them | bug | | [#659](https://github.com/mudler/vllm.cpp/issues/659) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. LTX-2.5 device select adopts M3a's platform seam but not its companion capability guard: `ltx2_video.cpp` asks `CurrentPlatform().device_type()` and `TryGetBackend(...)` but never `supports_model_architecture`, so a PARTIAL backend (Metal 15/75 ops, Tenstorrent) is handed a queue and dies in a kernel bind where it used to be refused BY NAME (found while reviewing #553 for landing). Guard added at the same site; `tests/vllm/multimodal/test_diffusion_device_seam.cpp` holds it against an injected declining platform, and `test_minimax_h3_video_fold.cpp`'s own predicate asks all THREE questions — asking two made it red on a correct refusal, i.e. on exactly the partial-backend build this issue exists to serve | bug | -| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226 @ 11cc1d589`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553). New `dev_cast` bucket, with **29** individually-asserted RED spellings that are not in the tree (counted, not quoted: M20-M24, M29-M33, M35-M39, M41-M44) — including the literal operand `(vt::DeviceType)1`, which the first cut MISSED while catching `(vt::DeviceType)d`, and whose own discriminator test used an identifier and so could not see the gap it was written to rule out. Rounds 2-4 each found the same shape once more IN THE MESSAGE: round 2, nine spellings that wrote the target type the docstring named as its anchor; round 4, eight more — a C-style REFERENCE and POINTER target, the DECLARATION form's cv-qualifiers in three spellings (a FOURTH cv-position, while M36 asserted "all three"), `bit_cast`'s two-argument form, a cv-qualifier inside the pointer run, and `__builtin_bit_cast`. All compile-verified legal, all now RED, measured 0 new / 0 lost over **760** files with `\bDeviceType\b`=162 and `\bkCUDA\b`=18 as positive controls in the same pass; `sizeof (vt::DeviceType) + 1` was a pre-existing FALSE positive and is now excluded. The docstring no longer claims the property: it enumerates the spellings it matches, states each blind spot with a reason true of that entry alone (`*(vt::DeviceType*)&x`, a character-literal operand, `memcpy`), and names the residual as a property a text checker cannot decide — [#828](https://github.com/mudler/vllm.cpp/issues/828) tracks the AST-level check. M34/M40/M45 pin the negatives; M46 pins that the declared blind spots are still blind, so message and pattern cannot drift apart silently | bug | +| [#660](https://github.com/mudler/vllm.cpp/issues/660) | `LTX25-DEVICE-SEAM-SIBLING` | Spec [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md), `READY`. `check-device-leakage`'s `kcuda` bucket is the token grep `\bkCUDA\b`, so `minimax_h3_video.cpp:221-226 @ 11cc1d589`'s `static_cast(device)` hardcodes CUDA as enum value 1 and counts as 0. Gate strength plus an enum-ordering hazard; the H3 video lane should ask the same seam `ltx2_video.cpp` now does (found while reviewing #553). New `dev_cast` bucket, with **29** individually-asserted RED spellings that are not in the tree (counted, not quoted: M20-M24, M29-M33, M35-M39, M41-M44) — including the literal operand `(vt::DeviceType)1`, which the first cut MISSED while catching `(vt::DeviceType)d`, and whose own discriminator test used an identifier and so could not see the gap it was written to rule out. Rounds 2-4 each found the same shape once more IN THE MESSAGE: round 2, nine spellings that wrote the target type the docstring named as its anchor; round 4, eight more — a C-style REFERENCE and POINTER target, the DECLARATION form's cv-qualifiers in three spellings (a FOURTH cv-position, while M36 asserted "all three"), `bit_cast`'s two-argument form, a cv-qualifier inside the pointer run, and `__builtin_bit_cast`. All compile-verified legal, all now RED, measured 0 new / 0 lost over the scan roots (**760** files at `79ebbce42`, **765** after the merge — the count rots on every unrelated PR, so it carries its SHA) with `\bDeviceType\b`=162 and `\bkCUDA\b`=18 as positive controls in the same pass, unchanged across the merge; `sizeof (vt::DeviceType) + 1` was a pre-existing FALSE positive and is now excluded. The docstring no longer claims the property: it enumerates the spellings it matches, states each blind spot with a reason true of that entry alone (`*(vt::DeviceType*)&x`, a character-literal operand, `memcpy`), and names the residual as a property a text checker cannot decide — [#828](https://github.com/mudler/vllm.cpp/issues/828) tracks the AST-level check. M34/M40/M45 pin the negatives; M46 pins that the declared blind spots are still blind, so message and pattern cannot drift apart silently | bug | | [#674](https://github.com/mudler/vllm.cpp/issues/674) | — | `main` is RED on `sanitize-cpu (address,undefined)` since `cefacd2d0`: `Ltx2LoadVaeWeights` (`ltx2_loader.cpp:1325`) reinterpret_casts the safetensors mmap to `const uint16_t*`, and that payload offset carries NO alignment guarantee — UB everywhere, a real fault on `build-test-cpu-arm64` and Jetson/Orin sm_110. THIRD recurrence of one class after [#301](https://github.com/mudler/vllm.cpp/issues/301) (closed; it left the `vt::LoadUnaligned` seam) and [#627](https://github.com/mudler/vllm.cpp/issues/627) (`qwen3_5_weights.cpp`, still open). The coverage that caught it was ACCIDENTAL — the fixture's JSON header happens to land that tensor odd — so the fix owes a case that FORCES the odd offset and asserts the parity | bug | | [#664](https://github.com/mudler/vllm.cpp/issues/664) | — | **FIXED 2026-08-13, `row/FIX-WINDOWS-POSIX-VIDEO-ENGINE`.** `windows-msvc-cpu` / `windows-msvc-vulkan` were RED on EVERY open PR (9 sampled across 5 unrelated lanes) from ONE file: `video_engine.cpp` reached Windows with ``, `::stat` and `S_ISDIR` (landed `cefacd2d0`), which `check-windows-portability.py:1675-1688` flags under `full_source_posix` — every scanned source, not only the platform-boundary set. `main` was never a denominator because the Windows jobs are PR-only and `skipped` on push (#584). Repaired at the SOURCE, not the checker: `IsDir`/`Exists` now take the `std::error_code` overloads through a file-local `NativePath`, which preserves `::stat`'s return-false-for-an-uninspectable-path behaviour that the THROWING overloads would have turned into a `filesystem_error` escaping a registry query | bug | | [#608](https://github.com/mudler/vllm.cpp/issues/608) | `TOOLS-PARSER-BREADTH` | **W0 (record backfill) landed 2026-08-13; row `INVENTORIED` → `PARTIAL`. W1 then shipped `inkling`, taking the registry to 42 names / 38 families.** Re-derived from the two REGISTRIES rather than from recipe usage: **five** `--tool-call-parser` names are upstream-only at the pin — `openai`, `inkling`, `minimax_m3` (W1, recipe demand) and `cohere_command3`, `cohere_command4` (W2, ZERO recipe uses, so usage-driven audits miss them). **Only `inkling` was PORTABLE from vLLM source, and it LANDED** (a `ParserEngineToolAdapter` over the already-ported Inkling engine; the gap was the registry face, not the grammar). Of the four left: `minimax_m3` is backed by the Rust crate; `openai` is a stub delegating to `vllm/parser/harmony.py`, which IS vLLM source but wraps the out-of-tree `openai_harmony` package (the SGLang secondary-oracle check was run and REFUSED — vLLM implements this path, so the rule does not admit a secondary); both Cohere names are shims over the out-of-tree `cohere_melody` package. So W1-remaining/W2 each owe a recorded decision before code rather than a text port. `nemotron_json`, `kimi_k3` and `ling3` are in NEITHER registry and arrive with the pin advance, not here. W3 ports upstream's shared `ToolParserTestConfig` harness. The earlier "six missing" framing was usage-derived: it listed `nemotron_json` as portable (it is not registered at the pin) and missed both Cohere entries | feature | diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md index 9ac82eaf8..d86344cbb 100644 --- a/.agents/specs/ltx25-device-seam-sibling.md +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -451,10 +451,21 @@ exactly **6** mutants go RED (M36, M41-M45) and M20/M33/M40/M46 stay GREEN, so a blanket failure cannot pass for six findings; applied, 0 RED. Suite 48 → **54** tests, the +6 fully attributed to M41-M46. -**File count re-derived.** #660's roadmap row and this spec both said "740 -files"; the scanned set is **760**, measured with `\bDeviceType\b` = **162** and -`\bkCUDA\b` = **18** as positive controls in the same pass (the 162 is why the -old number's other figures still hold). Both records are corrected. +**File count re-derived, and anchored to a SHA, because it rots.** #660's +roadmap row and this spec both said "740 files". The scanned set is **760 at +`79ebbce42`** and **765 at the merge below**, because main added five source +files under the scan roots in between. Both numbers were measured with +`\bDeviceType\b` = **162** and `\bkCUDA\b` = **18** as positive controls in the +same pass, and both are unchanged by those five files, which is why every other +figure in this section holds across the merge. + +That drift is the finding, not an errand. A file count of one tree stored inside +another file is a measurement that every unrelated pull request invalidates, +which is the shape AGENTS.md forbids under "never store a measurement of one +file inside another file". "740" was not wrong when it was written; it rotted. +So both records now carry the SHA the number was measured at, and a reader who +finds a mismatch knows to re-derive rather than to distrust the rest. The +durable statement is the ratio the controls give, not the absolute count. ## Now From be9b0a6fdc46d6fef4267dd867d8247b0bcd4cdf Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Sat, 15 Aug 2026 11:01:23 +0000 Subject: [PATCH 08/12] record(LTX25-DEVICE-SEAM-SIBLING): re-derive five anchors at the merge, and retire the roadmap claim the intake move voided (#659, #660) FOLLOWING_AGENTS_PROTOCOL Every `path:NN` this row cites was re-derived at the merged tree and asserted UNIQUE rather than merely present, which is what catches an anchor that still resolves but resolves to the wrong thing. Thirty-one citations across the spec and the row's own source comments; five did not hold, and they split two ways. Main's drift, two: `model_loader.cpp:97` -> `:98` (the capability clause) and `:71-72` -> `:72-73` (the explicit-accelerator polarity the review's F6 argues from). Both moved under seventeen commits of `origin/main`. Already stale at `bf8bb745b`, three: `minimax_h3_video.h:63` -> `:69` and `ltx2_video.cpp:545-548` -> `:562-565`, each shifted by this row's OWN earlier commits, and `minimax_h3_video.cpp`'s in-source citation of `ltx2_video.cpp:549-580` -> `:566-610`. That is the recorded-anchor failure this project already has a name for: a line number rots inside the pull request that wrote it, and nothing but re-derivation notices. The row's `## Now` claimed a surface that no longer exists. Both scoped edits it carried lived in `roadmap_v1.md`'s `## Open issues` table, and #840 moved that table to the append-only `.agents/issue-index.md`. Under that regime a row is appended and never edited, GitHub holds the open and closed state, and the in-place annotation this row wrote is the `FIXED IN FLOW` shape that spec retired. So the row now writes NO record file, both index rows stay byte-for-byte as main holds them, and `## Now` says so with its reason instead of pointing at a table that was deleted. Also re-derived: the scanned-file figure the spec anchors to a SHA precisely because it rots. 776 files at this merge, with the stripped-text controls at `DeviceType`=163 and `kCUDA`=18 -- `kCUDA` unchanged and `DeviceType` +1 across the merge, which is the invariant the section claims, against +11 on the count, which is the thing it predicts. Two measurement traps are now written down beside the number, because each cost a probe and each made a CORRECT figure read as wrong: the controls are counted over the comment-and-string-stripped text (raw source gives 177 / 82), and the enumeration must be the checker's own `rglob`, since a `git ls-tree` walk filtered by string prefix swallows `include/vllm.h` and returns 777. No behaviour changes. One comment line of product source, one spec. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- .agents/specs/ltx25-device-seam-sibling.md | 41 ++++++++++++++++++---- src/vllm/multimodal/minimax_h3_video.cpp | 2 +- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md index d86344cbb..fd8d5af77 100644 --- a/.agents/specs/ltx25-device-seam-sibling.md +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -25,7 +25,7 @@ if (accelerator == vt::DeviceType::kCPU || That is "is there an accelerator, and is a backend registered for it". The precedent it cites, `SelectQueueForModel`, asks a third question — -`src/vllm/entrypoints/model_loader.cpp:97`: +`src/vllm/entrypoints/model_loader.cpp:98`: ```cpp (architecture.empty() || plat.supports_model_architecture(architecture)) @@ -82,7 +82,7 @@ about the token and a weaker statement about the property than it appears to be. 1. `minimax_h3_video.cpp` resolves its device through the platform seam rather than by integer cast, mirroring what `ltx2_video.cpp` now does. The public `MiniMaxH3VideoDeviceType(int32_t)` contract in - `include/vllm/multimodal/minimax_h3_video.h:63` is preserved — `0` is CPU, + `include/vllm/multimodal/minimax_h3_video.h:69` is preserved — `0` is CPU, anything else is refused or resolved, never cast. 2. `ltx2_video.cpp` asks `supports_model_architecture` alongside the two questions it already asks, and refuses **by name**, naming the platform and @@ -294,9 +294,9 @@ back to the CPU reference … and runs correctly, just slowly — which is stric better than dying inside a kernel bind"). Both diffusion lanes instead **throw**. That is correct, but it is the *explicit-device* path's polarity, not the `kAuto` path's: `device = 1` is an explicit accelerator request, and -`model_loader.cpp:71-72` already says of that path "an explicit accelerator whose +`model_loader.cpp:72-73` already says of that path "an explicit accelerator whose queue cannot be created must FAIL the load loudly, never silently serve on CPU" — -the same argument `ltx2_video.cpp:545-548` makes for refusing rather than serving +the same argument `ltx2_video.cpp:562-565` makes for refusing rather than serving the CPU forward behind an accelerator handle. So the lanes mirror the capability question from one path and the failure polarity from the other, and both halves are the seam's own. @@ -467,6 +467,20 @@ So both records now carry the SHA the number was measured at, and a reader who finds a mismatch knows to re-derive rather than to distrust the rest. The durable statement is the ratio the controls give, not the absolute count. +**Re-derived again at the `4a4ab89cb` merge, and it rotted again — as predicted.** +The scanned set is **776 files**, with the controls at `\bDeviceType\b` = **163** +and `\bkCUDA\b` = **18**. `kCUDA` is unchanged across seventeen commits of main +and `DeviceType` moved by one, which is the invariant this section actually +claims; the file count moved by eleven, which is the thing it says will rot. Two +notes for the next reader, because each cost a probe here and each made a correct +figure look wrong. The controls are counted over the +**comment-and-string-stripped** text — the text the pattern is actually matched +against — and NOT over raw source, where the same two patterns read **177 / 82**. +And the enumeration must be the checker's own `rglob` over `SCAN_ROOTS`: a +`git ls-tree` walk filtered with a string prefix returns **777**, because +`include/vllm.h` starts with the characters `include/vllm` while sitting outside +the `include/vllm` root. That off-by-one is the probe's, not the record's. + ## Now `READY`, implemented and awaiting a fresh review on @@ -475,10 +489,25 @@ are on the branch with their RED, GREEN and mutation evidence in the PR body; next is a fresh reviewer — not the implementer — on the immutable head, then the operator's own gate rerun. -The row stays `READY` in `roadmap_v1.md` deliberately. A lifecycle move to -`ACTIVE` owes `docs/STATUS.md` and `docs/BENCHMARKS.md` in the same change +The row stays `READY` deliberately. A lifecycle move to `ACTIVE` owes +`docs/STATUS.md` and `docs/BENCHMARKS.md` in the same change (scripts/check-doc-checkpoint.py), and those are projections of what the project CLAIMS — which this row does not change until it lands. Writing them from an unmerged PR would also put two shared files under a lock for the length of a review. The operator moves the state, and writes those two surfaces, when it merges. + +**This row now writes no record file at all, and that is the correct outcome +rather than an omission.** It previously annotated the `#659` and `#660` rows of +`roadmap_v1.md`'s `## Open issues` table in place. `#840` +(`POLICY-ISSUE-INTAKE`, spec [`issue-intake.md`](issue-intake.md)) moved that +table out to `.agents/issue-index.md`, which is append-only and carries +`merge=union`: a row is appended and never edited, and GitHub holds the open and +closed state, so closing `#659` and `#660` costs the index no edit. The in-place +annotation this row carried is exactly the `FIXED IN FLOW` shape that spec +retired, and under a union driver it would have been duplicated rather than +merged. The two index rows are therefore left byte-for-byte as `origin/main` +holds them, `LTX25-DEVICE-SEAM-SIBLING` has no portfolio row in `roadmap_v1.md` +(checked, with `ENG-WEIGHT-OFFLOAD` at two hits as a positive control in the same +command), and the evidence that annotation carried lives here, in the surface +that owns it. diff --git a/src/vllm/multimodal/minimax_h3_video.cpp b/src/vllm/multimodal/minimax_h3_video.cpp index 375a0e882..bca2d182f 100644 --- a/src/vllm/multimodal/minimax_h3_video.cpp +++ b/src/vllm/multimodal/minimax_h3_video.cpp @@ -223,7 +223,7 @@ void FillNoise(std::vector& out, uint64_t seed) { // // `device` is the public video ABI's selector: 0 is the CPU, 1 is "the // accelerator". WHICH accelerator is the PLATFORM's question, not this model -// file's — the same question `ltx2_video.cpp:549-580` and `SelectQueueForModel` +// file's — the same question `ltx2_video.cpp:566-610` and `SelectQueueForModel` // (src/vllm/entrypoints/model_loader.cpp:75-104) ask. // // This used to be `static_cast(device)`, which is not a mapping From f0b4650298c0277307f5f35fbe2e4281a1b41cd4 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Sat, 15 Aug 2026 13:13:21 +0000 Subject: [PATCH 09/12] fix(LTX25-DEVICE-SEAM-SIBLING): the "refuses by name" assertion could not fail, and the H3 half was unreached (#659, #660, #828) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Repairs the eight findings of the fresh review of `7502004aa`. The review found the change correct and the gate honest; every finding is an untestable assertion, a reachability gap, or a record that claimed more than it had. F1. `test_diffusion_device_seam.cpp` asserted `msg.find(kLtx2VideoFamily)`, and `Fail()` in ltx2_video.cpp prefixes EVERY message with "ltx-2.5 video: ", which contains that string verbatim. The assertion was satisfied by boilerplate on every refusal the file can throw — including the two the same case asserts it is NOT — and no defect in the message could make it fail. Measured: substituting "" for the family name in the DECLINES `Fail` BUILT and left the suite GREEN. Both lanes now assert the QUOTED SLOT, `architecture ''`, through one `QuotedArchitecture()` helper. The H3 side escaped only because its prefix spells the family with an underscore against a hyphenated family, which is a coincidence of spelling and not a property; it is hardened the same way. F2. `## Nothing lands dead`: the H3 half had a production chain and nothing entering through it. Replacing `MiniMaxH3VideoEngine::Load`'s `MiniMaxH3VideoDeviceType(params.device)` with the pre-row `params.device == 0 ? kCPU : kCUDA` BUILT and left both suites GREEN — the fold suite's CUDA-load case does enter through `Load`, but its FakeCudaPlatform reports kCUDA, so the seam and the cast return the same answer and it cannot separate them. Two H3 cases now enter at `LoadVideoEngine` against the declining PartialXpuPlatform, and the two LTX cases moved there too, so both lanes are entered at the same production point and neither skips the registry hop. That mutation is now RED: 6 cases, 4 passed / 2 failed, and the two failures are the new cases. F3. The anchor sweep that claimed completeness was incomplete, and this pull request's own edits are what falsified it. Six citations were stale, all rotted inside this branch, two of them seven and fifteen lines from the `:54` citation that SHA-anchors itself with exactly the reasoning that applies to them. Four more range citations started or ended on the wrong line. All corrected; the completeness sentence is withdrawn with its reason and replaced by a statement of method and of what the method cannot see. F5. `dev_cast` fires on a plain copy-initialisation, `vt::DeviceType d{other}`, which converts nothing. It is not narrowed, because `vt::DeviceType d{raw}` is the real conversion M36/M41 pin and is textually identical. The docstring gains a third section, WHAT dev_cast OVER-MATCHES, with the cost stated — the baseline is a hard 0, so the first such line under the scan roots fails the ratchet and needs DSR-ALLOW — and M47 pins it in the M46 shape with the value-init as the negative control in the same test. F6. Four comments still called the video ABI selector CUDA, including the public `vllm_video_model_params.device` field and `minimax_h3_video.h:88` nineteen lines below the docstring this row rewrote. `include/vllm/config/device.h:19` repeats the same sentence and is corrected with them. `docs/USAGE.md` gains the paragraph that owes. F7. #828 was named in product code and indexed nowhere. Appended to `.agents/issue-index.md` with no owning row and listed under the spec's new `## Owed`, which is the shape the protocol defines for an issue a row files and does not fix. F4 and F8 are a `## Now` pointing at closed PR #671 and the pre-rebuild branch, and one inverted sentence in the test header. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- .agents/issue-index.md | 1 + .agents/specs/ltx25-device-seam-sibling.md | 201 +++++++++++++++--- docs/USAGE.md | 12 +- include/vllm.h | 13 +- include/vllm/config/device.h | 5 +- include/vllm/multimodal/minimax_h3_video.h | 4 +- include/vllm/multimodal/video_engine.h | 5 +- scripts/check-device-leakage.py | 38 +++- src/vllm/multimodal/minimax_h3_video.cpp | 5 +- tests/scripts/test_device_leakage.py | 38 ++++ .../multimodal/test_diffusion_device_seam.cpp | 117 +++++++++- 11 files changed, 396 insertions(+), 43 deletions(-) diff --git a/.agents/issue-index.md b/.agents/issue-index.md index 6a9221b8f..d60faa11b 100644 --- a/.agents/issue-index.md +++ b/.agents/issue-index.md @@ -221,3 +221,4 @@ rather than merged. `scripts/check-agent-record.py` gates both. | [#513](https://github.com/mudler/vllm.cpp/issues/513) | `SPEC-DSPARK` | DFlash/DSpark draft loading hardcodes the VL weight prefix, so a text-only Qwen3.5 target cannot share its embedding | bug | | [#536](https://github.com/mudler/vllm.cpp/issues/536) | `ROAD-V1-C3` | C3's named tail records DSpark as unspiked three days after it landed, and files TLI under the wrong lane | bug | | [#886](https://github.com/mudler/vllm.cpp/issues/886) | `POLICY-NOTHING-LANDS-DEAD` | The seam checkers police where a capability routes and nothing polices whether anything reaches it, so a change can land wired downward and driven only by its own test, as tensor parallelism did (guide [`reachability.md`](reachability.md)) | feature | +| [#828](https://github.com/mudler/vllm.cpp/issues/828) | — | `check-device-leakage.py`'s `dev_cast` bucket enforces a set of SPELLINGS, not the property "an integer becomes a `vt::DeviceType`": four review rounds each found a spelling the previous round's message already claimed, every one closing at ZERO hits, so widening is not the answer. The structural fix is an AST-level check (clang tooling), where the destination type canonicalises and the source type is known. Listed under `## Owed` in [`ltx25-device-seam-sibling.md`](specs/ltx25-device-seam-sibling.md) | feature | diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md index fd8d5af77..5f52a1f45 100644 --- a/.agents/specs/ltx25-device-seam-sibling.md +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -15,7 +15,10 @@ because they are in this lane and this campaign owns them. the lane it was aimed at and left two things standing. **(a) #659 — the seam was adopted, its companion guard was not.** -`src/vllm/multimodal/ltx2_video.cpp:549-562` now asks two questions: +`src/vllm/multimodal/ltx2_video.cpp:549-562 @ 11cc1d589` — anchored on the base +SHA for the same reason `:54` below is, because this row rewrites that block and +an unanchored number would point into the middle of the repair the moment it +lands — now asks two questions: ```cpp const vt::DeviceType accelerator = vllm::platforms::CurrentPlatform().device_type(); @@ -45,7 +48,9 @@ the GPU' claim false". A partial backend that binds and dies has the same property one level down: it is a device claim the build cannot honour. **(b) #660 — the gate that certified (a) is a token grep, and the sibling lane -spells its way past it.** `scripts/check-device-leakage.py:78` is +spells its way past it.** `scripts/check-device-leakage.py:78 @ 62406c30e` — +anchored, because this row rewrites that file's head and the line is now `:188` +— is ```python RE_KCUDA = re.compile(r"\bkCUDA\b") @@ -66,8 +71,11 @@ It hardcodes the ABI's `0/1` into the enum by integer cast and scores **zero** in the `kcuda` bucket. The same defect, in the sibling diffusion lane, under a different spelling. -**The sharpest form of it:** `tests/vllm/models/test_minimax_h3_video_fold.cpp:162` -asserts `MiniMaxH3VideoDeviceType(1) == vt::DeviceType::kCUDA`. The *test* spells +**The sharpest form of it:** +`tests/vllm/models/test_minimax_h3_video_fold.cpp:162 @ 62406c30e` asserts +`MiniMaxH3VideoDeviceType(1) == vt::DeviceType::kCUDA` — anchored, because this +row is what rewrites that assertion and the file now carries it at `:220` +reading `== accelerator`. The *test* spells the token honestly and is counted; the *source* launders it and is not. The gate therefore reads the confession and misses the act. @@ -109,7 +117,7 @@ about the token and a weaker statement about the property than it appears to be. There is no upstream for this: vLLM has no LTX-2.5 or MiniMax-H3 video engine, and the device seam is ours. The mirror source is therefore **our own -`SelectQueueForModel`** (`src/vllm/entrypoints/model_loader.cpp:59-104`), which +`SelectQueueForModel`** (`src/vllm/entrypoints/model_loader.cpp:60-105`), which is the shape every other model path already uses, and `include/vllm/platforms/interface.h:263`, which defines the capability question. Mirroring an internal seam is what "route it through the shared surface" means; @@ -159,8 +167,9 @@ RED-first for each of the three. constructible in the CPU test build, the test injects a stub platform rather than skipping — a skipped test here is the whole finding. 2. `MiniMaxH3VideoDeviceType` keeps its contract: `0` → `kCPU`, `-1` and `2` - throw (`test_minimax_h3_video_fold.cpp:161-164` already assert this and must - stay green **unchanged**), and `1` resolves through the seam rather than by + throw (`test_minimax_h3_video_fold.cpp:161-164 @ 62406c30e` already assert + this and must stay green **unchanged**), and `1` resolves through the seam + rather than by cast. The new assertion is that on a CPU-only build `1` is **refused**, which the cast could never do. 3. The checker's three-spelling adversarial test above, each spelling asserted @@ -197,13 +206,15 @@ count is RED even when it reads green. ## Findings from implementation **The brief and this spec disagreed on one line, and this spec won.** The -dispatch brief said `test_minimax_h3_video_fold.cpp:161-164` must stay green -*unchanged*, but §4.2 above enumerates what "the contract" means — `0 → kCPU`, -`-1` and `2` throw — and separately requires that on a CPU-only build `1` is -**refused**. Line 162 asserted `MiniMaxH3VideoDeviceType(1) == kCUDA`, which is -precisely the cast's answer and cannot survive the change. It is now +dispatch brief said `test_minimax_h3_video_fold.cpp:161-164 @ 62406c30e` must +stay green *unchanged*, but §4.2 above enumerates what "the contract" means — +`0 → kCPU`, `-1` and `2` throw — and separately requires that on a CPU-only build +`1` is **refused**. Line 162 asserted `MiniMaxH3VideoDeviceType(1) == kCUDA`, +which is precisely the cast's answer and cannot survive the change. It is now build-conditional and asserts BOTH arms: `== accelerator` where one is -registered, refused-by-name where none is. 161/163/164 are untouched. +registered, refused-by-name where none is. 161/163/164 are untouched. Every +number in this paragraph is `@ 62406c30e`: on the branch the four assertions sit +at `:191-193` and the two arms at `:215-238`. **`test_minimax_h3_video_fold.cpp`'s CUDA-load case registered a BACKEND and no PLATFORM.** It could, because the cast never asked whether the build had an @@ -286,9 +297,9 @@ predicate is now three-way and asserts *which* refusal, since a right refusal fo a wrong reason is a wrong diagnosis that reads as a right one. **The decline CONSEQUENCE inverts the cited precedent, deliberately.** The PR -described the change as mirroring `model_loader.cpp:97`, and it mirrors that -site's *question* while inverting its *answer*. `:97`'s capability test lives on -the `kAuto` path, whose response to a decline is to fall through to `:103` and +described the change as mirroring `model_loader.cpp:98`, and it mirrors that +site's *question* while inverting its *answer*. `:98`'s capability test lives on +the `kAuto` path, whose response to a decline is to fall through to `:104` and **serve on CPU**; `metal.cpp:65-69` states that policy in as many words ("falls back to the CPU reference … and runs correctly, just slowly — which is strictly better than dying inside a kernel bind"). Both diffusion lanes instead **throw**. @@ -296,7 +307,7 @@ That is correct, but it is the *explicit-device* path's polarity, not the `kAuto` path's: `device = 1` is an explicit accelerator request, and `model_loader.cpp:72-73` already says of that path "an explicit accelerator whose queue cannot be created must FAIL the load loudly, never silently serve on CPU" — -the same argument `ltx2_video.cpp:562-565` makes for refusing rather than serving +the same argument `ltx2_video.cpp:567-570` makes for refusing rather than serving the CPU forward behind an accelerator handle. So the lanes mirror the capability question from one path and the failure polarity from the other, and both halves are the seam's own. @@ -369,12 +380,24 @@ members, and `std::vector`. `tenstorrent.cpp`'s `supports_model_architecture` from `:52` to `:55`; the four citations on this branch (`ltx2_video.cpp`, `test_diffusion_device_seam.cpp`, and §0 and the residual note above) are corrected. `metal.cpp:70`, -`interface.h:263` and `model_loader.cpp:97` were re-derived on the merged tree -and all three still hold. Every `path:NN` anchor in this row's files was -resolved mechanically; the only remaining unresolvable ones are upstream Python -paths and two `examples/*/main.cpp` citations that carry their own `@ fc636c76` -and so are provenance rather than drift. The `minimax_h3_video.cpp:221-226` -citation in §0 is now anchored on the base SHA for the same reason. +`interface.h:263` and `model_loader.cpp:98` were re-derived on the merged tree +and all three still hold. The `minimax_h3_video.cpp:221-226 @ 11cc1d589` +citation in §0 is anchored on the base SHA for the same reason. + +**The completeness claim this paragraph originally carried was false, and round 4 +withdrew it.** It said "Every `path:NN` anchor in this row's files was resolved +mechanically; the only remaining unresolvable ones are upstream Python paths and +two `examples/*/main.cpp` citations". `be9b0a6fd` repeated it as "thirty-one +citations, five did not hold", and both were counting a set that did not include +the citations this branch's own edits had just rotted. Round 4 found six more +(§"Findings from review round 4"), all of them stale *inside this pull request*, +two of them seven and fifteen lines from the `:54` citation that SHA-anchors +itself with exactly the reasoning that applies to them. An enumeration that +certifies its own completeness is the defect this row keeps finding in its own +instruments, and it found it here in the record rather than in the checker. +No paragraph in this spec now claims that every anchor was checked; §"Findings +from review round 4" states what was re-derived, how, and what the method cannot +see. ## Findings from review round 3 (PR #671, head `79ebbce42`) @@ -481,12 +504,138 @@ And the enumeration must be the checker's own `rglob` over `SCAN_ROOTS`: a `include/vllm.h` starts with the characters `include/vllm` while sitting outside the `include/vllm` root. That off-by-one is the probe's, not the record's. +## Findings from review round 4 (PR #898, head `7502004aa`) + +The review returned **FAIL** on a change it found correct and a gate it found +honest. All eight findings are record, test-strength or message defects, and +every one of them is this row's own thesis pointed back at the row. + +**F1 — the "refuses BY NAME" assertion could not fail.** The LTX case asserted +`msg.find(kLtx2VideoFamily)`, and `Fail()` in `ltx2_video.cpp` prefixes EVERY +message with `"ltx-2.5 video: "`, which contains the family string verbatim. So +the assertion was satisfied by boilerplate on every refusal the file can throw, +including the two the same case asserts it is NOT. Proven, not deduced: +substituting `""` for the family name in the DECLINES `Fail` BUILT and +left the suite GREEN. The H3 side escaped only by a spelling coincidence — its +prefix is `"minimax_h3 video: "` with an underscore against a hyphenated +`"minimax-h3"` family — which is not a property and is now not relied on. Both +lanes assert the QUOTED SLOT, `architecture ''`, through one +`QuotedArchitecture()` helper that carries the reason. + +**F2 — the H3 half violated `## Nothing lands dead`.** The chain existed +(`vllm_video_engine_load` → `LoadVideoEngine` → the `minimax_h3` registration → +`MiniMaxH3VideoEngine::Load` → `MiniMaxH3VideoDeviceType`), but nothing entered +through it: every H3 device assertion called the resolver directly. Replacing the +`Load`-time call with the pre-row defect `params.device == 0 ? kCPU : kCUDA` +BUILT and left `test_diffusion_device_seam` and `test_minimax_h3_video_fold` +GREEN. The fold suite's `CUDA load creates exactly one queue` case does enter +through `Load`, but its `FakeCudaPlatform` reports `kCUDA`, so the seam and the +cast return the same answer and it cannot separate them. Two H3 cases now enter +at `LoadVideoEngine` against the declining `PartialXpuPlatform`, mirroring the +two LTX cases; and the two LTX cases were moved from `Ltx2VideoEngine::Load` to +`LoadVideoEngine` as well, so both lanes are entered at the same production +point and neither skips the registry hop. + +**F3 — the anchor sweep that claimed completeness was incomplete, and its own +edits are what falsified it.** Six citations were stale, all of them rotted +INSIDE this pull request. Two of them sat seven and fifteen lines from `:54`, +which SHA-anchors itself with exactly the reasoning that applies to them. + +| citation | was | is | +|---|---|---| +| `check-device-leakage.py:78` (`RE_KCUDA`) | unanchored | `@ 62406c30e` (on the branch: `:188`) | +| `test_minimax_h3_video_fold.cpp:162` (the `kCUDA` assertion) | unanchored | `@ 62406c30e` (on the branch: `:220`, `== accelerator`) | +| `test_minimax_h3_video_fold.cpp:161-164`, twice | unanchored | `@ 62406c30e` (on the branch: `:191-193` / `:215-238`) | +| `ltx2_video.cpp:549-562` (the two questions) | unanchored | `@ 11cc1d589` | +| `ltx2_video.cpp:562-565` (the refusal-to-fake-it argument) | `:562-565` | `:567-570` | +| `model_loader.cpp:97` (the capability clause), twice | `:97` | `:98` | + +Four more were tightened rather than repaired, because a range citation that +starts on the wrong line is the same defect one size smaller: `model_loader.cpp`'s +`SelectQueueForModel` is `:60-105` and was cited `:59-104`; its auto arm is +`:76-104` and was cited `:75-104`; the `kAuto` fall-through to CPU is `:104` and +was cited `:103`; and `ltx2_video.cpp`'s device block runs to the end of the +capability refusal at `:614`, where the citation stopped at `:610` and cut the +`Fail` in half. + +The completeness sentence is withdrawn, above, with the reason. What replaces it +is a statement of METHOD and of what the method cannot see: every `path:NN` on a +line this pull request ADDS was extracted mechanically from +`git diff -U0 $(git merge-base e8048ef63 HEAD)`, resolved at the final tree, and +read. It deliberately excludes citations this row did not write — the LTX lane's +upstream Python anchors and the other rows' C++ anchors in the same files — +because re-deriving those is a different row's work, and folding them in is how +the last sweep came to believe it had checked everything. A citation this row +inherits and did not touch is therefore NOT covered by this paragraph. + +Two limits of the method, stated because the previous sweep's failure was +believing it had none. The extractor reads a `path:NN` token; it cannot tell a +LIVE citation from one being QUOTED — the `was` column of the table above writes +`:97`, `:562-565` and two unanchored fold-test numbers deliberately, as the +record of what was wrong, and a later sweep will resolve them to the wrong lines +and must not "repair" them. And the count of citations is itself a measurement of +one tree, so it is not stored here: re-derive it rather than compare against a +number that rots for reasons unrelated to this row, which is the same rule the +scanned-file count above already carries. + +**F4 — `## Now` pointed at a closed pull request** (#671) and the pre-rebuild +branch. Corrected, with the relationship between the two stated rather than the +old number silently swapped out, because the round-1 to round-3 findings above +were made against #671's heads and a reader needs to be able to find them. + +**F5 — `dev_cast` over-matches a plain copy-initialisation.** +`vt::DeviceType d{other}` — a local copy, a member default-init, or an init from +a call returning `DeviceType` — fires alternative (3) while converting nothing. +Measured: 1 hit each, against 0 for `vt::DeviceType d{}`. It is NOT narrowed, +because `vt::DeviceType d{raw}` is the real conversion M36/M41 pin and is +textually identical; narrowing to remove the false positive deletes the true +positive. The docstring gains a third section, `WHAT dev_cast OVER-MATCHES`, +stating it with its cost — `dev_cast`'s baseline is a hard 0, so the first such +line under the scan roots fails the ratchet and needs `DSR-ALLOW` — and M47 pins +it in the M46 shape, with the value-init as the negative control in the same +test. Nothing in the tree writes the form today. + +**F6 — `include/vllm.h` still called the selector CUDA.** The public ABI field +`vllm_video_model_params.device`, the v14 `vllm_model_params.device` note that +cites it as precedent, `video_engine.h`'s mirror and `minimax_h3_video.h:88` +nineteen lines below the docstring this row rewrote all read `1 cuda`. +`include/vllm/config/device.h:19` repeats the same sentence and is corrected with +them; it is the fifth instance of one claim, not a fifth claim. + +**F7 — #828 was cited in product code and indexed nowhere.** +`check-device-leakage.py:58,139,158` and this spec name it as what closes the +declared blind spot, and it had no row in `.agents/issue-index.md`. +`check-agent-record.py` passed only because an absent row is nothing to count, +which is the instrument fault this row keeps finding. Appended with no owning row +and listed under `## Owed` below, which is the shape the protocol defines for an +issue a row files and does not fix. + +**F8** — an inverted sentence in the test header, which read as though the +refusal and the kernel death both happened. + +## Owed + +- [#828](https://github.com/mudler/vllm.cpp/issues/828) — the AST-level + `dev_cast` check. `check-device-leakage.py:58,139,158` and §"Findings from + review round 3" both name it as what closes the declared blind spot, and + `scripts/check-device-leakage.py`'s docstring names it as what would enforce + the property rather than a spelling list. It is **not built here**: this row + ships the interim regular expression, its measured spelling list, its declared + blind spots and its declared over-match. Filed with all four rounds' evidence, + because "we kept finding more spellings" is the argument for it. Indexed in + [`issue-index.md`](../issue-index.md) with no owning row, which is why it is + listed here. + ## Now `READY`, implemented and awaiting a fresh review on -`row/LTX25-DEVICE-SEAM-SIBLING` (PR #671). All three changes plus the F5 repair -are on the branch with their RED, GREEN and mutation evidence in the PR body; -next is a fresh reviewer — not the implementer — on the immutable head, then the +`row/LTX25-DEVICE-SEAM-SIBLING-REBUILD` ([PR +#898](https://github.com/mudler/vllm.cpp/pull/898)). PR #671, named by the +earlier rounds above, is CLOSED — it carried the same work on the pre-rebuild +branch, and the round-1 to round-3 findings recorded above were made against its +heads. All three changes plus the F5 repair and the round-4 repairs are on the +rebuild branch with their RED, GREEN and mutation evidence in the PR body; next +is a fresh reviewer — not the implementer — on the immutable head, then the operator's own gate rerun. The row stays `READY` deliberately. A lifecycle move to `ACTIVE` owes diff --git a/docs/USAGE.md b/docs/USAGE.md index 732f0f462..da5348155 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -1980,11 +1980,19 @@ only when it reproduces the identical weight contract the shapes describe, and supplying one for a checkpoint that already declares its own is refused rather than ordered. +`vllm_video_model_params.device` is `0` for the CPU and `1` for **the +accelerator this build resolves** — not for CUDA. The value is unchanged and it +is CUDA on a CUDA build, but it is read through the platform seam rather than as +an enum value, so the same `1` selects Metal, Vulkan or Tenstorrent on a build +that registers one of those, and is refused by name on a build that registers +none. The C ABI's text-generation `vllm_model_params.device` is a separate, +later selector with its own `0 = auto / 1 = cpu / 2 = cuda` numbering. + The LTX-2.5 arm runs on the CPU in f32 and on CUDA in bf16. `device = 0` takes the f32 parity forward; `device = 1` stages the DiT to the GPU one tensor at a time and runs the device-resident forward, so a CUDA handle means a CUDA forward. -On a build with no CUDA backend, `device = 1` is refused by name rather than -served the CPU forward behind a CUDA handle. It is also refused when the build's +On a build with no accelerator backend, `device = 1` is refused by name rather +than served the CPU forward behind an accelerator handle. It is also refused when the build's accelerator is a PARTIAL backend that declines this architecture — Metal and Tenstorrent each register the kernels for a named short list of models, and a backend that has not registered this one now says so by name instead of binding diff --git a/include/vllm.h b/include/vllm.h index 92ec2ed03..f2449b2b4 100644 --- a/include/vllm.h +++ b/include/vllm.h @@ -439,8 +439,9 @@ typedef struct vllm_model_params { * silently replaced by another (mirror of vLLM assigning an explicit * device verbatim, device.py:61-66). * 0 must stay auto so a zero-initialized struct preserves pre-v14 behaviour; - * the cpu-before-cuda value order follows the v12 precedent - * (vllm_video_model_params.device: 0 cpu, 1 cuda) shifted by the auto slot. + * the cpu-before-accelerator value order follows the v12 precedent + * (vllm_video_model_params.device: 0 cpu, 1 the resolved accelerator) + * shifted by the auto slot. * Any other value fails vllm_engine_load with VLLM_ERR_INVALID_ARGUMENT. */ int32_t device; /* ── KV-pool sizing (ABI v16) ────────────────────────────────────────────── @@ -874,7 +875,13 @@ typedef struct vllm_video_model_params { * are byte-structurally identical, so it must be DECLARED; NULL/empty makes * every generate refuse with the guidance (the #77 guard). */ const char* partition; - int32_t device; /* 0 cpu, 1 cuda */ + /* 0 is the CPU; 1 is THE ACCELERATOR THIS BUILD RESOLVES, through the + * platform seam (CurrentPlatform + TryGetBackend + + * supports_model_architecture), never the enum value 1. It is therefore CUDA + * on a CUDA build and refused BY NAME on a build with no accelerator backend, + * or one whose partial backend declines this architecture (#659, #660). The + * ABI value is unchanged; what it means was never "cuda". */ + int32_t device; int32_t dequant_bf16; /* 0 keep-quant, 1 dequant/stream bf16 */ int32_t fp4_resident; /* NVFP4+cuda: keep FP4 packed, Marlin W4A16 GEMM */ /* ── v18 additions (the generalized seam) ───────────────────────────────── diff --git a/include/vllm/config/device.h b/include/vllm/config/device.h index e0f9495b0..6ab0a69b2 100644 --- a/include/vllm/config/device.h +++ b/include/vllm/config/device.h @@ -15,8 +15,9 @@ // - The integer values are the C-ABI wire contract // (vllm_model_params.device, ABI v14): 0 MUST be auto so a zero-initialized // struct preserves the pre-v14 accelerator-first behaviour byte for byte; -// cpu-before-cuda then follows the shipped v12 precedent -// (vllm_video_model_params.device: 0 cpu, 1 cuda) shifted by the auto slot. +// cpu-before-accelerator then follows the shipped v12 precedent +// (vllm_video_model_params.device: 0 cpu, 1 the accelerator that build +// resolves) shifted by the auto slot. #ifndef VLLM_CONFIG_DEVICE_H_ #define VLLM_CONFIG_DEVICE_H_ diff --git a/include/vllm/multimodal/minimax_h3_video.h b/include/vllm/multimodal/minimax_h3_video.h index f59a0fdba..9bd086aa3 100644 --- a/include/vllm/multimodal/minimax_h3_video.h +++ b/include/vllm/multimodal/minimax_h3_video.h @@ -85,7 +85,9 @@ struct MiniMaxH3VideoModelParams { // declared-but-unknown and the #77 guard refuses every full render // (MiniMaxH3PartitionFromFlag / MiniMaxH3CheckTaskPartition). std::string partition; - int32_t device = 0; // 0 cpu, 1 cuda + // 0 cpu, 1 the accelerator this build resolves — see MiniMaxH3VideoDeviceType + // above; never `static_cast(device)` (#660). + int32_t device = 0; int32_t dequant_bf16 = 0; // 0 keep-quant, 1 dequant/stream bf16 // NVFP4 + cuda only: keep the packed FP4 resident and route the quantized // projections through the Marlin W4A16 GEMM (the pre-fold --fp4-resident). diff --git a/include/vllm/multimodal/video_engine.h b/include/vllm/multimodal/video_engine.h index 3119fa8ca..05edc4fa2 100644 --- a/include/vllm/multimodal/video_engine.h +++ b/include/vllm/multimodal/video_engine.h @@ -59,7 +59,10 @@ struct VideoModelParams { // value that is not registered is refused (it is never treated as a hint). std::string family; - int32_t device = 0; // 0 cpu, 1 cuda + // 0 is the CPU; 1 is the accelerator this build RESOLVES through the platform + // seam, not the enum value 1 (#659, #660). Refused by name when this build has + // no accelerator backend, or its partial backend declines the family. + int32_t device = 0; int32_t dequant_bf16 = 0; // 0 keep-quant, 1 dequant/stream bf16 int32_t fp4_resident = 0; // keep packed FP4 resident + quantized GEMM int64_t encoder_max_layers = 0; // 0 => all layers diff --git a/scripts/check-device-leakage.py b/scripts/check-device-leakage.py index 7b8e2f866..06d35ff98 100644 --- a/scripts/check-device-leakage.py +++ b/scripts/check-device-leakage.py @@ -138,7 +138,43 @@ and four rounds of finding a new one is the evidence that a spelling list is not the class. That gap does not close by widening; it closes at #828. -tests/scripts/test_device_leakage.py M20-M46 pin what it does catch, each +WHAT `dev_cast` OVER-MATCHES. The two lists above answer "what is caught" and +"what is missed". Neither answers "what is caught that is not the defect", and a +gate whose message omits its own false positives is the same instrument fault in +the other direction: the reader takes a RED as proof of leakage. + + * a plain COPY-INITIALISATION whose target is a DeviceType and whose operand + ALREADY IS one — `vt::DeviceType d{other}`, a member default-init + `struct S { vt::DeviceType d{kCPU}; }`, or an init from a call + `vt::DeviceType d{platform.device_type()}`. Alternative (3) fires and nothing + is converted. Measured on this pattern: 1 hit each; `vt::DeviceType d{}` + scores 0, because the empty-braces lookahead already rejects a + value-initialisation. + + This is NOT narrowed, and the reason is that narrowing it would delete a real + catch. `vt::DeviceType d{raw}` — the declaration spelling of the conversion, + which review round 3 found and M36/M41 pin — is textually identical to + `vt::DeviceType d{other}`. A text checker cannot tell the two apart, which is + the same statement as "nothing here type-checks", and is exactly what #828 + resolves. The `const_cast(t)` entry above is the other side of + it and must keep firing for the same reason. + + The COST is not zero, so it is stated rather than dismissed: `dev_cast`'s + baseline is a hard 0 outside the one allowlisted `platform.cpp` hit, so the + FIRST such copy-init written under `src/vllm/` or `include/vllm/` fails the + ratchet as a `DSR REGRESSION`. The remedy is `// DSR-ALLOW(): ` + on the line, which is the same remedy the wire-decode case gets, and it is + visible in CI output rather than in the diff. Nothing in the scanned tree + writes this form today — a repo-wide probe over the scan roots finds + `dev_cast` at exactly its allowlisted hit — so this is a documented cost of + the next line, not a live failure. + + M47 pins it, with `vt::DeviceType d{}` as the negative control in the same + test: if a later change makes the copy-init form stop firing, M47 goes RED + and this entry must be corrected in the same change. That is the M46 shape, + applied to a false positive instead of a blind spot. + +tests/scripts/test_device_leakage.py M20-M47 pin what it does catch, each spelling asserted on its own — including, in M29, the literal operand, because a discriminator tested only on the case it was tuned for is a guard that certifies itself. diff --git a/src/vllm/multimodal/minimax_h3_video.cpp b/src/vllm/multimodal/minimax_h3_video.cpp index bca2d182f..179d412df 100644 --- a/src/vllm/multimodal/minimax_h3_video.cpp +++ b/src/vllm/multimodal/minimax_h3_video.cpp @@ -223,8 +223,9 @@ void FillNoise(std::vector& out, uint64_t seed) { // // `device` is the public video ABI's selector: 0 is the CPU, 1 is "the // accelerator". WHICH accelerator is the PLATFORM's question, not this model -// file's — the same question `ltx2_video.cpp:566-610` and `SelectQueueForModel` -// (src/vllm/entrypoints/model_loader.cpp:75-104) ask. +// file's — the same question `ltx2_video.cpp:566-614` (the comment through the +// end of the capability refusal) and `SelectQueueForModel`'s auto arm +// (src/vllm/entrypoints/model_loader.cpp:76-104) ask. // // This used to be `static_cast(device)`, which is not a mapping // at all. It reads the ABI selector AS AN ENUM VALUE and is correct only for as diff --git a/tests/scripts/test_device_leakage.py b/tests/scripts/test_device_leakage.py index 9e675c9b6..c450d964d 100644 --- a/tests/scripts/test_device_leakage.py +++ b/tests/scripts/test_device_leakage.py @@ -890,6 +890,44 @@ def test_M46_the_declared_blind_spots_are_STILL_blind(self) -> None: ) self.assertEqual(self.tree.scan().counts["dev_cast"], 1) + def test_M47_the_declared_OVER_MATCH_still_fires(self) -> None: + # M46's shape, applied to a FALSE POSITIVE instead of a blind spot. + # + # Alternative (3) matches `vt::DeviceType d{x}` on the target type alone, + # so a plain COPY-INITIALISATION whose operand is already a DeviceType + # fires while converting nothing. The docstring's "WHAT `dev_cast` + # OVER-MATCHES" section states that, because a reader who takes a RED as + # proof of leakage is misled by a message that omits it. + # + # It is not narrowed, and the reason is that `vt::DeviceType d{raw}` — + # the declaration spelling of the real conversion, which M36 and M41 pin + # — is textually identical. Narrowing to remove the false positive + # deletes the true positive with it. So this test pins the CURRENT + # behaviour: if a later change makes any of these stop firing, it goes + # RED and the docstring entry must be corrected in the same change. + # + # All five compile (g++ 13.3 -std=c++20 -Wall -Wextra -fsyntax-only, + # exit 0), and each is asserted on its OWN file rather than appended to a + # shared one, so a spelling that stopped firing cannot be hidden by the + # next one still firing. + probe = "src/vllm/model_executor/models/probe.cpp" + for source in ( + "void L(vt::DeviceType other) { vt::DeviceType d{other}; (void)d; }\n", + "struct S { vt::DeviceType d{vt::DeviceType::kCPU}; };\n", + "void F(const Plat& p) { vt::DeviceType d{p.device_type()}; (void)d; }\n", + "struct T { vt::DeviceType const d{vt::DeviceType::kCPU}; };\n", + ): + with self.subTest(source=source.strip()): + self.tree.write(probe, source) + self.assertEqual(self.tree.scan().counts["dev_cast"], 1) + # The NEGATIVE control in the same test, and the boundary of the entry: a + # value-initialisation converts nothing AND does not fire, because the + # empty-braces lookahead already rejects it. Without this line the four + # assertions above would be consistent with "alternative (3) matches every + # `vt::DeviceType` declaration", which is a different and larger claim. + self.tree.write(probe, "void V() { vt::DeviceType d{}; (void)d; }\n") + self.assertEqual(self.tree.scan().counts["dev_cast"], 0) + def test_M28_dsr_allow_exempts_a_dev_cast_and_says_so_loudly(self) -> None: # The legitimate case the risk register names: a deserialization boundary # that reads a device off the wire. It buys an exemption only with a row diff --git a/tests/vllm/multimodal/test_diffusion_device_seam.cpp b/tests/vllm/multimodal/test_diffusion_device_seam.cpp index f554e9230..e4ed0b580 100644 --- a/tests/vllm/multimodal/test_diffusion_device_seam.cpp +++ b/tests/vllm/multimodal/test_diffusion_device_seam.cpp @@ -9,8 +9,8 @@ // of 75 ops and Tenstorrent a comparable slice, and both name exactly two // architectures (src/vllm/platforms/metal.cpp:70, // src/vllm/platforms/tenstorrent.cpp:55). On such a build a `device = 1` -// diffusion load WAS refused by name and became a queue bind that dies later -// inside a kernel with a shape error. +// diffusion load was NO LONGER refused by name, and became a queue bind that +// dies later inside a kernel with a shape error. // // CUDA cannot see any of this: `supports_model_architecture` defaults to true, // so on the box that runs the gates all three questions have always passed. That @@ -26,6 +26,19 @@ // (include/vt/device.h: kCPU = 0, kCUDA = 1). `kXPU` is 4. So an assertion that // device 1 resolves to kXPU is one the old integer cast could not satisfy under // any circumstances — it is the seam being asked, not a constant being returned. +// +// WHERE EACH CASE ENTERS (AGENTS.md `## Nothing lands dead`). Both lanes are +// entered through a PRODUCTION entry point, not by calling the resolver: +// `vllm_video_engine_load` (include/vllm.h) → `LoadVideoEngine` +// (src/vllm/multimodal/video_engine.cpp) → the family registration → the +// engine's `Load`, which is where each lane's device question is asked. The +// direct `MiniMaxH3VideoDeviceType(1)` cases below stay, because they localise a +// failure — but they are NOT the reach proof, and on their own they measure a +// free function rather than a capability. Measured: replacing H3's `Load`-time +// `MiniMaxH3VideoDeviceType(params.device)` with the pre-row +// `params.device == 0 ? kCPU : kCUDA` left every direct case, and the whole of +// test_minimax_h3_video_fold, GREEN. The case that enters at `LoadVideoEngine` +// is the one that goes RED. #include #include @@ -37,6 +50,7 @@ #include "vllm/multimodal/ltx2_video.h" #include "vllm/multimodal/minimax_h3_video.h" +#include "vllm/multimodal/video_engine.h" #include "vllm/platforms/interface.h" #include "vt/backend.h" #include "vt/device.h" @@ -118,15 +132,55 @@ vllm::multimodal::VideoModelParams DeviceOneParams() { return mp; } +// The H3 sibling of DeviceOneParams(), entered through the SAME generic struct +// so it can go in at `LoadVideoEngine` with the family declared. The path does +// not exist for the same reason: a load that got past the capability clause +// would die opening it, with a different message. +vllm::multimodal::VideoModelParams H3DeviceOneParams() { + vllm::multimodal::VideoModelParams mp; + mp.family = vllm::multimodal::kMiniMaxH3VideoFamily; + mp.dit_path = "/nonexistent/h3-dit-that-is-never-opened.gguf"; + mp.device = 1; + return mp; +} + +// The PRODUCTION entry, for BOTH lanes: `vllm_video_engine_load` +// (include/vllm.h) calls `LoadVideoEngine` through src/capi/vllm_c.cpp, and it +// dispatches on the declared family to the registration in the engine's own TU, +// which calls that engine's `Load`. Nothing in this helper names an engine type, +// which is the point — it is the path a user arrives on, and entering at +// `Ltx2VideoEngine::Load` or `MiniMaxH3VideoEngine::Load` instead would skip the +// registry hop and prove one link less. std::string LoadError(const vllm::multimodal::VideoModelParams& mp) { try { - (void)vllm::multimodal::Ltx2VideoEngine::Load(mp); + (void)vllm::multimodal::LoadVideoEngine(mp); } catch (const std::exception& e) { return e.what(); } return ""; } +// The architecture name in the refusal's QUOTED SLOT, rather than anywhere in +// the message. +// +// `Fail()` in src/vllm/multimodal/ltx2_video.cpp prefixes EVERY message it +// throws with "ltx-2.5 video: ", and `kLtx2VideoFamily` is the string +// "ltx-2.5". So `msg.find(kLtx2VideoFamily)` is satisfied by that boilerplate on +// every refusal this file can produce — including the two refusals the +// assertions below exist to distinguish this one FROM — and no defect in the +// message can make it fail. Measured: replacing the family name with +// "" inside the DECLINES `Fail` left it GREEN. +// +// The row's thesis for #659 is that a partial backend declines BY NAME, so the +// assertion has to be that the architecture appears where the refusal names it. +// The H3 pair happens not to collide — its prefix spells the family with an +// underscore ("minimax_h3 video: ") while the family is hyphenated +// ("minimax-h3") — but that is a coincidence of spelling and not a property, so +// the H3 cases are built the same way. +std::string QuotedArchitecture(std::string_view family) { + return std::string("architecture '") + std::string(family) + "'"; +} + } // namespace TEST_CASE("ltx2 video: a platform that DECLINES the architecture refuses device 1 by name") { @@ -146,7 +200,8 @@ TEST_CASE("ltx2 video: a platform that DECLINES the architecture refuses device // BY NAME: the platform, the architecture, and the fact that the backend // declined rather than that something was malformed. CHECK(msg.find("xpu") != std::string::npos); - CHECK(msg.find(vllm::multimodal::kLtx2VideoFamily) != std::string::npos); + CHECK(msg.find(QuotedArchitecture(vllm::multimodal::kLtx2VideoFamily)) != + std::string::npos); CHECK(msg.find("DECLINES") != std::string::npos); CHECK(msg.find("supports_model_architecture") != std::string::npos); // And it must not be the OTHER refusal: a backend IS registered here, so @@ -205,10 +260,62 @@ TEST_CASE("minimax_h3 video: a platform that DECLINES the architecture refuses d } INFO(msg); CHECK(msg.find("xpu") != std::string::npos); - CHECK(msg.find(vllm::multimodal::kMiniMaxH3VideoFamily) != std::string::npos); + CHECK(msg.find(QuotedArchitecture(vllm::multimodal::kMiniMaxH3VideoFamily)) != + std::string::npos); CHECK(msg.find("DECLINES") != std::string::npos); CHECK(msg.find("no accelerator backend is registered") == std::string::npos); // 0 still resolves, because the CPU asks none of these questions. CHECK(vllm::multimodal::MiniMaxH3VideoDeviceType(0) == vt::DeviceType::kCPU); } + +// ── REACH (AGENTS.md `## Nothing lands dead`) ──────────────────────────────── +// +// The two cases above call `MiniMaxH3VideoDeviceType` directly, which measures +// the free function and NOT whether anything routes to it. The H3 engine's +// `Load` is the only production caller (src/vllm/multimodal/minimax_h3_video.cpp, +// the `MiniMaxH3VideoDeviceType(params.device)` line), and it is reached from +// `vllm_video_engine_load` → `LoadVideoEngine` → the `minimax_h3` registration. +// Delete that one call and every other H3 device assertion in this tree stays +// green; this case is what turns red. +// +// It is the H3 mirror of the two LTX cases at the top of this file, and it +// exists because the LTX half already had this proof and the H3 half did not. +TEST_CASE("minimax_h3 video: a DECLINING platform refuses device 1 through LoadVideoEngine") { + RegisterPartialAccelerator(/*accepts_everything=*/false); + + REQUIRE(vllm::platforms::CurrentPlatform().device_type() == vt::DeviceType::kXPU); + REQUIRE(vt::TryGetBackend(vt::DeviceType::kXPU) != nullptr); + REQUIRE_FALSE(vllm::platforms::CurrentPlatform().supports_model_architecture( + vllm::multimodal::kMiniMaxH3VideoFamily)); + + const std::string msg = LoadError(H3DeviceOneParams()); + INFO(msg); + REQUIRE_FALSE(msg.empty()); + CHECK(msg.find("xpu") != std::string::npos); + CHECK(msg.find(QuotedArchitecture(vllm::multimodal::kMiniMaxH3VideoFamily)) != + std::string::npos); + CHECK(msg.find("DECLINES") != std::string::npos); + CHECK(msg.find("supports_model_architecture") != std::string::npos); + // Not the OTHER refusal: a backend IS registered for the resolved platform, so + // blaming a missing one is a wrong diagnosis that reads as a right one. + CHECK(msg.find("no accelerator backend is registered") == std::string::npos); + // Nor the failure the guard exists to prevent, one step further on. Reverting + // the `Load` call site to the integer cast lands here instead: `kCUDA` with no + // CUDA backend registered in this process. + CHECK(msg.find("/nonexistent/") == std::string::npos); +} + +TEST_CASE("minimax_h3 video: a COMPLETE backend is not refused through LoadVideoEngine") { + // The mirror arm, for the same reason the LTX pair has one: two absences pass + // on any OTHER wrong failure, so the guard's "does not refuse a working + // configuration" half needs a POSITIVE assertion about where the load got to. + RegisterPartialAccelerator(/*accepts_everything=*/true); + + const std::string msg = LoadError(H3DeviceOneParams()); + INFO(msg); + REQUIRE_FALSE(msg.empty()); + CHECK(msg.find("DECLINES") == std::string::npos); + CHECK(msg.find("supports_model_architecture") == std::string::npos); + CHECK(msg.find("/nonexistent/h3-dit-that-is-never-opened.gguf") != std::string::npos); +} From 0600a7ef12d675c49b6b9230c0485646bbd8f66c Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Sat, 15 Aug 2026 13:28:09 +0000 Subject: [PATCH 10/12] record(LTX25-DEVICE-SEAM-SIBLING): re-derive the anchors the merge moved by 43 lines, and name the mutants that actually red MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `0785cfc4d` edited `src/vllm/multimodal/ltx2_video.cpp` above this branch's device block, so every line number this row cites inside that block shifted: the refusal-to-fake-it argument from `:567-570` to `:610-613`, and the block `minimax_h3_video.cpp` cites from `:566-614` to `:609-657`. Re-derived at the merged tree with a token that must match EXACTLY ONCE in its file, and with a deliberately wrong line as a positive control in the same run: 27 HOLDS, 1 STALE, and the STALE one is the control. A run in which the control does not report STALE proves nothing about the ones that report HOLDS. The spec records that this is the second time in one review round that these anchors rotted, and the rule it implies: take the merge first, derive the anchors at the tree that is pushed. Deriving them before a merge measures a denominator that is about to move. Two smaller corrections in the same sweep, both of the same kind — a number written from memory rather than from a run. The F5 argument for not narrowing `dev_cast` named M36 and M41 as the real declaration-form catches that a narrowing would delete. Measured: narrowing (3)'s initialiser to a digit reds **M32 and M36**, not M41. The docstring, the M47 comment and the spec now name the two the mutation actually reds, because an argument that rests on which test would fail has to name the test that does. And the branch-side line numbers beside the `@ 62406c30e` fold-test anchors were one line high: the three untouched contract assertions are at `:192-194`, and the two arms that replace `:162` at `:218-238`. `:191` is the `TEST_CASE` line. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- .agents/specs/ltx25-device-seam-sibling.md | 23 +++++++++++++++------- scripts/check-device-leakage.py | 2 +- src/vllm/multimodal/minimax_h3_video.cpp | 2 +- tests/scripts/test_device_leakage.py | 2 +- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md index 5f52a1f45..a579b13f3 100644 --- a/.agents/specs/ltx25-device-seam-sibling.md +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -214,7 +214,7 @@ which is precisely the cast's answer and cannot survive the change. It is now build-conditional and asserts BOTH arms: `== accelerator` where one is registered, refused-by-name where none is. 161/163/164 are untouched. Every number in this paragraph is `@ 62406c30e`: on the branch the four assertions sit -at `:191-193` and the two arms at `:215-238`. +at `:192-194` and the two arms that replace `:162` at `:218-238`. **`test_minimax_h3_video_fold.cpp`'s CUDA-load case registered a BACKEND and no PLATFORM.** It could, because the cast never asked whether the build had an @@ -307,7 +307,7 @@ That is correct, but it is the *explicit-device* path's polarity, not the `kAuto` path's: `device = 1` is an explicit accelerator request, and `model_loader.cpp:72-73` already says of that path "an explicit accelerator whose queue cannot be created must FAIL the load loudly, never silently serve on CPU" — -the same argument `ltx2_video.cpp:567-570` makes for refusing rather than serving +the same argument `ltx2_video.cpp:610-613` makes for refusing rather than serving the CPU forward behind an accelerator handle. So the lanes mirror the capability question from one path and the failure polarity from the other, and both halves are the seam's own. @@ -545,9 +545,9 @@ which SHA-anchors itself with exactly the reasoning that applies to them. |---|---|---| | `check-device-leakage.py:78` (`RE_KCUDA`) | unanchored | `@ 62406c30e` (on the branch: `:188`) | | `test_minimax_h3_video_fold.cpp:162` (the `kCUDA` assertion) | unanchored | `@ 62406c30e` (on the branch: `:220`, `== accelerator`) | -| `test_minimax_h3_video_fold.cpp:161-164`, twice | unanchored | `@ 62406c30e` (on the branch: `:191-193` / `:215-238`) | +| `test_minimax_h3_video_fold.cpp:161-164`, twice | unanchored | `@ 62406c30e` (on the branch: the three untouched at `:192-194`, the two arms replacing `:162` at `:218-238`) | | `ltx2_video.cpp:549-562` (the two questions) | unanchored | `@ 11cc1d589` | -| `ltx2_video.cpp:562-565` (the refusal-to-fake-it argument) | `:562-565` | `:567-570` | +| `ltx2_video.cpp:562-565` (the refusal-to-fake-it argument) | `:562-565` | `:610-613` | | `model_loader.cpp:97` (the capability clause), twice | `:97` | `:98` | Four more were tightened rather than repaired, because a range citation that @@ -555,8 +555,9 @@ starts on the wrong line is the same defect one size smaller: `model_loader.cpp` `SelectQueueForModel` is `:60-105` and was cited `:59-104`; its auto arm is `:76-104` and was cited `:75-104`; the `kAuto` fall-through to CPU is `:104` and was cited `:103`; and `ltx2_video.cpp`'s device block runs to the end of the -capability refusal at `:614`, where the citation stopped at `:610` and cut the -`Fail` in half. +capability refusal, where the citation stopped four lines short and cut the +`Fail` in half. That last one is now `:609-657`, having been `:566-614` for the +length of one merge — see the next paragraph. The completeness sentence is withdrawn, above, with the reason. What replaces it is a statement of METHOD and of what the method cannot see: every `path:NN` on a @@ -568,6 +569,14 @@ because re-deriving those is a different row's work, and folding them in is how the last sweep came to believe it had checked everything. A citation this row inherits and did not touch is therefore NOT covered by this paragraph. +**It rotted again during this round, which is the paragraph's own point made +twice.** `0785cfc4d` (`LTX25-RETIRE-DEAD-ARMS`) landed on main mid-gate and +edited `ltx2_video.cpp` above this branch's device block, moving all four of its +live line anchors by 43. The re-derivation caught it because it was re-run after +the merge rather than before it, and a run whose denominator moves afterwards +proves nothing. So the merge is taken FIRST and the anchors are derived at the +tree that is pushed. + Two limits of the method, stated because the previous sweep's failure was believing it had none. The extractor reads a `path:NN` token; it cannot tell a LIVE citation from one being QUOTED — the `was` column of the table above writes @@ -587,7 +596,7 @@ were made against #671's heads and a reader needs to be able to find them. `vt::DeviceType d{other}` — a local copy, a member default-init, or an init from a call returning `DeviceType` — fires alternative (3) while converting nothing. Measured: 1 hit each, against 0 for `vt::DeviceType d{}`. It is NOT narrowed, -because `vt::DeviceType d{raw}` is the real conversion M36/M41 pin and is +because `vt::DeviceType d{raw}` is the real conversion M32/M36 pin and is textually identical; narrowing to remove the false positive deletes the true positive. The docstring gains a third section, `WHAT dev_cast OVER-MATCHES`, stating it with its cost — `dev_cast`'s baseline is a hard 0, so the first such diff --git a/scripts/check-device-leakage.py b/scripts/check-device-leakage.py index 06d35ff98..45ccb0afc 100644 --- a/scripts/check-device-leakage.py +++ b/scripts/check-device-leakage.py @@ -153,7 +153,7 @@ This is NOT narrowed, and the reason is that narrowing it would delete a real catch. `vt::DeviceType d{raw}` — the declaration spelling of the conversion, - which review round 3 found and M36/M41 pin — is textually identical to + which review rounds 1 and 3 found and M32/M36 pin — is textually identical to `vt::DeviceType d{other}`. A text checker cannot tell the two apart, which is the same statement as "nothing here type-checks", and is exactly what #828 resolves. The `const_cast(t)` entry above is the other side of diff --git a/src/vllm/multimodal/minimax_h3_video.cpp b/src/vllm/multimodal/minimax_h3_video.cpp index 179d412df..65a69cda0 100644 --- a/src/vllm/multimodal/minimax_h3_video.cpp +++ b/src/vllm/multimodal/minimax_h3_video.cpp @@ -223,7 +223,7 @@ void FillNoise(std::vector& out, uint64_t seed) { // // `device` is the public video ABI's selector: 0 is the CPU, 1 is "the // accelerator". WHICH accelerator is the PLATFORM's question, not this model -// file's — the same question `ltx2_video.cpp:566-614` (the comment through the +// file's — the same question `ltx2_video.cpp:609-657` (the comment through the // end of the capability refusal) and `SelectQueueForModel`'s auto arm // (src/vllm/entrypoints/model_loader.cpp:76-104) ask. // diff --git a/tests/scripts/test_device_leakage.py b/tests/scripts/test_device_leakage.py index c450d964d..9c0fc030d 100644 --- a/tests/scripts/test_device_leakage.py +++ b/tests/scripts/test_device_leakage.py @@ -900,7 +900,7 @@ def test_M47_the_declared_OVER_MATCH_still_fires(self) -> None: # proof of leakage is misled by a message that omits it. # # It is not narrowed, and the reason is that `vt::DeviceType d{raw}` — - # the declaration spelling of the real conversion, which M36 and M41 pin + # the declaration spelling of the real conversion, which M32 and M36 pin # — is textually identical. Narrowing to remove the false positive # deletes the true positive with it. So this test pins the CURRENT # behaviour: if a later change makes any of these stop firing, it goes From 4e7a708adac594d9fe11e52ba15e8679edcdcf40 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Sat, 15 Aug 2026 13:44:49 +0000 Subject: [PATCH 11/12] fix(LTX25-DEVICE-SEAM-SIBLING): re-derive the READER ANCHORS that this branch's own merge shifted by 30 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `0785cfc4d` records nine line numbers in `ltx2_video.cpp`'s `READER ANCHORS` comment and gates them with `test_ltx2_video`'s "the recorded reader anchors are the ones in the source". This branch adds the capability guard ABOVE all nine, so the merge moved every one of them by exactly 30 and the gate went RED with `recorded == derived` false. `git merge` reported no conflict and `git merge-tree` would have reported none either, because nothing overlapped textually. The defect is semantic: one side records line numbers of a file the other side edits. A clean merge is not a merge that passes, and the only thing that catches this class is running the suite on the merged tree. Re-derived from the failure's own instruction — it prints the actual list — and confirmed: `test_ltx2_video` 40 cases / 40 passed, 827 assertions / 827 passed. Two notes for whoever reads this next. The number that rotted belongs to `0785cfc4d`, not to this row, so this is not a record this row owns editing away from — it is the record this row's diff invalidated, which AGENTS.md puts in the pull request whose change made it stale. And the anchor gate that caught it is the same instrument this row has been arguing for in prose all round: a recorded `path:NN` that nothing re-derives is a claim, and one that a test re-derives is a fact. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- src/vllm/multimodal/ltx2_video.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vllm/multimodal/ltx2_video.cpp b/src/vllm/multimodal/ltx2_video.cpp index 62bc63892..56d0c634b 100644 --- a/src/vllm/multimodal/ltx2_video.cpp +++ b/src/vllm/multimodal/ltx2_video.cpp @@ -281,7 +281,7 @@ constexpr char kLtx2DurationHeadPathExtra[] = "duration_head_path"; // they are no longer trusted: the list below is derived from this file on every // run and compared, and the failure prints the replacement to paste in. // READER ANCHORS (derived and gated by test_ltx2_video): -// 660 715 811 827 829 899 924 1029 1070 +// 690 745 841 857 859 929 954 1059 1100 const char* const kKnownLoadExtras[] = { kLtx2AudioPromptEmbedsExtra, kLtx2PipelineKindExtra, kLtx2ModelVersionExtra, kLtx2AllowUnportedExtra, kLtx2MaxPhaseExtra, kLtx2DitConfigPathExtra, From 2ce0625efc95bc076f0f47bb37c2c33434fa653d Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Sat, 15 Aug 2026 14:56:20 +0000 Subject: [PATCH 12/12] record(LTX25-DEVICE-SEAM-SIBLING): the anchor that rotted between two halves of one commit An anchor into a file the change is itself editing is stale by default. This one did not need a merge to rot: `f0b465029` carries BOTH the F3 anchor table, which records `check-device-leakage.py`'s `RE_KCUDA` at `:188`, AND the F5 `WHAT dev_cast OVER-MATCHES` docstring section, which inserts 36 lines above that regular expression and moves it to `:224`. The F5 half of the commit falsified the F3 half of the same commit. Corrected in both places in this spec, and in the pull request body, re-derived at the tree being pushed. The `:54` self-citation went the same way and is the sharper case, because it is the paragraph the row uses as its EXEMPLAR of good anchoring practice. At `7502004aa` `:54` was section 0(b)'s `minimax_h3_video.cpp:221-226 @ 11cc1d589` citation -- the "seven and fifteen lines" distances resolve against it exactly, which is how it was identified rather than guessed. `f0b465029` then added the anchors F3 had asked for, section 0 grew, and `:54` became a blank line. All three uses now name that citation in PROSE. A line number pointing into this file is the one citation no re-derivation of the TREE can ever check, and this row has now rotted it twice, so the fix is to stop writing one. The method paragraph claimed two limits and had three. The third is what let both of these through: a bare `:NN` whose path sits in a NEIGHBOURING token is never extracted, because the pattern requires path and number to be glued. Both findings are that shape. The limit is now stated, and its count deliberately is not stored, per the rule the same paragraph already carries about a measurement of one tree kept inside another file. Re-derivation: every citation extracted, bare continuations included, and every live repo-local one checked at this tree with the needle for each derived from what the spec CLAIMS the span holds rather than read back out of the cited file -- a validator that reads its expectation from its target re-derives f(x) == f(x) and cannot fail. A deliberately wrong row rode in the same run as a positive control and reported STALE, so the harness is known to discriminate. 50 rows FRESH, control STALE. Two observations reported and NOT repaired because neither is wrong: `ltx2_video.cpp:609-657` opens one line inside the preceding comment separator rather than on the block head at `:614`, and round 4's repair was to its tail, which is correct at `:657`; and `check-device-leakage.py:58,139,158` is a genuine three-way citation whose needle cannot be unique by construction. Records only. No `src/`, `include/`, `tests/` or `scripts/` file is touched. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code] --- .agents/specs/ltx25-device-seam-sibling.md | 75 +++++++++++++++++++--- 1 file changed, 65 insertions(+), 10 deletions(-) diff --git a/.agents/specs/ltx25-device-seam-sibling.md b/.agents/specs/ltx25-device-seam-sibling.md index a579b13f3..8408d2ed0 100644 --- a/.agents/specs/ltx25-device-seam-sibling.md +++ b/.agents/specs/ltx25-device-seam-sibling.md @@ -16,9 +16,10 @@ the lane it was aimed at and left two things standing. **(a) #659 — the seam was adopted, its companion guard was not.** `src/vllm/multimodal/ltx2_video.cpp:549-562 @ 11cc1d589` — anchored on the base -SHA for the same reason `:54` below is, because this row rewrites that block and -an unanchored number would point into the middle of the repair the moment it -lands — now asks two questions: +SHA for the same reason §0(b)'s `minimax_h3_video.cpp:221-226 @ 11cc1d589` +citation below is, because this row rewrites that block and an unanchored number +would point into the middle of the repair the moment it lands — now asks two +questions: ```cpp const vt::DeviceType accelerator = vllm::platforms::CurrentPlatform().device_type(); @@ -49,7 +50,7 @@ property one level down: it is a device claim the build cannot honour. **(b) #660 — the gate that certified (a) is a token grep, and the sibling lane spells its way past it.** `scripts/check-device-leakage.py:78 @ 62406c30e` — -anchored, because this row rewrites that file's head and the line is now `:188` +anchored, because this row rewrites that file's head and the line is now `:224` — is ```python @@ -391,8 +392,9 @@ two `examples/*/main.cpp` citations". `be9b0a6fd` repeated it as "thirty-one citations, five did not hold", and both were counting a set that did not include the citations this branch's own edits had just rotted. Round 4 found six more (§"Findings from review round 4"), all of them stale *inside this pull request*, -two of them seven and fifteen lines from the `:54` citation that SHA-anchors -itself with exactly the reasoning that applies to them. An enumeration that +two of them seven and fifteen lines (measured at `7502004aa`) from §0(b)'s +`minimax_h3_video.cpp:221-226 @ 11cc1d589` citation, which SHA-anchors itself +with exactly the reasoning that applies to them. An enumeration that certifies its own completeness is the defect this row keeps finding in its own instruments, and it found it here in the record rather than in the checker. No paragraph in this spec now claims that every anchor was checked; §"Findings @@ -538,12 +540,13 @@ point and neither skips the registry hop. **F3 — the anchor sweep that claimed completeness was incomplete, and its own edits are what falsified it.** Six citations were stale, all of them rotted -INSIDE this pull request. Two of them sat seven and fifteen lines from `:54`, +INSIDE this pull request. Two of them sat seven and fifteen lines (measured at +`7502004aa`) from §0(b)'s `minimax_h3_video.cpp:221-226 @ 11cc1d589` citation, which SHA-anchors itself with exactly the reasoning that applies to them. | citation | was | is | |---|---|---| -| `check-device-leakage.py:78` (`RE_KCUDA`) | unanchored | `@ 62406c30e` (on the branch: `:188`) | +| `check-device-leakage.py:78` (`RE_KCUDA`) | unanchored | `@ 62406c30e` (on the branch: `:224`) | | `test_minimax_h3_video_fold.cpp:162` (the `kCUDA` assertion) | unanchored | `@ 62406c30e` (on the branch: `:220`, `== accelerator`) | | `test_minimax_h3_video_fold.cpp:161-164`, twice | unanchored | `@ 62406c30e` (on the branch: the three untouched at `:192-194`, the two arms replacing `:162` at `:218-238`) | | `ltx2_video.cpp:549-562` (the two questions) | unanchored | `@ 11cc1d589` | @@ -577,16 +580,29 @@ the merge rather than before it, and a run whose denominator moves afterwards proves nothing. So the merge is taken FIRST and the anchors are derived at the tree that is pushed. -Two limits of the method, stated because the previous sweep's failure was +Three limits of the method, stated because the previous sweep's failure was believing it had none. The extractor reads a `path:NN` token; it cannot tell a LIVE citation from one being QUOTED — the `was` column of the table above writes `:97`, `:562-565` and two unanchored fold-test numbers deliberately, as the record of what was wrong, and a later sweep will resolve them to the wrong lines -and must not "repair" them. And the count of citations is itself a measurement of +and must not "repair" them. The count of citations is itself a measurement of one tree, so it is not stored here: re-derive it rather than compare against a number that rots for reasons unrelated to this row, which is the same rule the scanned-file count above already carries. +And — the limit that let round 5 find two more — **a bare `:NN` whose path sits +in a NEIGHBOURING token is never extracted at all.** The pattern requires the +path and the number to be glued, so `` `:224` `` written one line below +`check-device-leakage.py:78`, and a bare `` `:54` `` pointing at this file +itself, are invisible to it. They are not a rare shape — on the tree this +paragraph is pushed on they are a large minority of this spec's citations, and +the exact figure is deliberately NOT stored here for the reason the limit above +already gives. Re-derive it. That class is what the sweep which wrote this +paragraph could not see, and both of round 5's anchor findings belong to it. An +extractor for them must carry the antecedent path forward and must report when +there is none, because "no antecedent" is exactly the signature of a citation +into this file rather than into the tree. + **F4 — `## Now` pointed at a closed pull request** (#671) and the pre-rebuild branch. Corrected, with the relationship between the two stated rather than the old number silently swapped out, because the round-1 to round-3 findings above @@ -622,6 +638,45 @@ issue a row files and does not fix. **F8** — an inverted sentence in the test header, which read as though the refusal and the kernel death both happened. +## Findings from review round 5 (PR #898, head `bf77c944e`) + +Every code, test and checker repair passed. All four findings are record +defects, and two of them are the same one: **an anchor into a file the change is +itself editing is stale by default, and this one rotted between the two halves +of a SINGLE commit.** + +**FF1 — `check-device-leakage.py`'s `RE_KCUDA` was recorded at `:188` and the +recording commit is what moved it to `:224`.** `f0b465029` carries both the F3 +anchor table, which writes `:188`, and the F5 `WHAT dev_cast OVER-MATCHES` +docstring section, which adds 36 lines above `RE_KCUDA`. So the F5 half +falsified the F3 half of the same commit, and no merge was involved — the usual +suspect for anchor drift, and here it was innocent. Corrected to `:224` in both +places and in the pull request body, re-derived at the tree that is pushed. + +**FF4 — the `:54` self-citation pointed at a blank line, and it is the paragraph +used as the exemplar of GOOD anchoring practice.** Three places cited "the `:54` +citation that SHA-anchors itself". At `7502004aa`, `:54` was §0(b)'s +`minimax_h3_video.cpp:221-226 @ 11cc1d589` citation, and the distances "seven +and fifteen lines" resolve against it exactly. `f0b465029` then added the very +anchors F3 asked for, §0 grew, and `:54` became blank. It is now named in PROSE +rather than by line number, because a number pointing into this file is the one +citation no re-derivation of the TREE can ever check, and this row has now +rotted it twice. + +**Method.** Every citation was extracted — bare continuations included, which is +the change — and every live repo-local one re-derived at the pushed tree, with +the needle for each derived from what this +spec CLAIMS the span contains rather than read back out of the cited file — a +validator that reads its expectation from its target re-derives `f(x) == f(x)` +and cannot fail. A deliberately wrong row was carried as a positive control in +the same run and reported STALE, so the harness is known to discriminate. 50 +rows FRESH, control STALE. Two further observations, reported and NOT repaired +because neither is wrong: `ltx2_video.cpp:609-657` opens one line inside the +preceding comment separator rather than on the block head at `:614` (the round-4 +repair was to its TAIL, which is correct at `:657`), and +`check-device-leakage.py:58,139,158` is a genuine three-way citation whose needle +cannot be unique by construction. + ## Owed - [#828](https://github.com/mudler/vllm.cpp/issues/828) — the AST-level