diff --git a/.github/workflows/run_pytest.yml b/.github/workflows/run_pytest.yml index a782614..09b7f24 100644 --- a/.github/workflows/run_pytest.yml +++ b/.github/workflows/run_pytest.yml @@ -75,6 +75,29 @@ jobs: path: _siblings/views-appwrite fetch-depth: 0 + # Fetched for the checks that need only views-datafactory's TRACKED files. The one + # that motivated it is `test_delivery_coverage.py::test_manifest_matches_datafactory_ + # land_minus_land_gaul`, C-30's drift tripwire on the 76-cell exclusion manifest: it + # reads `src/datafactory_query/{land,land_gaul}_pgids.json`, both tracked here, and + # until 2026-08-17 it ran only on a laptop — while this repository told a partner in + # writing that the manifest "cannot drift without failing loudly". It could; nothing + # in the gate was watching. Three more checks came with it (the release gate, the + # region-set check, the wire-cast dtype check); see C-46 for the full accounting. + # + # This checkout was tried on 2026-08-03 and reverted, and the revert note said the + # sibling could not be fetched because its GAUL parquets are untracked. That was the + # wrong diagnosis. `data/raw/gaul_admin/` IS tracked (it holds a geojson); only the + # parquets are not — so `test_gaul_lookup_fidelity`'s `.is_dir()` gate passed and the + # comparison died on FileNotFoundError. The gate now checks for the seven parquets + # themselves, so that half skips honestly here and still runs where they exist. + - name: Checkout views-datafactory (sibling) + uses: actions/checkout@v3 + with: + repository: views-platform/views-datafactory + ref: main + path: _siblings/views-datafactory + fetch-depth: 0 + - name: Set up Python uses: actions/setup-python@v4 with: @@ -97,6 +120,7 @@ jobs: - name: Run tests env: VIEWS_APPWRITE: ${{ github.workspace }}/_siblings/views-appwrite + VIEWS_DATAFACTORY: ${{ github.workspace }}/_siblings/views-datafactory run: | set -e poetry run pytest tests/ diff --git a/docs/ADRs/013_sampled_forecast_wire_contract.md b/docs/ADRs/013_sampled_forecast_wire_contract.md index 6911d44..3c3fd91 100644 --- a/docs/ADRs/013_sampled_forecast_wire_contract.md +++ b/docs/ADRs/013_sampled_forecast_wire_contract.md @@ -661,6 +661,40 @@ geography. **Source:** the sidecar is built from this repo's ADR-011 GAUL lookup (`views_postprocessing/data/gaul_lookup.parquet`, area-majority cell→region mapping sourced from views-datafactory); the #91 sink leg attaches it per run. +**§5.1a Nullable int64 was considered and rejected** *(clarification 2026-08-17, +MINOR — no change to the rule, and no `contract_version` bump; this records an +alternative the 2026-07-19 ruling did not weigh).* The partner has now twice asked +for the `*_code` columns as integers (#278; views-postprocessing#272), and the +justification given each time — that an integer column cannot carry a missing +value — is a property of NumPy-backed pandas, **not** of Parquet. Parquet and Arrow +both carry nullable integers natively, and this repo's own lookup stores all three +code columns as `int64` with zero nulls; the float is introduced by our writer +(`contract/wire/sidecar.py`), not by the source. So the alternative is real and the +old reason for dismissing it was wrong. + +It is rejected anyway, on a measured ground rather than that one. What §5.1 requires +is a schema that does not depend on the data. Nullable int64 does not deliver that +at the layer the consumer observes — it relocates the dependence. Measured +2026-08-17 (pyarrow 23.0.1, pandas 3.0.5): an int64 Parquet column containing **no** +null reads back as `int64` under a default `pd.read_parquet`, and the same column +containing **one** null reads back as `float64`. Under float64 the consumer sees one +dtype always; under nullable int64 they would see `int64` usually and `float64` +whenever a run happened to contain a missing code — which is the data-dependent +schema the 2026-07-19 ruling rejected, moved from our writer to their reader. The +escape (`dtype_backend="numpy_nullable"`) is a consumer-side commitment this repo +can neither verify nor enforce (cf. C-87, C-92). Independently, faoapi's reader +`reindex`es the sidecar onto the forecast's gids and then calls `.to_numpy()`, both +of which return float64 from a nullable integer column — so the change would not +even reach the consumer as integers. + +**Consequence for the partner, and it is the useful half:** because the delivered +region excludes the GAUL-uncovered cells (`delivery/coverage.py`), no delivered code +is ever missing — `tests/test_gaul_lookup_fidelity.py::test_lookup_has_no_nulls` +holds this in CI — so `astype("int64")` on read is lossless for this product. That +is a property of the delivered **region**, not of the contract: a future region with +no exclusion list could carry genuinely missing codes, which is exactly why the +column type stays float64. + **§5.2 Consistency.** The sidecar's cell-id set must equal the forecast's cell-id set (views-postprocessing's existing coverage/identity invariants, to be extended to the sidecar in the #91 leg — not yet built as of 2026-07-19). The sidecar hash is pinned in **the Hop-B run manifest diff --git a/docs/CICs/UNFAOPostProcessorManager.md b/docs/CICs/UNFAOPostProcessorManager.md index e98b6d2..1e1e242 100644 --- a/docs/CICs/UNFAOPostProcessorManager.md +++ b/docs/CICs/UNFAOPostProcessorManager.md @@ -3,7 +3,7 @@ **Status:** Active **Owner:** PRIO MD&D Team -**Last reviewed:** 2026-08-05 +**Last reviewed:** 2026-08-19 **Related ADRs:** ADR-001, ADR-002, ADR-008, ADR-009 --- @@ -89,11 +89,12 @@ Assumptions that are not met **must cause failure**, not fallback behavior. The - **Missing required metadata columns after enrichment:** Raises `ValueError` listing missing columns - **Null values in required metadata columns:** Raises `ValueError` with null count and affected column name (C-01 resolved — validation active) - **Dataset initialization failure:** Raises `ValueError` in `_save()` if datasets are None -- **Appwrite upload failure:** Propagates exception from `DatastoreModule` +- **Appwrite upload failure:** Propagates exception from `DatastoreModule`. **Inside the wire leg** it is wrapped as `contract.wire.sink.TornRunError` (C-105, 2026-08-19), naming the run, how many of how many objects landed, and their names and file ids — the consumer cannot see a torn run (the manifest is the commit marker and never landed), the listed objects are **not** removed, and a re-run uploads all of them again under the same names. **The historical leg is not covered by that wrapper**: it uploads after the wire run is committed, so a failure there leaves a visible, complete forecast run alongside the *previous* run's historical artifact. Recorded as remaining scope in C-105 +- **Delivery invisible to the consumer:** raises `delivery.findability.DeliveryNotFindableError` (C-94, added 2026-08-18). After both legs are uploaded, the manager queries the partner store as the consumer does — `name == product.CONSUMER_DOCUMENT_NAME`, per category — and asserts the newest document it finds **is the one this run just uploaded**. Two distinct refusals: nothing found at all, and *found the previous run's* (*"the newest forecast document is X, but this run uploaded Y"*). The run-scoping is the whole guard — asking merely whether any document exists is a question the previous delivery already answers yes to, so the check could never fail from delivery 2 onward. This is the one failure mode where every upload reports success and the consumer still sees nothing; run-0's historical leg stranded exactly that way (C-79). It runs only inside the §11.4 interlock, and queries through a store with pipeline-core's automatic `name == model_name` filter suppressed, so it verifies the declared name rather than the views-models directory name that happens to match (C-77). **It does not detect a delivery that never ran, or stale data served from the consumer's cache** — both recorded as gaps in C-94 - **Wrong forecast selected:** structurally impossible since #149. Selection is by **run manifest** — a commit marker whose contents are hash-verified — not by scanning the bucket for the newest `category="forecast"` upload. Declared identity is additionally checked **per shard header** against the launched ensemble inside `TargetLease.load()` (`contract/wire/source_selection.py:73-81`), so identity comes from the artifact's own content. The metadata-field check this bullet used to describe (`delivery/identity.py`) was retired in #150 and the legacy reader it served in #149; register C-25 is closed as *superseded by mechanism* - **Launch config incomplete:** raises `LaunchConfigError` naming the missing key. A launcher that omits `wire_contract` or declares a `data_format` other than `feature_frame` is **refused**, never quietly routed into a fallback (ADR-003, register C-63) - **Region coverage mismatch:** Raises `CoverageError` in `_check_coverage()` (called from `_validate()`) if a pinned region's delivered cell count is wrong (S1/C-34) or a GAUL-uncovered excluded cell leaks into the delivery (S4/C-30) -- **Fabricated historical tail:** `_read_historical_frame()` drops months beyond the producer's `last_valid_month_id` at the read (`_clip_observed_history` was the pandas equivalent, retired with that path in #149) so unobserved zero-padding is not shipped as observed history (S2/C-26); **degrades open** (skips the clip with a WARNING) if the boundary cannot be resolved +- **Fabricated historical tail:** `_read_historical_frame()` drops months beyond the producer's `last_valid_month_id` at the read (`_clip_observed_history` was the pandas equivalent, retired with that path in #149) so unobserved zero-padding is not shipped as observed history (S2/C-26). Two outcomes when the boundary is unavailable, and they are different on purpose (C-103, 2026-08-17): if the producer simply publishes no boundary — or the read fails — it **degrades open**, skipping the clip with a WARNING that states the unobserved tail will ship; if the producer client cannot be imported at all it **refuses** (`source_metadata.ProducerClientUnavailable`), because a broken environment is not a producer fact - **Upload provenance:** the historical artifact's `description` carries structured provenance (lookup version, region, expected/actual cell counts, unmapped count) built by `delivery/provenance.py` (`build_provenance` → `compact_description`) via the manager's `_historical_frame_description()` (S5/C-15). The **forecast** side carries no such description: its guarantee is the wire's verified chain — per-shard content hashes recorded in the §4.2 run manifest, header asserts on load, and manifest-last commit ordering. That is identity and integrity, not the C-15 provenance field set; the §4.2 manifest's keys are exactly `contract_version`, `run_id`, `targets`, `shards`, `expected_months`, `expected_cell_count`, `sidecar` — and it carries **no** `lookup_version`, `region` or `unmapped_count`. `_delivery_description()` was the pandas-path equivalent and was deleted with it in #149 The following **must never** fail silently: diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index f90ef8d..2dda7c5 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -4,9 +4,9 @@ |-------------------|--------------------------------------| | Project | views-postprocessing | | Owner | Dylan Pinheiro / PRIO MD&D Team | -| Last Updated | 2026-08-15 | -| Total Concerns | 102 | -| Open Concerns | 22 | +| Last Updated | 2026-08-21 | +| Total Concerns | 109 | +| Open Concerns | 29 | | Resolved Concerns | 80 | --- @@ -54,7 +54,7 @@ covered a single open entry (see Historical clusters below). ### Cluster J: Delivery aftercare has no mechanism **Root cause:** the delivery pipeline is write-only — nothing exists downstream of upload for correction, recall, or provenance audit. -**Entries:** C-22 (acute), C-15, C-24 +**Entries:** C-22 (acute), C-15, C-24, C-105 (added 2026-08-16 — a torn upload attempt is aftercare the write-only path has no answer for) **Highest tier:** 3 **Fix strategy:** the C-22 correction procedure (issue #15) plus pipeline-core #245's structured metadata field to retire the description-as-carrier abuse. **Resolution scope:** Partial (process, not code). @@ -275,7 +275,7 @@ Cross-refs: **C-94**, **C-96**, þing-01 `orð_dómr.md` D2, issue #249. | ID | C-94 | | Tier | 2 — the failure mode is invisible by construction and lands on the live FAO path: upload succeeds, storage is billed, the consumer's endpoint returns empty, nothing raises anywhere. ADR-013 §4.1a's *"invisible to the consumer, not merely degraded."* | | Source | `/expert-code-review` of the standing decisions, 2026-08-12 | -| Trigger | **Re-specified twice on 2026-08-13; the first attempt was not exclusive and its own worked example matched two arms.** (a) A delivery is reported empty **and an upload occurred after the bucket reached the state under investigation** — that is what the preflight below would catch, and the time bound is what the first attempt omitted. (b) `APPWRITE_READ_API_KEY` is provisioned, at which point the deferral has no remaining cost. *(A third arm — "reported empty with no upload since" — was drafted and withdrawn: it is not observable from this repository, which the amendment says four lines on, and ADR-014 §4 requires a trigger someone can notice. It is a gap, and is stated as one below rather than dressed as a trigger.)* | +| Trigger | **Re-specified twice on 2026-08-13; the first attempt was not exclusive and its own worked example matched two arms.** (a) A delivery is reported empty **and an upload occurred after the bucket reached the state under investigation** — that is what the preflight below would catch, and the time bound is what the first attempt omitted. (b) `APPWRITE_READ_API_KEY` is provisioned, at which point the deferral has no remaining cost. *(A third arm — "reported empty with no upload since" — was drafted and withdrawn: it is not observable from this repository, which the amendment says four lines on, and ADR-014 §4 requires a trigger someone can notice. It is a gap, and is stated as one below rather than dressed as a trigger.)* **Rewritten 2026-08-18, because arm (b) expired without firing** (register conventions: a trigger whose event has already occurred reads identically to a pending one). The preflight was built *without* `APPWRITE_READ_API_KEY`, so "it is provisioned" can no longer arm anything. What remains live is arm (a), now narrowed: **a delivery is reported empty, an upload occurred since, and the preflight did NOT raise** — that combination means the check is looking in the wrong place, and it is the only arm this repository can still be surprised by. | | Owner | This repository, for the mechanism. The credential is the operator's. | | Location | `views_postprocessing/contract/wire/sink.py` (the upload path, where nothing verifies); `views_postprocessing/delivery/`. | @@ -293,6 +293,21 @@ FAO emailed at **09:15 UTC** that `faoapi.viewsforecasting.org` returned no data **So the trigger was mis-specified, not the mechanism.** "A delivery is reported empty" names a symptom with at least two causes, and this entry's preflight addresses only one of them. +**Partial mitigation, 2026-08-18 — the preflight is built.** After both legs are uploaded, each manager asks the partner store the question the consumer asks — `name == product.CONSUMER_DOCUMENT_NAME`, `category ∈ {forecast, historical}` — and refuses a falsy answer (`delivery/findability.py`, `DeliveryNotFindableError`). The two legs are checked separately on purpose: 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 in run-0 (C-79). + +**Two decisions inside it that a later reader should not have to re-derive:** + +1. **It runs on the existing key, not the registry's `APPWRITE_READ_API_KEY` slot.** Verified in the Appwrite console 2026-08-18: the live `VIEWS Pipeline Core` key already carries `documents.read`, `rows.read`, `buckets.read` and `files.read`. A separate read credential would buy no isolation here, because the preflight runs *inside the delivery process*, which already holds the write key it just uploaded with. C-96's permission is about the operation being read-only, and it is. The registry slot stays `planned` for a preflight that runs **outside** the delivery, where the isolation would be real. +2. **It queries through a store with pipeline-core's automatic `name == model_name` filter suppressed** (`_build_partner_read_store`). `get_latest_file_id` delegates to `get_predictions_by_metadata`, which merges the path manager's model name into every query — so without the suppression the check would verify the views-models *directory* name, which equals the declared consumer name only by coincidence (**C-77**). Verifying the coincidence rather than the contract would leave this green while a rename took the delivery dark, which is the precise failure it exists to see. + +**The read-back is scoped to THIS run, and that is the whole guard.** The first implementation asked *"is there any document under the consumer's name for this category"* — a question the **previous** delivery already answers yes to. Documents accumulate across runs (that is what makes "latest" meaningful to the consumer), so from delivery 2 onward the check could never fail: run-2's upload 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. Caught by `/code-review high` before merge. `_ContractStorePort.upload` now returns the uploaded `file_id` (it was discarding it), the sink carries the manifest's id out — it is 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. + +**A failed read-back is not an invisible delivery.** `findability.unverified` names that separately (`FindabilityUnverifiedError`): a store error after a successful upload means the delivery is UNVERIFIED, not known invisible, and quarantining on it would be an outage the guard manufactured. Same distinction C-103 draws between a missing producer client and a producer that publishes no boundary, and C-99 between an unrecognised store result and a real one — three instances now of the same rule, that *could not ask* and *asked and got nothing* call for different operator actions. + +**A note on which pipeline-core you read, because it changed a review's conclusion.** The same review reported that `unverified()` was dead code, on the grounds that `get_predictions_by_metadata` swallows a failed search and returns `[]` — so a store error would arrive as `None` and be reported as an invisible delivery. That is true of **2.3.0**, which is what the drifted developer venv holds (C-104). It is false of **3.0.1**, which `poetry.lock` pins and CI installs: there the method **raises `MetadataSearchIncomplete`**, with a comment in pipeline-core saying why — *"Returning [] here would tell every caller 'no predictions match', which is a statement about the shelf rather than about the lookup… a false negative to an external counterparty"* (views-pipeline-core C-241, its Cluster J). Verified 2026-08-19 by reading `3.0.1` from the views-pipeline-core checkout rather than the installed package. So the split holds where it runs. **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. + +**The tier does not move, and the reason it does not is the point.** The Tier 2 rationale was *"upload succeeds, storage is billed, the consumer's endpoint returns empty, nothing raises anywhere."* For that cause, something now raises. What holds the entry at 2 is the two causes below, which this does not touch and which remain invisible — the tier now rests on the gaps rather than on the mechanism. + **Two uncovered causes, stated as gaps rather than dressed as triggers.** Neither is observable from here, so neither can be a trigger under ADR-014 §4 — a trigger nobody can notice is a wish: 1. *The bucket is empty because nothing was delivered.* This repository is not told when a delivery is due and has no view of whether the last one is still present. That is the 2026-08-12 case. @@ -685,7 +700,7 @@ It is a worked example wearing a risk's clothes. **Give it a real trigger or mov | ID | C-84 | | Tier | 2 — not silent. The delivery fails loudly and completely, which is the correct behaviour and also the whole problem: there is no degraded mode, no fallback identity, and the date is known in advance. A foreseeable total outage that nobody has scheduled work against is a structural risk, not an operational surprise. | | Source | views-appwrite coordinate registry v1.4.3/v1.4.4 — operator console read, 2026-08-05 (þing-02 A3(i)) | -| Trigger | **A date, unusually — 2026-11-17.** The registry records `VIEWS Pipeline Core` expiring 12:35 and `UN FAO` 16:10 that afternoon. Act when the un_fao delivery is next scheduled within a month of it, or when anyone plans a rotation, whichever is first. | +| Trigger | **A date, unusually — 2026-11-17.** The registry records `VIEWS Pipeline Core` expiring 12:35 and `UN FAO` 16:10 that afternoon. Act when the un_fao delivery is next scheduled within a month of it, or when anyone plans a rotation, whichever is first. **Since 2026-08-19 the first arm fires by itself**: `tests/test_credential_expiry.py` goes red from 2026-10-18, so the date no longer depends on anyone remembering it. | | Owner | Simon, and only Simon — issuing and installing keys is a console action. This entry exists so the date is visible from *this* repo's planning surface rather than only from the platform's. | | Location | `views_postprocessing/{unfao,crafd}/appwrite_env.py` — the declared coordinates; the values live in the environment and the registry, never here. | @@ -697,6 +712,12 @@ The FAO delivery authenticates with the `UN FAO` key. That key expires **2026-11 **Deliberately not fixed here, and the reason is C-84's own shape.** A preflight that checks key validity means an authenticated call at startup, and the only project to make it against is production — which **þing-01 D2** forbids for tests and this would not quite be — and which that verdict explicitly permits as *read-only preflight validation*, so the obstacle here is the authenticated call, not the prohibition (see C-95). 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. Registered so the date is not discovered by an outage. +**The trigger now fires on its own (2026-08-19), and this is not the mechanism above.** The first arm of the trigger — *"act when the un_fao delivery is next scheduled within a month of it"* — was a trigger nobody could notice: it fired in someone's memory or not at all, which is the same defect that withdrew the third arm of C-94's trigger and which ADR-014 §4 exists to forbid. `tests/test_credential_expiry.py` declares the two expiries and fails from 30 days out, naming the dates, the 3h35m gap, who owns the rotation (operator; views-appwrite#12, key split views-faoapi#338), and the three ways to make it pass — rotate and update the constant, update the constant if a key was replaced early, or set `ACKNOWLEDGED_UNTIL`. **The third is the only one available to someone without console access**, which is most people who will meet this gate; omitting it here would reproduce the merge-queue-hostage outcome the acknowledgement exists to prevent. + +**It is emphatically not the key-validity preflight this entry rejected.** No authenticated call, no credential, no network — a calendar and two declared datetimes. The rejection above stands and is unaffected: what was wrong was building a mechanism to *discover* a fact already known; what was missing was making the known fact impossible to forget. **The acknowledgement is the load-bearing part, and the first draft did not have it.** `/code-review high` found two design faults that would each have ended with the test deleted. (a) A literal pin on the two datetimes made the tripwire's own prescribed remediation — *rotate, then update `KEY_EXPIRY`* — fail a second test whose message said not to adjust the constant. A guard that refuses its own documented fix is worse than no guard, and it would have landed on the one person who could not route around it. The pin is gone. (b) From 2026-10-18 the gate would have been red for **every unrelated pull request**, clearable only by an operator console action the repository cannot perform — which is precisely what `pyproject.toml` 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*, which **cannot be set on or after the expiry** — so it postpones attention and can never replace it. + +Four companion tests keep it honest rather than decorative: the firing branch is exercised against a probe **derived from** `KEY_EXPIRY` (**C-102** — a guard that has never run is unproven; deriving it rather than hardcoding means the proof survives a rotation instead of quietly expiring with it); an acknowledgement past the expiry is refused; `LEAD_DAYS` is floored, because shaving a week off the warning neuters the guard while leaving it looking present; and the outage-day text is checked, since the first draft would have told an operator the keys expired *"in -3 days"* while the seam was down. All verified by mutation. + Cross-refs: **C-81** (the same operator session's other half — branch protection and the CI token), **C-27** (no rotation mechanism for a secret value upstream), **C-57** (the pinned-registry detector, which is how this arrived here at all — it demanded the v1.4.4 bump and the bump is what surfaced the expiry), þing-02 A3(i), views-appwrite C-65 and C-66. --- @@ -809,7 +830,7 @@ Cross-referenced there to their **#248 / #347** (the same defect class on the Ap | Tier | 2 — the exclusion manifest and cell-count contract are pinned in code and were exercised live at global scale in run-0; residual is upstream-regression risk, not an unguarded silent-corruption path | | Source | `expert-code-review` (2026-06-12), verified by direct data inspection; **merged with C-34** (`expert-code-review` 2026-06-12) during review-rr 2026-07-31 | | Trigger | When a region's expected cell count or exclusion manifest changes upstream — a views-datafactory region redefinition (`regions.py`, the bundled `*_pgids.json`), a new GAUL curation like ADR-043, or a region-string change in views-models `config_queryset.py` — verify `EXPECTED_CELLS_BY_REGION` and `EXCLUDED_GIDS_BY_REGION` are re-derived from the live producer rather than trusted as frozen | -| Location | `views_postprocessing/delivery/coverage.py:56` (`land_gaul: 64_742`), `:92` (`EXCLUDED_GIDS_BY_REGION`), `:99`; `views_postprocessing/unfao/managers/unfao.py:397` (`_check_coverage`), `:300` (`_validate`); views-models `postprocessors/un_fao/configs/config_queryset.py`; views-datafactory `src/datafactory_query/regions.py` | +| Location | `views_postprocessing/delivery/coverage.py:56` (`land_gaul: 64_742`), `:92` (`EXCLUDED_GIDS_BY_REGION`), `:99`; `views_postprocessing/unfao/managers/unfao.py::_check_coverage`, `:300` (`_validate`); views-models `postprocessors/un_fao/configs/config_queryset.py`; views-datafactory `src/datafactory_query/regions.py` | Verified 2026-06-12: of the datafactory's 64,818 `land`-region cells, 64,736 have complete area-majority metadata; exactly 82 are unassigned across all 7 GAUL fields — all remote sub-Antarctic islands FAO's GAUL 2024 boundaries do not cover (Macquarie, Auckland Islands, Prince Edward; sample gids 51078, 51798, 53979, 62356, 94776, 99027). The mitigation must be a named exclusion-list constant with the gids, count-asserted in both the enricher and a test, logged at WARNING, and disclosed to FAO — not a generic `code != -1` filter, which would silently absorb future coverage regressions. Generalizes the previously documented "5 ocean cells" of africa_me_legacy (those 5 are among the excluded set). @@ -817,6 +838,10 @@ Verified 2026-06-12: of the datafactory's 64,818 `land`-region cells, 64,736 hav **Mitigation landed (S4, 2026-06-26, `sprint/fao-input-integrity`):** the 76 excluded gids are pinned as a frozen manifest in `delivery/coverage.py` (`EXCLUDED_GIDS_BY_REGION`), the count is corrected to 64,742, `assert_no_excluded_cells` is wired into the manager's `_check_coverage` **region-gated** (a no-op for unpinned `africa_me_legacy`, so its 5 ocean cells are unaffected), the 76 are disclosed in `docs/fao_excluded_cells.md`, and a test cross-checks the manifest against the datafactory sibling when present (drift tripwire). **Residual:** still Tier 1 until the live `land_gaul` run (views-platform/views-models#127) exercises it end-to-end — the guard is unit-proven but not yet run against a real global delivery. +**The tripwire now runs in the gate, and it did not until 2026-08-17.** *"When present"* meant a developer laptop: views-datafactory was not fetched in CI, so `test_manifest_matches_datafactory_land_minus_land_gaul` skipped on every pull request. That mattered more than it looked, because on 2026-08-17 this repository told FAO in writing that the exclusion list *"is frozen in code and asserted against the producer in our test suite, so it cannot drift without failing loudly"* — a guarantee the gate was not carrying. The sibling is now fetched (see C-46 for why the earlier attempt was reverted and why that reason did not survive checking), and the tripwire reads `src/datafactory_query/{land,land_gaul}_pgids.json`, both of which views-datafactory tracks. + +This does **not** move the tier. The residual above is unchanged: the guard is now enforced continuously rather than incidentally, but what holds C-30 at Tier 1 is the absence of a real global delivery exercising it end-to-end, and no CI wiring supplies that. + **RESIDUAL DISCHARGED 2026-07-27 — run-0 exercised the guard live.** The stated residual was "*still Tier 1 until the live `land_gaul` run (views-models#127) exercises it end-to-end — the guard is unit-proven but not yet run against a real global delivery.*" **Run-0 delivered on 2026-07-27** against producer run `rusty_bucket_forecasting_20260727_095355`: `region=land_gaul`, coverage gate reported **64,742 distinct cells / 28,356,996 rows** for the historical frame, the forecast leg shipped 108 shards + sidecar + manifest, and the process exited cleanly with no loud failures. The pinned count and the 76-gid exclusion manifest were both correct against a real global delivery. **Tier recalibrated from 1 to 2 during review-rr (2026-07-31):** the silent-corruption path is now guarded and proven, so what remains is regression risk under upstream change — which is exactly what the rewritten trigger watches. **MERGED: C-34 (Spatial coverage has no contract) absorbed here, review-rr 2026-07-31.** C-34 registered the absence of any expected-cell-count assertion, with the coverage decision split across three repos (views-models region string → views-datafactory cell-set → consequences here). Both concerns are now implemented by **one module** (`delivery/coverage.py`) and were discharged by **one event** (run-0), so tracking them separately doubled the maintenance without adding signal. C-34's distinctive contribution — that the trigger is an *upstream* region/cell-set change in either of two other repos — is carried into the merged trigger and Location above. C-34 remains as a forwarding stub in Resolved Concerns. @@ -1055,6 +1080,171 @@ Recorded rather than left implicit because "the operator did a console session" --- +### C-103: The clip that keeps fabricated months off the wire depends on a package this repo does not declare, and its absence is swallowed + +| Field | Value | +|-------|-------| +| ID | C-103 | +| Tier | **3** — re-tiered down from 2 on 2026-08-17, when the verification question below was answered and the premise did not hold. The residual is real but narrower: not "the dependency is missing", which a different guard already refuses, but "any failure to read the boundary degrades open". | +| Source | `/repo-assimilation` (2026-08-16), measured | +| Trigger | When a delivery logs *"last_valid_month_id could not be read; skipping the observed-range clip"* — that is now the only route to an unclipped delivery, and it is a real one (a network failure or a reshaped `.zattrs` reaches it). Decide then whether degrade-open is still the right side for that case, or whether the partner should be told the tail is unverified. | +| Owner | This repository, for the swallow and the declaration. The producer owns the fact itself. | +| Location | `views_postprocessing/contract/source_metadata.py::last_valid_month_id` (the classification); `views_postprocessing/{unfao,crafd}/managers/*.py::_read_historical_frame` (the two branches). **Not `pyproject.toml`** — the original entry listed it as a risk site on the assumption the dependency was undeclared everywhere; it is the launcher's to declare and both launchers do, so there is nothing to add here. **Cited by symbol, not by line (2026-08-21).** The line numbers went stale twice in four days — both times because a later change in the same branch moved them, and the second time the entry carried an explicit *"re-read"* claim that was false by the time it merged. A citation that decays faster than the review cycle is worse than a vaguer one that does not. | + +`source_metadata.last_valid_month_id` lazily imports `datafactory_query.defaults`. Measured 2026-08-16: that package is in neither `pyproject.toml` nor `poetry.lock`, and `import datafactory_query` raises `ModuleNotFoundError` in the project venv. Its only caller wraps the call in `except Exception: lv = None` and then returns the historical frame **unclipped**, logging one WARNING — so "the dependency is missing" and "the producer publishes no boundary attribute" leave through the same branch with the same outcome, and that outcome is unobserved zero-padded months shipping to the partner as observed history. The lazy import states its own reason — *"so this module loads without the heavy datafactory dependency present (e.g. in unit-test environments)"* — but nothing at the call site distinguishes a unit-test environment from a delivery. + +**ANSWERED 2026-08-17, and the answer moves the tier.** The question was whether the production launcher supplies `datafactory_query`. It does, twice over: + +- views-models `postprocessors/un_fao/requirements.txt` and `postprocessors/un_crafd/requirements.txt` both pin **`views-datafactory>=1.9.0,<2.0.0`**, which is what ships the `datafactory_query` module (there is no separate distribution — `pip download datafactory-query` finds nothing; it is one of nine packages in views-datafactory's wheel). +- More decisively, the postprocessor's `config_queryset.py` imports `datafactory_query.defaults` **at module scope** and raises a `RuntimeError` naming the fix if it is absent. A missing client therefore makes `get_queryset()` return `None`, which `launch_config.assert_queryset_was_importable` turns into a refusal (**C-83**) *before* `_read_historical_frame` is ever reached. + +So the scenario this entry was filed on — a missing dependency silently shipping fabricated months on the live path — **cannot occur**. It was already guarded, by a check written for a different reason. The Tier 2 rested on a premise measured only in *this* repository's venv, and the instruction it borrowed from C-26 (*"do not downgrade on inspection of this repo alone"*) was the right instruction: the resolution came from reading the launcher, not from reading here. + +**What is genuinely left, and it is why this stays open at Tier 3.** The `except Exception` was never only about the import. A network failure, an auth error, a reshaped `.zattrs`, a timeout — all still leave through one branch, log one WARNING, and deliver the unobserved tail as observed history. That is the recorded C-26 degrade-open decision applied far more broadly than C-26 argued for. + +**Partial mitigation, 2026-08-17.** `source_metadata` now raises `ProducerClientUnavailable` instead of letting the import failure fall into the caller's broad `except`, and both managers re-raise it rather than degrading. Defence in depth for the paths `assert_queryset_was_importable` does not cover — a direct caller, a future launcher, a partner not going through the same queryset. The module also gained its first tests (`tests/test_source_metadata.py`, 7, mutation-proven against both the pre-fix return-`None` and a manager that collapses the two branches back into one); it had **none** before, which is how "return None like everything else" ever looked reasonable. The broad degrade-open is deliberately unchanged — narrowing it is a decision about what to tell the partner, not a refactor. + +**C-60 is this shape, and it was resolved by deleting the degradation.** There, a provenance stamp reached into the producer's ledger schema inside a bare `except … pass` and returned `"unknown"`; the fix was to raise. The difference here is that the degradation is deliberate and documented ("degrade-open, C-26") — which makes the question *whether the open side is still the right one*, not whether someone forgot. + +Cross-refs: **C-26** (the fabrication this clip exists to prevent), **C-07** (undeclared runtime dependencies, the same class, resolved), **C-60** (bare-except degradation, resolved by raising), **C-27** (a swallowed failure surfacing far from its cause), **D-07** (the decision that data facts come from the producer, which created this import). + +--- + +### C-104: A stale virtualenv turns 25 tests red, and 20 of them are the only tests that import either manager + +| Field | Value | +|-------|-------| +| ID | C-104 | +| Tier | 3 — no production impact. The cost is that a red suite stops carrying signal, on precisely the two modules with the thinnest coverage. | +| Source | `/repo-assimilation` (2026-08-16), measured | +| Trigger | When `pytest` reports failures in `tests/test_framework_contract.py` or `tests/test_store_construction.py`, check `pip show views-pipeline-core` against `poetry.lock` before reading them as defects. | +| Owner | This repository. | +| Location | `tests/test_framework_contract.py`, `tests/test_store_construction.py` (20 failures); `tests/test_wire_shard.py`, `tests/test_wire_sidecar.py`, `tests/test_hop_b_sink_e2e.py` (5 failures); `poetry.lock` versus the project venv | + +Measured 2026-08-16 in the project venv: 458 collected, **433 passed, 25 failed**, 39 xfailed, in 14.85s. The venv holds `views-pipeline-core 2.3.0` and `pyarrow 23.0.1`; `poetry.lock` pins **3.0.1** and **16.1.0**. The pyarrow half is known and predicted: 5 byte-parity failures reporting *"pinned toolchain violated: byte-parity oracle requires pyarrow 16.1.0, found 23.0.1"*, exactly what `tests/fixtures/wire_contract/README.md` says will happen under **C-72**. The pipeline-core half is documented nowhere: `ModuleNotFoundError: No module named 'views_pipeline_core.modules.dataloaders.datafactory_contract'`, raised at import of both managers, which takes out every test that constructs or inspects one. + +The consequence is that in a drifted checkout the two largest modules in the package — 387 lines each, 21% of the source — are not merely under-covered but **entirely unexercised**, and the suite reports that in a form indistinguishable from a real break. CI runs `poetry install` and gets the locked versions, so this is a local condition rather than a CI one — **C-81**'s asymmetry running in the other direction, with the laptop the weaker seat rather than the stronger. **C-36** is the precedent for what a suite that is red for a known reason costs: it stops being read. + +**Partial mitigation, 2026-08-17.** `tests/test_locked_environment.py` compares the installed versions of the runtime dependencies **declared in `pyproject.toml`** (read from there, not hardcoded) against `poetry.lock`, and fails with one message naming each drifted package, both versions, the command to run, and — the part that matters — that the other failures in the run are consequences rather than defects. Verified against the live drift: it reports `views-pipeline-core installed=2.3.0 locked=3.0.1` and `pyarrow installed=23.0.1 locked=16.1.0`. A second check owns the other direction — declared in `pyproject.toml` but absent from the lock — and the version check *skips* names it cannot find rather than reporting them as `locked=None`, so a stale lock is diagnosed once, correctly, instead of twice with one of the two sending the reader at their virtualenv. + +Three things it deliberately does not treat as drift: dependencies gated by `optional`, `python` or `markers` (legitimately absent from a given environment — reporting one as "run `poetry install`" would be advice that cannot work), name spellings that differ only by case or separator (both sides are PEP 503-normalized, so `PyYAML` and `views_frames` match their lock entries), and dev-group tools. The failure text names only the packages that actually drifted: the 2026-08-16 incident was pipeline-core and pyarrow, a future one will not be, and a diagnosis describing the wrong packages is the failure this file exists to remove. + +It does **not** fix the drift and does not skip. The 25 failures remain until someone runs `poetry install`; what changes is that a contributor can now tell in one line which kind of problem they have. That is the whole of the entry's cost — the failures were never wrong, they were unreadable — so the entry stays open only until the environment is actually reconciled, which is a machine action rather than engineering work. + +Dev-group tools are deliberately out of scope: `ruff`'s reported version varies with how it was installed, and the thing that actually broke CI on 2026-08-03 was its *rule set*, which `pyproject.toml` already pins explicitly. + +Cross-refs: **C-72** (owns the pyarrow half — that half is not re-registered here), **C-81** (CI-versus-local coverage asymmetry), **C-36** (a permanently-red suite cannot detect new regressions), **C-102** (the same argument in the other direction: a guard that never runs proves nothing, and a failure nobody can read is not a signal). + +--- + +### C-105: A run is uploaded file-by-file with no rollback and no idempotency — a mid-run failure leaves orphans and the retry adds more + +| Field | Value | +|-------|-------| +| ID | C-105 | +| Tier | 3 — no delivered value is corrupted. The store accumulates unreferenced objects that no artifact describes, in a bucket with no named retention owner. | +| Source | `/repo-assimilation` (2026-08-16) | +| Trigger | When the upload interlock is first opened for a live run (`wire_upload_enabled: True`), or when the retention owner D-12 defers is named — whichever comes first — decide what a torn attempt leaves behind and who removes it. | +| Owner | This repository for the mechanism; the operator for retention. | +| Location | `views_postprocessing/contract/wire/sink.py::deliver_run` (the upload phase) and `::_torn_run_error` | + +`deliver_run` uploads every shard, then the sidecar, then the run manifest, each through `_ContractStorePort.upload`, which raises on anything but explicit success (**C-79**). A raise at shard *k* of *n* is therefore correct in the one dimension the contract governs — no manifest means the run is invisible to the consumer, which is the §4.2 commit-marker design working — and silent in every other: the *k* uploaded objects remain, nothing records that they exist, and nothing removes them. Re-running the delivery re-uploads all *n* under the same names, and whether that supersedes or duplicates is a store semantic this repository asserts nowhere. At run-0 scale that is roughly 110 objects per attempt. + +`docs/operations/correction_procedure.md` covers the *wrong value* case — the contract has no retraction primitive, so a correction is a new complete run, manifest last. A torn attempt is a different case and is not covered by it. + +**Partial mitigation, 2026-08-19 — the tear is now documented, not removed.** `deliver_run` keeps an in-memory ledger of what it has uploaded, and a failure anywhere in the upload phase raises `TornRunError` naming the run, how many of how many objects were *confirmed* uploaded, and what is true about the consumer. Tested in `tests/test_torn_run.py` (8), mutation-proven three ways. + +**Three corrections `/code-review high` made to the first draft, each of which would have sent an operator the wrong way.** (a) The object that FAILED was omitted, and it is the likeliest orphan of the whole run: `_ContractStorePort.upload` raises precisely when the store returns failure *with the file already uploaded* (the C-79 shape), so the refusal now names it as a separate thing to go and look for. (b) File ids were truncated to five in the message and recorded nowhere else, so at run-0 scale ~104 ids existed only in a string nobody kept — the log ledger now carries `file_id` per upload and is the persistent record. (c) A failure on the *first* upload printed an empty list and a dangling period while telling the operator to audit a bucket. The message also no longer asserts categorically that the consumer cannot see the run: a manifest upload can fail after the store committed the document, and steering a re-run on a false certainty duplicates every object. + +The refusal says three things an operator otherwise has to establish by hand: the consumer **cannot see this run** (the manifest is the commit marker and never landed, so nothing partial is being served — §4.2 working as designed); the objects listed are **still there and were NOT removed**; and a re-run will upload all of them again under the same names, with supersede-or-duplicate being a store semantic this repository does not assert. + +**Remaining scope, found by `/review-diff` on the fix itself: the historical leg is not covered.** `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 is different rather than smaller: the manifest already landed, so the consumer sees a **complete, visible forecast run** sitting 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, and the wrapper's message would be wrong if it fired there. It does not fire there. Left uncovered deliberately rather than widening this change; the manager is at 434/450 of its line budget and the fix belongs with whoever takes the deletion decision below. + +**What is deliberately NOT done: deletion.** 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). So this entry stays open: the mess is now legible, and it is still a mess. Closing it needs a decision about who cleans up and whether the store supersedes — neither of which is engineering work here. + +Cross-refs: **C-94** (nothing observes the outcome of an upload at the time it happens), **C-79** (the single-file orphan this generalises), **C-58** (the delete surface deletion would have to go through), **D-12** (the unnamed retention owner this compounds with). Part of causal cluster: **Cluster J — Delivery aftercare has no mechanism**. + +--- + +### C-106: The §2 header builder — the module that owns the contract version — is reachable only from tests + +| Field | Value | +|-------|-------| +| ID | C-106 | +| Tier | 4 — unreached, not wrong. Registered because the identical shape has been closed four times here by deletion, and because this instance sits in a module whose *other* export is live on every delivery. | +| Source | `/repo-assimilation` (2026-08-16), measured | +| Trigger | When someone proposes changing `CONTRACT_VERSION`, or when this repository first acts as a Hop-A *producer* rather than only a consumer — at that point `build_header` acquires the caller it was written for and this entry is discharged. | +| Owner | This repository. | +| Location | `views_postprocessing/contract/wire/header.py::build_header`; `views_postprocessing/contract/gaul_schema.py::colrow` | + +Measured: `build_header` is called from `tests/test_wire_header.py` and `tests/test_wire_shard.py`, and nowhere else. On the delivery path the sink re-embeds the producer's Hop-A header untouched (`contract/wire/sink.py:111-113`, §10.2 *"the sink mints nothing"*), so the builder never runs in a delivery. The module is half-reached rather than dead: `CONTRACT_VERSION = "1.5"` is imported by `contract/wire/run_manifest.py:19` and written into every run manifest, so deletion is not the question — what `build_header` is *for* is. Separately, `gaul_schema.colrow` has zero callers anywhere, tests and build scripts included. Neither is a defect; both are surface a reader must make a decision about, and neither currently has one recorded. + +Cross-refs: **C-100** (the live sibling — a four-method port with three used methods), **C-64**, **C-75**, **C-45** (three prior instances of unreached declared surface, all resolved by deleting). + +--- + +### C-107: The doc-accuracy scan reads markdown only, so a docstring pointing at a moved file rots unwatched — two already have + +| Field | Value | +|-------|-------| +| ID | C-107 | +| Tier | 4 — navigational, with no correctness or reliability impact. Registered because this repository's stated discipline is that a docstring points at the one home of a fact, which makes a broken pointer a failure of the discipline rather than a typo. | +| Source | `/repo-assimilation` (2026-08-16), measured | +| Trigger | When the next module moves under `views_postprocessing/`, check its inbound docstring references as well as its markdown ones — or when someone proposes widening `tests/test_doc_accuracy.py`'s corpus, at which point C-97's objection applies and this entry states what the gap actually is. | +| Owner | This repository. | +| Location | `tests/test_doc_accuracy.py:76-78,133-134` (the scanned corpus); `views_postprocessing/delivery/coverage.py:5`; `views_postprocessing/delivery/observed_range.py:6` | + +`test_doc_accuracy` scans `README.md`, `docs/architecture/*.md`, package `README.md` files, ADRs and CICs. Python docstrings are outside that corpus. Two are already stale, and both point at files that moved in exactly the refactors whose *markdown* fallout the same test was extended to catch: `delivery/coverage.py` sends the reader to `views_postprocessing/unfao/extraction.py`, deleted in #151, and `delivery/observed_range.py` to `views_postprocessing/unfao/source_metadata.py`, moved to `contract/` in #153. + +**This is not a proposal to scan docstrings.** C-97 measured what that costs for the no-copy scan and argued it down under ADR-014 §3 — a guard that fires on ordinary prose gets deleted, after which the real rule is unguarded. The two scans are not the same (a deleted symbol or a repo-relative module path is a far narrower pattern than a store name in a sentence), so the objection is not decisive here — but it is the reason this is registered as a measured gap rather than fixed on sight. + +Cross-refs: **C-80** (the same guard, the adjacent corpus gap, resolved by widening), **C-97** (why widening a scan into docstrings is not automatic). + +--- + +### C-108: Two `xfail(strict=True)` deploy gates have never evaluated their own assertions — anywhere + +| Field | Value | +|-------|-------| +| ID | C-108 | +| Tier | 4 — no delivery correctness depends on them. Registered because `xfail(strict=True)` *reads* as an armed tripwire, and a future maintainer will believe views-datafactory#223 is being watched when nothing is watching it. | +| Source | `/code-review high` on PR #280, 2026-08-17 (finding 2), extended by measurement | +| Trigger | When views-datafactory#223 is closed, or when anyone cites these gates as evidence that the served artifact is being tracked — check they are not skipping first. | +| Owner | This repository for the gate; views-datafactory for the artifacts. | +| Location | `tests/test_datafactory_deploy_readiness.py` — `TestServedArtifactMatchesBranch::test_assembled_grid_not_older_than_gaul_parquets`, `TestServedArtifactProvenanceTracksGaul::test_provenance_includes_admin_digest` | + +Both gates read `data/assembled/grid.npy`, `data/assembled/provenance.json` and the GAUL parquets from the views-datafactory checkout. **None of those is tracked upstream, and `data/assembled/` is empty in the maintainer's own checkout** (measured 2026-08-17). So the tests were failing on a missing file, `xfail(strict=True)` was recording that as an expected failure, and the report read green. The staleness comparison and the `admin_digest` assertion — the things the gates exist to make — have never once been evaluated. + +The strict flip is the entire mechanism: when views-datafactory#223 is fixed the test should XPASS and turn the build red, forcing someone to look. A test that can only ever fail on `FileNotFoundError` can never XPASS, so the flip could not fire. ADR-014 §1 — a guarantee is attached to a check, or it is not a guarantee — and C-102's lesson recurring in a form that is harder to see, because here the guard *runs*. + +**Partially addressed in the same PR**, and deliberately only partially: both tests now `pytest.skip()` when their inputs are absent, so the state is visible in the report instead of disguised as a passing xfail. That converts a false green into an honest skip. It does **not** make the gate work — closing that needs the assembled artifacts reachable from CI, which is the same blocker as C-46's producer-comparison half and is not this repository's to solve. + +Cross-refs: **C-46** (the untracked-artifact blocker these share), **C-102** (a guard that has never run is unproven), **C-36** (the gates' original home). + + +--- + +### C-109: Register `Location` line numbers decay faster than the review cycle, and nothing checks them + +| Field | Value | +|-------|-------| +| ID | C-109 | +| Tier | 4 — no correctness impact. Registered because `Location` is the field a reader trusts to find the thing an entry describes, and a wrong one sends them to unrelated code with no signal that it is wrong. | +| Source | `/code-review max` on the release branch, 2026-08-21, then measured across the open set | +| Trigger | When an entry's `Location` is used to find code and the code is not there — or when anyone proposes a guard over the register's citations, at which point this entry says what such a guard would have to check and why the obvious version does not work. | +| Owner | This repository. | +| Location | `reports/technical_risk_register.md` — the `Location` field of every open concern that cites a line. | + +**Measured 2026-08-21.** Eleven of the 28 open entries cited a `file.py:line` in `Location`; converting four leaves **eight of 29**. Spot-checking six of the original eleven against the working tree, **three were already stale**: C-105's `sink.py:167-171` (written four days earlier) landed on `staging.mkdir`, C-106's `gaul_schema.py:87` on a section comment, and C-30's `unfao.py:397` on `return summary`. All three drifted because a *later change in the same week* moved the lines — nothing about the entries themselves changed. + +C-103 is the sharp case, and the reason this is a class rather than three typos: its `Location` went stale **twice in four days**, both times from a subsequent commit in the same branch, and the second time the entry carried an explicit *"line numbers re-read"* claim that was already false when it merged. A citation that decays faster than the review cycle is worse than a vaguer one that does not, because it is confidently wrong. + +**Converted rather than corrected, where the target is a function.** C-103, C-105, C-106 and C-30 now cite `path::symbol`. A symbol survives edits above it, which is the entire failure mode here. Line numbers remain where the target genuinely is a line — a specific literal, a table row — and those are the ones any future guard would have to cover. + +**Why the obvious guard does not work, stated so it is not proposed again cheaply.** Checking that a file has at least that many lines catches nothing: every stale citation above points at a real line. Checking *content* requires the entry to declare what it expects to find there, which is a second declaration that can itself go stale — the shape ADR-014 §2 warns about. The cheap and durable move is the convention (`::symbol`), not a test. + +Cross-refs: **C-103** (twice stale in four days — the case that made this visible), **C-107** (docstrings outside the doc-accuracy scan; the same "nothing checks the prose" family), **C-82** (governance prose carrying numbers nothing checks, resolved). + ## Disagreements ### D-12: Post-Run-0 infrastructure & naming intents — repo rename, internal-store transport, compute co-location @@ -2055,6 +2245,23 @@ Verified 2026-08-02: `grep -rn "/home/" tests/ scripts/ views_postprocessing/ -- **What is genuinely accepted, and should not be glossed:** a change merged to a sibling's `main` — a registry edition bump, say — *can* turn this repository red and block merges here until someone re-pins. That is not a defect being tolerated; it is the drift detector working, and the alternative is the state this entry was open about, where the drift was noticed only when a maintainer happened to run the suite. The cost is real and the trade is deliberate. +**The last residual closed 2026-08-17, and the reason it stayed open for two weeks is the finding.** ADR-016 added sibling checkouts but fetched only views-appwrite, so this entry's own subject — the deploy gate — still ran nowhere but a laptop. The stated blocker was that views-datafactory's GAUL parquets are untracked, so a checkout would turn honest skips into `FileNotFoundError`; that was tried on 2026-08-03 and reverted. **The observation was right and the diagnosis was wrong.** `test_gaul_lookup_fidelity` gated on `data/raw/gaul_admin/` being a *directory*, and that directory **is** tracked — it holds `supplement_azores.geojson` — while the seven parquets beside it are not. So a checkout satisfied the gate, the comparison ran, and it died. The sibling was never the problem; the gate was asking whether a folder existed when it needed to ask whether the files it reads existed. + +Reproduced 2026-08-17 against a tracked-files-only worktree (`git worktree add --detach`, which contains exactly what `actions/checkout` produces): `1 failed, 37 passed, 1 skipped`, the failure being `FileNotFoundError: .../gaul0_code.parquet`. After re-gating on the seven parquets themselves: no failures. views-datafactory is now fetched in `run_pytest.yml` and declared `ci_checkout=True`. + +**A second, larger instance of the same mistake was found while fixing the first.** All four sibling-aware tests in `test_gaul_lookup_fidelity.py` shared **one** gate keyed to the parquets — including two that read no parquet and one that reads no sibling at all. So they sat dark in CI for no reason anybody had chosen: + +| test | actually reads | was gated on | +|---|---|---| +| `test_lookup_values_match_the_producer_parquets` | the 7 GAUL parquets (untracked) | parquets — correct | +| `test_lookup_gid_set_equals_the_declared_region` | `land_gaul_pgids.json` — **tracked** | parquets | +| `test_coordinate_formula_matches_every_priogrid_cell` | `priogrid_cell.dbf`, and self-skips on it | parquets | +| `test_coord_dtypes_are_wire_stable` | **only the committed lookup** | parquets | + +The last one matters beyond tidiness: it is the check that `CODE_COLS` survive the §5.1 int64→float64 wire cast losslessly (`abs(v) < 2**53`) — the property ADR-013 §5.1a and the 2026-08-17 mail to FAO both rest on — and it needs no sibling whatsoever. Each test now gates on the artifact it reads. + +Measured across the whole change, CI goes from **426 passed / 6 skipped** to **430 passed / 3 skipped**: four checks move from skipped to running — C-30's exclusion tripwire, this entry's `TestReleaseGate::test_land_gaul_commit_is_in_a_release_tag`, the region-set check, and the wire-cast dtype check. The producer-comparison half still skips, honestly, and still needs the parquets published somewhere fetchable. + The cross-repo deploy-readiness gates introduced under C-36 are guarded by `skipif` on a **hardcoded local datafactory checkout path**, so they are **skipped in CI** and only ever execute on one developer's machine. There, `test_version_bumped_past_latest_tag` is currently **failing**: it is an `xfail(strict)` that flipped to XPASS because datafactory moved to `1.5.0`-dev past its `v1.4.0` tag — exactly the auto-flip C-36's resolution anticipated, but because of the hardcoded path the flip surfaces as a **local red** rather than a CI signal, and breaks local `pytest` runs (the suite is run with this test deselected). No correctness/reliability impact on the delivery → **Tier 4** (test hygiene). C-36 (resolved) converted these gates to strict-xfail but did not capture the local-path / CI-skip dimension. See also C-36 (the resolved strict-xfail conversion this extends), C-44 (the datafactory version-state coupling). diff --git a/tests/conftest.py b/tests/conftest.py index 8fdde29..65cf8b1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -122,13 +122,24 @@ class Sibling: SIBLINGS = { "views-datafactory": Sibling( env="VIEWS_DATAFACTORY", - ci_checkout=False, + ci_checkout=True, note=( - "PUBLIC, but its checks need the producer's raw GAUL parquets " - "(data/raw/gaul_admin/*.parquet), which are NOT in its git repository. " - "Checking it out converts an honest skip into a FileNotFoundError — measured " - "2026-08-03, tried and reverted. Closing this needs the data published " - "somewhere fetchable, not an access grant. Register C-46." + "PUBLIC, and fetched since 2026-08-17. The 2026-08-03 revert was real but " + "its cause was misread: `test_gaul_lookup_fidelity` gated on " + "`data/raw/gaul_admin/` being a DIRECTORY, and that directory IS tracked " + "(it holds supplement_azores.geojson) while the parquets beside it are not. " + "So a checkout satisfied the gate, the comparison ran, and it died on " + "FileNotFoundError — which read as 'this sibling cannot be checked out' when " + "it was 'that gate asks the wrong question'. Reproduced against a " + "tracked-files-only worktree on 2026-08-17, then fixed by gating on the " + "seven parquets themselves. " + "What the fetch buys: `test_delivery_coverage.py::" + "test_manifest_matches_datafactory_land_minus_land_gaul` reads " + "`src/datafactory_query/{land,land_gaul}_pgids.json`, both of which ARE " + "tracked, so C-30's exclusion-manifest drift tripwire now runs in the gate " + "rather than only on a laptop. The value-fidelity half still skips, honestly, " + "and closing THAT still needs the parquets published somewhere fetchable " + "rather than an access grant. Register C-46, C-30." ), ), "views-appwrite": Sibling( diff --git a/tests/test_clone_readiness.py b/tests/test_clone_readiness.py index 79413fe..54784d2 100644 --- a/tests/test_clone_readiness.py +++ b/tests/test_clone_readiness.py @@ -49,6 +49,7 @@ "views_postprocessing.delivery.coverage", "views_postprocessing.delivery.draws", "views_postprocessing.delivery.parity", + "views_postprocessing.delivery.findability", "views_postprocessing.delivery.observed_range", "views_postprocessing.delivery.provenance", "views_postprocessing.contract.wire.sink", diff --git a/tests/test_credential_expiry.py b/tests/test_credential_expiry.py new file mode 100644 index 0000000..36f9392 --- /dev/null +++ b/tests/test_credential_expiry.py @@ -0,0 +1,182 @@ +"""A dated tripwire for the platform key expiry (register C-84, issue #224). + +**What this is not.** C-84 considered a key-validity preflight and rejected it: *"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 stands. There is no authenticated call here, +no credential, no network — only a calendar. + +**What this is.** C-84's trigger reads *"act when the un_fao delivery is next scheduled +within a month of it"*. That is a trigger nobody can notice: it fires in someone's memory +or not at all, which ADR-014 §4 forbids and which withdrew the third arm of C-94's +trigger. The date is known 90 days ahead and the consequence is a total outage of every +identity on the seam, so the trigger is made to fire on its own. + +**The acknowledgement is the load-bearing part, and the first draft did not have it.** +A gate that goes red on a date, cannot be cleared from inside the repository, and blocks +every unrelated pull request is a gate that gets deleted — `pyproject.toml` says exactly +that about ruff, citing ADR-014 §3. Rotation is an operator console action this repo +cannot perform, so without an in-repo escape the tripwire would hold the merge queue +hostage from 2026-10-18 until someone with console access acted. ``ACKNOWLEDGED_UNTIL`` +is that escape: a declared date that says *we have seen this and will act by then*. It is +a deliberate, reviewed, dated edit — and it **cannot be set past the expiry**, so it can +postpone attention but never replace it. + +**There is deliberately no test pinning these datetimes to literals.** The first draft had +one, and it made the remediation the tripwire itself prescribes — *rotate, then update +``KEY_EXPIRY``* — fail a second test whose message said not to adjust the constant. A +guard that refuses its own documented fix is worse than no guard, and it would have landed +on the one person who could not route around it. +""" + +from __future__ import annotations + +from datetime import date, datetime, timedelta + +import pytest + +#: The platform keys this repository's deliveries authenticate with, and when they die. +#: Read from the operator console 2026-08-05 (þing-02 A3(i)); recorded in views-appwrite +#: coordinate registry v1.4.4 and in register C-84. Both of this repo's paths — the FAO +#: delivery and the CRAF'd delivery — run under `VIEWS Pipeline Core`, the earlier one. +#: +#: Naive datetimes, deliberately: the console reports local time and the window below is +#: 30 days, so an hour either way changes nothing. Adding tzinfo would imply a precision +#: the source does not have. +KEY_EXPIRY: dict[str, datetime] = { + "VIEWS Pipeline Core": datetime(2026, 11, 17, 12, 35), + "UN FAO": datetime(2026, 11, 17, 16, 10), +} + +#: How long before the earliest expiry this starts failing. C-84's own window, and the +#: time a rotation needs to be scheduled with an operator rather than squeezed in. +LEAD_DAYS = 30 + +#: Set to a date to silence the tripwire until then — *"seen, and being acted on"*. +#: Must be before the expiry (asserted below), so it postpones attention rather than +#: removing it. ``None`` means unacknowledged. +ACKNOWLEDGED_UNTIL: date | None = None + + +def days_until_earliest_expiry(today: date) -> tuple[str, int]: + """(which key dies first, days until it does) — a pure function of a date. + + Split out so the FAILING branch can be exercised in the suite. A tripwire whose + firing path has never run is unproven however carefully it was written (C-102), and + this one would otherwise first execute live in October, on the day it matters. + """ + name = min(KEY_EXPIRY, key=lambda k: KEY_EXPIRY[k]) + return name, (KEY_EXPIRY[name].date() - today).days + + +def expiry_warning(name: str, days_left: int) -> str: + """What the tripwire says when it fires.""" + spread = max(KEY_EXPIRY.values()) - min(KEY_EXPIRY.values()) + hours, remainder = divmod(int(spread.total_seconds()), 3600) + gap = f"{hours}h{remainder // 60:02d}m" + when = ( + f"in {days_left} days" if days_left > 0 + else "TODAY" if days_left == 0 + else f"{-days_left} days ago — the seam is already dead" + ) + listing = "\n".join( + f" {key:22} {stamp:%Y-%m-%d %H:%M}" + for key, stamp in sorted(KEY_EXPIRY.items(), key=lambda kv: kv[1]) + ) + return ( + f"the platform Appwrite keys expire {when}:\n{listing}\n\n" + f"Both of this repository's delivery paths — un_fao and crafd — authenticate with " + f"{name!r}, the earlier one. The two keys are {gap} apart, which is not a stagger: " + "neither can carry traffic while the other is replaced, so a rotation that assumes " + "a window has none. After the later time every identity on the seam is dead at " + "once — model and ensemble writes, both partner deliveries, and FAO's own read " + "access.\n\n" + "This repository cannot rotate anything and must not hold credentials " + "(þing-01 D3). Issuing and installing keys is an operator console action " + "(views-appwrite#12); the key split is views-faoapi#338.\n\n" + "THREE ways to make this pass, in order of preference:\n" + " 1. rotate the keys, then update KEY_EXPIRY to the new expiries;\n" + " 2. if a key was replaced early, update KEY_EXPIRY to match;\n" + " 3. set ACKNOWLEDGED_UNTIL to a date before the expiry — this says the rotation " + "is scheduled and stops the tripwire blocking unrelated work until then.\n" + "Deleting this test is the fourth way and it is the one that produces the outage " + "(register C-84, issue #224)." + ) + + +def test_the_platform_keys_are_not_about_to_expire(): + """Fails inside the lead window unless the date is explicitly acknowledged.""" + today = date.today() + name, days_left = days_until_earliest_expiry(today) + if days_left > LEAD_DAYS: + return + if ACKNOWLEDGED_UNTIL is not None and today <= ACKNOWLEDGED_UNTIL: + return + pytest.fail(expiry_warning(name, days_left)) + + +def test_an_acknowledgement_cannot_outlive_the_expiry(): + """The escape hatch postpones attention; it must not be able to remove it. + + An open-ended acknowledgement is just the deletion in finding 4's clothing, and it + would read as a live guard while being none. + """ + if ACKNOWLEDGED_UNTIL is None: + return + earliest = min(KEY_EXPIRY.values()).date() + assert ACKNOWLEDGED_UNTIL < earliest, ( + f"ACKNOWLEDGED_UNTIL is {ACKNOWLEDGED_UNTIL}, on or after the earliest expiry " + f"({earliest}). An acknowledgement that outlives the thing it acknowledges is a " + "silent deletion — the suite would stay green straight through the outage." + ) + + +def test_the_tripwire_actually_fires_inside_the_lead_window(): + """The failing branch, exercised now rather than first executing in October (C-102). + + The probe date is DERIVED from `KEY_EXPIRY`, so this keeps proving something after a + rotation. Hardcoding it — as the first draft did — meant the proof quietly expired + the moment the constant was legitimately updated. + """ + earliest = min(KEY_EXPIRY.values()).date() + probe = earliest - timedelta(days=LEAD_DAYS - 7) + name, days_left = days_until_earliest_expiry(probe) + + assert name == "VIEWS Pipeline Core", "the earlier key is the one both paths use" + assert 0 < days_left <= LEAD_DAYS, "the probe must sit inside the window it tests" + + message = expiry_warning(name, days_left) + assert f"expire in {days_left} days" in message + assert "3h35m apart" in message, "the gap is C-84's finding, not the dates" + assert "not a stagger" in message + assert "views-appwrite#12" in message, "the reader must be told who can act" + assert "ACKNOWLEDGED_UNTIL" in message, "and how to clear it without deleting it" + assert "Deleting this test" in message, ( + "the last option must be named, or it is the one that gets taken quietly" + ) + + +def test_the_tripwire_is_silent_outside_the_lead_window(): + earliest = min(KEY_EXPIRY.values()).date() + _, days_left = days_until_earliest_expiry(earliest - timedelta(days=LEAD_DAYS + 60)) + assert days_left > LEAD_DAYS, "a tripwire that is always red is one nobody reads" + + +def test_the_lead_window_is_not_quietly_shrunk(): + """Shaving days off `LEAD_DAYS` neuters this without deleting anything. + + A floor rather than an equality: widening the window is always safe, and pinning the + exact value would recreate the problem the removed literal-pin caused. + """ + assert LEAD_DAYS >= 30, ( + f"LEAD_DAYS is {LEAD_DAYS}. C-84's window is a month, and rotation needs an " + "overlap period scheduled with an operator — shortening the warning is how a " + "dated guard gets neutered while still looking present." + ) + + +def test_the_outage_day_message_still_reads(tmp_path): + """The text an operator reads while the seam is down must not say '-3 days'.""" + earliest = min(KEY_EXPIRY.values()).date() + for probe, expected in ((earliest, "TODAY"), (earliest + timedelta(days=3), "3 days ago")): + name, days_left = days_until_earliest_expiry(probe) + assert expected in expiry_warning(name, days_left) diff --git a/tests/test_datafactory_deploy_readiness.py b/tests/test_datafactory_deploy_readiness.py index acadef8..22be04b 100644 --- a/tests/test_datafactory_deploy_readiness.py +++ b/tests/test_datafactory_deploy_readiness.py @@ -105,7 +105,17 @@ class TestServedArtifactMatchesBranch: def test_assembled_grid_not_older_than_gaul_parquets(self): grid = _DF / "data/assembled/grid.npy" parquet = _DF / "data/raw/gaul_admin/gaul0_code.parquet" - assert grid.exists() and parquet.exists() + # Neither is tracked in views-datafactory. Before 2026-08-17 the module-level + # skipif covered that; now CI fetches the sibling, so without this the test + # xfails on a MISSING FILE rather than on the staleness it asserts — and a + # strict xfail that can only ever xfail can never flip, which is the entire + # mechanism (ADR-014 §1). + if not (grid.exists() and parquet.exists()): + pytest.skip( + "the assembled grid and/or GAUL parquets are absent — they are not " + "tracked in views-datafactory, so a checkout alone cannot answer this. " + "Skipping rather than xfailing keeps the strict flip meaningful." + ) assert grid.stat().st_mtime >= parquet.stat().st_mtime, ( "assembled grid is older than the GAUL parquets — re-assemble and " "re-export the zarr before deploying, or the served GAUL channels " @@ -127,7 +137,14 @@ class TestServedArtifactProvenanceTracksGaul: strict=True, ) def test_provenance_includes_admin_digest(self): - prov = json.loads((_DF / "data/assembled/provenance.json").read_text()) + provenance = _DF / "data/assembled/provenance.json" + if not provenance.exists(): + pytest.skip( + "data/assembled/provenance.json is absent — not tracked in " + "views-datafactory, so a checkout alone cannot answer this. Skipping " + "rather than xfailing keeps the strict flip meaningful." + ) + prov = json.loads(provenance.read_text()) sources = prov.get("sources", {}) assert "admin_digest" in sources, ( "provenance.sources has no admin_digest — GAUL parquet changes are " diff --git a/tests/test_delivery_coverage.py b/tests/test_delivery_coverage.py index 716e194..0dca72a 100644 --- a/tests/test_delivery_coverage.py +++ b/tests/test_delivery_coverage.py @@ -92,17 +92,22 @@ def test_no_excluded_cells_is_noop_when_region_unpinned(): assert assert_no_excluded_cells({62356, 94776}, excluded_for("africa_me_legacy")) is None -# Cross-check the frozen manifest against the live producer when its checkout is present -# (CI has no sibling → skip). This is the drift tripwire C-30 asks for. +# Cross-check the frozen manifest against the live producer. This is the drift tripwire +# C-30 asks for, and since 2026-08-17 it RUNS IN CI: `run_pytest.yml` fetches +# views-datafactory, and both pgid lists below are tracked there. It backs a guarantee +# given to FAO in writing, so it must fail as an assertion rather than as a traceback — +# hence the gate names both files the body reads, not just the first. _DATAFACTORY = sibling_repo("views-datafactory") _DF = None if _DATAFACTORY is None else _DATAFACTORY / "src" / "datafactory_query" @pytest.mark.skipif( - _DF is None or not (_DF / "land_pgids.json").exists(), + _DF is None or not all((_DF / f"{n}_pgids.json").exists() for n in ("land", "land_gaul")), reason=( - "views-datafactory checkout not found — set VIEWS_DATAFACTORY=/path/to/" - "views-datafactory, or place it alongside this repo" + "views-datafactory checkout not found, or it does not carry both " + "src/datafactory_query/{land,land_gaul}_pgids.json — set VIEWS_DATAFACTORY=" + "/path/to/views-datafactory, or place it alongside this repo. Both files are " + "tracked upstream, so a plain checkout is enough (this runs in CI)" ), ) def test_manifest_matches_datafactory_land_minus_land_gaul(): diff --git a/tests/test_findability.py b/tests/test_findability.py new file mode 100644 index 0000000..a6272ca --- /dev/null +++ b/tests/test_findability.py @@ -0,0 +1,224 @@ +"""The C-94 findability preflight: does the consumer's own query find the delivery? + +Two layers, matching how the repo tests every other delivery invariant: the rule on +primitives here, and the wiring — that the manager actually calls it, and calls it +through a store whose injected name filter is suppressed — as declaration checks. + +The wiring checks are source reads rather than behavioural ones. Constructing a manager +needs pipeline-core, a views-models path manager and a live Appwrite environment (C-40), +and the properties worth holding are two lines: that the preflight runs only when +something was uploaded, and that it asks under the DECLARED consumer name rather than +the path manager's (C-77). +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +from tests.conftest import PARTNER_PACKAGES +from views_postprocessing.delivery import findability + +_REPO = Path(__file__).resolve().parent.parent + + +def _calls_body(body) -> set[str]: + """Names called anywhere inside a list of statements.""" + found: set[str] = set() + for stmt in body: + for c in ast.walk(stmt): + if isinstance(c, ast.Call) and isinstance(c.func, ast.Name): + found.add(c.func.id) + return found + + +def _function_source(source: str, name: str) -> str: + """Exactly one function's source. + + Slicing to end-of-file instead would let any later occurrence in the module satisfy + these checks — the assertion would pass for text that is not in the function at all. + """ + fn = next( + n for n in ast.walk(ast.parse(source)) + if isinstance(n, ast.FunctionDef) and n.name == name + ) + return "\n".join(source.splitlines()[fn.lineno - 1:fn.end_lineno]) + + +def test_a_found_document_passes(): + findability.assert_findable( + "file-abc", expected_file_id="file-abc", consumer_name="un_fao", category="forecast" + ) + + +def test_nothing_found_is_refused(): + with pytest.raises(findability.DeliveryNotFindableError): + findability.assert_findable( + None, expected_file_id="x", consumer_name="un_fao", category="forecast" + ) + + +def test_the_refusal_names_the_query_that_found_nothing(): + with pytest.raises(findability.DeliveryNotFindableError) as excinfo: + findability.assert_findable( + None, expected_file_id="x", consumer_name="un_fao", category="historical" + ) + message = str(excinfo.value) + assert "un_fao" in message, "the refusal must name the consumer name it queried by" + assert "historical" in message, "and which leg was invisible" + assert "INVISIBLE" in message, ( + "the message must say the delivery is invisible rather than degraded — that " + "distinction is ADR-013 §4.1a and it is what tells an operator to quarantine" + ) + + +def test_an_empty_string_file_id_is_not_treated_as_found(): + """`None` is the documented 'no match', but a store returning '' is not a find. + + pipeline-core's `get_latest_file_id` warns and returns None on no match, and warns + again if a match is missing its `fileId` field — in which case it returns whatever + `.get("fileId", None)` produced. A falsy id is not something to deliver on. + """ + with pytest.raises(findability.DeliveryNotFindableError): + findability.assert_findable( + "", expected_file_id="x", consumer_name="un_fao", category="forecast" + ) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_manager_verifies_only_when_something_was_uploaded(partner): + source = (_REPO / "views_postprocessing" / partner / "managers" / f"{partner}.py").read_text() + tree = ast.parse(source) + save = next( + n for n in ast.walk(tree) + if isinstance(n, ast.FunctionDef) and n.name == "_save_contract" + ) + + def _calls(node): + return { + c.func.id + for c in ast.walk(node) + if isinstance(c, ast.Call) and isinstance(c.func, ast.Name) + } + + assert "_assert_delivery_is_findable" in _calls(save), ( + f"{partner}'s _save_contract no longer runs the C-94 preflight, so an upload " + "that lands somewhere the consumer cannot see it reports success (C-94)." + ) + # Structural, not textual: an ordering check on substrings stays green if the call + # is moved into the `else` branch or dedented out of the guard entirely — which is + # the regression this message claims to prevent. + guarded = [ + n for n in ast.walk(save) + if isinstance(n, ast.If) + and isinstance(n.test, ast.Name) + and n.test.id == "upload_enabled" + and "_assert_delivery_is_findable" in _calls_body(n.body) + ] + assert guarded, ( + f"{partner} runs the findability preflight outside the `if upload_enabled:` " + "body. With the interlock holding nothing was uploaded, so the check would " + "refuse every staged run for the absence of a delivery nobody made." + ) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_preflight_queries_the_declared_name_not_the_path_managers(partner): + """C-77: the two are equal today by coincidence, and only one is a declaration.""" + source = (_REPO / "views_postprocessing" / partner / "managers" / f"{partner}.py").read_text() + + assert "_build_partner_read_store" in source, ( + f"{partner} lost the read-back store builder; without it " + "`get_latest_file_id` merges the path manager's model name into the query" + ) + builder = source[source.index("def _build_partner_read_store"):source.index("class ")] + assert "store.model_path = None" in builder, ( + f"{partner}'s read-back store no longer suppresses pipeline-core's automatic " + "`name == model_name` filter, so the preflight verifies the views-models " + "directory name instead of the declared consumer name. A rename there would " + "leave this check green while the delivery went dark (C-77)." + ) + + preflight = _function_source(source, "_assert_delivery_is_findable") + assert "_build_partner_read_store" in preflight, ( + f"{partner}'s preflight must use the suppressed-injection store, not the " + "upload store, or it asks the wrong question" + ) + + # The declared name and the two legs are supplied by the CALLER, so that is where + # they must be asserted. Looking for them in the preflight would be looking in the + # wrong function — which the end-of-file slice used to hide. + save = _function_source(source, "_save_contract") + assert "product.CONSUMER_DOCUMENT_NAME" in save, ( + f"{partner} must pass the DECLARED consumer name to the preflight, never the " + "path manager's model name that happens to equal it (C-77)" + ) + for category in ('"forecast"', '"historical"'): + assert category in save, ( + f"{partner} no longer verifies {category}. Both legs are checked separately " + "because a run with one leg missing is invisible in half, and the historical " + "leg is the one that stranded in run-0 (C-79)." + ) + assert "manifest_file_id" in save, ( + f"{partner} no longer scopes the forecast read-back to this run's manifest, so " + "the previous delivery's document satisfies the check (C-94)" + ) + + +def test_unverified_is_not_the_same_refusal_as_not_findable(): + """A store that could not be asked is a different event from an empty answer. + + Quarantining a delivery because the *check* failed would be an outage the guard + manufactured. Same distinction as C-103 (a missing producer client vs a producer + publishing no boundary) and C-99 (an unrecognised store result vs a real one). + """ + exc = findability.unverified("forecast", TimeoutError("read timed out")) + assert isinstance(exc, findability.FindabilityUnverifiedError) + assert not isinstance(exc, findability.DeliveryNotFindableError), ( + "the two must not share a type, or a caller cannot act differently on them" + ) + message = str(exc) + assert "UNVERIFIED, not known invisible" in message + assert "TimeoutError" in message and "read timed out" in message, ( + "the refusal must quote what actually stopped the check" + ) + assert "forecast" in message, "and say which leg is unverified" + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_preflight_does_not_report_a_failed_query_as_an_invisible_delivery(partner): + source = (_REPO / "views_postprocessing" / partner / "managers" / f"{partner}.py").read_text() + preflight = _function_source(source, "_assert_delivery_is_findable") + assert "findability.unverified(" in preflight, ( + f"{partner}'s preflight no longer distinguishes a store error from an empty " + "answer, so a transient network failure after a successful delivery would be " + "reported as the delivery being invisible — and quarantined (C-94)." + ) + + +def test_the_previous_runs_document_does_not_satisfy_this_run(): + """The finding that made the guard worth having: without run-scoping it passes + from delivery 2 onward exactly when a C-79 orphan appears. + + Run-1 delivered, so a document exists. Run-2's upload reports success but its + metadata document is never created. The consumer's query returns run-1's id. Asking + "is anything there" answers yes; asking "is what I just uploaded there" answers no. + """ + with pytest.raises(findability.DeliveryNotFindableError) as excinfo: + findability.assert_findable( + "run-1-document", + expected_file_id="run-2-document", + consumer_name="un_fao", + category="historical", + ) + message = str(excinfo.value) + assert "run-1-document" in message and "run-2-document" in message, ( + "the refusal must name both what the consumer will find and what this run " + "uploaded, or an operator cannot tell staleness from absence" + ) + assert "PREVIOUS" in message, ( + "the consequence — the consumer goes on serving the previous delivery — is the " + "part that distinguishes this from an empty bucket" + ) diff --git a/tests/test_gaul_lookup_fidelity.py b/tests/test_gaul_lookup_fidelity.py index ee023c4..623c150 100644 --- a/tests/test_gaul_lookup_fidelity.py +++ b/tests/test_gaul_lookup_fidelity.py @@ -21,19 +21,32 @@ That is views-datafactory#387 (square-degree area math at high latitudes), and it must not be conflated with what this file guarantees. -Split by dependency, on purpose: - * **always-on** — self-consistency of the committed artifact, plus the coordinate - formula against a committed PRIO-GRID sample. Runs anywhere, including CI. - * **skipif** — full value comparison against the views-datafactory sibling - checkout. Stronger, but absent on most machines (cf. C-46, where a hardcoded - path made a cross-repo gate invisible; here the skip is explicit and the - always-on half still guards regressions). +**Split by the artifact each test actually reads** — four gates, not two, and the +distinction is load-bearing (corrected 2026-08-17): + + * **always-on** — self-consistency of the committed artifact, its wire-cast dtype + stability, and the coordinate formula against a committed PRIO-GRID sample. + Touches no sibling; runs anywhere, including CI. + * **`_needs_region_pgids`** — needs only `src/datafactory_query/*_pgids.json`, which + views-datafactory **tracks**. A plain checkout suffices, so this runs in CI too. + * **`_needs_gaul_parquets`** — needs `data/raw/gaul_admin/*.parquet`, **untracked** + upstream. A checkout is not enough (C-46). + * **`_needs_priogrid_dbf`** — needs the PRIO-GRID shapefile, likewise untracked. + +Until 2026-08-17 all four sibling-aware tests shared one gate keyed to the parquets, +so two tests that read no parquets — and one that reads no sibling at all — sat dark +in CI for no reason. Gating a test on an artifact it does not read is the same mistake +that made a tracked directory stand in for untracked files; both are recorded in C-46. + +**The rule this file now follows: a gate names the artifact its test opens.** Anything +looser has failed here three times — a directory standing in for files, one mark +serving four dependencies, and a path built from a `None` checkout before the +`.exists()` that was supposed to guard it. """ from __future__ import annotations import json -import os from pathlib import Path import numpy as np @@ -61,16 +74,70 @@ _REGION = "land_gaul" _DATAFACTORY = sibling_repo("views-datafactory") -_HAS_DATAFACTORY = _DATAFACTORY is not None and ( - _DATAFACTORY / "data" / "raw" / "gaul_admin" -).is_dir() -_needs_datafactory = pytest.mark.skipif( - not _HAS_DATAFACTORY, +_GAUL_ADMIN = None if _DATAFACTORY is None else _DATAFACTORY / "data" / "raw" / "gaul_admin" + +#: Gate on the FILES this half reads, never on the directory that holds them. +#: +#: `data/raw/gaul_admin/` **is** tracked in views-datafactory — it carries +#: `supplement_azores.geojson` — while the seven GAUL parquets beside it are not. So +#: `.is_dir()` is true in any fresh checkout, the comparison below then runs, and it +#: dies on `FileNotFoundError: .../gaul0_code.parquet` instead of skipping. +#: +#: That is exactly what happened on 2026-08-03, and it is the whole reason +#: views-datafactory was withdrawn from CI (C-46, and the note in +#: `tests/conftest.py::SIBLINGS` that this commit corrects). The cause was read as +#: "the sibling cannot be checked out" when it was "this gate asks the wrong +#: question". The neighbouring PRIO-GRID check at the bottom of this file already +#: had it right, gating on `priogrid_cell.dbf` itself. +#: +#: Named for what it gates rather than for the repository the files live in. The old +#: name, `_HAS_DATAFACTORY`, asserted the same conflation the bug did: a checkout can +#: be present while these are absent, and that is the normal case in CI. +_HAS_GAUL_PARQUETS = _GAUL_ADMIN is not None and all( + (_GAUL_ADMIN / f"{src}.parquet").exists() for src in SOURCE_RENAME +) +_needs_gaul_parquets = pytest.mark.skipif( + not _HAS_GAUL_PARQUETS, reason=( - "views-datafactory checkout not found — set VIEWS_DATAFACTORY=/path/to/" - "views-datafactory, or place it alongside this repo. Only the " - "producer-comparison half is skipped; the always-on tests still guard the " - "committed artifact." + "the producer's GAUL parquets (data/raw/gaul_admin/*.parquet) are not present. " + "They are NOT in views-datafactory's git repository, so a checkout alone is not " + "enough and CI cannot run this comparison — see C-46. On a developer machine, " + "point VIEWS_DATAFACTORY at a checkout that has them." + ), +) + +#: The region's pgid list, which views-datafactory DOES track — so a plain checkout is +#: enough and this runs in CI. Kept separate from the parquet gate on purpose: gating a +#: test on an artifact it does not read is how the whole 2026-08-03 confusion started. +_REGION_PGIDS = ( + None if _DATAFACTORY is None + else _DATAFACTORY / "src" / "datafactory_query" / f"{_REGION}_pgids.json" +) +#: The PRIO-GRID shapefile, also untracked upstream. Declared here rather than built +#: inside the test: `_DATAFACTORY` is None when nothing resolves, and `None / "data"` +#: is a TypeError, not a skip. The test used to be shielded from that by a mark it did +#: not need; removing the mark exposed it, which is the third instance in this file of +#: a gate and its test disagreeing about what must exist. +_PRIOGRID_DBF = ( + None if _DATAFACTORY is None + else _DATAFACTORY / "data" / "raw" / "priogrid" / "shapefile" / "priogrid_cell.dbf" +) +_needs_priogrid_dbf = pytest.mark.skipif( + _PRIOGRID_DBF is None or not _PRIOGRID_DBF.exists(), + reason=( + "the PRIO-GRID shapefile (data/raw/priogrid/shapefile/priogrid_cell.dbf) is not " + "present. Like the GAUL parquets it is not tracked in views-datafactory, so a " + "checkout alone is not enough and CI cannot run this half." + ), +) + +_needs_region_pgids = pytest.mark.skipif( + _REGION_PGIDS is None or not _REGION_PGIDS.exists(), + reason=( + f"views-datafactory checkout not found, or it does not carry " + f"src/datafactory_query/{_REGION}_pgids.json — set VIEWS_DATAFACTORY=/path/to/" + "views-datafactory, or place it alongside this repo. Unlike the GAUL parquets " + "this file IS tracked upstream, so a checkout alone is enough." ), ) @@ -238,7 +305,7 @@ def test_lookup_version_stamp_resolves(lookup): # ── skipif: the full comparison against the producer ───────────────────────── -@_needs_datafactory +@_needs_gaul_parquets def test_lookup_values_match_the_producer_parquets(lookup, gids): """C-43, the core forward-check: every value against views-datafactory. @@ -247,7 +314,7 @@ def test_lookup_values_match_the_producer_parquets(lookup, gids): """ mismatches = {} for src, dst in SOURCE_RENAME.items(): - table = pq.read_table(_DATAFACTORY / "data" / "raw" / "gaul_admin" / f"{src}.parquet") + table = pq.read_table(_GAUL_ADMIN / f"{src}.parquet") source = dict( zip( (int(g) for g in table.column("gid").to_pylist()), @@ -268,10 +335,9 @@ def test_lookup_values_match_the_producer_parquets(lookup, gids): ) -@_needs_datafactory +@_needs_region_pgids def test_lookup_gid_set_equals_the_declared_region(gids): - region_file = _DATAFACTORY / "src" / "datafactory_query" / f"{_REGION}_pgids.json" - region = set(json.loads(region_file.read_text())) + region = set(json.loads(_REGION_PGIDS.read_text())) got = set(int(g) for g in gids) assert got == region, ( f"lookup gid set != {_REGION} region: {len(region - got)} missing, " @@ -279,14 +345,12 @@ def test_lookup_gid_set_equals_the_declared_region(gids): ) -@_needs_datafactory +@_needs_priogrid_dbf def test_coordinate_formula_matches_every_priogrid_cell(): """The committed fixture samples 219 cells; the sibling lets us check all 259,200.""" import struct - dbf = _DATAFACTORY / "data" / "raw" / "priogrid" / "shapefile" / "priogrid_cell.dbf" - if not dbf.exists(): - pytest.skip("PRIO-GRID shapefile not present in the datafactory checkout") + dbf = _PRIOGRID_DBF with dbf.open("rb") as fh: header = fh.read(32) n_records = struct.unpack("float64 wire cast losslessly (sidecar/historical §5.1).""" for col in CODE_COLS: @@ -519,7 +582,7 @@ def test_a_short_digest_is_refused_rather_than_truncated_silently(): builder._lookup_version("land_gaul", {"land_gaul_region": {"content_digest": "abcd"}}) -def test_the_builder_and_the_tests_resolve_the_same_datafactory(): +def test_the_builder_and_the_tests_resolve_the_same_datafactory(monkeypatch): """The one thing worth guarding about the deliberate duplication (S7 / #188). ``scripts/build_gaul_lookup._resolve_datafactory`` and @@ -538,11 +601,17 @@ def test_the_builder_and_the_tests_resolve_the_same_datafactory(): # first — and it is exercised precisely when no checkout exists. So compare the # computed paths unconditionally: gating this on a checkout being present would # skip the one case the test is for, and skip it in CI, where it matters most. - if "VIEWS_DATAFACTORY" not in os.environ: - assert builder._resolve_datafactory() == _REPO.parent / "views-datafactory", ( - "the builder's fallback and the tests' fallback resolve different " - "directories; with no environment override they would disagree silently" - ) + # Assert the fallback UNCONDITIONALLY by removing the override for the duration. + # This was `if "VIEWS_DATAFACTORY" not in os.environ`, which was correct until CI + # started setting that variable (2026-08-17) — at which point the branch stopped + # running in the one place the comment above says it matters most, and the + # assertion below degenerated into comparing $VIEWS_DATAFACTORY with itself. + monkeypatch.delenv("VIEWS_DATAFACTORY", raising=False) + assert builder._resolve_datafactory() == _REPO.parent / "views-datafactory", ( + "the builder's fallback and the tests' fallback resolve different " + "directories; with no environment override they would disagree silently" + ) + monkeypatch.undo() resolved = sibling_repo("views-datafactory") if resolved is None: diff --git a/tests/test_locked_environment.py b/tests/test_locked_environment.py new file mode 100644 index 0000000..7f09632 --- /dev/null +++ b/tests/test_locked_environment.py @@ -0,0 +1,168 @@ +"""The environment the suite runs in is the environment the lockfile describes (C-104). + +**Why this exists.** On 2026-08-16 a `pytest` run in this repository reported 25 +failures across five modules. 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`, and the two majors of drift produced: + +* `ModuleNotFoundError: views_pipeline_core.modules.dataloaders.datafactory_contract` + wherever a test imports a manager — which is every test that constructs or inspects + one, i.e. the only coverage the two largest modules in the package have; +* five byte-parity failures against the ADR-013 §10 golden fixture, because parquet + bytes are not stable across pyarrow majors (C-72). + +Neither is distinguishable, from the failure output, from a real regression. A +contributor reading that wall has no way to tell "your venv is stale" from "you broke +the wire contract", and the second reading is the one that costs an afternoon. + +**These are ordinary test failures, not collection errors** — the manager tests import +lazily (`tests/test_framework_contract.py:50`), so the session runs to completion. +Measured 2026-08-17 in the drifted venv: 26 failed, 441 passed, **0 errors**. That +distinction is not pedantry: a collection error would interrupt the session and this +diagnostic would never get to run in the scenario it was written for. + +**What this does not do.** It does not fix the drift and it does not skip. It replaces +a wall of misleading failures with one that names the packages, both versions, and the +command. The others stay until `poetry install` runs — they are the *symptom*, this is +the *diagnosis*, and the repo's position (ADR-014 §1) is that a diagnosis belongs in a +check rather than in a register entry nobody reads at the moment it would help. + +**Scope: the unconditional runtime dependencies declared in `pyproject.toml`**, read +from there rather than hardcoded — a literal list goes stale the first time a +dependency is added, which is the failure `tests/conftest.py::PARTNER_PACKAGES` exists +to prevent one level up. Dependencies gated by `optional`, `python` or `markers` are +skipped: they are legitimately absent from some environments, and reporting one as +"NOT INSTALLED — run `poetry install`" would be advice that cannot work. Dev-group +tools are out too: `ruff`'s reported version varies with how it was installed, and the +thing that actually broke CI on 2026-08-03 was its *rule set*, which `pyproject.toml` +already pins explicitly. + +CI runs `poetry install` before `pytest`, so this passes there by construction. It is +aimed squarely at the developer machine, which is the seat that had the problem. +""" + +from __future__ import annotations + +import re +import tomllib +from importlib import metadata +from pathlib import Path + +import pytest + +_REPO = Path(__file__).resolve().parent.parent +_PYPROJECT = _REPO / "pyproject.toml" +_LOCK = _REPO / "poetry.lock" + +#: Spec keys that make a dependency conditional, so its absence is not drift. +_CONDITIONAL = ("optional", "python", "markers") + + +def _normalize(name: str) -> str: + """PEP 503 normalization. + + `poetry.lock` stores normalized names; `pyproject.toml` stores whatever the author + typed. `PyYAML` and `views_frames` are both legal declarations that would never + match a lock entry compared raw — and the failure would read "absent from + poetry.lock, run `poetry lock`", which would fix nothing because the lock is fine. + """ + return re.sub(r"[-_.]+", "-", name).lower() + + +def _declared_runtime_dependencies() -> list[str]: + """Unconditional runtime dependency names, normalized.""" + project = tomllib.loads(_PYPROJECT.read_text()) + try: + deps = project["tool"]["poetry"]["dependencies"] + except KeyError: + pytest.fail( + "pyproject.toml no longer declares [tool.poetry.dependencies] in the form " + "this guard reads — a PEP 621 [project] migration would do this. Teach the " + "test the new form; do not delete it, or nothing compares the environment " + "to the lock again (C-104). Same treatment as tests/test_release_version.py." + ) + return [ + _normalize(name) + for name, spec in deps.items() + if name != "python" + and not (isinstance(spec, dict) and any(k in spec for k in _CONDITIONAL)) + ] + + +def _locked_versions() -> dict[str, str]: + packages = tomllib.loads(_LOCK.read_text())["package"] + return {_normalize(p["name"]): p["version"] for p in packages} + + +def test_every_declared_dependency_is_in_the_lockfile(): + """A declared dependency absent from the lock means the lock was never regenerated. + + Owned here rather than by the version check below, which skips names it cannot + find — otherwise the same omission is reported twice, once correctly as a stale + lock and once misleadingly as "installed X, locked None, run `poetry install`". + """ + locked = _locked_versions() + missing = sorted(set(_declared_runtime_dependencies()) - set(locked)) + assert not missing, ( + f"declared in pyproject.toml but absent from poetry.lock: {missing}. The lock " + "is stale with respect to the declaration — run `poetry lock` and commit it. " + "This is not a virtualenv problem and `poetry install` will not fix it." + ) + + +def test_the_installed_runtime_dependencies_match_the_lockfile(): + """One honest failure instead of a wall of misleading ones (C-104).""" + locked = _locked_versions() + drift = [] + for name in _declared_runtime_dependencies(): + expected = locked.get(name) + if expected is None: + continue # not locked at all — the test above owns that, and says so better + try: + installed = metadata.version(name) + except metadata.PackageNotFoundError: + installed = None + if installed != expected: + drift.append((name, installed, expected)) + + if not drift: + return + + table = "\n".join( + f" {name:24} installed={installed or 'NOT INSTALLED':<12} locked={expected}" + for name, installed, expected in drift + ) + + # Say what THIS drift causes, not what some remembered drift once caused. The 2026 + # incident was pipeline-core and pyarrow; a future one will not be, and a diagnosis + # that describes the wrong packages is the failure this file exists to remove. + drifted = {name for name, _, _ in drift} + consequences = [] + if "views-pipeline-core" in drifted: + consequences.append( + " - views-pipeline-core: the managers import " + "`modules.dataloaders.datafactory_contract` at module scope, so every test " + "that imports a manager fails. Those are ordinary failures, not collection " + "errors — the whole suite still runs." + ) + if "pyarrow" in drifted: + consequences.append( + " - pyarrow: emitted parquet bytes are not stable across majors, so the " + "ADR-013 §10 byte-parity fixture fails (C-72)." + ) + detail = ( + "Known consequences of the packages above:\n" + "\n".join(consequences) + "\n\n" + if consequences + else "" + ) + + pytest.fail( + "this virtualenv is not the one poetry.lock describes:\n" + f"{table}\n\n" + "Run `poetry install`.\n\n" + f"{detail}" + "Other failures in this run are likely consequences of the drift above rather " + "than defects. Fix the environment before reading them as bugs.\n\n" + "If the drift is deliberate — trialling an upgrade — this test is telling you " + "the truth, and the rest of the suite is not testing the locked contract." + ) diff --git a/tests/test_source_metadata.py b/tests/test_source_metadata.py new file mode 100644 index 0000000..51d29a7 --- /dev/null +++ b/tests/test_source_metadata.py @@ -0,0 +1,197 @@ +"""The producer-fact seam: what happens when the producer cannot be asked (C-103). + +``contract/source_metadata.py`` is the single place this repository asks +views-datafactory for a data fact, and until 2026-08-17 it had **no tests at all** — +which is how the defect below survived: nothing described what the module should do +when the client is absent, so "return None like everything else" looked reasonable. + +The distinction these tests pin is the whole point of the module: + +* the producer publishes **no boundary** — a normal, older store. The caller degrades + open and delivers unclipped (C-26, a recorded decision). +* the producer **cannot be asked at all** — a broken environment. Degrading open here + ships the unobserved zero-padded tail as observed history, and does it with one + WARNING in a log nobody reads. + +Same shape as C-60, where a provenance stamp degraded to ``"unknown"`` on a bare +except and made every delivery untraceable in exactly the field it existed to answer. +""" + +from __future__ import annotations + +import sys +import types +from pathlib import Path + +import pytest + +from tests.conftest import PARTNER_PACKAGES +from views_postprocessing.contract import source_metadata + +_REPO = Path(__file__).resolve().parent.parent + +#: The numpy 1.x/2.x ABI break recorded in views-models +#: `postprocessors/un_fao/requirements.txt` on 2026-08-13, found by the pre-delivery +#: rehearsal. It is a `ValueError`, not an `ImportError` — which is why the guard +#: catches `Exception`. +_ABI_BREAK = ( + "numpy.dtype size changed, may indicate binary incompatibility. " + "Expected 96 from C header, got 88" +) + + +@pytest.fixture +def absent_client(monkeypatch): + """Make `datafactory_query` unimportable REGARDLESS of what is installed. + + The first draft of this module relied on the package happening to be absent in the + developer venv, which meant four of these tests would have failed in the launcher + prefix — the environment they describe. That is the C-30/C-46 shape again: a guard + whose proof rests on an ambient property nothing declares. + """ + monkeypatch.setitem(sys.modules, "datafactory_query", None) + monkeypatch.setitem(sys.modules, "datafactory_query.defaults", None) + + +@pytest.fixture +def exploding_client(monkeypatch): + """`datafactory_query` is INSTALLED and raises on load — the real-world case.""" + + class _Exploding: + __name__ = "datafactory_query.defaults" + + def __getattr__(self, name): + raise ValueError(_ABI_BREAK) + + module = types.ModuleType("datafactory_query") + defaults = _Exploding() + module.defaults = defaults + monkeypatch.setitem(sys.modules, "datafactory_query", module) + monkeypatch.setitem(sys.modules, "datafactory_query.defaults", defaults) + + +def test_an_absent_client_raises_rather_than_reporting_no_boundary(absent_client): + with pytest.raises(source_metadata.ProducerClientUnavailable): + source_metadata.last_valid_month_id() + + +def test_an_absent_client_is_told_to_install_the_package(absent_client): + with pytest.raises(source_metadata.ProducerClientUnavailable) as excinfo: + source_metadata.last_valid_month_id() + message = str(excinfo.value) + assert "not installed" in message + assert "views-datafactory" in message, "the refusal must name the package to install" + # The reader must be able to tell this apart from the degrade-open case. + assert "observed" in message, ( + "the refusal must say what degrading open would have cost — unobserved months " + "shipping as observed history — or it reads as a routine unavailability" + ) + + +def test_a_client_that_raises_on_load_is_NOT_reported_as_missing(exploding_client): + """The failure this environment actually had: a ValueError, not an ImportError. + + An `except ImportError` clause would let this reach the caller's degrade-open and + ship the unobserved tail — the precise case the guard exists to separate. + """ + with pytest.raises(source_metadata.ProducerClientUnavailable) as excinfo: + source_metadata.last_valid_month_id() + message = str(excinfo.value) + assert "present but raised while loading" in message + assert "ValueError" in message and "numpy.dtype size changed" in message, ( + "the refusal must quote what actually went wrong; the operator cannot act on " + "'could not be loaded' alone" + ) + assert "Do NOT reinstall" in message, ( + "reporting an installed-but-broken package as missing sends the operator to a " + "fix they have already applied" + ) + + +def test_the_refusal_chains_the_original_exception(absent_client): + """``raise ... from exc``: the traceback must still say what actually failed.""" + with pytest.raises(source_metadata.ProducerClientUnavailable) as excinfo: + source_metadata.last_valid_month_id() + assert isinstance(excinfo.value.__cause__, ImportError) + + +def test_it_is_logged_as_well_as_raised(caplog, absent_client): + """ADR-008: a refusal on the delivery path is logged persistently AND raised.""" + with caplog.at_level("ERROR"), pytest.raises(source_metadata.ProducerClientUnavailable): + source_metadata.last_valid_month_id() + assert any(r.levelname == "ERROR" for r in caplog.records), ( + "the refusal was raised but never logged; a traceback that dies inside a " + "scheduled run leaves no persistent record (ADR-008)" + ) + + +def test_the_producers_answer_passes_through_untouched(monkeypatch): + """When the client IS present, this module reads and returns — it does not decide. + + Including ``None``: a store that predates the attribute reports no boundary, and + that is the case the caller is entitled to degrade open on. + """ + import types + + for answer in (137, None): + fake = types.ModuleType("datafactory_query") + defaults = types.ModuleType("datafactory_query.defaults") + seen = {} + + def get_last_valid_month_id(zarr_url=None, _answer=answer): + seen["zarr_url"] = zarr_url + return _answer + + defaults.get_last_valid_month_id = get_last_valid_month_id + fake.defaults = defaults + monkeypatch.setitem(sys.modules, "datafactory_query", fake) + monkeypatch.setitem(sys.modules, "datafactory_query.defaults", defaults) + + assert source_metadata.last_valid_month_id("zarr://declared") == answer + assert seen["zarr_url"] == "zarr://declared", ( + "the declared store must be passed through; guessing the producer's " + "default here would be the inference ADR-003 forbids" + ) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_managers_do_not_swallow_the_refusal(partner): + """The degrade-open must not re-absorb what this module just refused. + + Parametrized over ``PARTNER_PACKAGES`` rather than a literal pair: eight guards + once hardcoded ``"unfao"`` and all eight went on passing over ``crafd/`` when it + landed (see ``tests/conftest.py``). A ninth would have been this one. + + A source check rather than a behavioural one, deliberately: constructing a manager + needs pipeline-core, a path manager and an Appwrite environment (C-40), and the + property worth pinning is one line — that ``ProducerClientUnavailable`` is re-raised + *before* the broad ``except``. Someone tidying the two branches back into one is + precisely how C-103 would return, and it would return silently. + """ + source = (_REPO / "views_postprocessing" / partner / "managers" / f"{partner}.py").read_text() + + # Scope to `_read_historical_frame`. Comparing offsets across the whole file would + # let a narrow branch on some unrelated `try` satisfy the ordering while the + # boundary read's branch was gone. + start = source.index("def _read_historical_frame") + body = source[start:source.index("\n def ", start)] + + narrow = body.count("except source_metadata.ProducerClientUnavailable:") + broad = body.count("except Exception:") + assert narrow == 1, ( + f"{partner}'s _read_historical_frame has {narrow} ProducerClientUnavailable " + "branches, expected 1. Without it a broken environment is indistinguishable " + "from a producer that publishes no boundary, and the delivery ships fabricated " + "months either way (C-103)." + ) + assert broad == 1, ( + f"{partner}'s _read_historical_frame has {broad} broad `except Exception:` " + "branches, expected 1 (the C-26 degrade-open). If it is gone the refusal may " + "be fine, but this check no longer describes the code — read it and rewrite it." + ) + assert body.index("except source_metadata.ProducerClientUnavailable:") < body.index( + "except Exception:" + ), ( + f"{partner} catches Exception before ProducerClientUnavailable, so the narrow " + "branch is unreachable" + ) diff --git a/tests/test_store_construction.py b/tests/test_store_construction.py index a7d1895..9509cdb 100644 --- a/tests/test_store_construction.py +++ b/tests/test_store_construction.py @@ -69,7 +69,7 @@ def test_the_builders_are_functions_not_methods(partner): module = _managers(partner) for name in ("_build_prod_forecasts_store", "_build_partner_store", - "_partner_appwrite_config"): + "_build_partner_read_store", "_partner_appwrite_config"): fn = getattr(module, name, None) assert fn is not None, ( f"[{partner}] {name} is gone from the module namespace. If it moved back " @@ -163,7 +163,8 @@ def test_no_coordinate_value_is_baked_into_the_builders(partner): module = _managers(partner) source = "".join( inspect.getsource(getattr(module, name)) - for name in ("_build_prod_forecasts_store", "_partner_appwrite_config") + for name in ("_build_prod_forecasts_store", "_partner_appwrite_config", + "_build_partner_read_store") ) for line in source.splitlines(): if "os.getenv(" in line: diff --git a/tests/test_torn_run.py b/tests/test_torn_run.py new file mode 100644 index 0000000..26c8966 --- /dev/null +++ b/tests/test_torn_run.py @@ -0,0 +1,222 @@ +"""A run that dies mid-upload must say what it left behind (register C-105). + +The contract already handles the **consumer's** side of a torn run 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 (ADR-013 §4.2). Nothing partial is +served. + +What was missing is our side. The objects that *did* land stay in the partner store, +and until 2026-08-19 nothing recorded that they had — an operator was left to diff the +bucket by hand. At run-0 scale a retry adds ~110 more under the same names. + +**Nothing is deleted, deliberately.** Removing objects from a partner bucket is +irreversible and an operator decision; the neighbouring delete surface is its own open +question (C-58, views-pipeline-core #333, blocked on a test key). These tests pin the +part this repository can honestly own: turning an invisible mess into a documented one. +""" + +from __future__ import annotations + +from pathlib import Path + +import numpy as np +import pytest + +import pyarrow as pa + +from views_postprocessing.contract.frames import build_prediction_frame +from views_postprocessing.contract.gaul_schema import CODE_COLS, COORD_COLS, METADATA_COLS +from views_postprocessing.contract.wire import sink +from views_postprocessing.unfao import product + +_GIDS = [100001, 100002, 100003, 100004, 100005, 100006] + + +class FakeLease: + """Preloaded values — this file tests the upload phase, not the inbound chain.""" + + def __init__(self, run_id, frame, headers): + self.run_id = run_id + self._value = (frame, headers) + + def load(self): + return self._value + + +def _synthetic_lookup() -> pa.Table: + """A lookup covering exactly `_GIDS`, built from the declared schema. + + Derived from `gaul_schema` rather than copied as a literal table: the columns are + the contract's, and a table hand-written here would drift from it silently. Built + locally rather than imported from another test module — reaching into a sibling + test's private helper couples two files that should be able to change apart. + """ + columns = {"priogrid_gid": pa.array(_GIDS, pa.int64())} + for col in METADATA_COLS: + if col in COORD_COLS: + columns[col] = pa.array([10.25 + i for i in range(len(_GIDS))], pa.float64()) + elif col in CODE_COLS: + columns[col] = pa.array(list(range(1, len(_GIDS) + 1)), pa.int64()) + else: + columns[col] = pa.array([f"{col}-{i}" for i in range(len(_GIDS))]) + return pa.table(columns) +_PRODUCT = {"consumer_name": product.CONSUMER_DOCUMENT_NAME, "s_min": product.S_MIN} + + +class FailAfter: + """Uploads `n` objects, then refuses — the C-79 shape mid-run.""" + + def __init__(self, n: int): + self.n = n + self.calls: list[str] = [] + + def upload(self, file_path, **kwargs): + if len(self.calls) >= self.n: + raise RuntimeError("store said no") + self.calls.append(Path(file_path).name) + return f"id-{len(self.calls)}" + + +def _one_target_leases(): + values = np.tile(np.array([[1.0, 2.0, 3.0, 4.0]], dtype=np.float32), (6, 1)) + time = np.full(6, 543, dtype=np.int64) + unit = np.array(_GIDS, dtype=np.int64) + frame = build_prediction_frame(values, time, unit) + headers = [{ + "run_id": "fixture_run_0", "target": "lr_ged_sb", "time_id": 543, + "sample_count": 4, "provenance": {"ensemble": "fixture_ensemble"}, + }] + return {"lr_ged_sb": FakeLease("fixture_run_0", frame, headers)} + + +def _deliver(store, tmp_path): + return sink.deliver_run( + _one_target_leases(), lookup=_synthetic_lookup(), staging_dir=tmp_path, + **_PRODUCT, store=store, upload_enabled=True, + ) + + +def test_a_torn_run_refuses_and_names_what_already_landed(tmp_path): + store = FailAfter(1) # the shard lands; the sidecar does not + with pytest.raises(sink.TornRunError) as excinfo: + _deliver(store, tmp_path) + message = str(excinfo.value) + assert "TORN" in message + assert "fixture_run_0" in message, "the refusal must name the run" + assert store.calls[0] in message, ( + "the refusal must name the objects already in the partner store; without them " + "an operator has to diff the bucket by hand (C-105)" + ) + assert "id-1" in message, "and their file ids, so they can be found again" + + +def test_it_says_how_many_of_how_many(tmp_path): + store = FailAfter(1) + with pytest.raises(sink.TornRunError) as excinfo: + _deliver(store, tmp_path) + # one shard + sidecar + manifest = 3 objects for this single-target run + assert "1 of 3" in str(excinfo.value) + + +def test_it_says_the_consumer_is_unaffected_and_nothing_was_removed(tmp_path): + """Both halves matter: no partial serve, and no silent cleanup either.""" + with pytest.raises(sink.TornRunError) as excinfo: + _deliver(FailAfter(1), tmp_path) + message = str(excinfo.value) + assert "almost certainly" in message and "cannot see this run" in message, ( + "an operator's first question is whether the partner is being served garbage. " + "The §4.2 commit marker means almost certainly not — but the code cannot KNOW " + "it, because a manifest upload can fail after the store committed the document. " + "Asserting it categorically would steer a re-run that duplicates every object." + ) + assert "NOT removed" in message, ( + "the refusal must be explicit that it deleted nothing — a reader who assumes " + "cleanup happened will not go looking" + ) + assert "correction_procedure.md" in message and "C-105" in message + + +def test_a_failure_on_the_manifest_is_still_torn(tmp_path): + """The last upload is the commit marker; losing it is the canonical torn run.""" + store = FailAfter(2) # shard + sidecar land, manifest does not + with pytest.raises(sink.TornRunError) as excinfo: + _deliver(store, tmp_path) + assert "2 of 3" in str(excinfo.value) + + +def test_a_clean_run_reports_no_tear(tmp_path): + store = FailAfter(99) + summary = _deliver(store, tmp_path) + assert summary["uploaded"] is True + assert summary["manifest_file_id"] == "id-3" + assert len(store.calls) == 3 + + +def test_the_object_that_failed_is_named_as_the_likeliest_orphan(tmp_path): + """The failing object is the one most likely to be an orphan, and it is not in the + confirmed list — because the list only holds uploads that returned. + + `_ContractStorePort.upload` raises precisely in the case its own comment documents: + the store "RETURNS success=False with the file already uploaded". So the object that + failed is the C-79 shape, sitting in the bucket with no metadata document. Listing + only the successes and calling it what remains would send an operator past the very + orphan this exists to surface. + """ + store = FailAfter(1) + with pytest.raises(sink.TornRunError) as excinfo: + _deliver(store, tmp_path) + message = str(excinfo.value) + assert "MAY ALSO HAVE LANDED" in message + assert "C-79" in message, "and say which shape to look for" + # the sidecar is what failed here; it must appear even though it is not "confirmed" + assert "sidecar" in message + + +def test_a_failure_on_the_very_first_upload_does_not_claim_an_empty_list(tmp_path): + """The commonest infrastructure failure: credentials expire, upload #1 refuses. + + The first draft printed "Already in the partner store, and NOT removed: ." — an + empty list with a dangling period, presented as a bucket to audit. + """ + with pytest.raises(sink.TornRunError) as excinfo: + _deliver(FailAfter(0), tmp_path) + message = str(excinfo.value) + assert "Nothing is confirmed in the partner store" in message + assert "NOT removed: ." not in message, "no dangling empty list" + # even here the failing object may have landed, so the caveat must still be present + assert "MAY ALSO HAVE LANDED" in message + + +def test_a_tear_is_not_a_malformed_run(tmp_path): + """Opposite retry semantics: SinkError means do-not-retry, a tear is transient.""" + with pytest.raises(sink.TornRunError) as excinfo: + _deliver(FailAfter(1), tmp_path) + assert not isinstance(excinfo.value, sink.SinkError), ( + "TornRunError must not share a base with the malformed-run family, or an " + "orchestration layer treating SinkError as do-not-retry would silently swallow " + "store outages — and test_hop_b_sink_e2e already asserts SinkError for malformed" + ) + + +def test_the_complete_ledger_is_logged_at_ERROR_not_only_INFO(tmp_path, caplog): + """The message truncates at five; the log must carry all of them, at a level a + launcher will actually have enabled. + + The per-upload ledger is `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 file_id anywhere, making the message's pointer to + "the run log" a promise to an empty file. + """ + store = FailAfter(2) # shard + sidecar land, manifest does not + with caplog.at_level("ERROR"), pytest.raises(sink.TornRunError): + _deliver(store, tmp_path) + + ledger = [r.getMessage() for r in caplog.records if "TORN-LEDGER" in r.getMessage()] + assert len(ledger) == 2, ( + f"expected one ERROR ledger line per confirmed upload, got {len(ledger)}" + ) + for name, line in zip(store.calls, ledger): + assert name in line and "file_id=" in line, ( + "each ledger line must carry the object name AND its file id — the id is " + "the only handle an operator has for finding it again" + ) diff --git a/views_postprocessing/contract/gaul_schema.py b/views_postprocessing/contract/gaul_schema.py index 1a49e49..e51f6da 100644 --- a/views_postprocessing/contract/gaul_schema.py +++ b/views_postprocessing/contract/gaul_schema.py @@ -19,6 +19,22 @@ `COLUMNS` below states each column's role and wire dtype once; everything else is derived from it. Reordering `COLUMNS` **is a wire change** and will fail the §10 byte-parity fixture — which is the intended consequence, not an accident. + +**Before proposing integer code columns, read ADR-013 §5.1a.** The partner has asked +twice (#278, #272), and will ask again; the reason recorded here until 2026-08-17 was +not a good one. "Codes are always float64" is *not* because an integer column cannot +hold a missing value — parquet and arrow carry nullable integers natively, and the +lookup this module describes stores all three code columns as `int64` with zero +nulls. The float is introduced by the builders below, not by the data. + +The rule survives on a different, measured ground: an int64 parquet column reads back +as `int64` under a default pandas read when it holds no null, and as `float64` when it +holds one, so nullable int64 would move the dtype instability from our writer to the +consumer's reader and make it depend on what a given run contained. §5.1a carries the +measurement, the consumer-side evidence, and the one useful consequence — that because +the delivered region excludes the GAUL-uncovered cells, no delivered code is ever +missing (`tests/test_gaul_lookup_fidelity.py::test_lookup_has_no_nulls`), so a +consumer's `astype("int64")` on read is lossless for this product. """ from __future__ import annotations diff --git a/views_postprocessing/contract/source_metadata.py b/views_postprocessing/contract/source_metadata.py index 49b163a..df2ee2f 100644 --- a/views_postprocessing/contract/source_metadata.py +++ b/views_postprocessing/contract/source_metadata.py @@ -20,6 +20,16 @@ logger = logging.getLogger(__name__) +class ProducerClientUnavailable(RuntimeError): + """``datafactory_query`` could not be loaded, so no producer fact can be read. + + One type, two causes, and the message says which: the package is not installed, or + it is installed and raised while loading. Both are broken environments; neither is + a producer that publishes no boundary. Named "unavailable" rather than "missing" + because the second cause is the one this environment has actually had. + """ + + def last_valid_month_id(zarr_url: str | None = None) -> int | None: """The producer's last *observed* month for the served zarr (a datafactory fact). @@ -31,9 +41,69 @@ def last_valid_month_id(zarr_url: str | None = None) -> int | None: zarr_url: the served zarr; ``None`` uses datafactory's default store (which the FAO queryset itself uses — ``ZARR_URL = DEFAULT_REMOTE.zarr_url``). - The import is lazy so this module loads without the heavy datafactory dependency - present (e.g. in unit-test environments). + Raises: + ProducerClientUnavailable: if ``datafactory_query`` cannot be loaded — + whether because it is absent or because importing it raised. + + **The dependency is the launcher's to supply, and it does.** ``datafactory_query`` + ships inside ``views-datafactory``; it is not separately installable and it is not + declared in this repository's ``pyproject.toml``. Both launchers declare it — + views-models ``postprocessors/{un_fao,un_crafd}/requirements.txt`` pin + ``views-datafactory>=1.9.0,<2.0.0``. The import stays lazy so this module, and + everything that imports it, loads in a unit-test environment without the producer's + (heavy) package present. + + **Why this raises rather than returning None (register C-103).** The caller degrades + open when the boundary is unavailable — a deliberate C-26 decision, because a producer + that publishes no ``last_valid_month_id`` is a normal, older store. A client that will + not load is not that. It is a broken environment, and returning ``None`` for it would + make the two indistinguishable and ship the unobserved zero-padded tail as observed + history. Same shape as C-60, where a provenance stamp degraded to ``"unknown"`` on a + bare except and made every delivery untraceable. + + This is defence in depth rather than the first line: a missing ``datafactory_query`` + already fails earlier and louder, because the postprocessor's ``config_queryset`` + imports it at module scope and raises, which ``launch_config. + assert_queryset_was_importable`` turns into a refusal before any frame is read + (C-83). Verified 2026-08-17. This guard exists for the paths that gate does not + cover — a direct caller, a future launcher, a partner that does not go through the + same queryset. """ - from datafactory_query.defaults import get_last_valid_month_id + try: + from datafactory_query.defaults import get_last_valid_month_id + except Exception as exc: + # NOT `except ImportError`. The failure this environment has actually had is a + # `ValueError: numpy.dtype size changed ... Expected 96 from C header, got 88` — + # the numpy 1.x/2.x ABI break recorded in views-models + # `postprocessors/un_fao/requirements.txt` on 2026-08-13, found by the + # pre-delivery rehearsal. An ImportError-only clause lets that sail into the + # caller's degrade-open and ship the unobserved tail, which is the exact case + # this guard exists to separate. + if isinstance(exc, ModuleNotFoundError) and (exc.name or "").startswith( + "datafactory_query" + ): + cause = ( + f"it is not installed ({exc.name!r} not found). It ships inside " + "views-datafactory — there is no separate distribution — and the " + "launcher is expected to supply it: " + "pip install 'views-datafactory>=1.9.0,<2.0.0'." + ) + else: + cause = ( + f"it is present but raised while loading: " + f"{type(exc).__name__}: {exc}. Do NOT reinstall views-datafactory on " + "the strength of this — the package is there. Check the environment " + "itself; the known instance is the numpy 1.x/2.x ABI break in the " + "shared envs/views-postprocessing prefix." + ) + err_msg = ( + f"the producer's last_valid_month_id cannot be read because " + f"datafactory_query could not be loaded: {cause} Refusing rather than " + "reporting 'no boundary published' — that is a different condition, and " + "conflating them lets unobserved months ship as observed history " + "(C-103, C-26)." + ) + logger.error(err_msg) # ADR-008: logged persistently AND raised + raise ProducerClientUnavailable(err_msg) from exc return get_last_valid_month_id(zarr_url) diff --git a/views_postprocessing/contract/wire/sink.py b/views_postprocessing/contract/wire/sink.py index 251d921..3c88b59 100644 --- a/views_postprocessing/contract/wire/sink.py +++ b/views_postprocessing/contract/wire/sink.py @@ -54,6 +54,72 @@ class SinkError(ValueError): """The assembled run cannot be delivered as declared.""" +class TornRunError(RuntimeError): + """An upload failed partway, leaving objects in the partner store. + + Deliberately **not** a ``SinkError``. That family means "the assembled run cannot be + delivered as declared" — malformed input, where retrying is pointless; a tear is a + transient infrastructure failure with the opposite retry semantics, and + ``tests/test_hop_b_sink_e2e`` already uses ``pytest.raises(SinkError)`` as the + malformed-run assertion. Sharing a base would let any future orchestration layer + that treats ``SinkError`` as do-not-retry silently swallow store outages. Same + reasoning, same base, as ``delivery.findability``'s two error types. + """ + + +def _torn_run_error(run_id, failed_on, uploaded, total, exc) -> TornRunError: + """The refusal for a run that died mid-upload, naming what may be left (C-105). + + The contract handles the *consumer's* side correctly and by design: the run manifest + is uploaded last, so a torn attempt has no commit marker and is invisible rather + than half-visible (§4.2). What it does not handle is our side — the objects that did + land stay there, and until this existed nothing recorded that they had. At run-0 + scale a retry adds ~110 more under the same names. + + **Nothing is deleted here, deliberately.** Removing objects from a partner bucket is + irreversible and an operator decision, not a delivery-path one; the neighbouring + delete surface is its own open question (C-58, views-pipeline-core #333, blocked on + a test key). This turns an invisible mess into a documented one, which is the part + this repository can honestly own. + """ + landed = ", ".join(f"{u['name']}#{u['file_id']}" for u in uploaded[:5]) + more = "" if len(uploaded) <= 5 else f" (+{len(uploaded) - 5} more; full ledger logged at ERROR)" + confirmed = ( + f"Confirmed in the partner store, and NOT removed: {landed}{more}." + if uploaded + else "Nothing is confirmed in the partner store — this was the first upload." + ) + # ADR-008: logged persistently AND raised — and the COMPLETE ledger goes here + # rather than into the message, which truncates at five. The per-upload ledger + # above is `logger.info`, and nothing in this package sets a level: pipeline-core + # removed its own `setLevel` precisely so the application owns it. A launcher + # running at WARNING would therefore have written no file_id anywhere, and the + # message's pointer to "the run log" would have been a promise to an empty file — + # leaving the operator diffing the bucket by hand, which is the state C-105 exists + # to remove. At ERROR the ledger survives any level a launcher is likely to choose. + logger.error( + "run %s TORN on %s: complete upload ledger follows (%d object(s) confirmed)", + run_id, failed_on, len(uploaded), + ) + for entry in uploaded: + logger.error(" TORN-LEDGER run=%s name=%s file_id=%s", run_id, entry["name"], entry["file_id"]) + + return TornRunError( + f"run {run_id!r} is TORN: {len(uploaded)} of {total} objects were confirmed " + f"uploaded before {failed_on!r} failed ({type(exc).__name__}: {exc}).\n" + f"{confirmed}\n" + f"{failed_on!r} MAY ALSO HAVE LANDED, as a file carrying no metadata document — " + "the store reports failure *after* uploading the file when the document write " + "fails, which is the C-79 orphan shape. Check for it as well as anything listed above; " + "it is the likeliest orphan of the whole run.\n" + "The manifest upload did not report success, so the consumer almost certainly " + "cannot see this run (§4.2 — the manifest is the commit marker). Verify that " + "before re-running: a re-run uploads every object again under the same names, " + "and whether the store supersedes or duplicates is not something this " + "repository asserts. See docs/operations/correction_procedure.md and C-105." + ) + + def deliver_run( per_target: dict, *, @@ -160,13 +226,30 @@ def deliver_run( common = {"name": consumer_name, "category": "forecast", "loa": "pgm"} - def _upload(file_name: str, doc_type: str, targets: list) -> None: - store.upload(staging / file_name, filename=file_name, doc_type=doc_type, targets=targets, **common) - logger.info("uploaded %s (type=%s, run=%s)", file_name, doc_type, run_id) # the ledger + # The ledger is kept in memory as well as in the log, so a torn run can say what + # it left behind rather than leaving an operator to diff the bucket (C-105). + uploaded: list[dict] = [] + total = len(shard_records) + 2 # shards + sidecar + manifest + + def _upload(file_name: str, doc_type: str, targets: list): + try: + file_id = store.upload( + staging / file_name, filename=file_name, doc_type=doc_type, targets=targets, **common + ) + except Exception as exc: + raise _torn_run_error(run_id, file_name, uploaded, total, exc) from exc + uploaded.append({"name": file_name, "file_id": file_id}) + logger.info( # the ledger — file_id included, it is the only persistent record + "uploaded %s (type=%s, run=%s, file_id=%s)", file_name, doc_type, run_id, file_id + ) + return file_id for record in shard_records: _upload(record["name"], SHARD_DOC_TYPE, [record["target"]]) _upload(sidecar_file, SIDECAR_DOC_TYPE, list(per_target)) - _upload(manifest_file, MANIFEST_DOC_TYPE, list(per_target)) # the commit marker + # The manifest is uploaded LAST, so it is the newest `category="forecast"` document + # in the store — which is exactly what the consumer's query returns. Carried out so + # the C-94 read-back can assert the consumer would find THIS run (register C-94). + summary["manifest_file_id"] = _upload(manifest_file, MANIFEST_DOC_TYPE, list(per_target)) summary["uploaded"] = True return summary diff --git a/views_postprocessing/crafd/managers/crafd.py b/views_postprocessing/crafd/managers/crafd.py index efb6152..5201056 100644 --- a/views_postprocessing/crafd/managers/crafd.py +++ b/views_postprocessing/crafd/managers/crafd.py @@ -17,7 +17,7 @@ from views_postprocessing.crafd.store_port import _ContractStorePort from views_postprocessing.contract.wire import sink as wire_sink from views_postprocessing.contract.wire import source_selection -from views_postprocessing.delivery import coverage, observed_range, provenance +from views_postprocessing.delivery import coverage, findability, observed_range, provenance from pathlib import Path logger = logging.getLogger(__name__) @@ -113,6 +113,36 @@ def _partner_appwrite_config(model_path) -> AppwriteConfig: ) +def _build_partner_read_store(model_path) -> DatastoreModule: + """The partner store for the C-94 read-back, with pipeline-core's automatic + ``name == model_name`` filter suppressed — otherwise the preflight would verify the + views-models directory name, which equals the declared consumer name only by + coincidence (**C-77**). C-94 records why it reuses the write key. + """ + store = DatastoreModule(appwrite_file_manager_config=_partner_appwrite_config(model_path)) + store.model_path = None + return store + + +def _assert_delivery_is_findable(model_path, consumer_name: str, uploaded: dict) -> None: + """C-94: ask the store the question the consumer asks, and refuse silence. + + A function, not a method (C-40 (a)) — its refusal is observable without a manager + or an Appwrite environment. ``uploaded`` maps each leg to the file id THIS run put + there; `delivery/findability.py` carries why that scoping is the whole guard. + """ + for category, expected in uploaded.items(): + try: + port = _ContractStorePort(_build_partner_read_store(model_path)) + found = port.latest_file_id({"name": consumer_name, "category": category}) + except Exception as exc: # could not ask != asked and got nothing (C-99, C-103) + raise findability.unverified(category, exc) from exc + findability.assert_findable( + found, expected_file_id=expected, consumer_name=consumer_name, category=category + ) + logger.info("Findability preflight passed: both legs retrievable under %r.", consumer_name) + + class CRAFDPostProcessorManager(PostprocessorManager, ForecastingModelManager): def __init__( self, @@ -136,8 +166,18 @@ def _read_historical_frame(self): ) try: lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) + except source_metadata.ProducerClientUnavailable: + # NOT degrade-open. A producer client that will not load is a broken + # environment, not a producer that publishes no boundary — and the whole + # point of the two branches is that they are different conditions (C-103). + raise except Exception: - logger.warning("last_valid_month_id unavailable; skipping clip (degrade-open, C-26).", exc_info=True) + logger.warning( + "last_valid_month_id could not be read; skipping the observed-range " + "clip (degrade-open, C-26). Any unobserved months above the producer's " + "boundary WILL ship as observed history in this delivery.", + exc_info=True, + ) lv = None if lv is None: self._historical_frame = frame @@ -322,7 +362,7 @@ def _save_contract(self) -> dict: Path(summary["staging_dir"]), lookup ) if upload_enabled: - store.upload( + hist_file_id = store.upload( hist_path, filename=hist_path.name, # The DECLARED consumer name, not `self._model_path.model_name` @@ -341,6 +381,13 @@ def _save_contract(self) -> dict: description=hist_description, ) logger.info("uploaded %s (historical, run %s)", hist_path.name, summary["run_id"]) + # C-94: nothing above observes the OUTCOME of an upload. Every call + # reported success in run-0 too, and the historical leg still stranded. + _assert_delivery_is_findable( + self._model_path, + product.CONSUMER_DOCUMENT_NAME, + {"forecast": summary["manifest_file_id"], "historical": hist_file_id}, + ) else: logger.info( "Interlock holding: historical artifact staged at %s (no store calls).", diff --git a/views_postprocessing/crafd/store_port.py b/views_postprocessing/crafd/store_port.py index cda03ff..b0ee757 100644 --- a/views_postprocessing/crafd/store_port.py +++ b/views_postprocessing/crafd/store_port.py @@ -63,7 +63,7 @@ def download(self, file_id: str) -> bytes: "these by name and cannot tell a failed download from an empty artifact." ) - def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, description=None) -> None: + def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, description=None) -> str | None: result = self._dsm.upload_data( file=file_path, filename=filename, @@ -98,3 +98,9 @@ def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, f"without a metadata document): {error}. The store reported " f"success={success!r} (result type {type(result).__name__})." ) + # The uploaded file's id, so the C-94 read-back can be scoped to THIS run. + # Discarding it (as this did until 2026-08-19) makes the only available check + # "is there any document under the consumer's name", which the previous run + # already satisfies — so the guard could never fail from delivery 2 onward. + data = getattr(result, "data", None) + return data.get("file_id") if isinstance(data, dict) else None diff --git a/views_postprocessing/delivery/findability.py b/views_postprocessing/delivery/findability.py new file mode 100644 index 0000000..508dea3 --- /dev/null +++ b/views_postprocessing/delivery/findability.py @@ -0,0 +1,107 @@ +"""Findability invariant: a delivered artifact must be retrievable under the name the +consumer actually queries (register C-94). + +Representation-free — a lookup result and the declared identity it was looked up by. +No store types, no pandas, no frames. The caller performs the query (it owns the port); +the rule about what the answer means lives here. + +**The failure this exists for is invisible by construction.** Every upload reports +success, storage is billed, and the consumer's endpoint returns empty. ADR-013 §4.1a +calls it *"invisible to the consumer, not merely degraded"*, and it has happened: run-0's +historical artifact was stranded on 2026-07-27 as a file with no metadata document +(register C-79). Nothing in this repository observed it. Every other mechanism the +platform aims at this is a **CI-time proxy** — we check our label against the registry, +the consumer checks theirs — and none of them observes the outcome of a real upload. + +**Scope, stated plainly, because this entry has already been over-claimed once.** This +catches *the delivery ran and the consumer cannot see it*. It does **not** catch: + +* *no delivery happened at all* — the 2026-08-12 empty bucket, which was an upstream + destructive migration with no run since. A post-upload check observes nothing when + there was no upload. This repository is not told when a delivery is due. +* *the bucket is fine but what is served is stale* — faoapi's warm per-key cache can + serve stale historical over an emptied bucket. + +Both are recorded as gaps in C-94 rather than dressed as things this covers. +""" + +from __future__ import annotations + + +class DeliveryNotFindableError(RuntimeError): + """An upload succeeded, and the consumer's own query cannot find it.""" + + +class FindabilityUnverifiedError(RuntimeError): + """The read-back could not be performed, so findability is unknown.""" + + +def unverified(category: str, exc: BaseException) -> FindabilityUnverifiedError: + """The refusal for *could not ask*, which is not *asked and got nothing*. + + Distinguished for the same reason ``source_metadata`` distinguishes a missing + producer client from a producer that publishes no boundary (C-103), and for the + same reason ``_ContractStorePort.download`` refuses an unrecognised result rather + than adapting to it (C-99): 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, and quarantining it on a transient store error + would be an outage manufactured by the guard. + """ + return FindabilityUnverifiedError( + f"the {category!r} leg uploaded, but the C-94 read-back could not be performed: " + f"{type(exc).__name__}: {exc}. The delivery is UNVERIFIED, not known invisible — " + "re-run the check before quarantining anything." + ) + + +def assert_findable(file_id, *, expected_file_id, consumer_name: str, category: str) -> None: + """Raise unless the store returned something for the consumer's own query. + + Args: + file_id: the result of querying the partner store for the newest document + matching the consumer's filters. ``None`` is pipeline-core's documented + "no match", but **any falsy value is refused**: `get_latest_file_id` also + warns and returns ``.get("fileId", None)`` when it finds a document that + is missing that field, so an empty id means "found something unusable" + rather than "found". Same polarity as ``_ContractStorePort.download``, + which refuses zero bytes for the reason C-99 records — an unrecognised + result is refused and named, not adapted to silently. + expected_file_id: the id THIS run uploaded for this leg — the manifest for the + forecast leg (uploaded last, so it is the newest such document) and the + historical artifact for its own. Without it the only available question is + *"does any document exist under the consumer's name"*, which the previous + delivery already answered yes to — so the check would pass on every run + after the first, precisely when a C-79-shaped orphan appeared. + consumer_name: the DECLARED store-document ``name`` the consumer filters on + (``product.CONSUMER_DOCUMENT_NAME``), never a path-manager or directory + name that happens to equal it (C-77). + category: the delivery leg being verified — ``"forecast"`` or ``"historical"``. + Checked separately on purpose: a run whose forecast landed and whose + historical did not is invisible in exactly one half, and a single + whole-delivery check would pass on it. + + Raises: + DeliveryNotFindableError: naming the query that found nothing. + """ + if file_id and file_id == expected_file_id: + return + if file_id: + raise DeliveryNotFindableError( + f"delivery is INVISIBLE to the consumer: the newest {category!r} document " + f"under name == {consumer_name!r} is {file_id!r}, but this run uploaded " + f"{expected_file_id!r}. The consumer will go on serving the PREVIOUS " + "delivery while this one reports success — which is why the check is scoped " + "to this run rather than asking whether any document exists (a question the " + "previous run already answered). Quarantine and inspect the partner bucket." + ) + raise DeliveryNotFindableError( + f"delivery is INVISIBLE to the consumer: uploads for category {category!r} " + f"reported success, but querying the partner store as the consumer does — " + f"name == {consumer_name!r}, category == {category!r} — returns nothing. " + "The artifacts may exist as files while carrying no metadata document, which " + "is how run-0's historical leg was stranded (C-79); a document under any other " + "name is equally invisible, because the consumer filters on this one " + "unconditionally (ADR-013 §4.1a). Quarantine the run and check the partner " + "bucket before re-delivering — the contract has no retraction primitive, so a " + "correction is a new complete run." + ) diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index 3b727f8..4d6ddf6 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -17,7 +17,7 @@ from views_postprocessing.unfao.store_port import _ContractStorePort from views_postprocessing.contract.wire import sink as wire_sink from views_postprocessing.contract.wire import source_selection -from views_postprocessing.delivery import coverage, observed_range, provenance +from views_postprocessing.delivery import coverage, findability, observed_range, provenance from pathlib import Path logger = logging.getLogger(__name__) @@ -113,6 +113,36 @@ def _partner_appwrite_config(model_path) -> AppwriteConfig: ) +def _build_partner_read_store(model_path) -> DatastoreModule: + """The partner store for the C-94 read-back, with pipeline-core's automatic + ``name == model_name`` filter suppressed — otherwise the preflight would verify the + views-models directory name, which equals the declared consumer name only by + coincidence (**C-77**). C-94 records why it reuses the write key. + """ + store = DatastoreModule(appwrite_file_manager_config=_partner_appwrite_config(model_path)) + store.model_path = None + return store + + +def _assert_delivery_is_findable(model_path, consumer_name: str, uploaded: dict) -> None: + """C-94: ask the store the question the consumer asks, and refuse silence. + + A function, not a method (C-40 (a)) — its refusal is observable without a manager + or an Appwrite environment. ``uploaded`` maps each leg to the file id THIS run put + there; `delivery/findability.py` carries why that scoping is the whole guard. + """ + for category, expected in uploaded.items(): + try: + port = _ContractStorePort(_build_partner_read_store(model_path)) + found = port.latest_file_id({"name": consumer_name, "category": category}) + except Exception as exc: # could not ask != asked and got nothing (C-99, C-103) + raise findability.unverified(category, exc) from exc + findability.assert_findable( + found, expected_file_id=expected, consumer_name=consumer_name, category=category + ) + logger.info("Findability preflight passed: both legs retrievable under %r.", consumer_name) + + class UNFAOPostProcessorManager(PostprocessorManager, ForecastingModelManager): def __init__( self, @@ -136,8 +166,18 @@ def _read_historical_frame(self): ) try: lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) + except source_metadata.ProducerClientUnavailable: + # NOT degrade-open. A producer client that will not load is a broken + # environment, not a producer that publishes no boundary — and the whole + # point of the two branches is that they are different conditions (C-103). + raise except Exception: - logger.warning("last_valid_month_id unavailable; skipping clip (degrade-open, C-26).", exc_info=True) + logger.warning( + "last_valid_month_id could not be read; skipping the observed-range " + "clip (degrade-open, C-26). Any unobserved months above the producer's " + "boundary WILL ship as observed history in this delivery.", + exc_info=True, + ) lv = None if lv is None: self._historical_frame = frame @@ -322,7 +362,7 @@ def _save_contract(self) -> dict: Path(summary["staging_dir"]), lookup ) if upload_enabled: - store.upload( + hist_file_id = store.upload( hist_path, filename=hist_path.name, # The DECLARED consumer name, not `self._model_path.model_name` @@ -341,6 +381,13 @@ def _save_contract(self) -> dict: description=hist_description, ) logger.info("uploaded %s (historical, run %s)", hist_path.name, summary["run_id"]) + # C-94: nothing above observes the OUTCOME of an upload. Every call + # reported success in run-0 too, and the historical leg still stranded. + _assert_delivery_is_findable( + self._model_path, + product.CONSUMER_DOCUMENT_NAME, + {"forecast": summary["manifest_file_id"], "historical": hist_file_id}, + ) else: logger.info( "Interlock holding: historical artifact staged at %s (no store calls).", diff --git a/views_postprocessing/unfao/store_port.py b/views_postprocessing/unfao/store_port.py index cda03ff..b0ee757 100644 --- a/views_postprocessing/unfao/store_port.py +++ b/views_postprocessing/unfao/store_port.py @@ -63,7 +63,7 @@ def download(self, file_id: str) -> bytes: "these by name and cannot tell a failed download from an empty artifact." ) - def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, description=None) -> None: + def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, description=None) -> str | None: result = self._dsm.upload_data( file=file_path, filename=filename, @@ -98,3 +98,9 @@ def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, f"without a metadata document): {error}. The store reported " f"success={success!r} (result type {type(result).__name__})." ) + # The uploaded file's id, so the C-94 read-back can be scoped to THIS run. + # Discarding it (as this did until 2026-08-19) makes the only available check + # "is there any document under the consumer's name", which the previous run + # already satisfies — so the guard could never fail from delivery 2 onward. + data = getattr(result, "data", None) + return data.get("file_id") if isinstance(data, dict) else None