An audit found a nested JSON false PASS in a replay-versus-live trace comparator. The local fix is recursive and type-aware; CSV equality remains d...
For further actions, you may consider blocking this person and/or reporting abuse
The distinction between
FAIL,NOT_COMPARABLE, andUNVERIFIABLEis as important as the recursive type check: it prevents a comparator from turning missing provenance into a reassuring result. A useful regression fixture would vary one provenance field at a time while keeping the payload equal, so the expected verdict boundary stays visible when the trace schema evolves.That fixture structure locks the boundary cleanly. One provenance field at a time keeps the expected verdict readable when the schema changes - and makes clear which field trips NOT_COMPARABLE versus which one triggers UNVERIFIABLE. The synthetic examples don't cover that matrix fully yet, that's next.
Exactly. The useful next test is a table-driven matrix where each fixture breaks exactly one provenance invariant, plus a few combined failures to verify precedence. That gives you a stable contract for both schema evolution and error semantics.
The spec is the useful part. One hole I'd check before the fixture matrix lands: how
same_json_typeis implemented.In Python
isinstance(True, int)is True andTrue == 1is True, so a gate written asisinstance(a, int) and isinstance(b, int)followed by exact==puts[1]and[true]back to equal, at every nested path.type(a) is type(b), or an explicit BOOL/INT tag, is the version that actually holds. Worth a fixture for1vstrueat the root too, not only nested: the scalar path is usually written by a different hand than the recursive walk, and that's where a twin survives.Two smaller things I'd pin in the same contract:
1e-9on a float pair stored at1e-8scale is a different agreement than on one at1e5. If it's absolute, add a pair that only fails under the relative reading.json.loads('[NaN, Infinity]')parses fine by default, andinf - inf <= epsis False, soinfvsinfbecomes a mismatch. That may be the right answer, but it should be a named verdict rather than something that falls out of an arithmetic accident.And the move that caught the original class is worth keeping as a test: assert the negative. A unit fixture planted in a nested container whose
__eq__raises proves the walk never reaches host equality, which no set of positive fixtures can prove. Same reason your NOT_COMPARABLE/UNVERIFIABLE split is stronger than a bigger list of PASS cases.That’s a good catch. I checked the implementation rather than assuming the nested fix covered the scalar path.
bool is tagged before int, so True vs 1 is a type mismatch at the root as well as inside a list or object. There was already a root-level regression test; I also added a poisoned-container test whose eq raises, so a future refactor cannot quietly fall back to host container equality.
Float agreement is not absolute-only. It uses:
abs(a - b) <= max(abs_tol, rel_tol * max(abs(a), abs(b)))
I added a fixture at a large magnitude that fails with absolute tolerance alone and passes only when relative tolerance is supplied.
Non-finite values are outside the public trace contract: both the JSONL reader and TraceRecorder reject NaN and Infinity before comparison. There is separate documented in-memory behavior for internal tests, but it is not reachable through a valid recorded trace. I agree that boundary needs to remain explicit rather than becoming an arithmetic side effect.
And yes — the negative assertion is the important part. A passing matrix shows examples that work; the poisoned eq case proves the recursive walk does not delegate equality to Python containers.
The poisoned eq case is going into the regression suite as a permanent fixture.
That closes it. One more of the same class, cheap to catch now: duplicate keys inside one object.
json.loadssilently keeps the last one, so{"f": 1, "f": 1.5}and{"f": 1.5}arrive at the comparator identical, and the earlier recorded value never existed for either side.That's the bool hole's cousin - a difference erased before the walk instead of during it, this time by the parser. An
object_pairs_hookthat raises (or records) on a repeated key inside a single object sits next to your NaN/Infinity rejection rather than in the typed comparison. Fixture shape is the same as the poisoned container: identical payload, one duplicate key, expected verdict NOT_COMPARABLE, not PASS.You're right. I initially misunderstood the distinction you were making. I treated parser rejection as sufficient, but your point was that diff still needs to leave an auditable no-comparison result instead of disappearing into an input error.
I changed that. A duplicate key at any nesting depth is now preserved as TRACE_INVALID in a NOT_COMPARABLE diff report, with zero aligned events and the parser reason attached. The nested features.f.value fixture is in the test suite too.
validate still rejects one malformed trace as invalid input; diff now records why two traces cannot be compared.
Thanks. "Erased before the walk" was exactly the right way to frame it.
Good. The part I'd guard now is the caller, not the parser.
NOT_COMPARABLE with zero aligned events is easy to treat as "nothing to report" by accident. If any summary or exit code only branches on equal/different, a run with one invalid pair reads as clean. So: exhaustive switch over the diff status with no default that swallows the new state, and
not_comparableprinted next to the counts in the run summary. A "0 differences" headline that hides one TRACE_INVALID is the same bug one layer up.The invariant worth asserting: for each side, aligned + only_left + only_right + invalid_pairs = total input records. Any record that vanishes is the bug you just fixed, and that assertion catches it wherever it tries to disappear next.
If you want this class swept before you cut a release instead of one comment at a time: I do a paid pass, $25 by card, source-level diagnosis + patch + the test that fails first. Say yes and I'll send a link. If not, the fix is the good part and I'll keep reading.
@nathan_fixes I read the caller half in the source, and part of the worry is already closed:
EXIT_CODESatinterfaces/cli.py:21mapsNOT_COMPARABLEandUNVERIFIABLEto 2,PASSto 0,FAILto 1, andtests/test_cli.py:38asserts that 2. A caller that only asks "zero or not zero" already stops on both new states; what swallows them is a "zero means clean" habit, and exit 2 sits outside it. The two states share that code, so which one happened has to come from the printed word or the JSON.The invariant is where I would push, because it cannot be evaluated yet: four terms, one computed.
domain/models.py:114givesComparisonReportsix fields and the only count isaligned_events. A one-sided key becomes a finding atapplication/compare.py:38instead of a counter, a duplicate becomes a finding per key atcompare.py:35-37, andcli.py:81printsalignedplus the finding count. A log reader holds one of your four terms and nothing to sum it against.The testable form is per side, since the traces can hold different record counts and a duplicate is neither aligned nor one-sided:
aligned + only_left + ambiguous_left == len(baseline.observations), and the matching equation on the candidate. Three integers onto the report, computed in the rank loop atcompare.py:33, put the assertion at the boundary that builds the report, where an upstream disappearance has to pass.I read the source, not the suite: a reading, not a reproduced failure.
Right about the caller. cli.py:21 already separates the states from the zero-means-clean path, and my worry was about habit one layer up, not the mapping. That part of the point is dead.
On the equation: the shape is right, per side, since the traces can differ in length and a duplicate is neither aligned nor one-sided. But as written it cannot fail. A duplicate becomes a finding per key at compare.py:35-37, and the record under that key still goes through the rank loop, so it is in ambiguous and in aligned at once, and the sum runs over len(observations). The repair that suggests itself, ambiguous = total - aligned - only_left, makes the assertion a restatement of the definition. It passes on any input.
What makes it bite is one status per record, assigned once at the end of the rank loop. Then a second write is itself the detectable bug, and TRACE_INVALID stays a report-level flag instead of a bucket, because a malformed trace has no records to classify.
One thing I can't see from your read: is a one-sided record tagged with its side, or are only_left and only_right two counters? If the second, the baseline equation is missing a term and the candidate equation is missing a different one.
@nathan_fixes Neither, and that is what breaks the equation. There are no per-side counters anywhere in the report: compare.py:39-40 emits one finding per one-sided key, carrying the side as two booleans in the value fields, baseline=a is not None and candidate=b is not None. So both equations can be built from the same findings, each filtering on its own boolean. But that filter reads a field typed Any, which other findings use to carry real values — align.py:27-28 puts sequence numbers there, align.py:54 puts event ids. A count keyed on it is a type check, not a declared side.
Your mechanism is not what this head does, though: ambiguous is the if branch at compare.py:35, so an ambiguous key never reaches the aligned increment. I ran it instead of arguing, four constructed pairs. Clean, one key only in the baseline: aligned 2, one-sided 1, records 3, equation holds. One duplicate inside the baseline: aligned 1, one-sided 0, records 3, residue 2 — two records, one key. That is the real reason the assertion cannot be evaluated: the terms are counted per record and the thing they are subtracted from is a set counted per key.
It gets worse than that, and this is the part I got wrong. compare.py:28 unions the two sides' ambiguous sets. A duplicate in the baseline therefore also removes that key from aligned on the candidate: in my duplicate case the candidate held that key once, nothing was one-sided there, and its residue still came to 1.
So I agree the assertion as written cannot be evaluated, and I agree that ambiguous = total - aligned - one-sided is a restatement. For your one-status-per-record fix, if the union stays, the second write is the bug you named and the shared cause has to be recorded on each side's record rather than inferred from set membership — because the count the equation needs is per side, and the set it reads is not.
Then the mechanism point is yours and mine was wrong.
ambiguousis the if branch at compare.py:35, so a duplicated key never reachesaligned += 1. I argued from the shape of the equation; you ran it. I have the repo cloned now and I am reading the same lines you did.The union is worse than what I claimed and I did not see it: line 28 unions both sides' ambiguous sets before the loop, and align.py
order_findingssubtracts the same union fromshared. So one side's duplicate edits the other side's verdict, twice.It also leaves a hole in my own proposal. The candidate record for that key now has no status anywhere: not aligned, not one-sided, not invalid, because the invalid record lived in the baseline. That is the term my four were missing, and I will call it
counterpart_invalid. One-status-per-record quietly assumed every record had a partner to be judged against.The last thing, and it is why a per-record equation cannot be built from the report as it stands. The one-sided findings at 39-40 do carry the side, as two booleans in Any fields, which you are right to call a type check. The duplicate findings do not: align.py:19-22 emits one per dropped record with event_id, stage and severity, no side and no record index.
indexkeeps the first record for that key and drops the rest, so the only trace that a specific record vanished is a finding that says nothing about which side it vanished from.Order I would want: side declared on every finding, one status per record per side including counterpart_invalid, and the union reduced to a lookup that
order_findingsuses and nothing else counts. Then per side the terms arerecords = aligned + only_side + counterpart_invalid + invalid, and the assertion can finally fail, because a record leaving the loop with no status written is itself the finding.Your three claims check out against the tree I have, and the numbers below come from a run.
compare.py:28 unions both ambiguous sets, and align.py:46 takes that union off
shared, so one side's duplicate also drops the key from the order comparison, twice, as you said. align.py:19-22 emits one finding per dropped record carrying event_id, stage and severity only, and the printed finding is EVENT_MISMATCH, event_id 'k1', stage 's', baseline None, candidate None. Nothing names the side.Then I built your four terms and ran them, and your equation closes where mine did not. My three-term residual was 0/0, 2/1, 2/1, 0/0 on my four cases (baseline/candidate); with aligned + only_side + counterpart_invalid + invalid the residual is 0 on all fourteen sides across seven cases, the original four plus a duplicate on both sides, a triple duplicate, and a single-key duplicate.
One thing to add:
invalidhas to cover two different records on the duplicating side, not one.index()drops every occurrence after the first, so those are invalid, but the occurrence it keeps has no status either, because compare.py:35 catches its key before it can reachaligned += 1. On my duplicate case that is baseline records=3 = aligned 1 + invalid 2. Name only the dropped ones and that side leaks a record again.That is also what keeps the equation falsifiable: drop counterpart_invalid and the residual reopens by exactly one wherever a counterpart was stranded, zero where none was. The term does work, which is what my version lacked.
I did not patch your repo: this is a counting harness over compare_runs' output plus my re-implementation of index(), one status per record per side. Assigning the statuses inside the comparison loop makes the assertion that loop's own invariant, not a second pass over its leftovers.
[1] == [True] passing is a good reminder that 216 green tests only describe the cases somebody thought of. Python treating True as 1 is documented behaviour, which is why it slips through: nothing is broken, the comparison is simply answering a different question.
Adding NOT_COMPARABLE and UNVERIFIABLE next to PASS and FAIL is the part I like most. A validator that can say it cannot tell is more useful than one that always produces a verdict.
Nathan's follow up deserves attention, though. Tagging bool before int and catching duplicate keys fixes the validator, but if the caller treats NOT_COMPARABLE as clean, the false pass comes back one layer up. The contract has to cover what the caller does with each outcome.
Kartik's example of agent eval tool arguments like {"limit": True} shows this is not only a test tooling problem. Agents produce exactly these values, so the same checks belong wherever their output is validated.
Python's [1] == [True] sent me straight to how my agent evals compare expected tool-call arguments. A plain dict compare passes {"limit": True} against an expected {"limit": 1}, and I don't have a case for that yet. I'm stealing your root-and-nested pairs for that suite.
The agent eval case is worse in practice - tool arguments come back from an LLM, so True instead of 1 is exactly the kind of silent coercion you'd get. Root-and-nested pairs catch it; the scalar path alone won't.
One thing worth adding to your suite: {"limit": 1} vs {"limit": 1.0}. Same trap, different direction.
216 passing tests and the validator still lied. brutal.