test(#323): state the grain contract as behaviour, not as names#636
Merged
Conversation
The existing D1 guard is a static scan of core's source. It can prove core
does not IMPORT or CALL the country-facing reducers, and nothing beyond that: a
locally-defined helper of any name could reduce grain silently and pass. That
limit is not hypothetical -- the scan was a substring grep, and it held
`development` red for a week by flagging `country._collapse_to_cluster_grain`,
a legitimate private helper sharing a suffix with a banned name.
So state the contract as properties of what core RETURNS. It is not "core
never reduces rows" -- Site 2's household->cluster projection legitimately
reduces. It is:
P1 CONSERVATION output is exactly the distinct declared keys. General in a
way a name-check cannot be: it does not care WHERE a row
was dropped, so a future site breaks it whatever it is called.
P2 NO FABRICATION every returned row existed, verbatim, in the input.
P3 ASYMMETRY destructive collapse is loud; lossless collapse is SILENT.
P4 ACCURACY the reported count matches an independent oracle.
P3's silent half is load-bearing: 6.46M of the corpus's ~7.5M duplicate rows
are exact duplicates, so warning on the raw count would bury the ~542k real
losses -- and a warning nobody reads is how this bug survived being closed once.
P4's oracle is written from the design note's definition rather than from core's
implementation, so it cannot pass by agreeing with a bug in the thing it checks.
`test_p4_a_vacuous_report_would_not_satisfy_this_file` guards the guard: P1/P4
would pass trivially if core stopped reporting at all.
## P2 fails today, and that is the point
Marked `xfail(strict=True)` so CI stays green while the defect is on the record
and the ratchet cannot be forgotten -- the moment core stops fabricating, the
test XPASSes and strict makes that a failure.
`groupby().first()` takes the first non-null value PER COLUMN, so complementary
rows yield a row that never existed:
(a1, <NA>) + (<NA>, b2) -> (a1, b2)
Site 2's warning already names this ("it yields a COMPOSITE"). Site 1's does
NOT -- it says "These rows are gone from the returned data", which reads as
though the survivor is one of the real rows. It is not. Same defect, reported
honestly at one site and misleadingly at the other; no static check of any kind
can see either.
Also pins the sanctioned exception: `_ADDITIVE_MEASURE_COLUMNS` (food_acquired)
sums rather than selects, so P2 does not apply and conservation of the TOTAL
replaces it -- otherwise the additive path is the one place in core with no
stated contract.
Companion to the AST guard (PR #635): that one pins the wiring, this one the
behaviour. Tests only; no `lsms_library/*.py` touched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
I had P2 wrong, and wrong in the direction that would have caused a regression.
The first cut asserted "every returned row existed verbatim in the input", and
marked its failure a LIVE DEFECT via xfail(strict=True). The reasoning was that
groupby().first() skips NA per column, so complementary rows yield a combination
appearing nowhere in the source.
That combination is the INTENDED behaviour. `NaN` is absence, not
contradiction: if one row records Region and another records Rural, the cluster
has both, and keeping both discards no observed value. The repo's own
`reduce_to_agreed` returns the SAME composite deliberately -- see
`test_nan_is_absence_not_contradiction`. Verified directly:
input (Dakar, <NA>) + (<NA>, True)
reduce_to_agreed -> (Dakar, True)
.first(skipna=True) -> (Dakar, True) # identical
.first(skipna=False) -> (Dakar, <NA>) # loses an OBSERVED value
So the attractive one-word "fix" -- `.first(skipna=False)` -- is a regression: it
returns <NA> for values the survey actually recorded. A PR for it was drafted
and abandoned on this evidence.
P2 is restated as the property that actually encodes the contract: every cell
holds an OBSERVED value or NA. Core satisfies it. It is not vacuous -- it
fails for any reducer that computes a new value (a mean, a midpoint) instead of
selecting an observed one.
Adds `test_p2_complementary_missingness_is_COMPLETION_not_fabrication`, which
pins the doctrine so the regression cannot be introduced later by someone
reasoning as I did.
Where a composite IS wrong: when the rows describe DIFFERENT REAL ENTITIES --
two clusters merged by a key unique only within a district. That is a broken
IDENTIFIER and D1 says fix the identifier; no reducer can detect it, which is
why no property here tries to.
15 passed, no xfail.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 22, 2026
Merged
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.
Companion to #635. That PR pins the wiring; this one pins the behaviour.
Why a behavioural suite
The D1 guard is a static scan. It can prove core does not import or call the country-facing reducers — and nothing beyond that. A locally-defined helper of any name could reduce grain silently and pass it. That limit is not hypothetical: the scan was a substring grep, and it held
developmentred for a week by flaggingcountry._collapse_to_cluster_grain, a legitimate private helper that merely shared a suffix.So state the contract as properties of what core returns. Crucially, the contract is not "core never reduces rows" — Site 2’s household→cluster projection legitimately reduces:
P3’s silent half is load-bearing. 6.46M of the corpus’s ~7.5M duplicate rows are exact duplicates, so warning on the raw count would bury the ~542k real losses — and a warning nobody reads is how this bug survived being closed once already.
P4’s oracle is written from the design note’s definition, not from core’s implementation, so it cannot pass by agreeing with a bug in the thing it checks. And
test_p4_a_vacuous_report_would_not_satisfy_this_fileguards the guard — P1/P4 would pass trivially if core simply stopped reporting.P2 fails today, and that is the point
Marked
xfail(strict=True): CI stays green, the defect is on the record, and the ratchet cannot be forgotten — the moment core stops fabricating, the test XPASSes andstrictturns that into a failure.groupby().first()takes the first non-null value per column, so complementary rows yield a row that never existed:Verified against the real core path. It is reported (
DESTROYED 1 of 2), so this is not a silent-loss bug — but what core returns is synthetic, which is worse than dropping a row.The asymmetry in how this is described is worth a look:
Same defect; reported honestly at one site and misleadingly at the other. No static check of any kind can see either.
Also pinned
The sanctioned exception.
_ADDITIVE_MEASURE_COLUMNS(food_acquired) sums rather than selects, so P2 does not apply and conservation of the total replaces it — otherwise the additive path is the one place in core with no stated contract.Result
13 passed, 1 xfailed. Tests only; no
lsms_library/*.pytouched.