diff --git a/docs/ADRs/012_revised_ontology.md b/docs/ADRs/012_revised_ontology.md index 3c66894..1aa567d 100644 --- a/docs/ADRs/012_revised_ontology.md +++ b/docs/ADRs/012_revised_ontology.md @@ -35,7 +35,7 @@ clearly belong to one is out of scope and must be redesigned or rejected. | Category | Purpose | Authority | Stability | |----------|---------|-----------|-----------| -| **Delivery Invariants** | Representation-free rules over primitives that a delivery must satisfy (coverage, forecast identity, observed-range, provenance). Live in `views_postprocessing/delivery/`. | Authoritative — they define what a valid delivery is | Stable — changes are governance decisions | +| **Delivery Invariants** | Representation-free rules over primitives that a delivery must satisfy (coverage, no-collapse, gid parity, observed-range, provenance). Live in `views_postprocessing/delivery/`. *Forecast identity was one of these until 2026-07-31 — see the amendment below.* | Authoritative — they define what a valid delivery is | Stable — changes are governance decisions | | **Representation Seam** | The single pandas-aware module (`unfao/extraction.py`) that turns the delivery's external representation into the primitives the invariants consume. | Derived — isolates the representation so invariants stay representation-free | Evolving — the one place a representation change (e.g. pandas → frames, C-40) lands | | **Enrichment Asset + Engine** | The precomputed GAUL lookup (`data/gaul_lookup.parquet`) and the merge that joins it (`GaulLookupEnricher`). | Authoritative for geographic metadata | Stable — the lookup is rebuilt only when the producer (datafactory) releases new GAUL data | | **Pipeline Manager** | The thin `UNFAOPostProcessorManager` — a concrete pipeline-core postprocessor (Template-Method subclass) that *orchestrates* read/transform/validate/save and **calls** the invariants (never inherits them). | Derived — implements delivery using the categories above | Evolving — changes as partner requirements change | @@ -81,3 +81,38 @@ Seam" wording will need a light touch (the seam stays; its internals change). - Stability is a design constraint, not a preference (inherited from ADR-001 §Stability Rules). - Dependency direction and cross-repo topology are governed by [ADR-002](002_topology_and_dependency_rules.md). - Class-level contracts are governed by ADR-006 (see `docs/CICs/`). + +--- + +## Amendment 2026-07-31 — forecast identity re-homed to the wire layer (#150, epic #148) + +**`views_postprocessing/delivery/identity.py` is retired.** The "Delivery Invariants" row +above listed *forecast identity* among the authoritative rules; that module no longer +exists, and this amendment records where the rule went so a reader of the row is not +looking for deleted code. + +**The rule is not weaker — it is enforced from better evidence.** The retired +`assert_forecast_identity` compared one selected store document's `name`/`loa` against the +configured ensemble. Since the ADR-013 contract path became the only path (#149), the same +guarantee is enforced in `unfao/wire/source_selection.py:73-81`: `TargetLease.load()` +checks **every shard header's declared `provenance.ensemble`** against the launched +ensemble, and refuses the run on a mismatch. Identity now comes from the artifact's own +declared content rather than from a metadata field on a single document, and it is checked +per shard rather than once per run. + +**Why it was retired rather than kept.** Its only caller was the legacy reader deleted in +#149. An invariant module that nothing calls is documentation shaped like code — and +`delivery/` is precisely the package the coming clones (views-crafdapi, +views-productionapi) will copy wholesale, so an unenforced rule there would be inherited as +if it were live. Register **C-64** existed to force this decision rather than let the +deletion happen silently as a side effect of #149. + +**Consequence for register C-25** ("forecast input selected by category-only filter — +newest file wins regardless of producer"): its recorded mitigation *was* this function, so +on paper the entry loses its guard. In fact its hazard is now structurally impossible — the +contract path selects by **run manifest**, a commit marker with hash-verified contents, so +"newest upload wins" is not a thing the selection can do. C-25 is closed as **superseded by +mechanism**, not as *mitigated*. + +**Coverage:** `tests/test_wire_source_selection.py::test_wrong_declared_ensemble_refuses_at_load`. + diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 548485c..843cba8 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -6,8 +6,8 @@ | Owner | Dylan Pinheiro / PRIO MD&D Team | | Last Updated | 2026-07-31 | | Total Concerns | 71 | -| Open Concerns | 34 | -| Resolved Concerns | 37 | +| Open Concerns | 32 | +| Resolved Concerns | 39 | --- @@ -215,28 +215,6 @@ See also C-17 (RESOLVED — implicit column naming between mapper and manager), --- -### C-25: Forecast input selected by category-only filter — newest file wins regardless of producer - -| Field | Value | -|-------|-------| -| ID | C-25 | -| Tier | 2 | -| Source | `cross-repo-investigation` (2026-06-12) | -| Trigger | When any new producer uploads to the prod_forecasts bucket with `category: "forecast"`, verify the postprocessor still picks up the intended ensemble's file — selection is newest-`$createdAt`-wins with no loa, model-name, or run-id filter | -| Location | `views_postprocessing/unfao/managers/unfao.py:247` (legacy selection, `LEGACY_FORECAST_FILTERS` at `:34`), `:260` (identity assertion); contract path selects by manifest instead — `unfao/wire/source_selection.py:39`; views-pipeline-core `modules/datastore/datastore.py:475-511` | - -`_read_forecast_data()` calls `download_latest_file(filters={"category": "forecast"})`. "Latest" is resolved by sorting metadata documents on `$createdAt` descending and taking the first (datastore.py:475-511). There is no filter on `loa`, `name`, `targets`, or any run identifier. Today only the production ensemble uploads with this category, so the newest file is the right file by circumstance, not by contract. If a second model, a test run, or a backfill ever uploads to the same bucket with `category: "forecast"`, the postprocessor silently enriches and ships the wrong predictions to FAO. The same single-filter pattern exists downstream: views-faoapi selects from the unfao_bucket by category only (views-faoapi `api.py:488`), so a stray upload there reaches FAO directly. Compounding factor: Appwrite has no retention — every historical upload remains a candidate forever; correctness depends entirely on upload discipline. - -This concern became visible during the cross-repo investigation (`docs/cross_repo_integration_report.md` §2.4, §4.2); it was previously implicit in the C-13 narrative (timeouts) but is a distinct failure mode: C-13 is "the call hangs," C-25 is "the call succeeds with the wrong file." - -**Mitigation landed (S3, 2026-06-26, `sprint/fao-input-integrity`):** `_read_forecast_data` now resolves the file id, fetches its metadata, and asserts identity (`delivery/identity.assert_forecast_identity`) against the configured ensemble (`{name: ensemble_path_manager.model_name, loa: "pgm"}`) **before** download — a stray `category="forecast"` upload now fails loud instead of shipping silently. **Residual (verify before relying on it):** the guard assumes the producer's uploaded `name`/`loa` equal `model_name`/`"pgm"`; this is checked against pipeline-core's code but **not a live Appwrite upload**. If the contract differs, the guard fails loud on *every* run — caught at the first smoke-test delivery (option A / S6 #57), **not** silently — so the residual is an availability / false-positive risk, not a corruption one. Confirm the field match at the option-A run; until then C-25 stays open. - -**Update 2026-07-31 (`repo-assimilation`) — the registered mitigation is LEGACY-ONLY; the live path is protected differently.** The identity assertion recorded above (`delivery/identity.assert_forecast_identity`) is called at `unfao.py:260`, inside the **legacy** `_read_forecast_data` leg. Production runs the contract path, which selects by **run manifest** (`unfao/wire/source_selection.py`) and never calls it. That is a *stronger* guarantee — a manifest is a commit marker with hash-verified contents, not a metadata field match — so the concern is better mitigated than this entry states, but **not by the mechanism this entry names**. Re-read accordingly; and note the mitigation's stated residual ("confirm the field match at the option-A run") is now moot for the live path. See **C-64**. - -See also C-13 (no timeout on the same calls), C-15 (upload metadata lacks provenance to detect this downstream), C-43 (the same "verify at the first live run" debt pattern on the enrichment swap). - ---- - ### C-26: Unconditional fillna(0.0) fabricates "no conflict" from missing upstream data | Field | Value | @@ -679,24 +657,6 @@ Cross-refs: **C-40** (the manager this lives in), **#145** (the retired path's s --- -### C-64: `delivery/identity.py` is an authoritative delivery invariant that production never calls - -| Field | Value | -|-------|-------| -| ID | C-64 | -| Tier | 3 | -| Source | `repo-assimilation` (2026-07-31) | -| Trigger | When **views-crafdapi**/**views-productionapi** copy `delivery/` as the reusable invariant core — verify whether the identity rule is enforced on the path the clone actually uses, or inherited as documentation only | -| Location | `views_postprocessing/delivery/identity.py` (53 lines); sole caller `views_postprocessing/unfao/managers/unfao.py:260` — inside the **legacy** `_read_forecast_data` leg; `docs/ADRs/012_revised_ontology.md` (lists it under "Delivery Invariants — Authoritative") | - -`assert_forecast_identity` verifies that a selected forecast document's `name`/`loa` match the configured ensemble before download. It is reachable **only** from the legacy reader. The contract path selects by **manifest** (`wire/source_selection.py`) and never consults it — which is a *stronger* guarantee, not a gap. The risk is the mismatch between ADR-012 naming it authoritative and the code never running it: a clone inherits a rule that looks enforced and is not. - -**This also updates C-25's status** — that entry's registered mitigation *is* this function, so the mitigation applies to the legacy path only. C-25's actual protection on the live path is manifest selection, which is why the entry should be re-read rather than assumed still-mitigated-as-written. - -Cross-refs: **C-25** (whose mitigation this is), **C-67** (the ADR-012 drift this instantiates), **Cluster L**. - ---- - ### C-65: `unfao/extraction.py` is four-fifths unreachable and duplicates `frame_extraction.py` | Field | Value | @@ -898,6 +858,50 @@ See also C-40 (the inheritance/representation coupling this migration unwinds), ## Resolved Concerns +### C-25: Forecast input selected by category-only filter — newest file wins regardless of producer — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-25 | +| Resolved | 2026-07-31 | +| Resolution | **Superseded by mechanism (#150, epic #148) — not merely mitigated.** This entry's hazard was "newest `category=forecast` upload wins regardless of producer". Its recorded mitigation was `delivery.identity.assert_forecast_identity`, which #150 retired. The hazard nonetheless cannot occur: since #149 the contract path is the only path, and it selects by **run manifest** — a commit marker whose contents are hash-verified — so recency-based selection is not an operation the code can perform. Declared identity is additionally checked per shard header at `wire/source_selection.py:73-81`. Closed as structurally impossible rather than guarded. | +| Tier | 2 | +| Source | `cross-repo-investigation` (2026-06-12) | +| Trigger | When any new producer uploads to the prod_forecasts bucket with `category: "forecast"`, verify the postprocessor still picks up the intended ensemble's file — selection is newest-`$createdAt`-wins with no loa, model-name, or run-id filter | +| Location | `views_postprocessing/unfao/managers/unfao.py:247` (legacy selection, `LEGACY_FORECAST_FILTERS` at `:34`), `:260` (identity assertion); contract path selects by manifest instead — `unfao/wire/source_selection.py:39`; views-pipeline-core `modules/datastore/datastore.py:475-511` | + +`_read_forecast_data()` calls `download_latest_file(filters={"category": "forecast"})`. "Latest" is resolved by sorting metadata documents on `$createdAt` descending and taking the first (datastore.py:475-511). There is no filter on `loa`, `name`, `targets`, or any run identifier. Today only the production ensemble uploads with this category, so the newest file is the right file by circumstance, not by contract. If a second model, a test run, or a backfill ever uploads to the same bucket with `category: "forecast"`, the postprocessor silently enriches and ships the wrong predictions to FAO. The same single-filter pattern exists downstream: views-faoapi selects from the unfao_bucket by category only (views-faoapi `api.py:488`), so a stray upload there reaches FAO directly. Compounding factor: Appwrite has no retention — every historical upload remains a candidate forever; correctness depends entirely on upload discipline. + +This concern became visible during the cross-repo investigation (`docs/cross_repo_integration_report.md` §2.4, §4.2); it was previously implicit in the C-13 narrative (timeouts) but is a distinct failure mode: C-13 is "the call hangs," C-25 is "the call succeeds with the wrong file." + +**Mitigation landed (S3, 2026-06-26, `sprint/fao-input-integrity`):** `_read_forecast_data` now resolves the file id, fetches its metadata, and asserts identity (`delivery/identity.assert_forecast_identity`) against the configured ensemble (`{name: ensemble_path_manager.model_name, loa: "pgm"}`) **before** download — a stray `category="forecast"` upload now fails loud instead of shipping silently. **Residual (verify before relying on it):** the guard assumes the producer's uploaded `name`/`loa` equal `model_name`/`"pgm"`; this is checked against pipeline-core's code but **not a live Appwrite upload**. If the contract differs, the guard fails loud on *every* run — caught at the first smoke-test delivery (option A / S6 #57), **not** silently — so the residual is an availability / false-positive risk, not a corruption one. Confirm the field match at the option-A run; until then C-25 stays open. + +**Update 2026-07-31 (`repo-assimilation`) — the registered mitigation is LEGACY-ONLY; the live path is protected differently.** The identity assertion recorded above (`delivery/identity.assert_forecast_identity`) is called at `unfao.py:260`, inside the **legacy** `_read_forecast_data` leg. Production runs the contract path, which selects by **run manifest** (`unfao/wire/source_selection.py`) and never calls it. That is a *stronger* guarantee — a manifest is a commit marker with hash-verified contents, not a metadata field match — so the concern is better mitigated than this entry states, but **not by the mechanism this entry names**. Re-read accordingly; and note the mitigation's stated residual ("confirm the field match at the option-A run") is now moot for the live path. See **C-64**. + +See also C-13 (no timeout on the same calls), C-15 (upload metadata lacks provenance to detect this downstream), C-43 (the same "verify at the first live run" debt pattern on the enrichment swap). + +--- + +### C-64: `delivery/identity.py` is an authoritative delivery invariant that production never calls — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-64 | +| Resolved | 2026-07-31 | +| Resolution | **Retired, by decision rather than by side effect (#150, epic #148).** `delivery/identity.py` was deleted along with `tests/test_identity.py` and the two S3 cases in `test_input_integrity_e2e.py`. The rule survives in a stronger form: `unfao/wire/source_selection.py:73-81` checks **every shard header's declared `provenance.ensemble`** against the launched ensemble inside `TargetLease.load()`, so identity is established from the artifact's own content rather than one store document's metadata field, and per shard rather than once per run. Covered by `test_wire_source_selection.py::test_wrong_declared_ensemble_refuses_at_load`. Recorded as a dated amendment to ADR-012, whose "Delivery Invariants" row had listed forecast identity as authoritative — the entry existed precisely to stop that row silently describing deleted code. | +| Tier | 3 | +| Source | `repo-assimilation` (2026-07-31) | +| Trigger | When **views-crafdapi**/**views-productionapi** copy `delivery/` as the reusable invariant core — verify whether the identity rule is enforced on the path the clone actually uses, or inherited as documentation only | +| Location | `views_postprocessing/delivery/identity.py` (53 lines); sole caller `views_postprocessing/unfao/managers/unfao.py:260` — inside the **legacy** `_read_forecast_data` leg; `docs/ADRs/012_revised_ontology.md` (lists it under "Delivery Invariants — Authoritative") | + +`assert_forecast_identity` verifies that a selected forecast document's `name`/`loa` match the configured ensemble before download. It is reachable **only** from the legacy reader. The contract path selects by **manifest** (`wire/source_selection.py`) and never consults it — which is a *stronger* guarantee, not a gap. The risk is the mismatch between ADR-012 naming it authoritative and the code never running it: a clone inherits a rule that looks enforced and is not. + +**This also updates C-25's status** — that entry's registered mitigation *is* this function, so the mitigation applies to the legacy path only. C-25's actual protection on the live path is manifest selection, which is why the entry should be re-read rather than assumed still-mitigated-as-written. + +Cross-refs: **C-25** (whose mitigation this is), **C-67** (the ADR-012 drift this instantiates), **Cluster L**. + +--- + ### C-56: Run-0 was OOM-killed at 23.8 GB — the pandas delivery path could not fit global volume on the 31 GB host — RESOLVED | Field | Value | diff --git a/tests/test_identity.py b/tests/test_identity.py deleted file mode 100644 index bcef518..0000000 --- a/tests/test_identity.py +++ /dev/null @@ -1,46 +0,0 @@ -"""Unit tests for the representation-free forecast-identity invariant (S3 / C-25).""" - -import pytest - -from views_postprocessing.delivery.identity import ( - ForecastIdentityError, - assert_forecast_identity, -) - - -def test_matching_identity_passes(): - selected = {"name": "fatalities_ensemble", "loa": "pgm", "category": "forecast"} - expected = {"name": "fatalities_ensemble", "loa": "pgm"} - assert_forecast_identity(selected, expected) # no raise - - -def test_name_mismatch_fails_loud_naming_expected_and_found(): - selected = {"name": "stray_model", "loa": "pgm"} - expected = {"name": "fatalities_ensemble", "loa": "pgm"} - with pytest.raises(ForecastIdentityError) as exc: - assert_forecast_identity(selected, expected) - msg = str(exc.value) - assert "name" in msg - assert "fatalities_ensemble" in msg # expected - assert "stray_model" in msg # found - - -def test_loa_mismatch_raises(): - with pytest.raises(ForecastIdentityError): - assert_forecast_identity({"name": "m", "loa": "cm"}, {"name": "m", "loa": "pgm"}) - - -def test_missing_key_is_treated_as_mismatch(): - with pytest.raises(ForecastIdentityError): - assert_forecast_identity({"loa": "pgm"}, {"name": "m", "loa": "pgm"}) - - -def test_only_expected_keys_are_checked(): - # extra metadata on `selected` is ignored — the caller decides what identity means. - selected = {"name": "m", "loa": "pgm", "category": "forecast", "targets": ["a"]} - assert_forecast_identity(selected, {"name": "m"}) # no raise - - -def test_label_appears_in_message(): - with pytest.raises(ForecastIdentityError, match="historical"): - assert_forecast_identity({"name": "x"}, {"name": "y"}, label="historical") diff --git a/tests/test_input_integrity_e2e.py b/tests/test_input_integrity_e2e.py index a2ad4fd..9c7db1c 100644 --- a/tests/test_input_integrity_e2e.py +++ b/tests/test_input_integrity_e2e.py @@ -13,8 +13,12 @@ S1 coverage _check_coverage cells_of -> assert_complete_coverage S2 observed range _read_historical_frame months_of -> fabricated_months -> drop_months_above - S3 forecast identity (RETIRED with the legacy reader, #149 — selection is by run - manifest; `delivery/identity.py` itself is retired in #150) + S3 forecast identity RE-HOMED, not lost (#150). The rule — "the delivery must + refuse a forecast that is not the launched ensemble's" — now + lives in `wire/source_selection.py:73-81`, checked per shard + header against the DECLARED provenance rather than one + document's metadata field. Covered by + `test_wire_source_selection.py::test_wrong_declared_ensemble_refuses_at_load`. S4 land_gaul _check_coverage cells_of -> assert_no_excluded_cells S5 provenance _historical_frame_description build_provenance(<- cells_of/unmapped_cell_count) """ @@ -24,7 +28,7 @@ import pandas as pd import pytest -from views_postprocessing.delivery import coverage, identity, observed_range, provenance +from views_postprocessing.delivery import coverage, observed_range, provenance from views_postprocessing.unfao import extraction from views_postprocessing.unfao.gaul_schema import METADATA_COLS @@ -84,20 +88,16 @@ def test_s2_month_beyond_boundary_is_not_delivered_as_zero(): assert 102 not in delivered and 103 not in delivered # not shipped as observed zero -# S3 — forecast identity (C-25) ----------------------------------------------------- -def test_s3_decoy_forecast_file_is_rejected_through_seam(): - decoy = _FakeMetaResult({"name": "stray_model", "loa": "pgm", "category": "forecast"}) - selected = extraction.file_metadata(decoy) - expected = {"name": "fatalities_ensemble", "loa": "pgm"} - with pytest.raises(identity.ForecastIdentityError, match="stray_model"): - identity.assert_forecast_identity(selected, expected) - - -def test_s3_matching_forecast_file_passes_through_seam(): - good = _FakeMetaResult({"name": "fatalities_ensemble", "loa": "pgm", "category": "forecast"}) - selected = extraction.file_metadata(good) - expected = {"name": "fatalities_ensemble", "loa": "pgm"} - assert identity.assert_forecast_identity(selected, expected) is None +# S3 — forecast identity: RE-HOMED to the wire layer (#150) ------------------------- +# The invariant is not gone and is not weaker. It moved to where the evidence is: +# `TargetLease.load()` checks EVERY shard header's declared `provenance.ensemble` +# against the launched ensemble (`wire/source_selection.py:73-81`), so identity is +# established from the artifact's own content rather than from one store document's +# metadata field. Exercised by +# `test_wire_source_selection.py::test_wrong_declared_ensemble_refuses_at_load`. +# +# `extraction.file_metadata` — the seam these tests used — survives and is still +# covered by `test_extraction.py`. # S4 — land_gaul exclusions (C-30) -------------------------------------------------- diff --git a/views_postprocessing/delivery/identity.py b/views_postprocessing/delivery/identity.py deleted file mode 100644 index d05b0d2..0000000 --- a/views_postprocessing/delivery/identity.py +++ /dev/null @@ -1,53 +0,0 @@ -"""Forecast-file identity invariant: deliver the forecast we asked for, not whatever -landed last (register C-25, S3). - -Representation-free — primitives only (plain dicts of file metadata). No pandas, no -datastore types. The manager feeds it the selected file's identity via -``views_postprocessing/unfao/extraction.py``; the rule lives here. - -Why this matters: the FAO forecast is selected from the prediction store by ``category`` -alone (newest-wins) — no filter on ensemble name or level. Today only the production -ensemble uploads with ``category="forecast"``, so the newest file is the right one *by -circumstance, not by contract*. This guard fails loud if the selected file's identity -does not match the configured ensemble, so a stray upload (a test run, a second model, a -backfill) can never be silently shipped to the partner. -""" - -from __future__ import annotations - - -class ForecastIdentityError(ValueError): - """The selected forecast file does not match the requested ensemble identity.""" - - -def assert_forecast_identity( - selected: dict, expected: dict, *, label: str = "forecast" -) -> None: - """Raise unless ``selected`` matches ``expected`` on every key ``expected`` names. - - Only the keys present in ``expected`` are checked (e.g. ``{"name", "loa"}``); any other - metadata on ``selected`` (targets, category, timestamps) is ignored. This keeps the - caller in control of what identity means without the invariant guessing. - - Args: - selected: metadata of the file actually selected for delivery. - expected: the identity the configured ensemble requires; each key must match. - label: short tag for the message (e.g. ``"forecast"``). - - Raises: - ForecastIdentityError: naming every mismatched key, expected vs found. - """ - mismatches = { - key: (expected[key], selected.get(key)) - for key in expected - if selected.get(key) != expected[key] - } - if mismatches: - detail = ", ".join( - f"{key}: expected {exp!r}, found {got!r}" - for key, (exp, got) in mismatches.items() - ) - raise ForecastIdentityError( - f"{label}: identity mismatch — {detail}. A stray upload to the prediction " - f"store would otherwise ship the wrong predictions to the partner." - )