fix(#1663): a stale attention-rung stem excused a DELETED marker in the file it named - #1665
Merged
Merged
Conversation
…he file it named `47a918d8f` (#1579, issue #1545) and `90e8c3c85` (#1557, issue #1549) landed the routing the allowlist was parked for, and left their three stems behind. That is by design -- `scripts/check-attention-rung-consistency.py` reports a discharged stem as `STALE (not a failure)` and exits 0, so the removing row never has to edit this file and the deletion falls to the next preflight. This is that preflight. What the design did not say is what the window costs. A listed stem excuses its whole translation unit, not only the call that earned the entry, so every marker in a covered file is decorative while the stem sits there. Measured on `db648fb88`, deleting the `// VT-ATTN-NAIVE:` marker at `ltx2.cpp:958`: 3 stems listed: rc=0, "7 carry a recorded reason, 1 unmarked and excused" stems removed: rc=1, "src/vllm/model_executor/models/ltx2.cpp:966" Both arms restored byte-for-byte against a pre-taken sha256. So the entries were a live hole in the guard #1544 exists to be, open from `90e8c3c85` until here. The three earned their green independently, and asymmetrically: `muse_glimmer_vision` names `vt::Attention` nowhere, while `ltx2` and `ltx2_device` still name it at the host CPU-only arm -- where `kAttention` and `kAttentionDenseFlash` are the same registered function -- and at the `VLLM_LTX2_DIT_FLASH_ATTN=0` arm of a same-binary A/B, each carrying its own reason. The expected set in `test_allowlist_holds_only_the_in_flight_stems` moves to the empty set in the same change, which the allowlist header requires. Empty is not a weaker assertion: `drift_sites` now excuses nothing, so `test_shipped_tree_is_green` measures the tree on its markers alone, and a new case asserts the three stems positively rather than by their absence from a list. The header keeps the reason each stem left, so an empty parking lot does not read as an abandoned one. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
…a verdict F1, and it is the one that mattered. The `#1631` owed entry in `attention-rung-visibility.md` said the checker comment at `:252-255` "denies an equality that held ... and no longer does". Wrong, and wrong in the direction that misleads: if the equality no longer held, the comment denying it would be RIGHT. It still holds, and after this row it holds for a stronger reason -- with an empty allowlist, any green tree has `sites == marked` and `excused == 0` by construction, so the equality cannot fail. The same sentence also credited the 8/8/0 triple to #1663; `db648fb88` already read 8 sites and 8 marked before this row touched anything, because `47a918d8f` and `90e8c3c85` moved it. #1663 moved only the allowlisted-stem count, 3 to 0. F2. The marker is at `ltx2.cpp:959`, not `:958` -- `:958` is `a.causal = false;`. Corrected in the spec and in the index row, which is append-only and could not have been corrected after the merge. F3. The `KERNEL-ATTN-DENSE-FLASH` evidence cell still read "checker green (9 sites / 6 marked / 3 unmarked and excused by the 3 in-flight allowlisted stems)". Every number was false before this row and all three would be false again after it, which is the point: a count of one file stored in another reds on every row that legitimately adds or removes a call. The cell stops quoting the triple rather than restating it, since restating it rebuilds the drift lock AGENTS.md `## Records` forbids and #1629 already names in this checker's own test. The checker prints the live numbers; the suite pins what is invariant. F4. `test_every_allowlisted_stem_names_a_real_model_source` now iterates over an empty set. It is DORMANT, not dead, and the file says so: it guards a file that is currently empty and fires on the first thing added to it, which is measured rather than asserted -- appending one bogus stem reds it together with the pinning case. Gate after the repairs: checker rc=0, suite 35 of 35 OK, issue-index append-only rc=0 and `1 0` against `origin/main`. `test_cpu_x86_llamacpp_floor` reds at loadavg 31 with `NO_QUIET_WINDOW` and `busy=161%`, which is #618 and not reachable from a diff of three Markdown files and one Python comment. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
… the equality is wrong on every green run Three repairs from the scoped re-review of `f64effeca`, two of which correct claims this branch itself made. The F4 comment overstated by one clause. It said the dormant `test_every_allowlisted_stem_names_a_real_model_source` "fires the moment anything is added", in a comment whose whole subject is that the claim is measured. Both arms are measured now and they differ: appending `zzz_bogus_model` reds that case together with the pinning case, while appending `whisper_audio` -- a real stem with a real source file -- reds the pinning case and `test_deleting_a_marker_goes_red` and leaves the dormant case GREEN. It fires on the first TYPO, which is what it was built to report, and the comment now says that. The parenthetical was already correct; the clause a skimmer takes away was not. The F1 entry UNDERSTATED, which is the direction that costs #1631 its evidence. It said the checker comment at `:252-255` is wrong about a tree where it cannot be right. The comment is wrong on EVERY green run, for ANY allowlist, and always was. Its stated reason -- "a marked call inside an allowlisted file counts in `marked`" -- is true and does not separate the quantities, because such a call is counted in `marked` AND excluded from `excused` and cancels on both sides. The only shape that separates `excused` from `sites - marked` is an unmarked call in a non-allowlisted file, which is exactly `drift_sites`, so `main` returns 1 at `:248` and the OK line never prints. Enumerated rather than argued: 64 green configurations of marked and unmarked calls across one allowlisted and one non-allowlisted file, zero break the equality, and the single configuration that does break it is not green. F3's replacement text named the suite twice and spliced two clauses into something that read like a merge artefact. The cell now gives one reason for withholding two counts -- the site triple and the suite's case count -- instead of giving that reason twice in different words. Focused gate after the repairs: checker rc=0 at `8 sites / 8 marked / 0 excused by 0 allowlisted`, suite 35 of 35 OK, `check-issue-index-append-only.py` rc=0. Both mutation arms restored byte-for-byte against a pre-taken sha256. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
`origin/main` moved past this branch's base while it was in review, which made `check-commit-style.py --range origin/main..HEAD` refuse with "range base must be an ancestor of range head" and made preflight SKIP both trailer gates. The instrument was reading a base that is no longer on this history, not a defect in the commits. `.agents/issue-index.md` is the only shared record either side touched. #1672 appended one row (#1131); this branch appends one row (#1663). No id collides, and the resolved file is 577 rows with 577 unique ids. `git diff --numstat origin/main -- .agents/issue-index.md` is `1 0` after the merge, so the result is additions-only against the moved main rather than against a stale base. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
Second merge of `main` on this branch. GitHub reported the pull request `CONFLICTING` once `main` moved past the previous merge, because the forge does not honour the `merge=union` driver on `.agents/issue-index.md` and reads two appends near the tail as a conflict. Locally the driver applies and the merge is clean. Verified against the merged result rather than against the forge's verdict: 585 rows, 585 unique ids, zero duplicates, and `git diff --numstat origin/main -- .agents/issue-index.md` is `1 0` -- this branch's single #1663 row, additions only. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
Third merge of `main` on this branch, forced by this session's own #1670 landing ten rows into `.agents/issue-index.md`. GitHub reports `CONFLICTING` on that file because it does not honour the `merge=union` driver; locally the driver applies and the result is clean. Verified on the merged result against the pinned SHA rather than the moving name: 598 rows, 598 unique ids, zero duplicates, and `git diff --numstat 4503083 -- .agents/issue-index.md` is `1 0`, this branch's single #1663 row. This merge adds no content of this branch's own. Against `22ee35d7e`, the head whose CI rollup completed with zero pending, the four files this branch owns -- `scripts/attention-rung-allowlist.txt`, `tests/scripts/test_check_attention_rung_consistency.py`, `.agents/specs/attention-rung-visibility.md` and `.agents/kernel-matrix.md` -- are byte-identical. Everything else in the range is `main`'s own work arriving through two merges. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
47a918d8f(#1579, issue #1545) and90e8c3c85(#1557, issue #1549) landed therouting that
scripts/attention-rung-allowlist.txtwas parked for, and left theirthree stems behind. That is by design:
scripts/check-attention-rung-consistency.pyreports a discharged stem as
STALE (not a failure)and exits 0, so the removingrow never has to edit this file and the deletion falls to whoever runs preflight
next. This is that preflight. Closes #1663.
The window is not free, and that is the finding
## Risks/decisionsD4 of the spec says the deferral is safe because a staleentry is reported and never fatal. True of the exit code, and not the whole
story: an allowlisted stem excuses its ENTIRE translation unit, never only the
call that earned the entry. Every
VT-ATTN-NAIVE:marker in a covered file istherefore decorative for as long as the stem sits there.
Measured on
db648fb88by deleting the live marker atsrc/vllm/model_executor/models/ltx2.cpp:959:mainhas them7 carry a recorded reason, 1 unmarked and excusedsrc/vllm/model_executor/models/ltx2.cpp:966Both arms restored byte-for-byte against a sha256 taken before the mutation. So
the entries were a live hole in the guard #1544 exists to be, open from
90e8c3c85until here — not untidiness. The deferral D4 designs for is stillright, because it genuinely keeps the removing row off a shared file; what is
added to the spec is what it costs, so the next row that parks a stem reads it
with the cost attached.
The three earned their green separately, and asymmetrically
muse_glimmer_visionnamesvt::Attentionnowhere:47a918d8frouted theperception encoder's sole path to
vt::AttentionDenseFlash.ltx2andltx2_devicestill name it, at calls that now record their ownreason — the host arm, CPU-only by construction, where
kAttentionandkAttentionDenseFlashresolve to the same registered function(
src/vt/cpu/cpu_ops.cpp:3750-3761); and theVLLM_LTX2_DIT_FLASH_ATTN=0armof a same-binary A/B, which exists so both halves of the 47.84 s / 7.680 s
measurement run from one build.
One assertion covering all three would be false of one of them in either
direction, so the new test case asserts them apart.
The expected set moves in the same change
The allowlist's own header requires it: the checker does not fail on a stem set
that has drifted, and
tests/scripts/test_check_attention_rung_consistency.py::ShippedTreeTests::test_allowlist_holds_only_the_in_flight_stemsdoes, on an addition or a deletion alike.
An empty expected set is not a weaker assertion.
drift_sitesnow excusesnothing, so
test_shipped_tree_is_greenmeasures the shipped tree on its markersalone, and a silent append still reds this case exactly as before — verified by
appending
some_new_towerand measuring 3 failures, restored after. The newtest_the_formerly_allowlisted_stems_pass_on_their_own_meritre-statespositively what the allowlist used to assert by omission. The header keeps the
reason each stem left, so an empty parking lot does not read as an abandoned one.
Evidence
.agents/specs/attention-rung-visibility.md## Nowrecords the discharge andthe measured cost of the window. #1629 and #1631 are separate defects in the same
checker and are untouched.
Fresh review, and the four findings repaired in
f64effecaThe review confirmed the headline mutation independently and found four things.
F1 was the one that mattered, and it was a regression this branch
introduced. The
#1631owed entry was edited to say the checker comment at:252-255"denies an equality that held ... and no longer does" — wrong in thedirection that misleads, because if the equality no longer held then the comment
denying it would be RIGHT. It still holds, and after this row it holds for a
stronger reason: with an empty allowlist any green tree has
sites == markedandexcused == 0by construction, so it cannot fail. The same sentence credited the8/8/0 triple to #1663, when
db648fb88already read 8 sites and 8 marked —47a918d8fand90e8c3c85moved that; #1663 moves only the excused count.F2 — the marker is at
ltx2.cpp:959, not:958, which isa.causal = false;.Corrected here and in the index row, which is append-only and could not have been
corrected after the merge.
F3 — the
KERNEL-ATTN-DENSE-FLASHevidence cell readchecker green (9 sites / 6 marked / 3 unmarked and excused by the 3 in-flight allowlisted stems). Everynumber was already false on
db648fb88and all three would be false again afterthis row, which is the point: a count of one file stored in another reds on every
row that legitimately adds or removes a call. The cell stops quoting the triple
rather than restating it, because restating it rebuilds the drift lock AGENTS.md
## Recordsforbids — and that is the same defect #1629 names in this checker'sown test.
F4 —
test_every_allowlisted_stem_names_a_real_model_sourcenow iterates overan empty set. Dormant, not dead, and the file now says so: it guards a file that
is currently empty and fires on the first thing added to it, measured rather than
asserted, since appending one bogus stem reds it together with the pinning case.
test_cpu_x86_llamacpp_floorred twice during this work, at loadavg 31 withNO_QUIET_WINDOWandbusy=161%. That is #618, and it is not reachable from adiff of three Markdown files and one Python comment: the same suite passed on a
clean tree and on this tree at lower load.
Why this merged without a complete rollup on its exact SHA
mainmoved six times while this branch was in review, the last move being this session's own #1670 landing ten rows into.agents/issue-index.md. GitHub does not honour themerge=uniondriver on that file, so each move marked this PRCONFLICTINGand forced a merge commit, restarting a ~75-minute CI cycle against a branch that merges ~10 commits/hour. That loop has no fixed point.A complete rollup exists, and it covers every byte this branch contributes. At
22ee35d7ethe rollup settled with zero pending. Against that head, the four files this branch owns are byte-identical to what is being merged now:So the
+41/-1in the Python test — the one real test change here — was present and gated at the head that produced the complete rollup. Everything added to the range since ismain's own work arriving through three merges, each commit of which carriesmain's own verdict. This is not "the delta is small enough to ignore"; it is that the delta contains none of this change.Both SHAs are pinned deliberately.
origin/mainis a moving ref, and diffing it instead of a pinned commit made a sibling branch appear to delete 768 lines ofvocoder1d.cpp,cpu_conv1d_*and three test files, when the deletions were another commit landing between two commands.Every failure at that gated head was verified inherited by failure text, not by job name, against
main's baseline90e8c3c85:build-test-cputest_runner.cpp:1557sanitize-cpu (address,undefined)sanitize-cpu (thread)windows-msvc-cpu/-vulkan/W4 /WX ... negated by /wrefusal (#1649), fires before compilation, 0error C####, 0error LNK####build-newest-gccmain-baseline.py's newest row predates #1581The
test_runner.cpp:1557failure is #1602/#1608: the attention-backend selector refuses a non-multiple-of-16 block size beforeCheckKvCacheShapecan, so the case asserts a message only a ROCm/FLASH_ATTN build produces. Nothing outside that characterised set appeared on any run of this branch.Records verified on the merged result: 598 rows, 598 unique issue ids, zero duplicates,
1 0for this branch's single #1663 row. Local preflight green at the pushed head, no gate skipped.FOLLOWING_AGENTS_PROTOCOL
Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-opus-5 [claude-code]