#268: download refuses instead of returning None — C-79's untreated sibling

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 `<partner>/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/<partner>.py` to
+    `<partner>/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*.

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
@@ -19,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:
@@ -30,25 +41,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 +183,113 @@ 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"
+    )
+
+
+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."
+        )

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

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