S4 (#185) — retire the _validate replica; test the gate where it fires by Polichinel · Pull Request #200 · views-platform/views-postprocessing

Polichinel · 2026-08-02T01:44:06Z

Closes #185. Closes register C-03. Part of epic #181.

43 tests that could not fail

tests/test_validation.py defined its own validate_dataframe() twelve lines above the tests that exercised it. Forty-three parametrised cases against zero production code. If assert_metadata_complete had been deleted outright, every one would still have passed.

Its header said:

"The logic tested matches unfao.py:_validate() exactly."

False since #149, which stopped _validate null-gating entirely. Its docstring now reads "Neither payload is null-gated here" — it asserts the read resolved and calls _check_coverage. The gate moved to contract/historical and fires at artifact-build time.

And REQUIRED_METADATA_COLS was a nine-element literal — the hand copy of the contract C-70 was resolved to eliminate.

What replaces it

14 tests of historical.assert_metadata_complete, parametrised over the imported METADATA_COLS, plus source-scan pins that the gate has not drifted back into _validate and that the manager still invokes it.

The numbers are the argument:

	old (43 tests)	new (14 tests)
narrow the gate to one column	all pass	9 of 14 fail

The behaviour C-01 fixed — a null in a required GAUL column stops the delivery — is preserved in full. It is simply asserted against the function that performs it.

C-03, closed honestly

Residual 1 (the replica) is gone.

Residual 2 (enrich→validate end-to-end) is relocated to #18 by number, per the convention added this week: a relocation is not complete until the destination exists and is cited by number.

Every leg is covered — enrichment (16), artifact build (7), reader parity (3), invariants on primitives (8), the wire e2e (6), the null-gate (14). What remains is the manager orchestrating them, needing views-pipeline-core and a production-like Appwrite environment that þing-02 D2 forbids. That gap already had two trackers (#18 and an xfail(strict) probe); a third here was noise, not signal.

Three discharged probes retired

assert False probes asserting findings that are now fixed:

3.2 — "validation uses replica not real code"
4.3 — "filter_cols change not detected by validation tests"
4.3 — "enrichment test checks only 5 of 9 filter_cols"

C-36's own resolution predicted this residual: "the pure-assert False probes don't test the live condition, so a fixed finding won't auto-flip — inherent to marker-style tests." So a discharged probe cannot surface itself; it has to be retired by hand, or it permanently asserts a defect that no longer exists. S4 sets the first precedent: remove the probe, leave a dated DISCHARGED note in the module docstring. The modules stay — the campaign record is the point.

Scope extension, stated plainly rather than buried. The third probe is about test_enrichment.py, not the _validate replica, so it is beyond this story's stated boundary. I verified it false before removing: test_enrichment.py asserts list(lookup.columns) == METADATA_COLS — exact equality on all nine — then iterates the full set twice more for presence and nulls. The four columns it said had "no schema drift protection" are covered three times over. Leaving a known-false assert False beside one just retired seemed worse than the small overreach; Claim 4.3 now holds no live probe.

Review findings — both addressed

F1 (warning) — this story closes C-03, and the CIC still said "source-scan and replica tests" and cited "(C-03)" as the open tracker for the end-to-end gap. The story closing a concern was leaving the contract document asserting it open — the #154/#158 class, in the epic built to end it. Corrected: "seam tests", no replica, and the residual now points at #18 with þing-02 D2 named.

F2 (suggestion) — C-43's See also described C-03 in the present tense as an open gap. Annotated with its resolution and the relocation target.

Verification

ruff check .                                     # clean
pytest -q                                        # 287 passed, 41 xfailed, 5 known pyarrow (C-72)
pytest -q tests/test_validation.py               # 14 passed
git diff --stat origin/main -- tests/fixtures/   # empty

Register: 74 concerns, 21 open, 53 resolved.

🤖 Generated with Claude Code

…te where it fires tests/test_validation.py defined its own validate_dataframe() and ran 43 parametrised cases against it. Forty-three tests, zero production code — a closed loop that could not have failed if the real gate were deleted outright. Its header claimed "the logic tested matches unfao.py:_validate() exactly". False since #149, which stopped _validate null-gating entirely; its docstring now reads "Neither payload is null-gated here." It also carried REQUIRED_METADATA_COLS as a nine-element literal — the hand copy of the contract that C-70 was resolved to make single-source. Replaced by 14 tests of contract/historical.assert_metadata_complete, the code that actually gates a delivery, parametrised over the IMPORTED METADATA_COLS so a column added to gaul_schema.COLUMNS is gated automatically. Plus source-scan pins that the gate has not drifted back into _validate and that the manager still calls it. Mutation-tested: narrowing the gate to a single column fails 9 of the 14. The old suite passed that same mutation untouched. The behaviour C-01 fixed is preserved in full; it is simply asserted against the function that performs it. C-03 closes. Residual 1 (the replica) is gone. Residual 2 (the enrich->validate end-to-end path) is RELOCATED to #18 by number, per the convention that a relocation is not complete until the destination exists and is cited. Every leg is covered — enrichment, artifact build, reader parity, invariants on primitives, the wire e2e, and now the null-gate. What remains is the manager ORCHESTRATING them, which needs pipeline-core and a production-like Appwrite environment that þing-02 D2 forbids. That gap already had two trackers; a third here was noise. Three `assert False` campaign probes retired because the findings they assert are discharged: 3.2's "validation uses replica not real code", 4.3's "filter_cols change not detected", and 4.3's "enrichment test checks only 5 of 9" — the last verified before removal, since test_enrichment.py asserts list(lookup.columns) == METADATA_COLS, exact equality on all nine, then iterates the full set twice more. C-36's own resolution predicted this: "the pure-assert False probes don't test the live condition, so a fixed finding won't auto-flip." A discharged probe must be retired by hand or it permanently asserts a defect that no longer exists. Each removal leaves a dated note; the modules stay, because the campaign record is the point. Review caught the story closing C-03 while the CIC still cited it as open, next to a now-false "replica tests" — the exact drift class, in the contract document. Corrected to name #18 and þing-02 D2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…uards the claims needed Six findings from /code-review max (five parallel reviewers), all reproduced before fixing. The blind `np.asarray(gids, dtype=np.int64)` was wrong in three ways, one of them a regression against development: - a NaN gid became INT64_MIN with only a RuntimeWarning, and then the warning line itself raised `ValueError: cannot convert float NaN to integer` because it iterated the PRE-cast array. The path whose job is to report the problem gracefully was the path that crashed. A missing gid is ordinary data quality. - a non-integral float truncated SILENTLY: 54220.000000001 -> 54220, matching a real, different cell. Verified against the pandas merge this replaced — it returns null there, and even warns. A silent wrong match is worse than a crash. - pd.NA raised a bare TypeError with no log and no contract-shaped error. Replaced by `_as_cell_ids`, which DECLARES what a cell id is and marks anything else unusable rather than coercing it. Unusable ids gather to null exactly as unknown gids do, so the downstream gate sees a hole rather than a guess. Verified case by case against the old implementation: NaN, drifted float and object-with-None now match it exactly, and pd.NA is strictly better (old crashed). The lookup side had the same hole: a null key would take the same sentinel, collide with an unusable query id, and report FOUND — handing back another cell's metadata. That is the fabricated value this module's docstring forbids. __init__ now refuses it. The warning then named cell 0 for an unusable id — a cell nobody asked about. It now names unknown cells and separately counts rows that carried no usable id, because an id that is not an id has none to report. The AST guard added in this same PR had the defect it was written to catch. It walked the whole `if TYPE_CHECKING` node including `else:`, and substring-matched the test, so `not TYPE_CHECKING` read as a guard. Both evasions reproduced, both now caught, and pinned by a test rather than a comment — the `&`-not-short-circuiting shape, committed twice in one pull request. The CIC's guarantees were prose resting on a throwaway script. Committed: empty and null-key refusals, the four unusable-id behaviours, row/order/index preservation across five shapes plus a non-default index, and the object dtype. 29 tests. ADR-012 updated — my own test docstring said "ADR-012 names the seam, so update it rather than letting the claim drift", and the first draft did not. Three claims in the CIC that PR #200 retired in the manager's CIC were still standing here. Registered C-75: this class has no production caller and now holds a second copy of the delivery path's keyed gather. Tier 3 — the verification path and the delivery path implement the same algorithm twice, and the tests run through the copy that does not ship. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…merge (#210) * feat(enrichment): S4 (#89) — numpy/pyarrow keyed gather, drop the pandas merge Rewrite GaulLookupEnricher's lookup side to be pandas-free (epic #85): - Read the 888 KB GAUL lookup with pyarrow; hold it as numpy arrays + a sorted key index. Attaching metadata is a keyed `searchsorted` gather, not a pandas merge — absent gids gather to None so the downstream null gate still fails loud. The artifact is no longer materialised as a pandas frame, and the join no longer needs it to carry pandas index metadata (which unblocks S5/#90's pyarrow-native builder). - pandas becomes a TYPE_CHECKING-only import (it's in the enricher's interface — callers hand it DataFrames — not its implementation). - test_enrichment: the `lookup` fixture loads the artifact directly (the enricher no longer exposes a private pandas frame), tolerant of both the current pandas-index shape and S5's index-free one. - test_doc_accuracy: the pandas-importer guard now checks by AST, asserting ZERO runtime importers + type-only in enrichment.py (a regex couldn't tell a TYPE_CHECKING guard from a real import). Recovered from an uncommitted working tree on this (correctly-named) branch — complete, its 31 tests pass. (The repo's 5 wire shard/sidecar byte-parity + toolchain-pin failures are pre-existing and unrelated to this change.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(enrichment): S4 review findings — refuse an empty lookup, and say what the dtype change costs Four findings from the diff review, applied to the commit that preceded them. An empty lookup raised IndexError from inside the gather. The guard read `(len(self._keys) > 0) & (self._keys[clipped] == wanted)`, which LOOKS like a guard and is not one: `&` evaluates both operands, so the index happened anyway. Guard where the condition is knowable — __init__ now refuses an empty lookup with a message about the lookup, rather than letting the delivery later complain about missing metadata. The parity proof passed check_dtype=False, check_categorical=False and compared after reset_index — three flags that hide exactly what a merge-to-assign rewrite is most likely to change. Re-checked without them. Two results worth having: - name columns went category -> object. Measured, not guessed: on a 200-row output, category is 1,795,191 bytes and object is 60,061, because a categorical carries the artifact's full 64,742-entry dictionary whatever the output size. Lighter for the frames this object actually sees. Recorded in the code beside the C-32 note it might otherwise be read as contradicting. - on EMPTY input the old merge replaced the input's RangeIndex with an object-dtype Index; the gather leaves it alone. The only behavioural difference found, and it is in the direction the CIC already claimed. The CIC is corrected — fourth story running where it lagged the change, which is the pattern ADR-014 §5 was written for, recurring in the first story after the ADR landed. It now states the gather, the dtype change, the empty-lookup refusal, and that pandas is interface-only. Also dropped a dead alias and a duplicated row count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(enrichment): S4 review round 1 — a declared conversion, and the guards the claims needed Six findings from /code-review max (five parallel reviewers), all reproduced before fixing. The blind `np.asarray(gids, dtype=np.int64)` was wrong in three ways, one of them a regression against development: - a NaN gid became INT64_MIN with only a RuntimeWarning, and then the warning line itself raised `ValueError: cannot convert float NaN to integer` because it iterated the PRE-cast array. The path whose job is to report the problem gracefully was the path that crashed. A missing gid is ordinary data quality. - a non-integral float truncated SILENTLY: 54220.000000001 -> 54220, matching a real, different cell. Verified against the pandas merge this replaced — it returns null there, and even warns. A silent wrong match is worse than a crash. - pd.NA raised a bare TypeError with no log and no contract-shaped error. Replaced by `_as_cell_ids`, which DECLARES what a cell id is and marks anything else unusable rather than coercing it. Unusable ids gather to null exactly as unknown gids do, so the downstream gate sees a hole rather than a guess. Verified case by case against the old implementation: NaN, drifted float and object-with-None now match it exactly, and pd.NA is strictly better (old crashed). The lookup side had the same hole: a null key would take the same sentinel, collide with an unusable query id, and report FOUND — handing back another cell's metadata. That is the fabricated value this module's docstring forbids. __init__ now refuses it. The warning then named cell 0 for an unusable id — a cell nobody asked about. It now names unknown cells and separately counts rows that carried no usable id, because an id that is not an id has none to report. The AST guard added in this same PR had the defect it was written to catch. It walked the whole `if TYPE_CHECKING` node including `else:`, and substring-matched the test, so `not TYPE_CHECKING` read as a guard. Both evasions reproduced, both now caught, and pinned by a test rather than a comment — the `&`-not-short-circuiting shape, committed twice in one pull request. The CIC's guarantees were prose resting on a throwaway script. Committed: empty and null-key refusals, the four unusable-id behaviours, row/order/index preservation across five shapes plus a non-default index, and the object dtype. 29 tests. ADR-012 updated — my own test docstring said "ADR-012 names the seam, so update it rather than letting the claim drift", and the first draft did not. Three claims in the CIC that PR #200 retired in the manager's CIC were still standing here. Registered C-75: this class has no production caller and now holds a second copy of the delivery path's keyed gather. Tier 3 — the verification path and the delivery path implement the same algorithm twice, and the tests run through the copy that does not ship. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(enrichment): S4 review round 2 — bound the conversion, stop parsing, prove the mask Round 1 removed one silent coercion and introduced two more. Round 2 found them. The object branch used `float(value)`, which PARSES. It accepted the string "54220" as a cell id — where the pandas merge this replaces raised "You are trying to merge on object and int64 column". A string gid column is a declaration error and the old path said so; parsing it is inference, and it is the drifted-float defect pointing the other way. It now accepts only values that already ARE integers. `bool` is excluded for the same reason: True is not cell 1. Neither branch bounded int64. `uint64` max wrapped to -1 and `1e30` to INT64_MIN, both flagged VALID — an id nobody wrote, marked usable. Harmless only because real gids are 1..259,200, which is luck of the key range rather than a property the code enforced. Both branches now bound explicitly. `& usable` in `_gather` was the one new guard that survived its own removal: unusable ids substitute to 0, and 0 is not a real gid, so the key comparison already failed. It looked like decoration. It is not, and the case is now committed: with a lookup containing gid 0, a NaN row silently receives cell 0's geography without the mask. That is the fabricated value the docstring forbids, produced from an input the caller never wrote. The out-of-range test I wrote in round 2 built every case with dtype=object, so all of them entered the element-wise branch and the int and float bounds it claimed to cover were never executed — removing them left the suite green. Each case now enters the branch it tests. Mutation matrix after: dropping `& usable`, the float bound, the int bound, or the isinstance check each fails the suite. `np.isfinite` is the exception and is now documented as such rather than left to be rediscovered: inf fails the upper bound, NaN fails `arr == rint(arr)`, so the check is subsumed and defensive, not load-bearing. Saying so is cheaper than the next person mutation-testing it and wondering. 37 tests in test_enrichment.py; 337 in the suite. Wire untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(enrichment): S4 review round 3 — the float bound was off by one ULP, and my mutation proof was a replica Second /code-review max pass. Three correctness defects and a vacuous test, all reproduced before fixing. The float upper bound admitted exactly what it was added to exclude. _INT64_MAX is 2**63-1, which is odd and NOT representable in float64, so comparing a float against it promotes and rounds UP to 2**63. float(2**63) passed isfinite, integral and both bounds, then wrapped to INT64_MIN and was flagged valid. Now bounded strictly below 2.0**63, which IS exact. _INT64_MIN needed no change: -2**63 is a power of two. timedelta64 was accepted as a cell id, because isinstance(np.timedelta64(5,'ns'), np.integer) is True — a numpy quirk, and datetime64 is not, so the hole was specific to one type. A duration is not a cell id any more than True is cell 1. The test I wrote to prove the AST guard bites was itself a replica. It defined a local copy of the classifier and asserted against that; reintroducing the polarity bug into the REAL guard left all 16 tests green. That is the defect S4 of epic #181 retired from test_validation.py two days ago — 43 tests against a function the file defined itself — committed again by me, in the PR whose message boasts about catching its cousin. The classifier is now one module-level function with two callers, and both regressions now fail it. And the fix for that had a sting: routing the out-of-range cases through the public API, as the review recommended, LOST the proof. Publicly, "refused" and "accepted but wrapped to an absent id" both produce null, so reverting the float bound left the test green. The public assertion is the caller's guarantee; the mask assertion is the mutation proof; both are needed and both are now present, with a comment saying why. _gather returned four values, three of which existed only to build one log message. The warning moved inside it, matching this file's own convention where __init__ co-locates detection and logging at every guard. It returns one dict. My own error message credited the builder with refusing null keys. It does not: its null check runs after the key becomes the index, and DataFrame.isna() does not inspect an index. A null key is unreachable there only because an earlier astype('int64') raises — protection by accident. Registered as C-76, along with the empty-output case, which IS reachable and writes cells=0 without complaint. Also: the pandas error message was misquoted, ADR-012 cited a path that does not exist (unfao/wire/ -> contract/wire/), the CIC claimed eight input shapes where the pinned class has six and named one it does not exercise, the artifact size was wrong by 2% in four places, and a register quote was singular where C-40 is plural. C-75 gained the owner ADR-014 §4 requires and its first draft omitted while citing §4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Polichinel merged commit a067d61 into development Aug 2, 2026
4 checks passed

Polichinel deleted the feat/s4-retire-validate-replica branch August 2, 2026 01:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

S4 (#185) — retire the _validate replica; test the gate where it fires - #200

S4 (#185) — retire the _validate replica; test the gate where it fires#200
Polichinel merged 1 commit into
developmentfrom
feat/s4-retire-validate-replica

Polichinel commented Aug 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Polichinel commented Aug 2, 2026

43 tests that could not fail

What replaces it

C-03, closed honestly

Three discharged probes retired

Review findings — both addressed

Verification

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant