From fd3043f1620900e3f9febb12029990bccb4c341e Mon Sep 17 00:00:00 2001 From: Polichinl Date: Fri, 14 Aug 2026 22:58:48 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(store):=20#268=20=E2=80=94=20download?= =?UTF-8?q?=20refuses=20instead=20of=20returning=20None,=20and=20moves=20o?= =?UTF-8?q?ut=20of=20the=20manager?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit views-crafdapi found this in our code, on the first un_crafd delivery attempt, and it is C-79 with the method name changed. THE DEFECT. `download` chained .get() onto an unvalidated store result: self._dsm.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None) When `data` is PRESENT and null the {} default never applies, so the next .get raised AttributeError from inside a dict comprehension over pinned ids in TargetLease.load — three frames from the port, naming neither the file_id nor the fact that a download had failed. They spent an evening ruling out an OOM kill that turned out to be a different pid three minutes later. The method was byte-identical in both partners, so THE FAO LEG CARRIES IT TOO. It has simply not fired there yet. WHY NOTHING CAUGHT IT. tests/test_store_port.py was written for C-79 with five parametrised tests across both partners, one of them literally named test_an_unrecognised_result_is_refused_rather_than_assumed_good. It mentioned `download` zero times. And contract/store_metadata.py already wrote `.get("data", {}) or {}` — the guard this lacked, one file away, never applied. C-79's own resolution note was the specification the whole time: "an unrecognised result should be refused and named, not adapted to silently." THE FIX. Refuse anything that is not non-empty bytes, naming the file_id, that a DOWNLOAD failed, and the types actually received. Empty bytes are refused with the rest: no shard, sidecar or manifest is ever zero-length, so b"" is a failed download wearing a valid type, and returning it only moves the same crash to the parser. Kept byte-identical across partners, as C-79 chose for upload (C-33). This protects all three call sites, not only the one that crashed — two of them are manifest reads. Mutation-proven on three: restoring the original one-liner fails 18 of the module's tests, accepting empty bytes fails exactly 2 (one per partner), dropping the file_id from the message fails exactly 2. IT ALSO MOVED, because a guard said so. The refusal pushed managers/ to 469 lines against epic #148's 450 bound, and that budget's instruction is to move something out rather than raise the number. _ContractStorePort is not the manager, so it is now {partner}/store_port.py — 388 lines, 62 of headroom. Two identical files, per the standing per-partner-track decision. The port stopped naming DatastoreModule in its constructor on the way out. A DIP seam whose stated purpose is that nothing downstream sees the client's types should not name one; and a new module mentioning views_pipeline_core would have widened C-40's blast radius past the two files test_views_pipeline_core_is_confined_to_the_partner_managers pins. The contract is the four methods, and it is now stated as such. REGISTERED. C-99 for the defect, resolved here. C-100 for what reading the whole port turned up: `file_metadata` has no caller in the package — latest_file_id runs three times, download three times, upload from the sink, and the fourth method is reachable only from a test, with contract/store_metadata.py dead behind it. Not deleted here; that is a decision belonging with the second store (#97), not with a download bug. Suite 446 passed / 1 skipped / 39 xfailed, ruff clean. Closes #268. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 56 +++++++- tests/test_store_port.py | 120 +++++++++++++++++- .../contract/store_metadata.py | 6 +- views_postprocessing/crafd/managers/crafd.py | 57 +-------- views_postprocessing/crafd/store_port.py | 100 +++++++++++++++ views_postprocessing/unfao/managers/unfao.py | 57 +-------- views_postprocessing/unfao/store_port.py | 100 +++++++++++++++ 7 files changed, 373 insertions(+), 123 deletions(-) create mode 100644 views_postprocessing/crafd/store_port.py create mode 100644 views_postprocessing/unfao/store_port.py diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 640e686..ecac8f3 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -5,9 +5,9 @@ | Project | views-postprocessing | | Owner | Dylan Pinheiro / PRIO MD&D Team | | Last Updated | 2026-08-12 | -| Total Concerns | 98 | -| Open Concerns | 21 | -| Resolved Concerns | 77 | +| Total Concerns | 100 | +| Open Concerns | 22 | +| Resolved Concerns | 78 | --- @@ -183,6 +183,26 @@ This is the same shape as vpp_017 §7a, arrived at from the other side: a check --- +### C-100: The "four-method port" has three used methods and a dead third module behind it + +| Field | Value | +|-------|-------| +| ID | C-100 | +| Tier | 4 — no correctness impact; the code is unreachable, not wrong. Registered because deleting it is a decision (the second store, #97) rather than a cleanup, and because an unreachable method inside a seam four documents describe is the kind of thing that gets maintained forever by accident. | +| Source | Reading the whole port while fixing C-99, 2026-08-14 | +| Trigger | The second partner store (#97) is scoped, or anyone proposes deleting `contract/store_metadata.py` — at which point this entry says what it costs and what moves with it. | +| Location | `views_postprocessing/{unfao,crafd}/store_port.py` (`file_metadata`); `views_postprocessing/contract/store_metadata.py` | + +`_ContractStorePort.file_metadata` has **no caller in the package**. Measured: `latest_file_id` is called three times and `download` three times, both in `contract/wire/source_selection.py`; `upload` is called by the sink; `file_metadata` is called by nothing. Its only body is a call to `contract/store_metadata.py:file_metadata`, whose own module docstring says *"the one caller is `_ContractStorePort.file_metadata`"* — true, and the chain terminates there. The module has tests (`tests/test_store_metadata.py`) and no production reader. + +The "four methods" the docs describe (`docs/ADRs/015_the_pipeline_core_appwrite_import.md:70`, both `store_port.py:5`, `tests/test_store_port.py:20`) are not wrong — the port really does define four. What none of them says, because nobody had counted, is that three of them run and the fourth is reachable only from a test. + +**Not fixed here on purpose.** C-99's change was a correctness fix on a live delivery path; deleting a public-ish port method and a contract module in the same commit would have mixed a refusal with a removal. It is also not obviously a deletion: the second prediction store (#97) is scoped to be sample-bearing and multi-target, and reading a selected file's identity metadata is the kind of thing that partner may need. The decision is "delete it or give it a caller", and it belongs with #97 rather than with a download bug. + +Cross-refs: **C-99** (the fix that surfaced it), **C-97**, **C-33** (the same symbol exists twice by design). + +--- + ### C-97: Coordinate values sit in docstrings and comments, where the scan deliberately does not look | Field | Value | @@ -1087,6 +1107,36 @@ See also C-40 (the inheritance/representation coupling this migration unwinds), ## Resolved Concerns +### C-99: `_ContractStorePort.download` failed open where `upload` refuses — C-79's untreated sibling — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-99 | +| Tier | 2 — no silent corruption, but an unreadable failure on the live FAO delivery leg, in the one place that knows which file it was. | +| Source | views-postprocessing#268, filed from the views-crafdapi seat 2026-08-14 after the first `un_crafd` delivery attempt | +| Trigger | *(closed)* Any failed download — a yanked file, an expired key, a rate limit, a network blip — on either partner's contract path. | +| Location | `views_postprocessing/{unfao,crafd}/store_port.py` (`download`); previously `managers/{unfao,crafd}.py:38-41` | + +`download` chained `.get()` onto an unvalidated store result: + +```python +self._dsm.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None) +``` + +When `data` is **present and null**, the `{}` default never applies and the next `.get` raises `AttributeError: 'NoneType' object has no attribute 'get'` — from inside a dict comprehension over pinned ids in `TargetLease.load`, three frames from the port, naming neither the `file_id` nor the fact that a download had failed. views-crafdapi spent an evening ruling out an OOM kill (there was one in `dmesg`, three minutes later, on a different pid) before finding it. + +**It was C-79 with the method name changed.** C-79 fixed exactly this polarity on `upload`, in the same class, on 2026-08-05, and recorded the specification in its own resolution note: *"an unrecognised result should be refused and named, not adapted to silently."* That note was never applied a second time. Nine days later the untreated method cost another repo an evening. + +*Why nothing caught it.* `tests/test_store_port.py` was written for C-79 with five parametrised tests across both partners, including `test_an_unrecognised_result_is_refused_rather_than_assumed_good`. It mentioned `download` **zero times**. And `contract/store_metadata.py` already wrote `.get("data", {}) or {}` — the guard `download` lacked, one file away, unapplied. + +**Fixed 2026-08-14.** `download` now refuses anything that is not non-empty bytes, naming the `file_id`, that a *download* failed, and the types it actually got. Empty bytes are refused with the rest: no shard, sidecar or manifest is ever zero-length, so `b""` is a failed download wearing a valid type. Byte-identical in both partners, as C-79 chose for `upload` (C-33). Mutation-proven on three mutants — restoring the original one-liner fails 18 of the module's tests, accepting empty bytes fails exactly 2, dropping the `file_id` from the message fails exactly 2. + +**It also moved.** The refusal pushed `managers/` to 469 lines against epic #148's 450 bound, and that guard's instruction is to move something out rather than raise the number. `_ContractStorePort` is not the manager, so it went to `{partner}/store_port.py` — 388 lines now, 62 of headroom. The port stopped naming `DatastoreModule` in its constructor on the way: a DIP seam whose stated purpose is that nothing downstream sees the client's types should not name one, and a new module that mentioned `views_pipeline_core` would have widened C-40's blast radius past the two files `test_views_pipeline_core_is_confined_to_the_partner_managers` pins. + +Cross-refs: **C-79** (the same defect on `upload`, resolved), **C-100** (the dead fourth method, found while reading this one), **C-33**, **C-40**. + +--- + ### C-27: Loader construction failures swallowed — surface as remote AttributeError — RESOLVED | Field | Value | diff --git a/tests/test_store_port.py b/tests/test_store_port.py index cc99094..268bafc 100644 --- a/tests/test_store_port.py +++ b/tests/test_store_port.py @@ -1,4 +1,4 @@ -"""The store port's refusal, tested — register C-79. +"""The store port's refusals, tested — register C-79 (``upload``) and C-99 (``download``). **This code had zero tests until 2026-08-05**, while the comment beside it called it "the whole mechanism". It is: pipeline-core's store, on a metadata failure *after* the @@ -7,6 +7,13 @@ invisible to the consumer, which is what happened to run-0's historical artifact on 2026-07-27. This port is the thing that turns that into a refusal. +``download`` is the same fault in the method next door, and it went untested here for +another nine days: it chained ``.get()`` onto an unvalidated result, so a store result +whose ``data`` was null raised ``AttributeError`` three frames away instead of naming +the file that failed. views-crafdapi lost an evening to it on 2026-08-13. C-79's own +resolution note — *an unrecognised result should be refused and named, not adapted to +silently* — was already the specification; it had simply never been applied twice. + Testable now for a reason worth stating: the standing excuse for source-scanning manager-side facts is that the managers need Appwrite env and a views-models path manager to instantiate. ``_ContractStorePort`` needs neither — it takes a store object @@ -30,25 +37,44 @@ class _Result: error: str | None = None +@dataclass +class _Downloaded: + """Shaped like the store's download result: ``.to_dict()["data"]["file_bytes"]``. + + ``data`` is declared as ``object`` rather than ``dict`` on purpose — the whole of + C-99 is what happens when it is not a dict. + """ + + data: object + + def to_dict(self): + return {"data": self.data} + + class _FakeStore: - """Records the upload and returns whatever result the test declares.""" + """Records the upload and returns whatever results the test declares.""" - def __init__(self, result): + def __init__(self, result, downloaded=None): self.result = result + self.downloaded = downloaded self.calls = [] + self.downloads = [] def upload_data(self, **kwargs): self.calls.append(kwargs) return self.result + def download_prediction(self, file_id): + self.downloads.append(file_id) + return self.downloaded -def _port(partner: str, result): + +def _port(partner: str, result, downloaded=None): """The partner's port, wrapping a fake store. Needs no Appwrite environment.""" - pytest.importorskip("views_pipeline_core", reason="the port wraps its DatastoreModule") module = __import__( - f"views_postprocessing.{partner}.managers.{partner}", fromlist=["_ContractStorePort"] + f"views_postprocessing.{partner}.store_port", fromlist=["_ContractStorePort"] ) - store = _FakeStore(result) + store = _FakeStore(result, downloaded) return module._ContractStorePort(store), store @@ -153,3 +179,83 @@ def test_the_port_forwards_every_declared_field(partner, tmp_path): assert forwarded["name"] == "un_fao" assert forwarded["category"] == "historical" assert forwarded["type"] == "model", "doc_type must arrive as the store's `type`" + + +# --------------------------------------------------------------------------- +# `download` — the same polarity, on the method C-79 missed (register C-99). +# --------------------------------------------------------------------------- + +_FILE_ID = "68b0f2c19a4e7d3c5a11" + + +def _download(port): + return port.download(_FILE_ID) + + +@pytest.mark.parametrize("payload", [b"shard-bytes", bytearray(b"shard-bytes")]) +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_a_downloaded_artifact_is_returned_as_bytes(partner, payload): + """The happy path, and the one conversion the port is allowed to make. + + ``bytearray`` is accepted and normalised because it is bytes by any useful + definition; everything else is refused below. If this test did not exist the + refusal could be tightened until nothing passed and the suite would not notice. + """ + port, store = _port(partner, _Result(success=True), _Downloaded({"file_bytes": payload})) + assert _download(port) == b"shard-bytes" + assert store.downloads == [_FILE_ID], "the port must forward the pinned id unchanged" + + +@pytest.mark.parametrize( + "downloaded, why", + [ + (_Downloaded(None), "data is present and null — the crash of 2026-08-13"), + (_Downloaded({}), "data carries no file_bytes at all"), + (_Downloaded({"file_bytes": None}), "file_bytes is present and null"), + (_Downloaded({"file_bytes": ""}), "file_bytes is a str, not bytes"), + (_Downloaded({"file_bytes": b""}), "file_bytes is bytes but empty"), + (_Downloaded("not-a-dict"), "data is not a mapping"), + (None, "the store returned nothing at all"), + (object(), "the result has no to_dict()"), + ], +) +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_a_download_that_is_not_bytes_is_refused_rather_than_returned_as_none( + partner, downloaded, why +): + """Fail CLOSED. This is C-79's polarity applied to the method next door. + + The original ``.get("data", {}).get("file_bytes", None)`` handled exactly one of + these — a *missing* ``data`` key. Every other row here either returned ``None`` to a + caller that could not tell it from an empty artifact, or raised ``AttributeError`` + from inside a dict comprehension three frames away. + + ``b""`` is refused with the rest deliberately: no shard, sidecar or manifest is ever + zero-length, so an empty payload is a failed download wearing a valid type, and + returning it only moves the same crash to the parser. + """ + port, _ = _port(partner, _Result(success=True), downloaded) + with pytest.raises(RuntimeError, match="did not return usable bytes"): + _download(port) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_download_refusal_names_the_file_id_and_what_it_got(partner): + """The defect was never that it failed — it was that the failure said nothing. + + The crash an operator actually saw was ``'NoneType' object has no attribute 'get'``, + raised inside a dict comprehension over pinned ids. It named no file, did not say a + download had failed, and sent views-crafdapi looking for an OOM kill that turned out + to be a different process. The id is in hand at this point; a refusal that drops it + is barely better than the crash. + """ + port, _ = _port(partner, _Result(success=True), _Downloaded(None)) + with pytest.raises(RuntimeError) as excinfo: + _download(port) + message = str(excinfo.value) + assert _FILE_ID in message, "the refusal must name the file_id it was given" + assert "download" in message, "the refusal must say that a DOWNLOAD failed" + assert "NoneType" in message, ( + "the refusal must name what it actually got, or the reader cannot tell a store " + "that returned nothing from one whose result shape moved" + ) diff --git a/views_postprocessing/contract/store_metadata.py b/views_postprocessing/contract/store_metadata.py index ffe4708..1672b37 100644 --- a/views_postprocessing/contract/store_metadata.py +++ b/views_postprocessing/contract/store_metadata.py @@ -15,9 +15,9 @@ frame readers became unreachable and this function was the sole survivor of a module named for a seam it was never part of. -The one caller is ``_ContractStorePort.file_metadata`` in the manager, which adapts -``DatastoreModule`` to the wire's ports (DIP) — so the store's document shape is -known here, and nowhere above. +The one caller is ``_ContractStorePort.file_metadata`` in each partner's +``store_port.py``, which adapts the store client to the wire's ports (DIP) — so the +store's document shape is known here, and nowhere above. """ from __future__ import annotations diff --git a/views_postprocessing/crafd/managers/crafd.py b/views_postprocessing/crafd/managers/crafd.py index 903e47b..efb6152 100644 --- a/views_postprocessing/crafd/managers/crafd.py +++ b/views_postprocessing/crafd/managers/crafd.py @@ -12,8 +12,9 @@ from datetime import datetime import os from views_pipeline_core.modules.dataloaders.datafactory_contract import declared_data_format -from views_postprocessing.contract import frame_extraction, gaul_lookup, historical, launch_config, source_metadata, store_metadata +from views_postprocessing.contract import frame_extraction, gaul_lookup, historical, launch_config, source_metadata from views_postprocessing.crafd import appwrite_env, product +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 @@ -22,60 +23,6 @@ logger = logging.getLogger(__name__) -class _ContractStorePort: - """Adapts ``DatastoreModule`` to the wire ports (ADR-013 epic #105; DIP — - ``wire/source_selection`` and ``wire/sink`` never see Appwrite types).""" - - def __init__(self, datastore: DatastoreModule) -> None: - self._dsm = datastore - - def latest_file_id(self, filters: dict): - return self._dsm.get_latest_file_id(filters=filters) - - def file_metadata(self, file_id: str) -> dict: - return store_metadata.file_metadata(self._dsm.get_file_metadata(file_id)) - - def download(self, file_id: str) -> bytes: - return ( - self._dsm.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None) - ) - - def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, description=None) -> None: - result = self._dsm.upload_data( - file=file_path, - filename=filename, - name=name, - type=doc_type, - category=category, - loa=loa, - targets=targets, - description=description, - ) - # On a metadata failure the store logs, then RETURNS success=False with the - # file already uploaded (pipeline-core modules/appwrite/file.py — the file is - # the claim; its line number moves between releases). It never raises, so a - # caller that discards the result ships an invisible orphan: run-0's historical - # artifact, 2026-07-27. This check is the whole mechanism. - # - # **Refuse unless success is explicitly True** (register C-79). The earlier - # `if success is False` failed OPEN: a result that was None, or lacked the - # attribute, or carried a non-bool, sailed through as though the upload had - # worked. Today `upload_data` has a single return path and `success` is a - # `bool` dataclass field, so the two polarities agree — but the moment that - # stops being true is exactly this entry's trigger, and fail-open is the wrong - # side to be on when the subject is "did the delivery actually land". - # - # The old `to_dict()` fallback is gone with it: dead on the real path, and an - # unrecognised result should be refused and named, not adapted to silently. - success = getattr(result, "success", None) - if success is not True: - error = getattr(result, "error", None) or "unknown store error" - raise RuntimeError( - f"upload of {filename!r} did not fully succeed (file may be an orphan " - f"without a metadata document): {error}. The store reported " - f"success={success!r} (result type {type(result).__name__})." - ) - def _build_prod_forecasts_store(ensemble_name: str | None) -> DatastoreModule: """The shared internal store (ADR-013's "shared shelf"), built from the diff --git a/views_postprocessing/crafd/store_port.py b/views_postprocessing/crafd/store_port.py new file mode 100644 index 0000000..cda03ff --- /dev/null +++ b/views_postprocessing/crafd/store_port.py @@ -0,0 +1,100 @@ +"""The prediction store behind a four-method port — the DIP seam of ADR-013 epic #105. + +``wire/source_selection`` and ``wire/sink`` drive the store through this object and +never see the client's types. That is the whole point of the seam, so the constructor +takes **any** object carrying the four methods below rather than naming a concrete +client class — the contract is the methods, not the type. + +Both refusals here are the same rule applied twice: *an unrecognised result should be +refused and named, not adapted to silently.* ``upload`` learned it as register C-79 +(2026-08-05), ``download`` as C-99 (2026-08-14) after the shape it did not check cost +views-crafdapi an evening. Tests: ``tests/test_store_port.py``. +""" + +from views_postprocessing.contract import store_metadata + + +class _ContractStorePort: + """Adapts a prediction-store client to the wire ports. + + ``datastore`` is any object exposing ``get_latest_file_id``, ``get_file_metadata``, + ``download_prediction`` and ``upload_data``. + """ + + def __init__(self, datastore) -> None: + self._dsm = datastore + + def latest_file_id(self, filters: dict): + return self._dsm.get_latest_file_id(filters=filters) + + def file_metadata(self, file_id: str) -> dict: + return store_metadata.file_metadata(self._dsm.get_file_metadata(file_id)) + + def download(self, file_id: str) -> bytes: + """Fetch a pinned artifact's bytes, refusing any result that is not bytes. + + Fail CLOSED, for the reason C-79 recorded of ``upload`` below: *an unrecognised + result should be refused and named, not adapted to silently.* Register C-99. + """ + result = self._dsm.download_prediction(file_id) + # `.get("data", {})` was the defect: when the key is PRESENT and null the default + # never applies, so `.get("file_bytes")` raised AttributeError three frames away + # in a dict comprehension, naming neither the file_id nor the fact that a + # download had failed (views-crafdapi#44, 2026-08-13 — it cost an evening). + to_dict = getattr(result, "to_dict", None) + payload = to_dict() if callable(to_dict) else None + data = payload.get("data") if isinstance(payload, dict) else None + file_bytes = data.get("file_bytes") if isinstance(data, dict) else None + + if isinstance(file_bytes, (bytes, bytearray)): + if file_bytes: + return bytes(file_bytes) + # Zero bytes is refused too: no shard, sidecar or manifest is ever empty, and + # returning b"" only moves the same failure to the parser. + observed = "'file_bytes' was present but empty (0 bytes)" + else: + observed = ( + f"the store result was {type(result).__name__}, its 'data' was " + f"{type(data).__name__}, its 'file_bytes' was {type(file_bytes).__name__}" + ) + raise RuntimeError( + f"download of file_id {file_id!r} did not return usable bytes: {observed}. " + "Refused here, where the file_id is still in hand — the caller assembles " + "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: + result = self._dsm.upload_data( + file=file_path, + filename=filename, + name=name, + type=doc_type, + category=category, + loa=loa, + targets=targets, + description=description, + ) + # On a metadata failure the store logs, then RETURNS success=False with the + # file already uploaded (pipeline-core modules/appwrite/file.py — the file is + # the claim; its line number moves between releases). It never raises, so a + # caller that discards the result ships an invisible orphan: run-0's historical + # artifact, 2026-07-27. This check is the whole mechanism. + # + # **Refuse unless success is explicitly True** (register C-79). The earlier + # `if success is False` failed OPEN: a result that was None, or lacked the + # attribute, or carried a non-bool, sailed through as though the upload had + # worked. Today `upload_data` has a single return path and `success` is a + # `bool` dataclass field, so the two polarities agree — but the moment that + # stops being true is exactly this entry's trigger, and fail-open is the wrong + # side to be on when the subject is "did the delivery actually land". + # + # The old `to_dict()` fallback is gone with it: dead on the real path, and an + # unrecognised result should be refused and named, not adapted to silently. + success = getattr(result, "success", None) + if success is not True: + error = getattr(result, "error", None) or "unknown store error" + raise RuntimeError( + f"upload of {filename!r} did not fully succeed (file may be an orphan " + f"without a metadata document): {error}. The store reported " + f"success={success!r} (result type {type(result).__name__})." + ) diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index e787bcd..3b727f8 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -12,8 +12,9 @@ from datetime import datetime import os from views_pipeline_core.modules.dataloaders.datafactory_contract import declared_data_format -from views_postprocessing.contract import frame_extraction, gaul_lookup, historical, launch_config, source_metadata, store_metadata +from views_postprocessing.contract import frame_extraction, gaul_lookup, historical, launch_config, source_metadata from views_postprocessing.unfao import appwrite_env, product +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 @@ -22,60 +23,6 @@ logger = logging.getLogger(__name__) -class _ContractStorePort: - """Adapts ``DatastoreModule`` to the wire ports (ADR-013 epic #105; DIP — - ``wire/source_selection`` and ``wire/sink`` never see Appwrite types).""" - - def __init__(self, datastore: DatastoreModule) -> None: - self._dsm = datastore - - def latest_file_id(self, filters: dict): - return self._dsm.get_latest_file_id(filters=filters) - - def file_metadata(self, file_id: str) -> dict: - return store_metadata.file_metadata(self._dsm.get_file_metadata(file_id)) - - def download(self, file_id: str) -> bytes: - return ( - self._dsm.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None) - ) - - def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, description=None) -> None: - result = self._dsm.upload_data( - file=file_path, - filename=filename, - name=name, - type=doc_type, - category=category, - loa=loa, - targets=targets, - description=description, - ) - # On a metadata failure the store logs, then RETURNS success=False with the - # file already uploaded (pipeline-core modules/appwrite/file.py — the file is - # the claim; its line number moves between releases). It never raises, so a - # caller that discards the result ships an invisible orphan: run-0's historical - # artifact, 2026-07-27. This check is the whole mechanism. - # - # **Refuse unless success is explicitly True** (register C-79). The earlier - # `if success is False` failed OPEN: a result that was None, or lacked the - # attribute, or carried a non-bool, sailed through as though the upload had - # worked. Today `upload_data` has a single return path and `success` is a - # `bool` dataclass field, so the two polarities agree — but the moment that - # stops being true is exactly this entry's trigger, and fail-open is the wrong - # side to be on when the subject is "did the delivery actually land". - # - # The old `to_dict()` fallback is gone with it: dead on the real path, and an - # unrecognised result should be refused and named, not adapted to silently. - success = getattr(result, "success", None) - if success is not True: - error = getattr(result, "error", None) or "unknown store error" - raise RuntimeError( - f"upload of {filename!r} did not fully succeed (file may be an orphan " - f"without a metadata document): {error}. The store reported " - f"success={success!r} (result type {type(result).__name__})." - ) - def _build_prod_forecasts_store(ensemble_name: str | None) -> DatastoreModule: """The shared internal store (ADR-013's "shared shelf"), built from the diff --git a/views_postprocessing/unfao/store_port.py b/views_postprocessing/unfao/store_port.py new file mode 100644 index 0000000..cda03ff --- /dev/null +++ b/views_postprocessing/unfao/store_port.py @@ -0,0 +1,100 @@ +"""The prediction store behind a four-method port — the DIP seam of ADR-013 epic #105. + +``wire/source_selection`` and ``wire/sink`` drive the store through this object and +never see the client's types. That is the whole point of the seam, so the constructor +takes **any** object carrying the four methods below rather than naming a concrete +client class — the contract is the methods, not the type. + +Both refusals here are the same rule applied twice: *an unrecognised result should be +refused and named, not adapted to silently.* ``upload`` learned it as register C-79 +(2026-08-05), ``download`` as C-99 (2026-08-14) after the shape it did not check cost +views-crafdapi an evening. Tests: ``tests/test_store_port.py``. +""" + +from views_postprocessing.contract import store_metadata + + +class _ContractStorePort: + """Adapts a prediction-store client to the wire ports. + + ``datastore`` is any object exposing ``get_latest_file_id``, ``get_file_metadata``, + ``download_prediction`` and ``upload_data``. + """ + + def __init__(self, datastore) -> None: + self._dsm = datastore + + def latest_file_id(self, filters: dict): + return self._dsm.get_latest_file_id(filters=filters) + + def file_metadata(self, file_id: str) -> dict: + return store_metadata.file_metadata(self._dsm.get_file_metadata(file_id)) + + def download(self, file_id: str) -> bytes: + """Fetch a pinned artifact's bytes, refusing any result that is not bytes. + + Fail CLOSED, for the reason C-79 recorded of ``upload`` below: *an unrecognised + result should be refused and named, not adapted to silently.* Register C-99. + """ + result = self._dsm.download_prediction(file_id) + # `.get("data", {})` was the defect: when the key is PRESENT and null the default + # never applies, so `.get("file_bytes")` raised AttributeError three frames away + # in a dict comprehension, naming neither the file_id nor the fact that a + # download had failed (views-crafdapi#44, 2026-08-13 — it cost an evening). + to_dict = getattr(result, "to_dict", None) + payload = to_dict() if callable(to_dict) else None + data = payload.get("data") if isinstance(payload, dict) else None + file_bytes = data.get("file_bytes") if isinstance(data, dict) else None + + if isinstance(file_bytes, (bytes, bytearray)): + if file_bytes: + return bytes(file_bytes) + # Zero bytes is refused too: no shard, sidecar or manifest is ever empty, and + # returning b"" only moves the same failure to the parser. + observed = "'file_bytes' was present but empty (0 bytes)" + else: + observed = ( + f"the store result was {type(result).__name__}, its 'data' was " + f"{type(data).__name__}, its 'file_bytes' was {type(file_bytes).__name__}" + ) + raise RuntimeError( + f"download of file_id {file_id!r} did not return usable bytes: {observed}. " + "Refused here, where the file_id is still in hand — the caller assembles " + "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: + result = self._dsm.upload_data( + file=file_path, + filename=filename, + name=name, + type=doc_type, + category=category, + loa=loa, + targets=targets, + description=description, + ) + # On a metadata failure the store logs, then RETURNS success=False with the + # file already uploaded (pipeline-core modules/appwrite/file.py — the file is + # the claim; its line number moves between releases). It never raises, so a + # caller that discards the result ships an invisible orphan: run-0's historical + # artifact, 2026-07-27. This check is the whole mechanism. + # + # **Refuse unless success is explicitly True** (register C-79). The earlier + # `if success is False` failed OPEN: a result that was None, or lacked the + # attribute, or carried a non-bool, sailed through as though the upload had + # worked. Today `upload_data` has a single return path and `success` is a + # `bool` dataclass field, so the two polarities agree — but the moment that + # stops being true is exactly this entry's trigger, and fail-open is the wrong + # side to be on when the subject is "did the delivery actually land". + # + # The old `to_dict()` fallback is gone with it: dead on the real path, and an + # unrecognised result should be refused and named, not adapted to silently. + success = getattr(result, "success", None) + if success is not True: + error = getattr(result, "error", None) or "unknown store error" + raise RuntimeError( + f"upload of {filename!r} did not fully succeed (file may be an orphan " + f"without a metadata document): {error}. The store reported " + f"success={success!r} (result type {type(result).__name__})." + ) From b4692d225677ca4cc335e2cc2791e3f579a556a8 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Fri, 14 Aug 2026 23:11:10 +0200 Subject: [PATCH 2/2] fix(guards): the review found the move dodged the budget it claimed to satisfy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five reviewers on the download fix. The download refusal itself survived — one agent traced the pipeline-core SDK and confirmed the realistic failures (yanked file, expired key, rate limit, network blip) all arrive as success=False rather than as exceptions, that nothing on the path returns memoryview, that bytes(b) is a no-op passthrough on the common path, and that no artifact on the contract path is legitimately zero-length. What did not survive was how I described the part around it. THE MOVE DODGED THE BUDGET RATHER THAN SATISFYING IT. I reported "388 lines, 62 of headroom" as compliance. Re-measured: managers/ fell 441 -> 388 while each partner package grew 441 -> 488. The guard counts managers/, and store_port.py is a sibling OF managers/, so 47 lines left the budget's view rather than the codebase. The budget's own docstring had already named this exact failure — "an 800-line helper module beside a 406-line manager was previously unbudgeted, which is the same regrowth wearing a different filename" — and closed it one level in. I evaded it one level out. This is C-98 again: a guard that watches a proxy reports on the proxy, and the number it prints is true and irrelevant. test_the_partner_package_stays_within_its_line_budget now bounds the whole partner package at 700 (measured today: unfao 626, crafd 635), and is mutation-proven by dropping a 200-line module beside the manager and watching it fire. The extraction still stands — a store adapter is not the manager. C-100 SAID "Measured" AND THEN DID NOT MEASURE ONE OF THE FOUR. It gave exact counts for latest_file_id and download and wrote "upload is called by the sink". upload has THREE call sites: contract/wire/sink.py:164 and each partner's historical artifact at managers/.py:325. Corrected, with the sites named. C-33's EXTRACTION TRIGGER HAS FIRED, TWICE, AND NOBODY SAID SO. Its trigger is "a third in-repo partner package, or the first bug that must be hand-patched identically in both manager files". C-79 was that bug; C-99 is the same fault in the sibling method, patched by hand in both again. The decision is still to duplicate, but the reason is now different and is recorded: the shape the two incidents showed is a result-check, not the store-identity DeliveryProfile this entry proposes extracting, which would have prevented neither. What did change is that the duplication is now held mechanically — test_the_two_partners_ports_have_not_drifted. And the docstring is precise about what that does NOT buy: it would not have caught C-79 or C-99, because both files stayed byte-identical while carrying the defect in the untreated method. It closes the partner-vs-partner axis, which was never the axis that bit. FOUR STALE LOCATIONS, from moving a class four documents point at. C-40 cited the port at unfao.py:37-78 in two places, C-15/C-24 at unfao.py:37-64, and the register header still said Last Updated 2026-08-12. The one doc that WAS updated in the first commit — contract/store_metadata.py — showed the fix pattern was known and applied one file over, which is ADR-014 §1's failure mode exactly. Suite 449 passed / 1 skipped / 39 xfailed, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 26 ++++++++++++++---- tests/test_doc_accuracy.py | 43 ++++++++++++++++++++++++++++++ tests/test_store_port.py | 34 +++++++++++++++++++++++ 3 files changed, 98 insertions(+), 5 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index ecac8f3..6a160ca 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -4,7 +4,7 @@ |-------------------|--------------------------------------| | Project | views-postprocessing | | Owner | Dylan Pinheiro / PRIO MD&D Team | -| Last Updated | 2026-08-12 | +| Last Updated | 2026-08-14 | | Total Concerns | 100 | | Open Concerns | 22 | | Resolved Concerns | 78 | @@ -35,7 +35,7 @@ covered a single open entry (see Historical clusters below). **Update 2026-08-03 — the cluster halved at the 3.0.0 bump.** Six of its ten entries closed with the pin: the inherited surface stopped being a liability for timeouts (C-13), silent provisioning (C-58), the transitive drag (C-62) and the undeclared SDK (C-07). What remains is the root — the double inheritance itself — and the three genuinely upstream-owned data concerns (C-26, C-27, C-28). The cluster's thesis held: fixing the surface upstream fixed them here with a pin and no code. **Amended 2026-08-03:** the root's defining measurement — pipeline-core imported by exactly one module — became **two** when `crafd/managers/crafd.py` landed (PR #211). The count is still pinned by an explicit allowlist, so the cluster's boundary holds; what changed is that every fix in it now has two landing sites. See C-33 for why the second copy is deliberate and what triggers its removal. **Highest tier:** 1 (C-26) -**Fix strategy:** the thin-shell de-inheritance C-40 prescribes — and which is **half-built**: the sink side landed (`_ContractStorePort`, `unfao.py:37-78`) and the invariants are already pipeline-core-free modules the manager calls (`delivery/*`, `unfao/historical.py`, `unfao/wire/`). The remaining half is the **input** side (loader + `PGMDataset`), gated on pipeline-core Epic #186/#207. +**Fix strategy:** the thin-shell de-inheritance C-40 prescribes — and which is **half-built**: the sink side landed (`_ContractStorePort`, moved to `/store_port.py` 2026-08-14 by C-99) and the invariants are already pipeline-core-free modules the manager calls (`delivery/*`, `unfao/historical.py`, `unfao/wire/`). The remaining half is the **input** side (loader + `PGMDataset`), gated on pipeline-core Epic #186/#207. **Resolution scope:** Partial — C-26/C-27/C-28 are upstream-owned; de-inheritance makes them visible and testable, not fixed. ### Cluster H: Go-global verification debt — discharged unevenly by run-0 @@ -193,7 +193,7 @@ This is the same shape as vpp_017 §7a, arrived at from the other side: a check | Trigger | The second partner store (#97) is scoped, or anyone proposes deleting `contract/store_metadata.py` — at which point this entry says what it costs and what moves with it. | | Location | `views_postprocessing/{unfao,crafd}/store_port.py` (`file_metadata`); `views_postprocessing/contract/store_metadata.py` | -`_ContractStorePort.file_metadata` has **no caller in the package**. Measured: `latest_file_id` is called three times and `download` three times, both in `contract/wire/source_selection.py`; `upload` is called by the sink; `file_metadata` is called by nothing. Its only body is a call to `contract/store_metadata.py:file_metadata`, whose own module docstring says *"the one caller is `_ContractStorePort.file_metadata`"* — true, and the chain terminates there. The module has tests (`tests/test_store_metadata.py`) and no production reader. +`_ContractStorePort.file_metadata` has **no caller in the package**. Measured: `latest_file_id` is called three times and `download` three times, both in `contract/wire/source_selection.py`; `upload` three times — `contract/wire/sink.py:164` plus each partner's historical artifact at `managers/.py:325`; `file_metadata` is called by nothing. Its only body is a call to `contract/store_metadata.py:file_metadata`, whose own module docstring says *"the one caller is `_ContractStorePort.file_metadata`"* — true, and the chain terminates there. The module has tests (`tests/test_store_metadata.py`) and no production reader. The "four methods" the docs describe (`docs/ADRs/015_the_pipeline_core_appwrite_import.md:70`, both `store_port.py:5`, `tests/test_store_port.py:20`) are not wrong — the port really does define four. What none of them says, because nobody had counted, is that three of them run and the fourth is reachable only from a test. @@ -838,6 +838,16 @@ Mitigation: a small `DeliveryProfile` (bucket/collection/database ids, category, 3. **Partially mitigated by þing-01 #134.** `unfao/appwrite_env.py` now declares the env **names** centrally (`CONNECTION_ENV`, `PROD_FORECASTS_ENV`, `UNFAO_ENV`) and validates them fail-loud before every `AppwriteConfig` construction, following the PLATFORM-001 coordinate registry. Names are no longer scattered string literals. **What is still hardcoded is store *identity*** — which names apply to which store, the targets list, and the category strings — so the `DeliveryProfile` case stands. Tier held at 2. 4. **The deferral condition has expired**: D-09 scheduled this "after the FAO global delivery ships." It shipped 2026-07-27. Ready for the "calm 1-day job" whenever #97 scoping lands. +**Update 2026-08-14 — the extraction trigger has now fired, twice, and this is the record of it.** + +This entry's remaining trigger reads: *"a **third** in-repo partner package, **or** the first bug that must be hand-patched identically in both manager files — whichever comes first."* The second arm has fired twice. C-79 (2026-08-05) fixed `_ContractStorePort.upload`'s fail-open result check by hand in both partners. C-99 (2026-08-14) fixed the identical fault in `download`, again by hand in both. + +**The decision is still to duplicate, and the reason has changed.** It is no longer "no second incident has shown the shape" — one has. It is that the shape the incidents showed is not the one this entry proposes extracting. C-33's mitigation is a `DeliveryProfile` carrying store identity, and neither C-79 nor C-99 was about store identity; both were a result-shape check that happens to live in a duplicated file. Extracting a `DeliveryProfile` would not have prevented either. + +**What has changed is that the duplication is now mechanically held.** `tests/test_store_port.py::test_the_two_partners_ports_have_not_drifted` fails if the two `store_port.py` files differ. Note precisely what that does *not* buy: it would not have caught C-79 or C-99, because both files stayed byte-identical throughout while carrying the defect in the untreated method. It closes the partner-vs-partner axis; the method-vs-method axis is closed by `tests/test_store_port.py` covering all four methods, which it now does for three and records the fourth as C-100. + +The `DeliveryProfile` extraction stays where D-09 put it: with the second store's scoping (#97). What is discharged here is the pretence that nobody had hit the trigger. + **Update 2026-08-03 (PR #211) — the thing this entry warned about has happened, and it is being kept on purpose.** This entry's own Tier-2 rationale was that the design *"forces copy-pasting a 273-line manager per store."* PR #211 added `views_postprocessing/crafd/` — a second partner package whose `managers/crafd.py` is a **line-for-line copy** of `unfao/managers/unfao.py`. Measured with @@ -898,7 +908,7 @@ See also C-24 (schema contract per store), C-77 (the fourth home for partner ide **Wire contract posted (2026-07-03) — the S6/#45 circular wait is dissolved.** A three-way audit (pipeline-core / producers / consumer+substrate, all on `origin/development` + maintainer-authored issues) established: (i) there are **two wire hops** (producer→store; vpp→faoapi) and the roadmap's arrow work covered only the second; (ii) **no publish path from PFE to the prediction store exists at all** — models#143's "no pipeline-core change required" is **falsified** (PFE's `use_prediction_store` is stored then only logged, `prediction_frame_ensemble.py:141/:799`; `PredictionIOManager._upload_to_prediction_store` raises `NotImplementedError`, `io.py:117`); (iii) full global draws ≈ **9.5 GB/target**, so the wire mandates per-month sharding; (iv) the "platform ADR-046" cited as the format authority **does not exist** (phantom). **ADOPTED 2026-07-15 as ADR-013** *(post-adoption: F1 invisibility confirmed live — six stranded orange_ensemble forecast docs in unfao_bucket, forecast serving has been empty all along; both §11.4 legacy guards merged same day, Hop-B guard must reach production before vpp's first contract upload — **views-faoapi C-161**)* after five reviewed iterations (two seat reviews, reconciliation, owner-ratified F1) — maintainer sign-off on views-models#149. The v1 proposal history: Hop A = Track A zip archive per (run,target,month) + manifest-last commit marker (new **pipeline-core#269**); Hop B = per-month `views_frames.io.arrow` (#91/faoapi#100); interior = per-target 2-D `PredictionFrame`; the 9 GAUL columns move to a **gid-keyed sidecar**; the **#149 no-collapse boundary is named: vpp `delivery/draws.py`** (a new invariant, sibling of coverage/identity — follow-on vpp work with the durable vpp ADR after explicit sign-off); target vocabulary **decided: `lr_ged_sb/ns/os`**, producers rename at publish (models#146). -**Update 2026-07-31 (review-rr — the prescribed DIP mitigation has half landed, uncredited).** This entry's mitigation was: "*keep the subclass as a thin shell but extract `enrich` + `validate` + the 9-column contract into a pipeline-core-free core object the manager calls, and wrap the Appwrite I/O behind a small delivery-sink adapter (DIP).*" The **sink half exists**: `_ContractStorePort` (`unfao.py:37-78`) wraps `DatastoreModule` behind a four-method port (`latest_file_id` / `file_metadata` / `download` / `upload`), and the contract delivery path drives the store through it. The **invariant half also largely exists** as pipeline-core-free modules the manager calls: `delivery/coverage.py`, `identity.py`, `draws.py`, `parity.py`, `provenance.py`, `observed_range.py` (the package docstring pins them representation-free), plus `unfao/historical.py` and `unfao/wire/`. **Residual scope of this entry is now the input side and the shell itself:** the double inheritance at `:80` (consequences a/c/d), the inherited `ViewsDataLoader`/`PGMDataset` on the legacy branch, and the fact that the FAO logic still cannot be instantiated without the framework. Tier held at 2 — the blast radius argument is unchanged for what remains. This is the root of **Cluster G**. +**Update 2026-07-31 (review-rr — the prescribed DIP mitigation has half landed, uncredited).** This entry's mitigation was: "*keep the subclass as a thin shell but extract `enrich` + `validate` + the 9-column contract into a pipeline-core-free core object the manager calls, and wrap the Appwrite I/O behind a small delivery-sink adapter (DIP).*" The **sink half exists**: `_ContractStorePort` (`/store_port.py` since 2026-08-14 — it lived at `unfao.py:37-78` when this was written) wraps the store client behind a four-method port (`latest_file_id` / `file_metadata` / `download` / `upload`), and the contract delivery path drives the store through it. The **invariant half also largely exists** as pipeline-core-free modules the manager calls: `delivery/coverage.py`, `identity.py`, `draws.py`, `parity.py`, `provenance.py`, `observed_range.py` (the package docstring pins them representation-free), plus `unfao/historical.py` and `unfao/wire/`. **Residual scope of this entry is now the input side and the shell itself:** the double inheritance at `:80` (consequences a/c/d), the inherited `ViewsDataLoader`/`PGMDataset` on the legacy branch, and the fact that the FAO logic still cannot be instantiated without the framework. Tier held at 2 — the blast radius argument is unchanged for what remains. This is the root of **Cluster G**. **Update 2026-07-31 (`repo-assimilation`, clone-readiness pass — the coupling is CONTAINED, and this file is the only clone blocker).** Two measurements that change how this entry should be read: @@ -1133,6 +1143,12 @@ When `data` is **present and null**, the `{}` default never applies and the next **It also moved.** The refusal pushed `managers/` to 469 lines against epic #148's 450 bound, and that guard's instruction is to move something out rather than raise the number. `_ContractStorePort` is not the manager, so it went to `{partner}/store_port.py` — 388 lines now, 62 of headroom. The port stopped naming `DatastoreModule` in its constructor on the way: a DIP seam whose stated purpose is that nothing downstream sees the client's types should not name one, and a new module that mentioned `views_pipeline_core` would have widened C-40's blast radius past the two files `test_views_pipeline_core_is_confined_to_the_partner_managers` pins. +**Amendment, same day — the move was right and the way it was reported was not.** The first version of this change moved `_ContractStorePort` out of `managers/` and recorded "388 lines, 62 of headroom" as though the budget had been satisfied. Review measured what actually happened: the counted number fell from **441 to 388** while each partner package grew from **441 to 488**. The guard counts `managers/`, and the code moved to a sibling *of* `managers/`, so 47 lines left the budget's view rather than the codebase. + +The budget's own docstring had already named this failure — *"an 800-line helper module beside a 406-line manager was previously unbudgeted, which is the same regrowth wearing a different filename"* — and had closed it one level in. The evasion simply happened one level out. This is C-98's shape again: a guard that watches a proxy reports on the proxy, and the number it prints is true and irrelevant. + +`test_the_partner_package_stays_within_its_line_budget` now bounds the whole partner package at 700 (measured 2026-08-14: unfao 626, crafd 635), mutation-proven by dropping a 200-line module beside the manager — the exact evasion — and watching it fire. The extraction itself stands: a store adapter is not the manager, and the inner budget's instruction is to move something out. + Cross-refs: **C-79** (the same defect on `upload`, resolved), **C-100** (the dead fourth method, found while reading this one), **C-33**, **C-40**. --- @@ -1604,7 +1620,7 @@ Cross-refs: **C-44** (the bump that carried this), views-postprocessing#172, pip | Tier | 2 | | Source | `expert-review` (2026-06-02) | | Trigger | When configuring Appwrite connection parameters — in `_ContractStorePort` (contract path) or `_save`/`_read_forecast_data` (legacy path) — verify that timeout parameters are set on the underlying HTTP client; currently no timeout exists and a hung endpoint blocks the pipeline indefinitely | -| Location | `views_postprocessing/unfao/managers/unfao.py:37-64` (`_ContractStorePort` — all four contract-path store calls), `:247` (legacy selection), `:560`, `:571` (legacy uploads) | +| Location | `views_postprocessing/unfao/store_port.py` (`_ContractStorePort` — all four contract-path store calls; it was `managers/unfao.py:37-64` until 2026-08-14), `:247` (legacy selection), `:560`, `:571` (legacy uploads) | `prediction_store_manager.download_latest_file()` (line 131) and `dsm.upload_data()` (lines 262, 272) make network calls to Appwrite with no configured timeout. If the endpoint hangs (DNS resolution stalls, connection accepted but response never arrives, TLS handshake blocks), the pipeline blocks indefinitely. There is no watchdog timer, no circuit breaker, and no automated alert for a run that never completes. The only detection is manual observation that a scheduled run didn't finish. diff --git a/tests/test_doc_accuracy.py b/tests/test_doc_accuracy.py index 1c1c87d..7f9eeea 100644 --- a/tests/test_doc_accuracy.py +++ b/tests/test_doc_accuracy.py @@ -367,6 +367,16 @@ def test_internal_doc_links_resolve(): #: previously unbudgeted, which is the same regrowth wearing a different filename. _MANAGER_LINE_BUDGET = 450 +#: The same rule one level out, added 2026-08-14 because the directory bound was not +#: enough. C-99's fix pushed `managers/` to 469 and the response was to move +#: `_ContractStorePort` to `/store_port.py` — a sibling of `managers/`, not a +#: sibling inside it. The counted number fell 441 -> 388 while the partner package grew +#: by 47 lines, and the PR reported "62 of headroom" against a guard that could no +#: longer see the code. The move was right; reporting it as compliance was not. +#: Measured 2026-08-14: unfao 626, crafd 635. A ratchet, like the class budget — the +#: response to it binding is to move something OUT OF THE PACKAGE, not to raise it. +_PARTNER_PACKAGE_LINE_BUDGET = 700 + #: The manager CLASS, separately (C-40). 351 before the 2026-08-05 extraction, 272 after. #: A ratchet — see `test_the_manager_class_itself_stays_thin` for why it is not a target. _MANAGER_CLASS_BUDGET = 300 @@ -492,6 +502,39 @@ def test_the_manager_stays_within_its_line_budget(managers_dir): ) +@pytest.mark.parametrize("partner", _PARTNER_PACKAGES) +def test_the_partner_package_stays_within_its_line_budget(partner): + """The directory bound, one level out — because moving code past it is not shrinking. + + The budget above deliberately counts the manager *directory* rather than the manager + file, so that a helper module beside a thin manager could not go unbudgeted. On + 2026-08-14 the same evasion happened one directory further out and the guard did not + see it: `_ContractStorePort` moved from `managers/.py` to + `/store_port.py`, the counted number fell from 441 to 388, and the partner + package grew from 441 to 488 lines. + + That move was the right call — a store adapter is not the manager, and the budget's + own instruction is to move something out rather than raise the number. What was + wrong was calling the result "62 of headroom" when the guard had simply stopped + measuring the code. This test is what makes that sentence checkable, and it is the + same lesson as register C-98: a guard that watches a proxy reports on the proxy. + + A ratchet, not a target. If it binds, move something out of the partner package — + to `contract/` or `delivery/`, where the machinery lives — or say in the commit + message why the package genuinely needs to be bigger. + """ + package = _PKG / partner + sources = sorted(package.rglob("*.py")) + lines = sum(len(f.read_text().splitlines()) for f in sources) + assert lines <= _PARTNER_PACKAGE_LINE_BUDGET, ( + f"{partner}/ is {lines} lines across {len(sources)} files " + f"({[f.relative_to(package).as_posix() for f in sources]}), over the " + f"{_PARTNER_PACKAGE_LINE_BUDGET} bound. Moving code from managers/ into a " + "sibling module does not reduce the seam — it only moves it out of the inner " + "budget's view, which is what this outer one exists to notice." + ) + + @pytest.mark.parametrize("partner", _PARTNER_PACKAGES) def test_the_manager_class_itself_stays_thin(partner): """The directory budget above is anti-regrowth. This one is anti-*fusion*. diff --git a/tests/test_store_port.py b/tests/test_store_port.py index 268bafc..46c82a4 100644 --- a/tests/test_store_port.py +++ b/tests/test_store_port.py @@ -26,8 +26,12 @@ import pytest +from pathlib import Path + from tests.conftest import PARTNER_PACKAGES +_PKG = Path(__file__).resolve().parent.parent / "views_postprocessing" + @dataclass class _Result: @@ -259,3 +263,33 @@ def test_the_download_refusal_names_the_file_id_and_what_it_got(partner): "the refusal must name what it actually got, or the reader cannot tell a store " "that returned nothing from one whose result shape moved" ) + + +def test_the_two_partners_ports_have_not_drifted(): + """The duplication C-33 blesses is only safe while the copies stay equal. + + Both partners carry this file byte for byte, which is the standing per-partner-track + decision (C-33), not an accident. What makes that decision cheap is that a reader can + treat one file as the truth; what makes it dangerous is a fix applied to one copy and + not the other, which nothing in this repository would have noticed until now. + + **Be precise about what this does not catch.** It would *not* have caught C-99. That + drift was between two METHODS of the same class — ``upload`` was fixed in both + partners on 2026-08-05 and ``download`` in neither — so both files stayed perfectly + identical while carrying the defect for nine days. This guard closes the other axis, + the partner-vs-partner one, which is real but was never the thing that bit. + """ + sources = { + partner: (_PKG / partner / "store_port.py").read_text() + for partner in PARTNER_PACKAGES + } + first, *rest = sorted(sources) + for other in rest: + assert sources[first] == sources[other], ( + f"{first}/store_port.py and {other}/store_port.py have diverged. The port is " + "duplicated per partner on purpose (C-33), and the copies carry no " + "partner-specific content at all — so a difference here is a fix that landed " + "in one partner and not the other, which is how the same delivery bug ships " + "twice. Apply it to both, or if the divergence is deliberate, say so in " + "C-33 and replace this check with one that allows it." + )