A fresh review of PR #1132 at e7d0a1f7c (fixing #1123) returned FAIL: one blocking gate failure and four wrong claims. The code is right; six consecutive reviews on this row family have now found the code right and a sentence about it wrong. Row ENG-EXPERT-STREAM, spec .agents/specs/expert-streaming.md, issues #1123, #1124, #1126.
F1 [HIGH, BLOCKS LANDING] — commit-protocol-tag is red on two unauthored merge commits
058f5f31a and e7d0a1f7c both carry the default git merge subject and an empty body. Verified directly:
subject: Merge remote-tracking branch 'origin/main' into row/ENG-EXPERT-STREAM-DEVICE-FIT
body_bytes:
scripts/check-commit-trailers.py:336 walks rev-list --reverse base..head with no merge exclusion, so both fail the strict contract. CI agrees (commit-protocol-tag FAILURE). The PR body's claim that agent-preflight.sh --staged returned rc 0 with all 23 checkers ok is false at this head — reproduced locally as rc 1 in both variants.
Matched control, which is what makes this the PR's own defect rather than a lane artifact: PR #1119 carries three merge commits and passes, because it authors them — subject, bare FOLLOWING_AGENTS_PROTOCOL, full trailer block (0f12ce4e1).
Note CI's first trailer step skips merges (ci.yml:552) and the second does not (ci.yml:604), so a partial read of CI would miss this.
F2 [MEDIUM] — the false comment was not corrected, and three records say it was
include/vt/backend.h:79-80 still reads "ROCm/CUDA override with hipMemGetInfo/cudaMemGetInfo". That file is not in the diff (git diff <base> HEAD -- include/vt/backend.h is empty; last touched by unrelated 9302732f7). Yet the PR body says "The false comment is corrected in prose", the spec's ## Owed says "The comment is corrected in the same change", and the issue-index row for #1126 says "corrected in prose by #1123". The ## Owed row is internally contradictory — its left cell says the comment "still says" it.
This is the exact comment the spec blames for making the absent CUDA query look present.
F3 [MEDIUM] — "can never over-refuse" is false; measured 8.33 GiB window, untested direction
The spec and commit body assert the footprint is "A LOWER bound … so the refusal can never over-refuse". A tensor counted but never staged is a positive over-count, and one exists on every default load: MTP weights attach only under params.speculative_config.has_value() && method == "mtp" (model_loader.cpp:1411-1412), so the nextn block — 8,940,488,704 bytes (8.33 GiB), 2.2506 % — is counted and not staged. A budget landing in [staged, counted) refuses a weight set that fits.
gguf_device_fit.h's own header states this over-count honestly; the spec and commit body contradict it. The "under-count dominates" argument does not rescue it — the two errors are on different quantities and do not cancel. No test covers this direction: the fixture has two tensors, both staged, so the boundary cases pin > vs >= against a footprint equal to the true staged size.
Fix: adopt the header's wording, and add a case with a counted-but-unstaged tensor.
F4 [MEDIUM] — the only finding where the code can remove a working path
model_loader.h:202-210 claims ResolveModelDeviceType picks what SelectQueueForModel will pick, so "the two cannot drift". On the auto arm they can: SelectQueueForModel wraps vt::GetBackend(dev).CreateQueue() in try/catch and falls back to CPU (model_loader.cpp:135-141), whose own comment says a platform can be registered while CreateQueue still fails. ResolveModelDeviceType has no such catch, so on such a box the loader refuses naming device 'cuda' for a model that previously loaded and served on CPU.
Mutation (delete that CPU fallback) is NOT-CAUGHT — nothing in the tree pins it; test_device_selection covers only the explicit-named arm. An escape hatch exists (VT_DEVICE_WEIGHT_BUDGET_BYTES=0), so not fatal.
F5 / L1-L4 [LOW]
- "Release (what CI uses)" is wrong:
build-test-cpu (ci.yml:903-911) passes no CMAKE_BUILD_TYPE, so CI's test lane defines no NDEBUG. The only Release configures are the CUDA fat build with VLLM_CPP_BUILD_TESTS=OFF. Asserts were masked for no reason; re-running with asserts live changed nothing, so no defect was hidden — the claim is just false.
BuildMoeMarlinResident (:6010-6062) — the function is :6010-6215, allocations to :6064. A sixth stale anchor of the class the audit says it swept.
docs/USAGE.md is wrong in both halves of one bullet: a discrete NVIDIA GPU is CudaPlatform and does get the refusal; ROCm/Vulkan/Metal answer needs_weight_staging() == false and get no staging failure at all.
- The owed mutation leaves the probe wiring wholly unpinned (
platforms/cuda.cpp has 0 entries in compile_commands.json vs cpu.cpp's 3, confirmed with a positive control). A cheaper pin exists without CUDA: extract the policy assembly into a free function in a header, compiled everywhere and unit-tested, leaving cuda.cpp to pass the probe in.
- Anchor inconsistencies:
cuda_backend.cu:75-81 vs :77-81 (it is 77-81); gemma4_moe.cpp:439-447 (it is 441-447); the call-chain table mixes definition and call-site conventions.
Verified sound, for the record
The measured diagnosis reproduced completely, including both switch positions converging on d.b.Alloc(nb) at qwen3_5.cpp:1011, cuda_moe.cu/cuda_glue.cu containing no allocation (grep sanity-checked with a positive control), BuildMoeMarlinResident being unable to fire on a GGUF, and the entire census arithmetic to the byte: encodings sum exactly to 397,245,341,184 over 1702 records; 276 x 1,275,068,416 + 3 x 2,818,572,288 = 360,374,599,680 = 335.625 GiB; 1,275,068,416 / 512 = 2,490,368 matching the running lane's printed slot_bytes; 125,442,340 kB x 1024 = 128,452,956,160.
The risk that could have voided the whole fix resolves in its favour: shard 1 carries zero tensors, so a check seeing only the named shard would compute 0 and never fire — but GgufFile::Open merges all shards' tensor tables (gguf_reader.h:116-122), and there is exactly one GGUF branch in FromModelDir.
VT_DEVICE_WEIGHT_BUDGET_BYTES semantics are correct — 0 means unknown, and the check returns before computing a footprint. All three production entry points verified individually. Both merge orders with #1119 are sane: one trivial docs/ENVIRONMENT.md conflict (two table rows at the same place, keep both), everything else auto-merges, merged index 319 rows with 0 duplicates, and the counter-collision avoidance holds — #1132 touches neither engine-matrix.md nor check-agent-record.py.
Gate on the reviewed head: 1484/1484 build, 505/505 rc 0, 0 ENOSPC, load average 58-82 with no starvation flakes.
On the row deviation
Judged sound on the merits, wrong on the escalation. ENG-EXPERT-STREAM is a real READY row whose ## Owed already said "there is no device-slot arm"; check-agent-record.py hardcodes ENGINE=157, which #1119 already bumps to 158, so adding a row would collide on a shared script — precisely the record-lock AGENTS.md calls a defective gate. Every index row names the owning row and #1124/#1126/#1127 are under ## Owed, so ownership is satisfied. But a material disagreement with an explicit instruction is a NEEDS_DECISION return, not a well-argued unilateral call. It was recorded prominently rather than hidden, which is the mitigating half.
Claims audit by the reviewer: 50 examined, 10 wrong.
A fresh review of PR #1132 at
e7d0a1f7c(fixing #1123) returned FAIL: one blocking gate failure and four wrong claims. The code is right; six consecutive reviews on this row family have now found the code right and a sentence about it wrong. RowENG-EXPERT-STREAM, spec.agents/specs/expert-streaming.md, issues #1123, #1124, #1126.F1 [HIGH, BLOCKS LANDING] —
commit-protocol-tagis red on two unauthored merge commits058f5f31aande7d0a1f7cboth carry the defaultgit mergesubject and an empty body. Verified directly:scripts/check-commit-trailers.py:336walksrev-list --reverse base..headwith no merge exclusion, so both fail the strict contract. CI agrees (commit-protocol-tagFAILURE). The PR body's claim thatagent-preflight.sh --stagedreturned rc 0 with all 23 checkers ok is false at this head — reproduced locally as rc 1 in both variants.Matched control, which is what makes this the PR's own defect rather than a lane artifact: PR #1119 carries three merge commits and passes, because it authors them — subject, bare
FOLLOWING_AGENTS_PROTOCOL, full trailer block (0f12ce4e1).Note CI's first trailer step skips merges (
ci.yml:552) and the second does not (ci.yml:604), so a partial read of CI would miss this.F2 [MEDIUM] — the false comment was not corrected, and three records say it was
include/vt/backend.h:79-80still reads "ROCm/CUDA override with hipMemGetInfo/cudaMemGetInfo". That file is not in the diff (git diff <base> HEAD -- include/vt/backend.his empty; last touched by unrelated9302732f7). Yet the PR body says "The false comment is corrected in prose", the spec's## Owedsays "The comment is corrected in the same change", and the issue-index row for #1126 says "corrected in prose by #1123". The## Owedrow is internally contradictory — its left cell says the comment "still says" it.This is the exact comment the spec blames for making the absent CUDA query look present.
F3 [MEDIUM] — "can never over-refuse" is false; measured 8.33 GiB window, untested direction
The spec and commit body assert the footprint is "A LOWER bound … so the refusal can never over-refuse". A tensor counted but never staged is a positive over-count, and one exists on every default load: MTP weights attach only under
params.speculative_config.has_value() && method == "mtp"(model_loader.cpp:1411-1412), so thenextnblock — 8,940,488,704 bytes (8.33 GiB), 2.2506 % — is counted and not staged. A budget landing in[staged, counted)refuses a weight set that fits.gguf_device_fit.h's own header states this over-count honestly; the spec and commit body contradict it. The "under-count dominates" argument does not rescue it — the two errors are on different quantities and do not cancel. No test covers this direction: the fixture has two tensors, both staged, so the boundary cases pin>vs>=against a footprint equal to the true staged size.Fix: adopt the header's wording, and add a case with a counted-but-unstaged tensor.
F4 [MEDIUM] — the only finding where the code can remove a working path
model_loader.h:202-210claimsResolveModelDeviceTypepicks whatSelectQueueForModelwill pick, so "the two cannot drift". On the auto arm they can:SelectQueueForModelwrapsvt::GetBackend(dev).CreateQueue()in try/catch and falls back to CPU (model_loader.cpp:135-141), whose own comment says a platform can be registered whileCreateQueuestill fails.ResolveModelDeviceTypehas no such catch, so on such a box the loader refuses naming device'cuda'for a model that previously loaded and served on CPU.Mutation (delete that CPU fallback) is NOT-CAUGHT — nothing in the tree pins it;
test_device_selectioncovers only the explicit-named arm. An escape hatch exists (VT_DEVICE_WEIGHT_BUDGET_BYTES=0), so not fatal.F5 / L1-L4 [LOW]
build-test-cpu(ci.yml:903-911) passes noCMAKE_BUILD_TYPE, so CI's test lane defines noNDEBUG. The onlyReleaseconfigures are the CUDA fat build withVLLM_CPP_BUILD_TESTS=OFF. Asserts were masked for no reason; re-running with asserts live changed nothing, so no defect was hidden — the claim is just false.BuildMoeMarlinResident (:6010-6062)— the function is:6010-6215, allocations to:6064. A sixth stale anchor of the class the audit says it swept.docs/USAGE.mdis wrong in both halves of one bullet: a discrete NVIDIA GPU isCudaPlatformand does get the refusal; ROCm/Vulkan/Metal answerneeds_weight_staging() == falseand get no staging failure at all.platforms/cuda.cpphas 0 entries incompile_commands.jsonvscpu.cpp's 3, confirmed with a positive control). A cheaper pin exists without CUDA: extract the policy assembly into a free function in a header, compiled everywhere and unit-tested, leavingcuda.cppto pass the probe in.cuda_backend.cu:75-81vs:77-81(it is 77-81);gemma4_moe.cpp:439-447(it is 441-447); the call-chain table mixes definition and call-site conventions.Verified sound, for the record
The measured diagnosis reproduced completely, including both switch positions converging on
d.b.Alloc(nb)atqwen3_5.cpp:1011,cuda_moe.cu/cuda_glue.cucontaining no allocation (grep sanity-checked with a positive control),BuildMoeMarlinResidentbeing unable to fire on a GGUF, and the entire census arithmetic to the byte: encodings sum exactly to 397,245,341,184 over 1702 records; 276 x 1,275,068,416 + 3 x 2,818,572,288 = 360,374,599,680 = 335.625 GiB; 1,275,068,416 / 512 = 2,490,368 matching the running lane's printedslot_bytes; 125,442,340 kB x 1024 = 128,452,956,160.The risk that could have voided the whole fix resolves in its favour: shard 1 carries zero tensors, so a check seeing only the named shard would compute 0 and never fire — but
GgufFile::Openmerges all shards' tensor tables (gguf_reader.h:116-122), and there is exactly one GGUF branch inFromModelDir.VT_DEVICE_WEIGHT_BUDGET_BYTESsemantics are correct —0means unknown, and the check returns before computing a footprint. All three production entry points verified individually. Both merge orders with #1119 are sane: one trivialdocs/ENVIRONMENT.mdconflict (two table rows at the same place, keep both), everything else auto-merges, merged index 319 rows with 0 duplicates, and the counter-collision avoidance holds — #1132 touches neitherengine-matrix.mdnorcheck-agent-record.py.Gate on the reviewed head: 1484/1484 build, 505/505 rc 0, 0 ENOSPC, load average 58-82 with no starvation flakes.
On the row deviation
Judged sound on the merits, wrong on the escalation.
ENG-EXPERT-STREAMis a realREADYrow whose## Owedalready said "there is no device-slot arm";check-agent-record.pyhardcodesENGINE=157, which #1119 already bumps to 158, so adding a row would collide on a shared script — precisely the record-lock AGENTS.md calls a defective gate. Every index row names the owning row and #1124/#1126/#1127 are under## Owed, so ownership is satisfied. But a material disagreement with an explicit instruction is aNEEDS_DECISIONreturn, not a well-argued unilateral call. It was recorded prominently rather than hidden, which is the mitigating half.Claims audit by the reviewer: 50 examined, 10 wrong.