Wire the last two 0101 null-counter fixtures (148, 149) - #275
Merged
Conversation
148 asserts that a counter the provider did not report is omitted from the Generation's fixed usage record. Nothing was checking that. The langfuse_trace comparator matches usage by iterating the EXPECTED keys, so declining to declare `input` looks at nothing, and both invariants carrying the claim were implemented in no runner. An implementation emitting input=0 passed the fixture unchanged. Both invariants are now implemented against the Generation set, with a positive anchor so no Generation at all cannot satisfy the omission half trivially. None is the faithful proxy for wire key-absence here: the SDK adapter builds usage_details by skipping exactly the None fields, while the in-memory double models usage as a dataclass where every field exists.
The guard derived the read set from _assert_trace's own body, so delegating an invariant to a helper made it blind to that name. Moving 148's two invariants into _assert_generation_usage_omission did exactly that the moment it was written. It now walks the _assert_* helpers _assert_trace calls, one level, and fails if that walk finds nothing. Adds the reverse check too: a name in _PER_TRACE_INVARIANTS that no per-trace guard reads means the multi-trace path forwards a claim nothing evaluates.
OTel stores a sequence attribute as a tuple while YAML parses the fixture's list syntax into a list, and the two never compare equal. The span-tree matcher used a plain ==, so any expected attribute holding a sequence was unsatisfiable: the span could carry exactly the right value and still be rejected. Nothing surfaced this because no activated fixture asserted one through this matcher until 149 asserts gen_ai.response.finish_reasons. The failure also reads as an implementation defect rather than a harness gap, since the message reports a span that does not match. Comparison stays strict in every other respect: order, length, a differing element, and an absent attribute are all still rejected.
Its deferral reason described the subset-match gap that has now been closed, and the fixture runs in the sibling Langfuse runner like every other Langfuse-mapping fixture. Move it onto the set that records exactly that, rather than leaving prose in the deferral map.
149 asserts that a malformed wire counter is nulled by the provider mapping BEFORE it reaches either the typed event or the span, so it declares observers.contains_event and span_tree together. No driver read both: the typed-event driver reads observers only, the LLM-payload driver reads span_tree only. Routing it to either dropped half the fixture, and an earlier attempt to re-route it swapped which half. The new driver attaches the typed collectors and an OTel observer to one invocation and reads both. Two details it depends on, both found by running rather than reading: the observer must be shut down before the exporter is read, since that is what ends the invocation root span, and the provider must be bound to an explicit request model, since the existing fallback binds it to the model the mock response reports and so collapses the request/response distinction the fixture draws. Its six invariants restate what the concrete directives already pin and are registered as documentary. One is recorded as a negative control rather than as verified: a null usage record is unrepresentable here because Response.usage is not optional, so no conforming mutation can violate that claim.
There was a problem hiding this comment.
Pull request overview
Activates (wires) the remaining two proposal 0101 null-counter conformance fixtures (148, 149) by extending the observability harness so both Langfuse and OTel assertions are actually exercised and non-vacuous.
Changes:
- Adds a dedicated driver to run fixture 149 through a single invocation that emits both typed events and OTel spans, so neither half of the fixture is silently dropped.
- Fixes span-tree attribute comparison to correctly match sequence-valued span attributes (YAML list vs OTel tuple).
- Extends the Langfuse per-trace invariant plumbing and harness-fidelity guard so newly delegated invariant checks remain covered.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/conformance/test_observability.py | Wires fixture 149 with a new typed-event+span driver, fixes span attribute equality for sequences, and adds a request-model override to preserve request/response model distinction. |
| tests/conformance/test_observability_langfuse.py | Wires fixture 148 and implements the generation-usage omission invariants, adding them to the per-trace invariant allowlist. |
| tests/conformance/test_harness_fidelity.py | Improves the invariant-readset guard by walking one level into _assert_* helpers called by _assert_trace, preventing silent drops after refactors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The comment claimed the driver deliberately avoids _invoke_typed_fixture because of that helper's detach-before-drain ordering. Both halves were wrong: the code calls the helper on the next line, and the ordering turned out to make no difference to any fixture. What the driver actually depends on is shutting the observer down before reading the exporter, since that ends the invocation root span, so the comment now says that and cites the mutation that shows it. The observer was also shut down twice, once in each finally.
Correctness, in the code this PR added: - The comment recording one of 149's invariants as an unfalsifiable negative control was wrong. It reasoned about Response.usage, which is non-optional, but the invariant is about the EVENT, and LlmCompletionEvent.usage is nullable. Setting it to None is accepted and 149 goes red, so the claim is live like the other five. - The omitted arm of the 148 guard skipped any Generation whose usage was None, so declared on its own it was satisfied by an implementation emitting no usage record at all. It now anchors on record presence itself rather than relying on the sibling arm being co-declared, since declaration lives in the spec repo. - The new model parameter outranked the fixture's own calls_llm.model, so a driver's blanket default would silently replace a request model a case declared. It now sits below the declaration and above the mock fallback, which is what both adjacent comments already claimed. Structural, where a green run was proving less than it appeared: - Nothing cross-checked the two sets that activate a Langfuse fixture, so a half-done activation ran nowhere while the coverage guard counted it as covered and the skip message asserted it was tested elsewhere. - A guard keyed on an invariant name goes dead the moment the fixture stops declaring that name, which a spec-side rename does routinely. The names whose claim rests only on an invariant are now pinned to the fixture that must declare them. - The per-trace drift check counted a MENTION as evaluation, so gutting a guard to `if name: pass` left it green. It now requires a non-inert body, treats a bare early return as inert, resolves the local-binding idiom, and carries an explicit set for the one arm that is deliberately no-op. Three comments claimed 149's driver was the only one reading the typed-event and span halves together. That is false, and two of them sat within 25 lines of an entry contradicting them: the token-budget driver reads both. Running 149 through it shows what actually stops it, which is that it binds the request model from the mock response and validates invariants against families that reject 149's names. The deferral text this PR deleted had it right; the replacement did not. Also: the callee walk selects by signature rather than an _assert_ name prefix, the invariant-name extraction is one implementation rather than two copies that must agree, the 149 driver reports the exception it captured and drops a nested try for its sibling's flat shape, and both span-attribute comparators share one helper.
Fixture 148 pins that a counter the provider did not report is omitted from the Generation's usage record. Its conformance assertion runs against the in-memory double, where omission and a rendered null are the same state, so it can only check the record. The adapter is what turns that record into the wire usage_details map, and nothing covered it: neutralising the guard at all four sites left the whole suite green. The new test drives the real adapter and asserts the map it hands the SDK. A zero row rides along, because the obvious tidy of that guard is a truthiness check, and a genuinely reported 0 must still render. The conformance-side proxy is unchanged and still documented as a proxy. Making the double record the translated map is the larger fix and is tracked on its own, since it changes the evidence every Langfuse fixture rests on.
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.
Activates the two remaining proposal 0101 null-counter fixtures. Both were deferred, and in both cases the recorded reason turned out to be only part of the story.
148: it was passing, and asserting nothing
148 claims that a counter the provider did not report is omitted from the Generation's fixed
usagerecord. It passed on the very first run with no harness work at all, which is the highest-risk signal rather than the easiest win, so it got mutated instead of merged.Two independent reasons nothing was being checked:
langfuse_tracecomparator matchesusageby iterating the expected keys. A key the fixture declines to declare is never looked at, so omittinginputfrom the expected map asserts nothing aboutinputin the actual.An implementation emitting
input: 0passed the fixture unchanged. Both invariants are now implemented, with a positive anchor so that no Generation at all cannot satisfy the omission half trivially.149: no driver read both halves
149 asserts that a malformed wire counter (
"abc",-5,true) is nulled by the realcomplete()mapping before it reaches either the typed event or the span. It declaresobservers.contains_eventandspan_treetogether, and no driver read both, so routing it to either one silently dropped the other. The event half is the fixture's own stated discriminator: an implementation sourcing the event fromrawsurfaces the wire value there, which is precisely what the sibling fixtures cannot catch.The new driver reads both surfaces off one invocation.
Harness fixes this surfaced
Both found by running, not by reading:
[...]into a list, and the span-tree matcher compared them with a plain==. Any expected attribute holding a sequence could never match. No activated fixture asserted one through this matcher until now. Comparison stays strict on order, length, differing elements and absent attributes._build_simple_llm_graphfalls back to the model the mock response reports, which collapses the request/response distinction that 144 and 149 exist to draw. Adds an explicit override, matching the convention the fixture-144 driver already uses.Verification
Every claim was mutation-tested against
src/before being reported, per the acceptance rule inAGENTS.md. Coercing a malformed counter to0instead of nulling it fails the event half; emitting the input-usage span attribute anyway fails the span half; emitting a wrongfinish_reasonsstill fails through the loosened comparator; and dropping the observer shutdown fails the span-tree root lookup.One invariant is recorded as a negative control rather than as verified.
event_usage_is_present_record_of_nulls_not_null_recordcannot be violated by this implementation, becauseResponse.usageis typedUsagerather thanUsage | None, so a null usage record is unrepresentable and pydantic rejects the mutation before any assertion runs. The claim is real and useful for an implementation whose usage field is nullable; it just cannot fail for ours.The per-trace drift guard also grew a callee walk. It derived its read set from
_assert_trace's own body, so moving 148's invariants into a helper made it blind to them the moment the helper was written.Fixtures running nowhere: 5 to 3. The remaining gaps are 119, 152 and 153.
Full suite: 2140 passed, 499 skipped.