Repository navigation
fix(metadata): five assertions that could not fail on what they describe - #2074
Conversation
justinchuby
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2074 +/- ##
=======================================
Coverage 72.56% 72.56%
=======================================
Files 12 12
Lines 5231 5231
Branches 5231 5231
=======================================
Hits 3796 3796
Misses 1307 1307
Partials 128 128
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…itation The header already tells an editor to re-pin a drifted citation "by locating the named symbol". For citations into `validation.rs` that instruction could not be followed: seven of them carried a bare `path:line` and named no symbol at all, so a reader who found the number stale had nothing to search for. The document was asking for a repair it did not equip anyone to make. This is not hypothetical drift. #2074 rewrites the stale call-site comment at `validation.rs:4851` and is a net +5 lines above three of the ranges this document cites (`4858-4862`, `5059-5091`, `5102-5130`). A comment-only change moves coordinates just as surely as a behavioural one, so "no behaviour changed" is not a reason to expect citations to hold. When it lands, those three ranges will be five lines off; naming `validate_compaction_derivability` and `validate_packed_emit_companions` beside them makes the drift recoverable with one `grep` instead of silent. The name is the durable half of a citation: a line number is a coordinate into a tree the reader may not have checked out, while a symbol name survives every edit that does not rename it. The convention is recorded in the header so the next citation is written this way rather than repaired later. No citation range is added, removed, or altered by this commit — the ranges before and after are byte-identical, and none of the 31 cited files changed between `46478fc0f` (where the content-aware check last passed on all 97 citations) and `f66e84251`, so that verification transfers rather than needing a re-run. Docs only: no code, schema, or fixture changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Copilot <223556219+Copilot@users.noreply.github.com>
The two red Windows checks are not from this PR
This PR changes two files, both in The evidence that it is pre-existing rather than an argument that it should be: Not fixing it here: it is a different crate, unrelated to this change, and it The check that covers this change, |
…itation The header already tells an editor to re-pin a drifted citation "by locating the named symbol". For citations into `validation.rs` that instruction could not be followed: seven of them carried a bare `path:line` and named no symbol at all, so a reader who found the number stale had nothing to search for. The document was asking for a repair it did not equip anyone to make. This is not hypothetical drift. #2074 rewrites the stale call-site comment at `validation.rs:4851` and is a net +5 lines above three of the ranges this document cites (`4858-4862`, `5059-5091`, `5102-5130`). A comment-only change moves coordinates just as surely as a behavioural one, so "no behaviour changed" is not a reason to expect citations to hold. When it lands, those three ranges will be five lines off; naming `validate_compaction_derivability` and `validate_packed_emit_companions` beside them makes the drift recoverable with one `grep` instead of silent. The name is the durable half of a citation: a line number is a coordinate into a tree the reader may not have checked out, while a symbol name survives every edit that does not rename it. The convention is recorded in the header so the next citation is written this way rather than repaired later. No citation range is added, removed, or altered by this commit — the ranges before and after are byte-identical, and none of the 31 cited files changed between `46478fc0f` (where the content-aware check last passed on all 97 citations) and `f66e84251`, so that verification transfers rather than needing a re-run. Docs only: no code, schema, or fixture changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
changed the title
|
The assertion that fires ( Where it came from. Independent confirmation it is repo-wide. #2084 fails the same assertion with the same single missing entry This PR touches two files, both in I have not fixed it here. The right repair is a judgement call this PR is not the place to make — either guard the sync with The two Windows failures documented in the earlier comment on this PR remain separate and also pre-existing. |
|
macOS-only, in Running total for this PR: three red checks, none of them its own.
|
88dc274 to
b738a9a
Compare
|
Correcting my own table above, which went stale within the hour. The branch is now rebased onto current They were genuinely failing when I recorded them, and the evidence I gave then — an unrelated PR failing the same two tests by name — was sound. They were repaired on Current state, and the reason each is not this PR's:
The general point, since it is the same one the PR is about: a claim about CI is a claim with a timestamp, and "pre-existing" is not a durable property — it decays into "still failing" in the reader's head while the underlying fact moves. Recording the evidence rather than the verdict is what let this be checked instead of believed. |
…itation The header already tells an editor to re-pin a drifted citation "by locating the named symbol". For citations into `validation.rs` that instruction could not be followed: seven of them carried a bare `path:line` and named no symbol at all, so a reader who found the number stale had nothing to search for. The document was asking for a repair it did not equip anyone to make. This is not hypothetical drift. #2074 rewrites the stale call-site comment at `validation.rs:4851` and is a net +5 lines above three of the ranges this document cites (`4858-4862`, `5059-5091`, `5102-5130`). A comment-only change moves coordinates just as surely as a behavioural one, so "no behaviour changed" is not a reason to expect citations to hold. When it lands, those three ranges will be five lines off; naming `validate_compaction_derivability` and `validate_packed_emit_companions` beside them makes the drift recoverable with one `grep` instead of silent. The name is the durable half of a citation: a line number is a coordinate into a tree the reader may not have checked out, while a symbol name survives every edit that does not rename it. The convention is recorded in the header so the next citation is written this way rather than repaired later. No citation range is added, removed, or altered by this commit — the ranges before and after are byte-identical, and none of the 31 cited files changed between `46478fc0f` (where the content-aware check last passed on all 97 citations) and `f66e84251`, so that verification transfers rather than needing a re-run. Docs only: no code, schema, or fixture changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Copilot <223556219+Copilot@users.noreply.github.com>
b738a9a to
b82b485
Compare
|
Verified against the merge, not just this branch's head. Earlier evidence in this PR named
Merged Same numbers as at the head, now on the tree that would actually be built. Unchanged from before: both required checks ( |
b82b485 to
8103e53
Compare
|
Rebased onto The previous head was committed at Gate re-run on the rebased tree, not carried over from the previous head: All three commits kept their Correcting an earlier statement of mine in this thread. I previously described |
|
Reproduced with no PR content at all, on the rebase base itself:
So the rebase in the previous comment traded a non-required red for a required one: it cleared Two things I got wrong while diagnosing this, recorded because they both produced confident wrong answers:
Unchanged: metadata 344/0, |
…uts alone The comment above the serving carve-out claimed the admission is "decidable from the declared outputs alone". The code directly beneath it does not do that and must not: `output_companions` opens by walking the step tree for `emitted_outputs`, and carries `claimant_emitted` per expectation. That field is the whole reason a value claimed only by a declaration nothing writes is told so, instead of being advised to declare a row axis -- the advice a companion must never take. The weaker claim is not merely imprecise, it is an invitation. A reader implementing from it produces the outputs-only check and is not wrong to, and that check has a hole: declare a padded output beside any `shared` int64 vector, write neither, and the vector is admitted on the strength of a payload that does not exist. The design document carried the same sentence and was corrected; the code's copy was not, because the code was already right, so nothing forced the comment to move with it. Found by the design agent reading the merged surface against the doc. Note the comment eleven lines above, which makes a similar-sounding claim about the ragged-emission rule, is accurate and is left alone: that check reads `declared.contract.batch_layout` and nothing else. The difference is whether the check consults the step tree, so the two comments should not be aligned to each other. Signed-off-by: Justin Chu <justinchu@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The assertion was `!errors(&document).is_empty()` -- that something rejected, not that the adapter rule did. Today exactly one rule fires, so the test is precise by accident rather than by construction, and nothing holds that. If the ABI-pinning rule were narrowed away, any other refusal the fixture happened to trigger would keep this green while it had stopped testing adapters entirely. That is a defect class rather than a defect: an assertion that checks a rejection without checking which rule rejected cannot detect a rule being narrowed out of existence underneath it, because the outcome is preserved and only the message is lost. Found by auditing this suite against the same class identified in the design's acceptance matrix. Signed-off-by: Justin Chu <justinchu@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three fixtures for the serving carve-out asserted only that a document was
rejected. Each is refused for the reason its name describes, but a narrowed
carve-out would refuse it too -- through the generic message, whose advice is
to declare `request_aligned` or `token_packed`, which is precisely the layout
a companion may not have. An assertion satisfied by both the rule and its
narrowing cannot detect the narrowing, so the rejection was precise by
accident.
Each now also asserts that the generic advice is absent, and each negative was
mutation-verified against a distinct narrowing that leaves the positive
assertion standing:
- a companion of a result nobody produces: deleting the unwritten-claimant
branch routes both lengths to the generic message.
- a withheld valid_lengths: dropping `padding` from `output_companions` --
a revert of the widening, not a new mistake -- routes the sibling length
there instead.
- a withheld owner map: dropping the layout companions does the same to the
offsets beside it.
The first narrowing tried for the withheld-lengths case did not bite:
admitting a companion only when its claimant published every companion also
refuses that document, but through the wrong-shape message. That is recorded
beside the assertion, because a comment justifying a test with a mechanism the
test does not actually hold is the same defect this change removes -- one
degree weaker than the code it sits on, and invisible while the suite is
green.
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8103e53 to
4a61cde
Compare
What this fixes
A comment in shipped code states a claim about the validator beneath it that is
false, and false in the direction that invites a weaker reimplementation.
validation.rs, above the serving companion carve-out:The code immediately below does not do that, and must not.
output_companionsopens with
let emitted = emitted_outputs(workflow);and walks the step tree,then sets
claimant_emittedon everyCompanionExpectation. That field existsprecisely so a value claimed only by a declaration that no step writes hears
about the declaration, rather than being advised to declare a row axis -- which
is the one layout a companion may not have. The function's own doc comment says
this correctly; only the call-site comment is stale.
Why a comment is worth a PR
The claim is not merely imprecise. An implementer working from it produces the
outputs-only check, and that check has a hole: declare a padded output beside
any
sharedint64 vector, write neither, and the vector walks past the servingrule on the strength of a payload that does not exist. One emit and one bare
declaration are two answers to one question.
The design document of record carried the identical sentence and was corrected
in review. The code's copy was not -- because the code was already right, so
nothing forced the comment to move with it. That is the failure mode worth
naming: a wrong comment beside right code has nothing to make it fail, and the
test suite cannot see it.
What is deliberately not changed
Eleven lines above sits a similar-sounding claim about the ragged-emission
rule -- "checked from the declared output alone". That one is accurate: the
check reads
declared.contract.batch_layout.request_axis()and consults nosteps. The two comments should not be aligned to each other, because the
distinction between them is real and is exactly the thing that was lost. Only
the false one moved.
A second instance of the same class
a_video_program_needs_its_adapterasserted!errors(&document).is_empty()--that something rejected, not that the adapter rule did. Exactly one rule fires
today, so the test was precise by accident, and nothing held that. Narrow the
ABI-pinning rule away and any other refusal the fixture happened to trigger
would keep it green while it had stopped testing adapters.
It is the same defect as the comment above, in a different medium: an assertion
that cannot fail on the thing it names. A rejection test that does not pin
which rule rejected cannot detect that rule being narrowed out of existence,
because the outcome survives and only the message is lost. It now asserts the
message.
Three more, and what makes a negative assertion load-bearing
The design's acceptance row 25 was tightened to require that the withheld
-companion cases assert not only the reason they were refused but also that
neither receives the generic advice to declare a row axis. Three fixtures
asserted only the positive:
a_companion_of_a_result_nobody_produces_describes_nothinga_declared_companion_that_no_step_writes_is_not_publisheda_declared_ownership_companion_that_no_step_writes_is_not_published_eitherEach is refused for the reason it names, but a narrowed carve-out refuses it
too, through the generic message that advises
request_alignedortoken_packed-- the one layout a companion may not declare. The outcomesurvives the narrowing; only the reason is lost. So each now asserts the
absence of that advice, and each negative was mutation-verified against a
distinct narrowing that leaves the positive assertion standing: deleting the
unwritten-claimant branch, dropping
paddingfromoutput_companions, anddropping the layout companions.
The mutation earned its place twice. The first narrowing I hypothesised for
the withheld-lengths case -- admitting a companion only when its claimant
published every companion -- does refuse the document, but through the
wrong-shape message, not the generic one. Had I shipped the comment I first
wrote, the assertion would have been correct and its stated justification
false: one degree weaker than the code it sits on, which is exactly the defect
in the first commit here, arriving in the medium of a test comment.
One near-miss worth recording:
grepfor the generic message invalidation.rsreturns nothing, because the phrase is split across a linecontinuation inside the
format!. Taking that at face value would havecondemned a pre-existing, correct negative assertion as vacuous. A coordinate
is not the only thing that can resolve cleanly and mislead; so can a search
that finds nothing.
Provenance
Found by the design agent (#2010) reading the merged #2009 surface against the
design documents, after #2009 merged as
0448f2bc6. They could not fix itthemselves without their PR losing its docs-only property.
Verification
cargo test -p onnx-genai-metadata-- 344 (encoder_batching104), run on thebranch rebased onto current
main, not on the base it was cut from. Main hadmoved 14 commits underneath it; none touch
crates/onnx-genai-metadata/, whichis what makes the local result evidence about the merge result. "Is main
different" was the wrong question -- the one that decides whether a green run
still means anything is whether main moved under the thing that was verified.
cargo fmt --all -- --check,cargo clippy -p onnx-genai-metadata --all-targets -- -D warnings.test assertions.
validation.rsis unchanged since the first commit, so the +5line shift it causes for
docs/citations is unchanged too.