Release candidate: the §5.1 ruling, and six guards that were not guarding - #283
Merged
Conversation
Five entries, none of which duplicated an existing one — checked against all 22 open and 80 resolved concerns before writing: - C-103 (Tier 2) the observed-range clip depends on `datafactory_query`, which is declared nowhere and is not installed; its absence is swallowed by a bare `except Exception`, so a missing dependency and "the producer publishes no boundary" leave through the same branch and ship fabricated months either way. Carries the verification question it cannot answer from this seat. - C-104 (Tier 3) a stale virtualenv takes 25 tests red, 20 of them the only tests that import either manager. Measured: 433 passed / 25 failed against pipeline-core 2.3.0 + pyarrow 23.0.1 where the lock pins 3.0.1 + 16.1.0. - C-105 (Tier 3) a run uploads file-by-file with no rollback and no idempotency. Added to Cluster J. - C-106 (Tier 4) `wire/header.build_header` is reachable only from tests; so is `gaul_schema.colrow`. C-100's shape, one module over. - C-107 (Tier 4) the doc-accuracy scan reads markdown only, so two docstrings still point at modules deleted in #151 and moved in #153. Header counts and Cluster J updated; test_register_integrity green (40 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes the loop the partner has now opened twice (#278, views-postprocessing#272): they ask for integer GAUL codes, we say no, and the reason we give does not survive contact with the format we actually deliver. The old reason was wrong. "Codes are always float64" was justified by an integer column being unable to carry a missing value — a property of NumPy-backed pandas, not of parquet. Arrow and parquet carry nullable integers natively, and this repo's own `data/gaul_lookup.parquet` stores all three code columns as `int64` with zero nulls; the float is introduced by `contract/wire/sidecar.py`, not by the source. The 2026-07-19 ruling weighed "int64 when complete" and rejected it for making the schema depend on the data. It never weighed nullable int64. The rule survives on a measured ground instead. Measured 2026-08-17, pyarrow 23.0.1 / pandas 3.0.5: an int64 parquet column with no null reads back `int64` under a default `pd.read_parquet`; the same column with one null reads back `float64`. So nullable int64 does not remove the float — it moves it from our writer to the consumer's reader and makes it appear only sometimes, which is the data-dependent schema §5.1 exists to prevent. Independently, faoapi's reader `reindex`es the sidecar and calls `.to_numpy()`; both yield float64 from a nullable integer column, so the change would not reach them as integers anyway. The useful half is recorded too: because the delivered region drops the GAUL-uncovered cells, no delivered code is ever missing — held in CI by `tests/test_gaul_lookup_fidelity.py::test_lookup_has_no_nulls` — so a consumer's `astype("int64")` is lossless for this product. That is a property of the region, not the contract, which is why the type stays float64. No behaviour change, no `contract_version` bump: §5.1a records a rejected alternative, it does not alter the rule. `gaul_schema.py`'s docstring now says so at the declaration and points at §5.1a, so the next person to be asked finds the answer where they are standing rather than reopening it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#278: the §5.1 dtype ruling, and the register from the assimilation pass
…C-46)
On 2026-08-17 this repository told FAO in writing that the 76-cell exclusion list
"is frozen in code and asserted against the producer in our test suite, so it
cannot drift without failing loudly". It could. The assertion —
`test_manifest_matches_datafactory_land_minus_land_gaul` — skipped on every pull
request, because views-datafactory was not fetched in CI. The guarantee was
running on one laptop.
The stated reason it was not fetched did not survive checking. C-46 and
`conftest.SIBLINGS` both recorded that a checkout "converts an honest skip into a
FileNotFoundError — measured 2026-08-03, tried and reverted". The observation was
right; the diagnosis was wrong. `test_gaul_lookup_fidelity` gated on
`data/raw/gaul_admin/` being a DIRECTORY, and that directory IS tracked in
views-datafactory — it holds `supplement_azores.geojson` — while the seven GAUL
parquets beside it are not. So a checkout satisfied the gate, the value
comparison ran, and it died. The sibling was never the obstacle; the gate asked
whether a folder existed when it needed to ask whether the files it reads existed.
Reproduced before changing anything, against a `git worktree --detach` of
views-datafactory@HEAD — exactly what `actions/checkout` produces:
before 1 failed, 37 passed, 1 skipped
FileNotFoundError: .../data/raw/gaul_admin/gaul0_code.parquet
after 35 passed, 4 skipped, 0 failed
What changed:
- `test_gaul_lookup_fidelity._HAS_DATAFACTORY` gates on the seven parquets it
actually reads, not on their directory. The PRIO-GRID check at the bottom of
the same file already had this shape, gating on `priogrid_cell.dbf` itself.
- its skip reason now says the parquets are absent and that they are untracked
upstream, instead of "checkout not found" — which was false in precisely the
case that broke CI.
- `run_pytest.yml` fetches views-datafactory at `ref: main`, and declares
`VIEWS_DATAFACTORY`; `conftest.SIBLINGS` flips to `ci_checkout=True` with the
corrected reason. `test_ci_sibling_coverage` requires those two to agree, and
does.
Two checks move from skipped to running in the gate: C-30's exclusion tripwire,
and C-46's own `TestReleaseGate::test_land_gaul_commit_is_in_a_release_tag` —
the gate that entry says had "never run anywhere but one laptop". The
producer-comparison half still skips, honestly, and still needs the parquets
published somewhere fetchable.
C-30 is NOT re-tiered: what holds it at Tier 1 is the absence of a real global
delivery exercising it end to end, and CI wiring does not supply that.
Suite: 428 passed, 5 skipped, 39 xfailed under a CI-shaped sibling. The 25
failures are the pre-existing local venv drift registered as C-104, unchanged by
this commit. ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Findings from /review-diff on the previous commit. All four are the same defect that commit fixed, one level up: a gate asking about something other than what the test reads. `test_gaul_lookup_fidelity` had ONE sibling gate, keyed to the GAUL parquets, and applied it to four tests. Only one of them reads a parquet: test_lookup_values_match_the_producer_parquets 7 parquets (untracked) correct test_lookup_gid_set_equals_the_declared_region land_gaul_pgids.json — TRACKED test_coordinate_formula_matches_every_pg_cell priogrid_cell.dbf; self-skips test_coord_dtypes_are_wire_stable only the committed lookup So two checks that need nothing untracked, and one that needs no sibling at all, were dark in CI because they carried a mark for someone else's dependency. The last is the one worth naming: it asserts `abs(v) < 2**53` for every code column — that the §5.1 int64->float64 wire cast is lossless. That is precisely the property ADR-013 §5.1a records and the 2026-08-17 mail to FAO relies on, and it was skipped in the gate while resting on nothing but a committed fixture. Changes: - `_HAS_DATAFACTORY` -> `_HAS_GAUL_PARQUETS`, `_needs_datafactory` -> `_needs_gaul_parquets`. The old name asserted the conflation the bug was made of: a checkout can be present while these are absent, which is the normal case in CI. - new `_needs_region_pgids`, gating on the tracked pgid list, so the region-set check runs in CI. - the coordinate check loses the mark; its own `dbf.exists()` skip was already the right gate and is now the only one. - the dtype check loses the mark entirely; it is always-on. - the parquet comparison uses `_GAUL_ADMIN` instead of respelling the path, so the gate and the reader cannot drift apart — which is how this started. - module docstring rewritten: three gates, not two, with the rule stated. Measured. CI-shaped sibling (tracked files only): fidelity file goes 21 passed / 4 skipped -> 23 passed / 2 skipped, and both remaining skips name the artifact they want. Full suite CI-shaped: 430 passed / 3 skipped, up from 426 / 6 before this branch. With a full local sibling: 39 passed, nothing lost. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four findings, all mine, all the same defect the branch set out to fix.
1. HIGH — I broke the no-checkout case. Removing the mark from
`test_coordinate_formula_matches_every_priogrid_cell` exposed that its
`dbf.exists()` skip sits BELOW `dbf = _DATAFACTORY / ...`, and `_DATAFACTORY`
is None when nothing resolves. Measured: `TypeError: unsupported operand
type(s) for /: 'NoneType' and 'str'` — a red suite for any contributor
without a views-datafactory checkout. CI never saw it, because CI now always
has one. Fixed with a declared `_needs_priogrid_dbf` gate, like its
neighbours.
2. MEDIUM — two `xfail(strict=True)` deploy gates began EXECUTING in CI once the
sibling was fetched, and xfailed on missing files rather than on what they
assert. `data/assembled/{grid.npy,provenance.json}` are untracked upstream —
and measured today, `data/assembled/` is empty in the maintainer's own
checkout too, so these gates have never evaluated their assertions ANYWHERE.
A strict xfail that can only fail on FileNotFoundError can never XPASS, so
the flip that is the whole mechanism could not fire. They now skip when the
inputs are absent, which makes the state visible instead of disguising it as
a passing xfail. Registered as C-108; skipping does not make the gate work.
3. LOW/MED — the newly-live C-30 tripwire gated on `land_pgids.json` while
reading `land_gaul_pgids.json` too. Now that it runs in CI against
views-datafactory@main, a rename upstream would have surfaced as a traceback
instead of the drift assertion it exists to produce. Gate names both files.
The stale `(CI has no sibling → skip)` comment is corrected — it was the last
line in the repo still asserting the old state.
4. LOW — `test_the_builder_and_the_tests_resolve_the_same_datafactory` guards
the fallback under `if "VIEWS_DATAFACTORY" not in os.environ`, with a comment
saying that branch matters most in CI. Setting the variable in CI silenced
exactly that branch and left the surviving assertion comparing
$VIEWS_DATAFACTORY with itself. Now asserted unconditionally via
`monkeypatch.delenv`.
Also corrected two overclaims of my own: the workflow comment said the sibling
was fetched "for ONE check" and the register said "two", when four began
executing.
Measured. CI-shaped sibling: 430 passed / 5 skipped / 37 xfailed (two former
xfails are now honest skips). No sibling resolvable: no TypeError, 8 clean
skips. Full local sibling: nothing lost. ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…uns-in-ci C-30/C-46: the exclusion-manifest tripwire now runs in CI — the 2026-08-03 revert was the right observation with the wrong cause
…o boundary" (C-103)
C-103 was filed on an unverified premise. Verifying it moved the entry down a
tier and changed what the fix should be, so both are recorded here.
WHAT THE VERIFICATION FOUND. The question was whether production supplies
`datafactory_query`. It does, twice:
- views-models postprocessors/{un_fao,un_crafd}/requirements.txt both pin
views-datafactory>=1.9.0,<2.0.0, which is what ships the module. There is no
separate distribution — `pip download datafactory-query` finds nothing; it is
one of nine packages in views-datafactory's wheel.
- config_queryset.py imports datafactory_query.defaults at MODULE SCOPE and
raises naming the fix. A missing client makes get_queryset() return None,
which launch_config.assert_queryset_was_importable refuses (C-83) before
_read_historical_frame runs.
So the headline scenario — a missing dependency silently shipping fabricated
months on the live path — cannot occur. It was already guarded, by a check
written for something else. C-103 drops 2 -> 3.
WHAT IS ACTUALLY WRONG, AND IS FIXED HERE. The `except Exception` was never only
about the import. It made "the client is missing" and "the producer publishes no
boundary" the same event. The second is a normal older store and degrading open
is the recorded C-26 decision; the first is a broken environment, and degrading
open there ships the unobserved zero-padded tail as observed history on one
WARNING. C-60 is the same shape: a provenance stamp that degraded to "unknown"
on a bare except and made every delivery untraceable in the one field it existed
to answer.
- source_metadata raises ProducerClientMissing on ImportError, logged AND
raised (ADR-008), with a message naming the package, the install command,
and what degrading open would have cost.
- both managers re-raise it ahead of the broad except.
- the broad degrade-open is deliberately UNCHANGED. Narrowing it is a decision
about what to tell the partner when the boundary cannot be read, not a
refactor, and it is what keeps C-103 open.
TESTS. contract/source_metadata.py had NONE — which is how "return None like
everything else" ever looked reasonable. tests/test_source_metadata.py adds 7,
mutation-proven both ways: reverting the raise to `return None` fails 4 of them;
collapsing the manager's two branches into one fails the source check that pins
the ordering. That check is a source read rather than a behavioural one on
purpose — constructing a manager needs pipeline-core, a path manager and an
Appwrite environment (C-40), and the property worth holding is one line.
Suite: 439 passed, 1 skipped, 39 xfailed locally (25 failures are the C-104 venv
drift, unchanged). ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he CIC
Two findings on the previous commit.
1. The new manager check hardcoded ("unfao", "crafd") in its parametrize, when
`tests/conftest.py::PARTNER_PACKAGES` exists for exactly this and says why:
eight guards once named "unfao" literally, and all eight went on passing over
`crafd/` when it landed. This would have been the ninth. Now parametrized
over the declared list, like `test_store_construction`.
2. `docs/CICs/UNFAOPostProcessorManager.md:96` described `_read_historical_frame`
as degrading open "if the boundary cannot be resolved" — true before this
branch, and now only half true. It records both outcomes: degrade-open when
the producer publishes no boundary or the read fails, refusal when the client
cannot be imported. Review date moved to match its own content, per
`test_doc_accuracy::test_a_cic_review_date_is_not_older_than_its_own_content`.
Also simplified the ordering assertion in the same check. It searched for the
broad `except Exception:` from an offset before the refusal, which was fragile
and obscured that each manager has exactly one. It now asserts that count and
searches forward — re-mutation-proven: removing the narrow branch still fails it,
reordering the two branches still fails it.
Suite: 439 passed, 3 skipped, 37 xfailed. ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nistically
Six findings from /code-review high. The first two matter.
1. MEDIUM — the guard caught `ImportError`, and the failure this environment has
actually had is not one. views-models `postprocessors/un_fao/requirements.txt`
records, dated 2026-08-13, that resolving numpy 2.x into the shared
envs/views-postprocessing prefix makes the import die with
`ValueError: numpy.dtype size changed ... Expected 96 from C header, got 88`,
found by the pre-delivery rehearsal. An ImportError-only clause lets that sail
into the caller's degrade-open and ship the unobserved tail — the exact case
this guard exists to separate, and the likeliest one. Now catches Exception.
2. MEDIUM — four of the tests proved the guard by ambient accident: they relied
on `datafactory_query` happening to be absent in this venv, so in the launcher
prefix (where it IS installed) all four would have failed. That is the
C-30/C-46 shape a day after fixing it. Both states are now simulated
deterministically: an `absent_client` fixture, and an `exploding_client` one
that reproduces the numpy ABI ValueError.
3. LOW — the message asserted a diagnosis. An ImportError raised INSIDE an
installed package was reported as "not importable ... pip install
views-datafactory", sending the operator to a fix already applied. It now
distinguishes `ModuleNotFoundError` naming datafactory_query ("not installed",
with the install line) from anything else ("present but raised while loading",
quoting the error, and "Do NOT reinstall").
`ProducerClientMissing` -> `ProducerClientUnavailable`: one type, two causes,
and the old name asserted the cause that is not the common one.
4. LOW — `_read_historical_frame`'s docstring still said "degrade-open" flat.
Corrected in both managers. C-107 is the entry recording that docstrings sit
outside test_doc_accuracy's corpus, so nothing would have caught it.
5. LOW — C-103's Location was made stale by the previous commit (`:37` is now
mid-docstring). Re-read, and `pyproject.toml` dropped from it: the entry's own
body concludes the dependency is the launcher's to declare and that both
launchers do.
6. LOW — the manager tripwire compared file offsets without tying them to the
same `try`, and misreported a removed degrade-open as "grew a second broad
except". Now scoped to `_read_historical_frame` and counts both branches, so
the message says what actually happened.
Mutation-proven, all three ways: narrowing the catch back to ImportError fails
the ValueError test; dropping the installed-vs-missing classification fails the
message test; removing the degrade-open branch fails the tripwire with the
correct message.
Suite: 440 passed, 3 skipped, 37 xfailed. ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lip-fails-loud C-103: a missing producer client refuses instead of passing for 'no boundary' — and the Tier 2 premise did not survive verification
… (C-104)
On 2026-08-16 a pytest run here reported 25 failures across five modules and none
was a defect. The virtualenv held views-pipeline-core 2.3.0 and pyarrow 23.0.1
while poetry.lock pinned 3.0.1 and 16.1.0. Two majors of drift produce:
- ModuleNotFoundError on views_pipeline_core.modules.dataloaders.
datafactory_contract, imported at module scope by BOTH managers, which takes
out every test that constructs or inspects one — the only coverage the two
largest modules in the package have;
- five byte-parity failures against the ADR-013 §10 fixture, because parquet
bytes are not stable across pyarrow majors (C-72).
From the failure output neither is distinguishable from a real regression, and
"you broke the wire contract" is the reading that costs an afternoon.
tests/test_locked_environment.py compares the installed versions of the runtime
dependencies DECLARED in pyproject.toml against poetry.lock. Declared, not
hardcoded: a literal list goes stale the first time a dependency is added, which
is the failure conftest.PARTNER_PACKAGES exists to prevent one level up.
The failure names each drifted package, both versions, the command, and says
plainly that the other failures are consequences rather than defects — and that
if the drift is deliberate, the rest of the suite is not testing the locked
contract. A second check catches the other direction: declared in pyproject but
absent from the lock, which would otherwise read as a virtualenv problem when it
is a stale lock.
It fixes nothing and skips nothing. The 25 stay until `poetry install` runs. What
changes is that a contributor can tell in one line which kind of problem they
have — the failures were never wrong, they were unreadable.
Dev-group tools are out of scope on purpose: ruff's reported version varies with
how it was installed, and what actually broke CI on 2026-08-03 was its rule set,
already pinned explicitly in pyproject.toml.
Natural experiment rather than a mutation: this FAILS here, against the real
drift, and must PASS in CI, which installs from the lock.
ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eight findings. The first is the one that matters: the guard whose entire purpose is a readable diagnosis stated something measurably false. 1. "every test that touches a manager dies at collection" — it does not. The manager tests import lazily (tests/test_framework_contract.py:50), so the drift produces ordinary failures and the session runs to completion. Measured in the drifted venv: 26 failed, 441 passed, 0 errors. The claim was also self-defeating — pytest interrupts on collection errors, so had it been true this diagnostic would never have run in the scenario it was written for. Corrected in the message and the module docstring. 2. The consequences paragraph was hardcoded to the two packages that drifted on 2026-08-16 but fired verbatim for any drift: a views-frames bump would have been explained in terms of pipeline-core and parquet bytes. Now conditional on which names are actually in the drift set — a diagnosis naming the wrong packages is the failure this file exists to remove. 3. No PEP 503 normalization between pyproject keys and lock names. `PyYAML` or `views_frames` are legal declarations that would never match a lock entry, and the failure would have read "absent from poetry.lock, run `poetry lock`" — which fixes nothing, because the lock is fine. Both sides normalized. 4. Dependencies gated by `optional`, `python` or `markers` were treated as must-be-installed. They are legitimately absent from a given environment, and the message would have said "NOT INSTALLED — run `poetry install`", advice that cannot work. It would also have gone red in CI on the 3.11 runner for a python-gated dep. Skipped. Verified that `extras`-carrying deps are still KEPT, since that is our real views-pipeline-core declaration. 5. The claimed ordering guarantee did not hold: running the lock check first does not stop the version check reporting the same omission as `locked=None`. The version check now skips names absent from the lock and defers. 6. `installed=None, expected=None` was a silent pass. Same fix as 5 — unlocked names are the other test's business, and it says so better. 7. `project["tool"]["poetry"]["dependencies"]` raised a bare KeyError on a PEP 621 migration — the unreadable failure this module was written against. Now fails with the form it expects and an instruction not to delete it, matching tests/test_release_version.py's precedent. 8. Register `Last Updated` was still 2026-08-16, stale since C-103's additions. Verified: normalization on PyYAML/views_frames/Foo.Bar_baz; the conditional-skip rule against optional/python/markers/extras shapes; the message now naming only the packages that drifted. ruff clean; register and doc-accuracy guards green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tectable C-104: one honest failure instead of twenty-five misleading ones
…s (C-94)
C-94 is the one failure mode where everything reports success and the partner
sees nothing: the upload lands, storage is billed, and the consumer's endpoint
returns empty. It is not hypothetical — run-0's historical artifact stranded on
2026-07-27 as a file with no metadata document (C-79) and nothing here noticed.
Every other mechanism the platform aims at this is a CI-time proxy; none observes
the outcome of a real upload.
After both legs are uploaded, each manager now queries the partner store the way
the consumer does — `name == product.CONSUMER_DOCUMENT_NAME`, per category — and
refuses a falsy answer. `delivery/findability.py` holds the rule; the managers
own the query, because they own the port.
Two decisions worth not re-deriving:
- It runs on the EXISTING key. Verified in the Appwrite console 2026-08-18: the
live `VIEWS Pipeline Core` key already carries documents.read / rows.read /
buckets.read / files.read. The registry's separate APPWRITE_READ_API_KEY slot
would buy no isolation here, because the preflight runs inside the delivery
process, which already holds the write key it just uploaded with. That slot
stays for a preflight that runs OUTSIDE the delivery.
- It queries through a store with pipeline-core's automatic `name == model_name`
filter SUPPRESSED. `get_latest_file_id` merges the path manager's model name
into every query, so without that the check would verify the views-models
directory name — equal to the declared consumer name only by coincidence
(C-77). Verifying the coincidence would leave this green while a rename took
the delivery dark, which is the precise failure it exists to see.
Both legs are checked separately: a run whose forecast landed and whose
historical did not is invisible in exactly one half, and the historical leg is
the one that actually stranded.
Scope stated in the module rather than claimed: this catches "the delivery ran
and the consumer cannot see it". It does NOT catch "no delivery happened" (the
2026-08-12 empty bucket, an upstream migration with no run since) or "stale data
served from faoapi's warm cache". Both stay recorded as gaps in C-94, and the
tier stays at 2 because of them — the mechanism addresses the cause the tier
rationale named, and the gaps are what now hold it.
Three of the repo's own guards shaped this, all working as designed:
- the manager line budgets (450 file / 300 class) rejected the first draft, which
put the preflight in the class. Its message prescribed the fix — a module-level
function, callable without a manager or an Appwrite environment (C-40 (a)).
Now 435/450 and 285/300.
- `test_clone_readiness` refused the new module until it was declared in
`_MACHINERY`, where the import-purity checks can see it.
- my own new test refused `assert_findable("")` — an empty file id is "found
something unusable", not a find, the same polarity `_ContractStorePort.download`
learned as C-99. Fixed the code, not the test.
Mutation-proven: dropping the call, un-suppressing the name filter, and making
the rule always pass each fail their guard.
Suite: 449 passed, 3 skipped, 37 xfailed. The 26 failures are the C-104 venv
drift, unchanged. ruff clean. CIC updated with the new failure mode and its two
stated gaps.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iew-diff Two findings on the preflight. 1. SUBSTANTIVE. The preflight makes a live network call after the uploads have already succeeded. If that call RAISES rather than returning None, the error propagated from inside pipeline-core with nothing to say that the delivery landed and only the verification failed — so a transient store blip would read as "the delivery is invisible" and be quarantined. That is the same distinction this repo has now drawn three times: C-99 (an unrecognised store result is refused and named, not adapted to), C-103 (a missing producer client is not a producer that publishes no boundary), and now this. The two conditions call for different operator actions — a delivery that cannot be FOUND is quarantined; a delivery that could not be CHECKED may be perfectly fine. `findability.unverified()` names the second as `FindabilityUnverifiedError`, quoting what stopped the check. 2. Cosmetic: one blank line before the manager class where the file uses two everywhere else. Ruff did not catch it because the declared rule set is ["E4","E7","E9","F"] and blank lines are E3 — deliberately narrow (see the pyproject comment on why the set is pinned), so this is a style question the linter does not own. Budgets after: 438/450 file, 285/300 class. Mutation-proven — collapsing the two refusals back into one bare query fails the new wiring check. Suite: 452 passed, 3 skipped, 37 xfailed; the 26 failures are C-104 venv drift. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six findings. The first would have made the guard worthless from the second
delivery onward, which is worse than not having it.
1. HIGH. The query asked "is there ANY document under the consumer's name for
this category" — which the PREVIOUS delivery already answers yes to.
Documents accumulate across runs; that is what makes "latest" meaningful to
the consumer. So: run-2 uploads, `upload_data` reports success, its metadata
document is never created (the exact C-79 shape), the query returns run-1's
document, and the preflight logs "passed" while the consumer goes on serving
run-1. The call-site comment claiming nothing above observes the outcome of an
upload was true only for the very first delivery.
`_ContractStorePort.upload` now returns the uploaded `file_id` instead of
discarding it; the sink carries the manifest's id out (uploaded last, so it is
the newest `category="forecast"` document); and the check asserts the newest
document the consumer would find IS the one this run put there. The refusal
distinguishes "nothing found" from "found the previous run's", because those
are different operator situations.
2. NOT UPHELD, and the reason is worth more than the finding. The review said
`unverified()` was unreachable because `get_predictions_by_metadata` swallows
a failed search and returns []. True of 2.3.0 — what the drifted venv holds.
FALSE of 3.0.1, which poetry.lock pins and CI installs: there it RAISES
`MetadataSearchIncomplete`, with a comment giving our own argument back to us
("Returning [] here would tell every caller 'no predictions match', which is a
statement about the shelf rather than about the lookup"; views-pipeline-core
C-241). Verified by reading 3.0.1 from the sibling checkout rather than the
installed package. This is C-104's hazard in its most expensive form yet: not
a wall of red, but a confident and wrong conclusion about production drawn
from a stale environment. Recorded in C-94.
3. Store construction moved inside the try, so a missing APPWRITE_UNFAO_* var
after a successful upload reports "unverified" rather than a raw traceback.
4. The CIC and register both named `ProducerClientMissing`; the class is
`ProducerClientUnavailable`. Renamed yesterday, references left stale — which
is C-107 (docstrings and docs outside the doc-accuracy scan) landing on me
within two days of registering it.
5. The structural tests were textual and did not test what their messages
claimed. The interlock check only asserted the call appeared after the `if`,
so moving it into the `else` kept it green; the preflight slice ran to
end-of-file, so any later occurrence in the module satisfied it. Both now
parse the AST: the call must be inside the `if upload_enabled:` body, and each
slice is exactly one function. Tightening them immediately exposed that the
declared name and the two legs are supplied at the CALL SITE, so the
assertions were looking in the wrong function.
6. `_build_partner_read_store` was absent from the two C-40 structural guards in
test_store_construction, so neither "is a module-level function" nor "no
coordinate baked in" covered it. Declared in both.
Manager budgets held by trimming docstrings that restated the register rather
than pointing at it: 435/450 file, 289/300 class.
Mutation-proven: accepting any document (not just this run's) fails the new
run-scoping test; moving the call into the `else` branch now fails the interlock
test, which it would not have before.
Suite: 453 passed, 3 skipped, 37 xfailed. ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…light C-94: the findability preflight — ask the store what the consumer asks
The contract already handles the CONSUMER's side of a mid-upload failure
correctly and by design: the manifest is uploaded last, so an attempt that dies
before it has no commit marker and is invisible rather than half-visible (§4.2).
Nothing partial is served.
Our side was the gap. The objects that did land stayed in the partner store and
nothing recorded that they had — an operator was left to diff the bucket by hand,
and at run-0 scale a retry adds ~110 more under the same names.
`deliver_run` now keeps an in-memory ledger alongside the log, and any failure in
the upload phase raises `TornRunError` naming the run, how many of how many
objects landed, and their names AND file ids. The message states three things an
operator would otherwise have to establish themselves:
- the consumer cannot see this run, so nothing partial is being served;
- the listed objects are still there and were NOT removed;
- a re-run uploads all of them again under the same names, and whether the
store supersedes or duplicates is not something this repository asserts.
DELETION IS DELIBERATELY NOT DONE. Removing objects from a partner bucket is
irreversible and an operator decision rather than a delivery-path one, and the
neighbouring delete surface is its own open question (C-58,
views-pipeline-core #333, blocked on a test key). C-105 therefore stays open with
a Partial mitigation: the mess is now legible, and it is still a mess. Closing it
needs a decision about who cleans up, which is not engineering work here.
tests/test_torn_run.py (5), mutation-proven both ways — dropping the wrapper
fails 4, dropping the ledger fails 3.
Suite: 458 passed, 3 skipped, 37 xfailed. The 26 failures are C-104 venv drift.
ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three findings. 1. COVERAGE GAP, recorded rather than closed. `TornRunError` wraps the upload phase inside `deliver_run`. The historical artifact uploads AFTER the wire run is committed, from the manager, so a failure there raises unwrapped — and its consequence differs rather than being smaller: the manifest already landed, so the consumer sees a complete, visible forecast run next to the PREVIOUS run's historical artifact. Not corrupt (the historical is a full snapshot, so the older one is valid, just one run stale), and the delivery does report failure — but it is the one tear where "the consumer cannot see this run" is false. The wrapper does not fire there, so nothing says anything wrong; the gap is that nothing says anything at all. Left uncovered deliberately: the manager is at 435/450 of its line budget and this belongs with whoever takes C-105's deletion decision. Recorded in C-105 and in the CIC. 2. The test module reached into `tests/test_hop_b_sink_e2e` for `FakeLease` and the PRIVATE `_synthetic_lookup`, coupling two files that should be able to change apart. Both are now local, and the lookup is derived from `gaul_schema.METADATA_COLS` rather than copied as a literal table — a hand-written table here would drift from the contract silently. 3. `_torn` named a factory that returns an exception rather than raising one. `_torn_run_error`, with the return type declared. Also bumped the CIC review date, which its own guard caught: the body cited 2026-08-19 while the header still said 2026-08-18. Suite: 458 passed, 3 skipped, 37 xfailed. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n-run refusal Six findings. The first three would each have sent an operator the wrong way, which is a bad property for a message whose only job is to direct one. 1. HIGH. The refusal omitted the object that FAILED — and that object is the likeliest orphan of the entire run. `_ContractStorePort.upload` raises precisely in the case its own comment documents: the store "RETURNS success=False with the file already uploaded". So the failing object is the C-79 shape, sitting in the bucket with no metadata document, while the message listed only the successes and called that what remains. An operator who trusted it would walk straight past the orphan this change exists to surface. Now named explicitly, as a separate thing to look for. 2. MEDIUM. File ids were truncated to five in the message and recorded nowhere else — the log ledger line carried name, doc_type and run_id but not file_id. At run-0 scale that is ~104 ids existing only in a string nobody kept. The ledger line now carries file_id and is the persistent record; the message says so. 3. MEDIUM. A failure on the FIRST upload — credentials expire, bucket unreachable, the commonest infrastructure failure there is — printed "Already in the partner store, and NOT removed: ." An empty list with a dangling period, presented as a bucket to audit. Now says nothing is confirmed landed, while keeping the caveat from (1), which still applies. 4. LOW. "The consumer cannot see this run" was asserted categorically, and the code cannot know it: a manifest upload can fail after the store committed the document, in which case the consumer IS being served this run and a re-run duplicates every object. Softened to "almost certainly", with an instruction to verify. 5. LOW. TornRunError inherited SinkError, whose family means "cannot be delivered as declared" — malformed input, do not retry. A tear is transient infrastructure with the opposite semantics, and test_hop_b_sink_e2e already uses `pytest.raises(SinkError)` as the malformed-run assertion. Re-based on RuntimeError, matching delivery.findability's two error types, with the reasoning in the docstring. 6. LOW. Missing blank lines before a module constant in the test file. Three new tests (8 total). Mutation-proven: dropping the orphan caveat fails 2, re-basing on SinkError fails 1. Suite: 461 passed, 3 skipped, 37 xfailed. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nosable C-105: a torn run says what it left behind
#224 says "what is asked of this repo: nothing yet", and C-84 goes further: "The honest position is that this is a date to act on, not a mechanism to build, and inventing a mechanism would be building the wrong thing to feel busy." That verdict is about a key-VALIDITY preflight — an authenticated call at startup — and it stands. This is not that. No authenticated call, no credential, no network: a calendar and two declared datetimes. What it fixes is C-84's own trigger. Its first arm reads "act when the un_fao delivery is next scheduled within a month of it", which is a trigger nobody can notice: it fires in someone's memory or not at all. This repository has already written that down twice — ADR-014 §4, and the withdrawn third arm of C-94's trigger. A foreseeable total outage 90 days out deserves better than memory. tests/test_credential_expiry.py declares both expiries and fails from 30 days before the earlier one, naming the dates, the 3h35m gap (which is C-84's actual finding — it is not a stagger, neither key can carry traffic while the other is replaced), who owns the rotation (operator; views-appwrite#12, split at views-faoapi#338), and the two honest ways to make it pass: rotate and update the constant, or update the constant if a key was replaced early. Deleting the test is the third way and the message says it is the one that produces the outage. The dates cannot be derived from anywhere — the coordinate registry records secret SLOTS, never values or their lifetimes, which is #224's closing observation: no amount of drift detection surfaces this one. They come from the operator console read of 2026-08-05 (þing-02 A3(i)), and a second test pins them and the gap against C-84 so a typo fails rather than quietly moving the tripwire. Verified by simulating the clock at 2026-10-25: fires with the full message, 23 days out. Passes today at 90 days. Suite: 463 passed, 3 skipped, 37 xfailed. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two findings on my own tripwire. 1. THE FIRING BRANCH WAS UNPROVEN. The guard's failing path would first have executed in October, live, on the day it matters — which is C-102's exact lesson: a guard that has never run is unproven however carefully it was written. The decision and the message are now pure functions of a date (`days_until_earliest_expiry`, `expiry_warning`), and the failing branch is exercised against a simulated 2026-10-25. A third test asserts silence well outside the window, because a tripwire that is always red is one nobody reads. 2. Naive datetimes, now documented as deliberate rather than left to look like an oversight: the console reports local time and the window is 30 days, so an hour either way changes nothing. Adding tzinfo would imply a precision the source does not have. Mutation-proven in both directions, and the second one caught a hole I had made myself: shrinking LEAD_DAYS to 0 silently neuters the tripwire, and my first pass did not catch it because I had deleted the assertion that would — mid-cleanup, as a half-written line. Restored: a date 23 days out must be inside the window. LEAD_DAYS = 0 -> 1 failed LEAD_DAYS = 400 -> 2 failed typo an expiry -> 2 failed Suite: 465 passed, 3 skipped, 37 xfailed. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six findings. Two were design faults that would each have ended with the test deleted, which for a tripwire is the only failure mode that matters. 1. HIGH — the pin made the tripwire's own prescribed fix impossible. Its message says "rotate, then update KEY_EXPIRY"; a second test hardcoded the current literals and would have failed on exactly that edit, with a message saying "re-read the entry rather than adjusting the constant to match". Measured: setting the constant to a rotated date turned 2 tests and 5 assertions red. A guard that refuses its own documented remediation is worse than no guard, and in November it lands on the one person who cannot route around it. The pin is gone; verified that a simulated rotation now passes. 3. MEDIUM (and the one that would have got it deleted) — from 2026-10-18 the gate goes red for EVERY unrelated pull request, clearable only by an operator console action this repository cannot perform. That is what pyproject.toml already says about ruff, citing ADR-014 §3: a gate that starts red gets switched off. `ACKNOWLEDGED_UNTIL` is the in-repo escape — a declared, reviewed, dated edit meaning "seen, and being acted on" — and it cannot be set on or after the expiry, so it postpones attention and never replaces it. Verified both ways: setting it clears the gate; setting it past the expiry fails. 2. MEDIUM — the firing-branch proof hardcoded 2026-10-25 / 23 days, both derived from today's constant, so after any legitimate rotation the probe would fall outside the window and the C-102 claim would be void. The probe is now derived from KEY_EXPIRY and survives rotation. 4. LOW — the LEAD_DAYS guard only caught shrinking below 23; 23 through 30 all passed, so a week could be shaved with a green suite. Floored at 30, with widening left free. 5. LOW — the register claimed the datetimes were "pinned against this entry". Nothing reads the register; it was same-file literal duplication. Claim removed along with the pin. 6. LOW — after the expiry the message read "expire in -3 days", and the gap rendered "3:35:00" where the entry title says 3h35m. Now "TODAY" / "3 days ago — the seam is already dead", and "3h35m". This is the text an operator reads while the seam is down. Suite: 467 passed, 3 skipped, 37 xfailed. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
C-84/#224: make the 2026-11-17 key expiry fire on its own
Polichinel
changed the title
…ecords Five findings held on re-measurement; two the sweep raised did not and are not acted on (an "editable install" claim — the venv runs a plain stale 2.3.0 from site-packages, so the drift guard is correct; and an "impossible arithmetic" claim about C-46's figures — those were scoped to PR #280, which adds no tests, and reproduce exactly CI-shaped). 1. HIGH — the torn-run message truncates at five objects and pointed the reader at "the run log" for the rest. The only writer of a file_id was `logger.info`, and nothing in this package sets a level: pipeline-core removed its own `setLevel` so the application owns it. A launcher at WARNING would have written no id anywhere, making the pointer a promise to an empty file — and leaving the operator diffing the bucket by hand, which is the state C-105 exists to remove. The complete ledger is now emitted at ERROR, one greppable `TORN-LEDGER` line per object, which is also ADR-008's log-and-raise. Tested and mutation-proven. 2. The CIC described the SUPERSEDED findability check — "refuses a falsy answer" — which is the question C-94 says "the previous delivery already answers yes to". It now documents the run-scoping and both refusals, including the two-id one an operator would actually meet. 3. C-84 enumerated "the two honest ways to make it pass" while the test offers three, and the omitted one — ACKNOWLEDGED_UNTIL — is the only remedy available without console access. The register is where someone goes when the gate reddens; leaving it out reproduced the merge-queue-hostage outcome the acknowledgement exists to prevent. 4. C-103's Location cited unfao.py:137-151 as "the two branches" and claimed the lines were re-read. They are at 169 and 174; 137-151 is now the body of the C-94 preflight added later in the same branch. This is the second time in four days, so the fix is structural: cite by symbol. 5. Register header still said 2026-08-17 with seven later-dated records below it, and C-105 claimed the manager is at 435/450 when `wc -l` says 434. Registered as C-109, because the fourth is a class rather than a typo. Measured: eleven of 28 open entries cited a `.py:line`; spot-checking six found THREE already stale, all drifted by a later change in the same week. C-103, C-105, C-106 and C-30 converted to `path::symbol`; eight entries still cite lines where the target genuinely is one. The entry records why the obvious guard does not work, so it is not proposed again cheaply. Every symbol citation verified to resolve to a real `def`. Suite: 468 passed, 3 skipped, 37 xfailed; the 26 failures are C-104 venv drift. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
/code-review max findings: the ledger promise, and four stale records
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
1 participant
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.
Seven merged stories, each through the same loop — branch → PR →
/code-review high→/review-diff→ address every finding → merge. All green in CI. No delivered bytes change and nocontract_versionbump.Opened for review, not merged by me: merging to
mainis the production release for this repo.The through-line
Six of the seven are the same defect wearing different clothes: a guarantee whose check was not actually checking. In each case the code looked armed and was not, and nothing in the suite could tell.
ImportError; the failure this environment actually has is aValueErrorThe four findings worth reading
The exclusion guard's blocker was misdiagnosed for two weeks (#280). The recorded reason was that views-datafactory's parquets are untracked, so a checkout turns skips into
FileNotFoundError. True symptom, wrong cause:data/raw/gaul_admin/is tracked (it holds a geojson) while the parquets are not, so a.is_dir()gate passed and the comparison died. Four checks moved from skipped to running, including one C-46 says had "never run anywhere but one laptop".C-103's Tier 2 premise did not survive verification (#281). Both launchers pin
views-datafactory, and the queryset fails loud at import — so the scenario was already guarded by C-83. Re-tiered 2 → 3, honestly, in the entry. The review then found the real defect underneath.C-94 would have been worthless from delivery 2 onward (#287). It asked "is there any document under the consumer's name" — which the previous delivery already answers yes to. Now scoped to this run's uploaded file id.
A reviewer drew a confident, wrong conclusion about production from the drifted venv (#287). It reported a guard as dead code because pipeline-core 2.3.0 swallows failed searches. 3.0.1 — which the lock pins and CI installs — raises. That is C-104's hazard in its most expensive form, and it is recorded in C-94.
What this does not do
contract_versionbump, no fixture re-cut, no change to the wire.Two things that will surprise you later if unread
ACKNOWLEDGED_UNTILis set intests/test_credential_expiry.py. That is deliberate (C-84, The key behind both delivery paths expires 2026-11-17 12:35 (C-65) #224) and the failure message explains all three ways out. It cannot be acknowledged past the expiry.maincan now turn this repo red — ADR-016's deliberate trade, already accepted for views-appwrite. It is the drift detector working.Register
108 total / 28 open (was 102 / 22). New: C-103–C-108 — including C-108, two
xfail(strict=True)deploy gates that had never evaluated their assertions anywhere, becausedata/assembled/is empty in every checkout including the maintainer's.Verification
ruff check ./code-review high, 7 ×/review-diff— every round found real defects, all fixed and mutation-provenStill outstanding, deliberately