From 08b92223043c173d744e6eb93511b708108976e6 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 17 Aug 2026 11:29:29 +0200 Subject: [PATCH 1/3] fix(source-metadata): a missing producer client refuses; it is not "no boundary" (C-103) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-103 was filed on an unverified premise. Verifying it moved the entry down a tier and changed what the fix should be, so both are recorded here. WHAT THE VERIFICATION FOUND. The question was whether production supplies `datafactory_query`. It does, twice: - views-models postprocessors/{un_fao,un_crafd}/requirements.txt both pin views-datafactory>=1.9.0,<2.0.0, which is what ships the module. There is no separate distribution — `pip download datafactory-query` finds nothing; it is one of nine packages in views-datafactory's wheel. - config_queryset.py imports datafactory_query.defaults at MODULE SCOPE and raises naming the fix. A missing client makes get_queryset() return None, which launch_config.assert_queryset_was_importable refuses (C-83) before _read_historical_frame runs. So the headline scenario — a missing dependency silently shipping fabricated months on the live path — cannot occur. It was already guarded, by a check written for something else. C-103 drops 2 -> 3. WHAT IS ACTUALLY WRONG, AND IS FIXED HERE. The `except Exception` was never only about the import. It made "the client is missing" and "the producer publishes no boundary" the same event. The second is a normal older store and degrading open is the recorded C-26 decision; the first is a broken environment, and degrading open there ships the unobserved zero-padded tail as observed history on one WARNING. C-60 is the same shape: a provenance stamp that degraded to "unknown" on a bare except and made every delivery untraceable in the one field it existed to answer. - source_metadata raises ProducerClientMissing on ImportError, logged AND raised (ADR-008), with a message naming the package, the install command, and what degrading open would have cost. - both managers re-raise it ahead of the broad except. - the broad degrade-open is deliberately UNCHANGED. Narrowing it is a decision about what to tell the partner when the boundary cannot be read, not a refactor, and it is what keeps C-103 open. TESTS. contract/source_metadata.py had NONE — which is how "return None like everything else" ever looked reasonable. tests/test_source_metadata.py adds 7, mutation-proven both ways: reverting the raise to `return None` fails 4 of them; collapsing the manager's two branches into one fails the source check that pins the ordering. That check is a source read rather than a behavioural one on purpose — constructing a manager needs pipeline-core, a path manager and an Appwrite environment (C-40), and the property worth holding is one line. Suite: 439 passed, 1 skipped, 39 xfailed locally (25 failures are the C-104 venv drift, unchanged). ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 15 ++- tests/test_source_metadata.py | 118 ++++++++++++++++++ .../contract/source_metadata.py | 46 ++++++- views_postprocessing/crafd/managers/crafd.py | 12 +- views_postprocessing/unfao/managers/unfao.py | 12 +- 5 files changed, 195 insertions(+), 8 deletions(-) create mode 100644 tests/test_source_metadata.py diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 571a59e..3013b10 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -1064,15 +1064,24 @@ Recorded rather than left implicit because "the operator did a console session" | Field | Value | |-------|-------| | ID | C-103 | -| Tier | 2 — structural fragility with a named scenario: the guard's dependency is absent, the absence is caught by a bare `except Exception`, and the delivery proceeds unclipped. Held at 2 rather than 1 because of a gap, not a judgement — see the verification question below. | +| 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 the launcher's environment is next built or changed — a views-models deploy, a container rebuild, a `poetry install` on the delivery host — verify `import datafactory_query` succeeds there. The delivery will not tell you if it does not. | +| 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:37`; `views_postprocessing/unfao/managers/unfao.py:137-142`, `views_postprocessing/crafd/managers/crafd.py:137-142`; `pyproject.toml` (the dependency is absent) | `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. -**Verification question, stated as a gap rather than dressed as a finding.** Whether the production launcher's environment supplies `datafactory_query` could not be established from this repository; views-models builds that environment. If it does, this is a declaration gap and the swallow is the whole risk. If it does not, C-26's fabrication has been shipping unclipped since #126. **Do not downgrade on inspection of this repo alone** — the instruction C-26 already carries, for the same reason. +**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 `ProducerClientMissing` 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. diff --git a/tests/test_source_metadata.py b/tests/test_source_metadata.py new file mode 100644 index 0000000..5822187 --- /dev/null +++ b/tests/test_source_metadata.py @@ -0,0 +1,118 @@ +"""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 +from pathlib import Path + +import pytest + +from views_postprocessing.contract import source_metadata + +_REPO = Path(__file__).resolve().parent.parent + + +def test_a_missing_client_raises_rather_than_reporting_no_boundary(): + """``datafactory_query`` is genuinely absent here, so this needs no simulation.""" + with pytest.raises(source_metadata.ProducerClientMissing): + source_metadata.last_valid_month_id() + + +def test_the_refusal_names_the_package_and_why_it_is_not_None(): + with pytest.raises(source_metadata.ProducerClientMissing) as excinfo: + source_metadata.last_valid_month_id() + message = str(excinfo.value) + assert "views-datafactory" in message, "the refusal must name the package to install" + assert "datafactory_query" in message, "and the module that was actually missing" + # 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_the_refusal_chains_the_original_importerror(): + """``raise ... from exc``: the traceback must still say what could not be imported.""" + with pytest.raises(source_metadata.ProducerClientMissing) as excinfo: + source_metadata.last_valid_month_id() + assert isinstance(excinfo.value.__cause__, ImportError) + + +def test_it_is_logged_as_well_as_raised(caplog): + """ADR-008: a refusal on the delivery path is logged persistently AND raised.""" + with caplog.at_level("ERROR"), pytest.raises(source_metadata.ProducerClientMissing): + 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", ("unfao", "crafd")) +def test_the_managers_do_not_swallow_the_refusal(partner): + """The degrade-open must not re-absorb what this module just refused. + + 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 ``ProducerClientMissing`` 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() + assert "except source_metadata.ProducerClientMissing:" in source, ( + f"{partner}'s manager no longer re-raises ProducerClientMissing, so a broken " + "environment is once again indistinguishable from a producer that publishes " + "no boundary — and the delivery ships fabricated months either way (C-103)." + ) + refusal = source.index("except source_metadata.ProducerClientMissing:") + broad = source.index("except Exception:", refusal - 2000 if refusal > 2000 else 0) + assert refusal < broad, ( + f"{partner}'s manager catches Exception before ProducerClientMissing, so the " + "narrow branch is unreachable" + ) diff --git a/views_postprocessing/contract/source_metadata.py b/views_postprocessing/contract/source_metadata.py index 49b163a..4eed8d5 100644 --- a/views_postprocessing/contract/source_metadata.py +++ b/views_postprocessing/contract/source_metadata.py @@ -20,6 +20,10 @@ logger = logging.getLogger(__name__) +class ProducerClientMissing(RuntimeError): + """``datafactory_query`` is not importable, so no producer fact can be read.""" + + 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 +35,45 @@ 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: + ProducerClientMissing: if ``datafactory_query`` cannot be imported. + + **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 the absence 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. But a + *missing client* 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 ImportError as exc: + err_msg = ( + "datafactory_query is not importable, so the producer's " + "last_valid_month_id cannot be read. 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'. Refusing rather " + "than reporting 'no boundary published', which is a different condition and " + "would let unobserved months ship as observed history (C-103, C-26)." + ) + logger.error(err_msg) # ADR-008: logged persistently AND raised + raise ProducerClientMissing(err_msg) from exc return get_last_valid_month_id(zarr_url) diff --git a/views_postprocessing/crafd/managers/crafd.py b/views_postprocessing/crafd/managers/crafd.py index efb6152..9ba7c26 100644 --- a/views_postprocessing/crafd/managers/crafd.py +++ b/views_postprocessing/crafd/managers/crafd.py @@ -136,8 +136,18 @@ def _read_historical_frame(self): ) try: lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) + except source_metadata.ProducerClientMissing: + # NOT degrade-open. A missing producer client 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 diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index 3b727f8..700b5b5 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -136,8 +136,18 @@ def _read_historical_frame(self): ) try: lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) + except source_metadata.ProducerClientMissing: + # NOT degrade-open. A missing producer client 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 From 24d9f7a8b34decc94fb121087da8d8afed846ca4 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 17 Aug 2026 11:32:41 +0200 Subject: [PATCH 2/3] =?UTF-8?q?fix(tests,docs):=20/review-diff=20findings?= =?UTF-8?q?=20=E2=80=94=20enumerate=20partners,=20update=20the=20CIC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings on the previous commit. 1. The new manager check hardcoded ("unfao", "crafd") in its parametrize, when `tests/conftest.py::PARTNER_PACKAGES` exists for exactly this and says why: eight guards once named "unfao" literally, and all eight went on passing over `crafd/` when it landed. This would have been the ninth. Now parametrized over the declared list, like `test_store_construction`. 2. `docs/CICs/UNFAOPostProcessorManager.md:96` described `_read_historical_frame` as degrading open "if the boundary cannot be resolved" — true before this branch, and now only half true. It records both outcomes: degrade-open when the producer publishes no boundary or the read fails, refusal when the client cannot be imported. Review date moved to match its own content, per `test_doc_accuracy::test_a_cic_review_date_is_not_older_than_its_own_content`. Also simplified the ordering assertion in the same check. It searched for the broad `except Exception:` from an offset before the refusal, which was fragile and obscured that each manager has exactly one. It now asserts that count and searches forward — re-mutation-proven: removing the narrow branch still fails it, reordering the two branches still fails it. Suite: 439 passed, 3 skipped, 37 xfailed. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- docs/CICs/UNFAOPostProcessorManager.md | 4 ++-- tests/test_source_metadata.py | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/docs/CICs/UNFAOPostProcessorManager.md b/docs/CICs/UNFAOPostProcessorManager.md index e98b6d2..15cab32 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-17 **Related ADRs:** ADR-001, ADR-002, ADR-008, ADR-009 --- @@ -93,7 +93,7 @@ Assumptions that are not met **must cause failure**, not fallback behavior. The - **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.ProducerClientMissing`), 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/tests/test_source_metadata.py b/tests/test_source_metadata.py index 5822187..77e6226 100644 --- a/tests/test_source_metadata.py +++ b/tests/test_source_metadata.py @@ -24,6 +24,7 @@ import pytest +from tests.conftest import PARTNER_PACKAGES from views_postprocessing.contract import source_metadata _REPO = Path(__file__).resolve().parent.parent @@ -94,10 +95,14 @@ def get_last_valid_month_id(zarr_url=None, _answer=answer): ) -@pytest.mark.parametrize("partner", ("unfao", "crafd")) +@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 ``ProducerClientMissing`` is re-raised @@ -110,8 +115,14 @@ def test_the_managers_do_not_swallow_the_refusal(partner): "environment is once again indistinguishable from a producer that publishes " "no boundary — and the delivery ships fabricated months either way (C-103)." ) + # Each manager has exactly one broad `except Exception:` (the degrade-open), so a + # plain forward search is enough — searching from an offset would only obscure that. + assert source.count("except Exception:") == 1, ( + f"{partner}'s manager grew a second broad except; this check assumes one and " + "would compare against the wrong branch" + ) refusal = source.index("except source_metadata.ProducerClientMissing:") - broad = source.index("except Exception:", refusal - 2000 if refusal > 2000 else 0) + broad = source.index("except Exception:") assert refusal < broad, ( f"{partner}'s manager catches Exception before ProducerClientMissing, so the " "narrow branch is unreachable" From 175ef8e332616ebf87b074d0ad4b9201f31306b2 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 17 Aug 2026 11:40:41 +0200 Subject: [PATCH 3/3] fix(source-metadata): catch what actually fails, and prove it deterministically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from /code-review high. The first two matter. 1. MEDIUM — the guard caught `ImportError`, and the failure this environment has actually had is not one. views-models `postprocessors/un_fao/requirements.txt` records, dated 2026-08-13, that resolving numpy 2.x into the shared envs/views-postprocessing prefix makes the import die with `ValueError: numpy.dtype size changed ... Expected 96 from C header, got 88`, found by the pre-delivery rehearsal. An ImportError-only clause lets that sail into the caller's degrade-open and ship the unobserved tail — the exact case this guard exists to separate, and the likeliest one. Now catches Exception. 2. MEDIUM — four of the tests proved the guard by ambient accident: they relied on `datafactory_query` happening to be absent in this venv, so in the launcher prefix (where it IS installed) all four would have failed. That is the C-30/C-46 shape a day after fixing it. Both states are now simulated deterministically: an `absent_client` fixture, and an `exploding_client` one that reproduces the numpy ABI ValueError. 3. LOW — the message asserted a diagnosis. An ImportError raised INSIDE an installed package was reported as "not importable ... pip install views-datafactory", sending the operator to a fix already applied. It now distinguishes `ModuleNotFoundError` naming datafactory_query ("not installed", with the install line) from anything else ("present but raised while loading", quoting the error, and "Do NOT reinstall"). `ProducerClientMissing` -> `ProducerClientUnavailable`: one type, two causes, and the old name asserted the cause that is not the common one. 4. LOW — `_read_historical_frame`'s docstring still said "degrade-open" flat. Corrected in both managers. C-107 is the entry recording that docstrings sit outside test_doc_accuracy's corpus, so nothing would have caught it. 5. LOW — C-103's Location was made stale by the previous commit (`:37` is now mid-docstring). Re-read, and `pyproject.toml` dropped from it: the entry's own body concludes the dependency is the launcher's to declare and that both launchers do. 6. LOW — the manager tripwire compared file offsets without tying them to the same `try`, and misreported a removed degrade-open as "grew a second broad except". Now scoped to `_read_historical_frame` and counts both branches, so the message says what actually happened. Mutation-proven, all three ways: narrowing the catch back to ImportError fails the ValueError test; dropping the installed-vs-missing classification fails the message test; removing the degrade-open branch fails the tripwire with the correct message. Suite: 440 passed, 3 skipped, 37 xfailed. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 2 +- tests/test_source_metadata.py | 120 ++++++++++++++---- .../contract/source_metadata.py | 66 +++++++--- views_postprocessing/crafd/managers/crafd.py | 8 +- views_postprocessing/unfao/managers/unfao.py | 8 +- 5 files changed, 151 insertions(+), 53 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 3013b10..0cf8b12 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -1068,7 +1068,7 @@ Recorded rather than left implicit because "the operator did a console session" | 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:37`; `views_postprocessing/unfao/managers/unfao.py:137-142`, `views_postprocessing/crafd/managers/crafd.py:137-142`; `pyproject.toml` (the dependency is absent) | +| Location | `views_postprocessing/contract/source_metadata.py:74` (the classification); `views_postprocessing/unfao/managers/unfao.py:137-151`, `views_postprocessing/crafd/managers/crafd.py:137-151` (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. Line numbers re-read 2026-08-17 after the fix moved them. | `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. diff --git a/tests/test_source_metadata.py b/tests/test_source_metadata.py index 77e6226..51d29a7 100644 --- a/tests/test_source_metadata.py +++ b/tests/test_source_metadata.py @@ -20,6 +20,7 @@ from __future__ import annotations import sys +import types from pathlib import Path import pytest @@ -29,19 +30,57 @@ _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" +) -def test_a_missing_client_raises_rather_than_reporting_no_boundary(): - """``datafactory_query`` is genuinely absent here, so this needs no simulation.""" - with pytest.raises(source_metadata.ProducerClientMissing): + +@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_the_refusal_names_the_package_and_why_it_is_not_None(): - with pytest.raises(source_metadata.ProducerClientMissing) as excinfo: +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" - assert "datafactory_query" in message, "and the module that was actually missing" # 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 " @@ -49,16 +88,36 @@ def test_the_refusal_names_the_package_and_why_it_is_not_None(): ) -def test_the_refusal_chains_the_original_importerror(): - """``raise ... from exc``: the traceback must still say what could not be imported.""" - with pytest.raises(source_metadata.ProducerClientMissing) as excinfo: +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): +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.ProducerClientMissing): + 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 " @@ -105,25 +164,34 @@ def test_the_managers_do_not_swallow_the_refusal(partner): 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 ``ProducerClientMissing`` is re-raised + 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() - assert "except source_metadata.ProducerClientMissing:" in source, ( - f"{partner}'s manager no longer re-raises ProducerClientMissing, so a broken " - "environment is once again indistinguishable from a producer that publishes " - "no boundary — and the delivery ships fabricated months either way (C-103)." + + # 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)." ) - # Each manager has exactly one broad `except Exception:` (the degrade-open), so a - # plain forward search is enough — searching from an offset would only obscure that. - assert source.count("except Exception:") == 1, ( - f"{partner}'s manager grew a second broad except; this check assumes one and " - "would compare against the wrong branch" + 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." ) - refusal = source.index("except source_metadata.ProducerClientMissing:") - broad = source.index("except Exception:") - assert refusal < broad, ( - f"{partner}'s manager catches Exception before ProducerClientMissing, so the " - "narrow branch is unreachable" + 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/views_postprocessing/contract/source_metadata.py b/views_postprocessing/contract/source_metadata.py index 4eed8d5..df2ee2f 100644 --- a/views_postprocessing/contract/source_metadata.py +++ b/views_postprocessing/contract/source_metadata.py @@ -20,8 +20,14 @@ logger = logging.getLogger(__name__) -class ProducerClientMissing(RuntimeError): - """``datafactory_query`` is not importable, so no producer fact can be read.""" +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: @@ -36,7 +42,8 @@ def last_valid_month_id(zarr_url: str | None = None) -> int | None: FAO queryset itself uses — ``ZARR_URL = DEFAULT_REMOTE.zarr_url``). Raises: - ProducerClientMissing: if ``datafactory_query`` cannot be imported. + 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 @@ -46,13 +53,13 @@ def last_valid_month_id(zarr_url: str | None = None) -> int | None: everything that imports it, loads in a unit-test environment without the producer's (heavy) package present. - **Why the absence 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. But a - *missing client* 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. + **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`` @@ -64,16 +71,39 @@ def last_valid_month_id(zarr_url: str | None = None) -> int | None: """ try: from datafactory_query.defaults import get_last_valid_month_id - except ImportError as exc: + 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 = ( - "datafactory_query is not importable, so the producer's " - "last_valid_month_id cannot be read. 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'. Refusing rather " - "than reporting 'no boundary published', which is a different condition and " - "would let unobserved months ship as observed history (C-103, C-26)." + 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 ProducerClientMissing(err_msg) from exc + raise ProducerClientUnavailable(err_msg) from exc return get_last_valid_month_id(zarr_url) diff --git a/views_postprocessing/crafd/managers/crafd.py b/views_postprocessing/crafd/managers/crafd.py index 9ba7c26..c60996f 100644 --- a/views_postprocessing/crafd/managers/crafd.py +++ b/views_postprocessing/crafd/managers/crafd.py @@ -136,10 +136,10 @@ def _read_historical_frame(self): ) try: lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) - except source_metadata.ProducerClientMissing: - # NOT degrade-open. A missing producer client 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). + 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( diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index 700b5b5..4f98508 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -136,10 +136,10 @@ def _read_historical_frame(self): ) try: lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) - except source_metadata.ProducerClientMissing: - # NOT degrade-open. A missing producer client 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). + 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(