From a69d1a0a515a55db6200a246102989f0988cbd47 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Wed, 5 Aug 2026 07:48:29 +0200 Subject: [PATCH] =?UTF-8?q?fix(delivery):=20C-79=20and=20C-83=20=E2=80=94?= =?UTF-8?q?=20two=20refusals=20on=20the=20live=20FAO=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-79: the store port failed OPEN, and had zero tests The comment beside `_ContractStorePort.upload`'s result check calls it "the whole mechanism". It is: pipeline-core's store, on a metadata failure AFTER the file is uploaded, logs and RETURNS success=False rather than raising, so a caller that discards the result ships a file with no metadata document -- invisible to the consumer. That happened to run-0's historical artifact on 2026-07-27. The check was `if success is False`. A result that was None, or lacked the attribute, or carried a non-bool, sailed through as though the upload had worked. Now `is not True`. The dead to_dict() fallback went with it: an unrecognised result should be refused and named, not adapted to silently. The message reports what it actually received, because success=None (a moved contract) and success=False (a reported failure) send an operator to different places. tests/test_store_port.py -- 16 tests over both partners, where there were none. The standing excuse for source-scanning manager facts is that managers need Appwrite env and a views-models path manager; the port needs neither, so it never applied here. Its trigger fired on 2026-08-03 and nobody noticed. The entry read "the 3.0.0 bump is the next occasion"; the bump landed, C-44 closed on a wheel-level suite verification, and the return contract was never re-read. test_register_integrity cannot catch that -- its checks are structural and none asks whether a named external event has occurred. C-83: a failed import reported as a wrong declaration get_queryset() returns None for ANY exception importing config_queryset.py; declared_data_format(None) defaults to 'dataframe'; the format guard then tells the operator to set data_format: 'feature_frame' in a file that already says exactly that. launch_config.assert_queryset_was_importable now runs FIRST, and both managers read the queryset once and reuse it. The refusal says what happened and steers away from the config file -- "This is NOT a declaration problem: do not edit data_format until the module imports" -- toward the traceback pipeline-core logged. Logs before it raises. Three guards, because order IS the fix: the refusal fires and names the real fault; an importable queryset passes (the format question belongs to the next check); and per partner, get_queryset() is called exactly once with importability checked first. Not fixed here, deliberately: upstream still returns None for any import exception. We stopped passing it into a function whose contract is to default. Raising upstream would be better and is not ours; waiting for it would have left the misleading message on the live FAO path meanwhile. Mutation-proven three ways -- revert the polarity (4 fail), delete the importability check (1 fails), reverse the order (1 fails). 352 passed / 40 xfailed / 0 failed. ruff clean. Register 83/12/71. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 126 ++++++++------ tests/test_env_declaration.py | 60 +++++++ tests/test_store_port.py | 155 ++++++++++++++++++ .../contract/launch_config.py | 40 +++++ views_postprocessing/crafd/managers/crafd.py | 32 ++-- views_postprocessing/unfao/managers/unfao.py | 32 ++-- 6 files changed, 371 insertions(+), 74 deletions(-) create mode 100644 tests/test_store_port.py diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 548f439..7a0818c 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -6,8 +6,8 @@ | Owner | Dylan Pinheiro / PRIO MD&D Team | | Last Updated | 2026-08-03 | | Total Concerns | 83 | -| Open Concerns | 14 | -| Resolved Concerns | 69 | +| Open Concerns | 12 | +| Resolved Concerns | 71 | --- @@ -482,33 +482,6 @@ Cross-refs: **C-46** and **C-57** (both RESOLVED; this is the residual each reco --- -### C-83: A queryset that fails to import is reported as a queryset that declares the wrong format - -| Field | Value | -|-------|-------| -| ID | C-83 | -| Tier | 2 — no wrong data ships; the delivery refuses, which is correct. What is wrong is the reason it gives, and it gives it on the live FAO path, at the moment someone is trying to fix a failed run. It sends them to edit a file that is already right. | -| Source | `code-review max` (2026-08-03) — the views-pipeline-core 3.0.0 bump review | -| Trigger | The next time the FAO delivery refuses with *"the queryset declares data_format='dataframe'"*, check whether `config_queryset.py` actually imports before editing it. Most likely on a machine missing views-datafactory, or after any change to that file's own imports. | -| Owner | Whoever next touches `_read_historical_frame`'s precondition. The fix is ours — distinguishing the two cases takes one branch. | -| Location | `views_postprocessing/unfao/managers/unfao.py` and the same line in `crafd` — `declared_data_format(self._model_path.get_queryset())` feeding `launch_config.assert_frame_native_historical` | - -Three correct-in-isolation behaviours compose into a lie: - -1. pipeline-core's `ModelPathManager.get_queryset()` catches **any** exception from importing `config_queryset.py`, logs it, and returns `None`. -2. `declared_data_format(None)` returns `'dataframe'` — the documented default for a non-dict. -3. `assert_frame_native_historical('dataframe')` raises: *"the queryset declares `data_format='dataframe'` … **Set `data_format: 'feature_frame'` in the postprocessor's config_queryset**."* - -So a queryset that **failed to import** is indistinguishable from one that **declared the wrong format**, and the operator is told to fix a file that is already correct. Reproduced: in an environment without views-datafactory the real `un_fao` queryset reports `dataframe` while declaring `feature_frame`; in a complete environment the same file reports `feature_frame`. - -This is ADR-003's rule broken by composition rather than by anyone inferring anything: each layer declares faithfully, and the *absence* of an answer is silently given the shape of an answer. Cluster J's disease — *cannot distinguish "no" from "I could not tell"* — reached through a new door, because #126 made this repo depend on `declared_data_format` in the first place. - -**The fix is ours and it is small:** call `get_queryset()` once, and if it returns `None`, refuse with *that* — the queryset could not be imported — rather than passing `None` into a function whose contract is to default. Upstream could also raise instead of returning `None`, but we should not wait for that; we are the ones holding the ambiguous value. - -Cross-refs: **C-44** (the bump whose review found this), **C-40** (the inherited surface it arrives through), Cluster J (the *no* vs *could not tell* family), ADR-003, #126, #149. - ---- - ### C-82: Governance-artifact prose carries numbers and statuses that nothing checks | Field | Value | @@ -536,31 +509,6 @@ Cross-refs: **C-80** (the same disease in ADRs and CICs, and the mechanism that --- -### C-79: `_ContractStorePort.upload`'s result check is called "the whole mechanism" and has no test, and it fails open - -| Field | Value | -|-------|-------| -| ID | C-79 | -| Tier | 3 — the check works today and is correct for what the store actually returns, so nothing is shipping wrong. What is missing is any assertion that it keeps working, plus a polarity that would swallow an unrecognised result rather than refuse it. | -| Source | `code-review max` (2026-08-03) — PR #211 fourth pass, while verifying the corrected comment beside it | -| Trigger | When views-pipeline-core changes what `DatastoreModule.upload_data` returns — a different result type, a renamed field, or a raise where it used to report — check this port still refuses a partial upload. The 3.0.0 bump (C-44) is the next occasion. | -| Owner | Whoever takes the pipeline-core 3.0.0 bump; it is the same reading of the same return contract. | -| Location | `_ContractStorePort.upload` in `views_postprocessing/unfao/managers/unfao.py` and `views_postprocessing/crafd/managers/crafd.py` (byte-identical in both) | - -The port exists because the store **reports** a metadata failure without raising: after the file is uploaded it logs, then returns `OperationResult(success=False, code="PARTIAL_SUCCESS")`. A caller that discards the result ships a file with no metadata document — invisible to the consumer, which is what happened to run-0's historical artifact on 2026-07-27. This check is what converts that into a refusal. - -**Two things are wrong with how it is held.** - -*It is untested.* `grep -rn _ContractStorePort tests/` returns exactly one hit, in a docstring in `tests/test_selection_guard.py` noting that the port is **not** asserted. So the code the comment beside it calls *"the whole mechanism"* is carried by no check at all — ADR-014 §1, in the file that this change edited to say so. - -*It fails open.* The refusal is `if success is False`, and `success` is resolved by `getattr(result, "success", None)` with a `to_dict()` fallback. A result object that is neither shape yields `None`, which is not `False`, so the upload is accepted. That is the wrong polarity for a repository whose ADR-003 forbids inferring what should be declared: an unrecognised result is exactly the case where refusing is cheap and guessing is not. The `to_dict()` branch is also dead on the real path — `OperationResult` has a `success` attribute — so it is untested code guarding an untested case. - -Neither is urgent, because `OperationResult.success` is typed `bool` and is never `None` today. Both become live the moment the return contract moves, which is precisely when nobody will be looking at this file. - -Cross-refs: **C-40** (the pipeline-core surface this port wraps), **C-44** (the 3.0.0 bump that is the named trigger), **C-77** (the other unguarded thing on the same delivery leg), ADR-014 §1, #211, #146. - ---- - ## Disagreements ### D-12: Post-Run-0 infrastructure & naming intents — repo rename, internal-store transport, compute co-location @@ -629,6 +577,76 @@ See also C-40 (the inheritance/representation coupling this migration unwinds), ## Resolved Concerns +### C-83: A queryset that fails to import is reported as a queryset that declares the wrong format — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-83 | +| Tier | 2 — no wrong data ships; the delivery refuses, which is correct. What is wrong is the reason it gives, and it gives it on the live FAO path, at the moment someone is trying to fix a failed run. It sends them to edit a file that is already right. | +| Source | `code-review max` (2026-08-03) — the views-pipeline-core 3.0.0 bump review | +| Trigger | The next time the FAO delivery refuses with *"the queryset declares data_format='dataframe'"*, check whether `config_queryset.py` actually imports before editing it. Most likely on a machine missing views-datafactory, or after any change to that file's own imports. | +| Owner | Whoever next touches `_read_historical_frame`'s precondition. The fix is ours — distinguishing the two cases takes one branch. | +| Location | `views_postprocessing/unfao/managers/unfao.py` and the same line in `crafd` — `declared_data_format(self._model_path.get_queryset())` feeding `launch_config.assert_frame_native_historical` | + +Three correct-in-isolation behaviours compose into a lie: + +1. pipeline-core's `ModelPathManager.get_queryset()` catches **any** exception from importing `config_queryset.py`, logs it, and returns `None`. +2. `declared_data_format(None)` returns `'dataframe'` — the documented default for a non-dict. +3. `assert_frame_native_historical('dataframe')` raises: *"the queryset declares `data_format='dataframe'` … **Set `data_format: 'feature_frame'` in the postprocessor's config_queryset**."* + +So a queryset that **failed to import** is indistinguishable from one that **declared the wrong format**, and the operator is told to fix a file that is already correct. Reproduced: in an environment without views-datafactory the real `un_fao` queryset reports `dataframe` while declaring `feature_frame`; in a complete environment the same file reports `feature_frame`. + +This is ADR-003's rule broken by composition rather than by anyone inferring anything: each layer declares faithfully, and the *absence* of an answer is silently given the shape of an answer. Cluster J's disease — *cannot distinguish "no" from "I could not tell"* — reached through a new door, because #126 made this repo depend on `declared_data_format` in the first place. + +**The fix is ours and it is small:** call `get_queryset()` once, and if it returns `None`, refuse with *that* — the queryset could not be imported — rather than passing `None` into a function whose contract is to default. Upstream could also raise instead of returning `None`, but we should not wait for that; we are the ones holding the ambiguous value. + +Cross-refs: **C-44** (the bump whose review found this), **C-40** (the inherited surface it arrives through), Cluster J (the *no* vs *could not tell* family), ADR-003, #126, #149. + +**RESOLVED 2026-08-05 (B4).** `launch_config.assert_queryset_was_importable(queryset)` now runs **before** the format check, and both managers read the queryset once and reuse the value. + +The refusal says what actually happened — *"the postprocessor's config_queryset could not be imported … This is NOT a declaration problem: do not edit data_format until the module imports"* — and steers the operator toward the traceback pipeline-core logged, and toward a missing sibling checkout or dependency. It logs before it raises (ADR-008). + +**Three guards, because order is the fix.** One proves the refusal fires and names the real fault; one proves an importable queryset passes (the format question belongs to the *next* check, and keeping them separate is the whole point); one asserts, per partner, that `get_queryset()` is called exactly once and that importability is checked first. Mutation-proven by deleting the check and by reversing the order — both fail. + +**What is not fixed here, deliberately.** Upstream still returns `None` for any import exception, so the ambiguity exists at its source; we simply stopped passing it into a function whose contract is to default. Raising upstream would be better and is not ours to do — and waiting for it would have left the misleading message on the live FAO path meanwhile. + +--- + +### C-79: `_ContractStorePort.upload`'s result check is called "the whole mechanism" and has no test, and it fails open — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-79 | +| Tier | 3 — the check works today and is correct for what the store actually returns, so nothing is shipping wrong. What is missing is any assertion that it keeps working, plus a polarity that would swallow an unrecognised result rather than refuse it. | +| Source | `code-review max` (2026-08-03) — PR #211 fourth pass, while verifying the corrected comment beside it | +| Trigger | When views-pipeline-core changes what `DatastoreModule.upload_data` returns — a different result type, a renamed field, or a raise where it used to report — check this port still refuses a partial upload. The 3.0.0 bump (C-44) is the next occasion. | +| Owner | Whoever takes the pipeline-core 3.0.0 bump; it is the same reading of the same return contract. | +| Location | `_ContractStorePort.upload` in `views_postprocessing/unfao/managers/unfao.py` and `views_postprocessing/crafd/managers/crafd.py` (byte-identical in both) | + +The port exists because the store **reports** a metadata failure without raising: after the file is uploaded it logs, then returns `OperationResult(success=False, code="PARTIAL_SUCCESS")`. A caller that discards the result ships a file with no metadata document — invisible to the consumer, which is what happened to run-0's historical artifact on 2026-07-27. This check is what converts that into a refusal. + +**Two things are wrong with how it is held.** + +*It is untested.* `grep -rn _ContractStorePort tests/` returns exactly one hit, in a docstring in `tests/test_selection_guard.py` noting that the port is **not** asserted. So the code the comment beside it calls *"the whole mechanism"* is carried by no check at all — ADR-014 §1, in the file that this change edited to say so. + +*It fails open.* The refusal is `if success is False`, and `success` is resolved by `getattr(result, "success", None)` with a `to_dict()` fallback. A result object that is neither shape yields `None`, which is not `False`, so the upload is accepted. That is the wrong polarity for a repository whose ADR-003 forbids inferring what should be declared: an unrecognised result is exactly the case where refusing is cheap and guessing is not. The `to_dict()` branch is also dead on the real path — `OperationResult` has a `success` attribute — so it is untested code guarding an untested case. + +Neither is urgent, because `OperationResult.success` is typed `bool` and is never `None` today. Both become live the moment the return contract moves, which is precisely when nobody will be looking at this file. + +Cross-refs: **C-40** (the pipeline-core surface this port wraps), **C-44** (the 3.0.0 bump that is the named trigger), **C-77** (the other unguarded thing on the same delivery leg), ADR-014 §1, #211, #146. + +**RESOLVED 2026-08-05 (B4).** Two changes, and the second is the one that mattered. + +**Polarity.** `if success is False` became `if success is not True`. The old form failed **open**: a result that was `None`, or lacked the attribute, or carried a non-bool, sailed through as though the upload had worked. The dead `to_dict()` fallback went with it — an unrecognised result should be refused and *named*, not adapted to silently. The refusal now reports what it actually received, because `success=None` (a moved contract) and `success=False` (a reported failure) are different faults and send an operator to different places. + +**Tests, where there were none.** `tests/test_store_port.py` — 16 tests over both partners: the happy path, a reported failure carrying the store's own error, four unrecognised-result shapes, and the field list the port forwards. Mutation-proven by reverting the polarity, which fails four of them. + +**The standing excuse never applied here.** Manager-side facts are source-scanned because the managers need Appwrite env and a views-models path manager to instantiate. `_ContractStorePort` needs neither — it takes a store object and calls four methods on it. A fake store was always enough; nobody had tried. + +**Its trigger fired two days before this and nobody noticed.** The entry read *"the 3.0.0 bump is the next occasion"*; the bump landed 2026-08-03, C-44 was closed with a wheel-level verification of the suite, and the return contract was never re-read. `test_register_integrity.py` cannot catch that — its checks are structural and none evaluates whether a named external event has occurred. That gap is C-82's. + +--- + ### C-75: `GaulLookupEnricher` has no production caller, and now implements a second copy of the delivery path's keyed gather — RESOLVED | Field | Value | diff --git a/tests/test_env_declaration.py b/tests/test_env_declaration.py index 8be6bd1..9d3fa7b 100644 --- a/tests/test_env_declaration.py +++ b/tests/test_env_declaration.py @@ -263,6 +263,66 @@ def test_declared_names_match_the_manager_reads(partner): assert text.count("appwrite_env.assert_env_declared(") == 2 +def test_a_queryset_that_failed_to_import_is_refused_as_that(caplog): + """Register C-83 — the refusal must name the real fault, not a plausible one. + + `get_queryset()` returns ``None`` for **any** exception while importing + ``config_queryset.py``. Feeding that to ``declared_data_format`` yields + ``'dataframe'`` (its documented default for a non-dict), and the format guard then + tells the operator to set ``data_format: 'feature_frame'`` — in a file that already + says exactly that. + + Reproduced both ways on 2026-08-03: in an environment without views-datafactory the + real ``un_fao`` queryset reported ``dataframe`` while declaring ``feature_frame``; + in a complete environment the same file reported ``feature_frame``. + """ + import logging + + with caplog.at_level(logging.ERROR): + with pytest.raises(launch_config.LaunchConfigError) as excinfo: + launch_config.assert_queryset_was_importable(None) + + message = str(excinfo.value) + assert "could not be imported" in message + assert "data_format" in message and "do not edit" in message.lower(), ( + "the refusal must actively steer the operator AWAY from the config file — " + "that is the whole point, since the old message steered them into it" + ) + assert any(r.levelno >= logging.ERROR for r in caplog.records), ( + "ADR-008: a structural refusal leaves a persistent record, not just a traceback" + ) + + +def test_an_importable_queryset_passes_the_readability_check(): + """The guard must not stand in front of a queryset that imported fine. + + Anything not-None passes here; whether it declares the right format is the *next* + check's question, and keeping them separate is the entire fix. + """ + launch_config.assert_queryset_was_importable({"data_format": "feature_frame"}) + launch_config.assert_queryset_was_importable({}) # wrong, but importable + + +def test_both_managers_check_importability_before_asking_what_was_declared(): + """Order matters: reversed, the confusing message wins again. + + Source-scan because the managers need Appwrite env and a views-models path manager + to instantiate — the standing pattern for manager-side facts. + """ + for partner in _PARTNERS: + source = _manager_source(partner).read_text() + assert source.count("get_queryset()") == 1, ( + f"[{partner}] the queryset must be read ONCE and the value reused; reading " + "it twice invites the two checks to disagree about what they saw" + ) + importable = source.index("assert_queryset_was_importable") + declared = source.index("assert_frame_native_historical") + assert importable < declared, ( + f"[{partner}] the importability check must come FIRST. Reversed, a queryset " + "that failed to import is still reported as one declaring 'dataframe'." + ) + + # ── ADR-008 across BOTH entry validators (S1 / #182, register C-71) ────────── # # `appwrite_env` asserts the launcher assembled the *environment*; `launch_config` diff --git a/tests/test_store_port.py b/tests/test_store_port.py new file mode 100644 index 0000000..cc99094 --- /dev/null +++ b/tests/test_store_port.py @@ -0,0 +1,155 @@ +"""The store port's refusal, tested — register C-79. + +**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 +file is uploaded, logs the error and **returns** ``success=False`` rather than raising. +A caller that discards the result therefore ships a file with no metadata document — +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. + +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 +and calls four methods on it. A fake store is enough, so the excuse never applied here. +""" + +from __future__ import annotations + +from dataclasses import dataclass + +import pytest + +from tests.conftest import PARTNER_PACKAGES + + +@dataclass +class _Result: + """Shaped like pipeline-core's ``OperationResult``: a ``success`` bool plus error.""" + + success: object + error: str | None = None + + +class _FakeStore: + """Records the upload and returns whatever result the test declares.""" + + def __init__(self, result): + self.result = result + self.calls = [] + + def upload_data(self, **kwargs): + self.calls.append(kwargs) + return self.result + + +def _port(partner: str, result): + """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"] + ) + store = _FakeStore(result) + return module._ContractStorePort(store), store + + +def _upload(port, tmp_path): + payload = tmp_path / "artifact.parquet" + payload.write_bytes(b"x") + port.upload( + payload, + filename=payload.name, + name="un_fao", + doc_type="model", + category="historical", + loa="pgm", + targets=["lr_ged_sb"], + description="{}", + ) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_a_successful_upload_is_accepted(partner, tmp_path): + """The happy path must not raise, or the guard would block every delivery.""" + port, store = _port(partner, _Result(success=True)) + _upload(port, tmp_path) + assert len(store.calls) == 1, "the port must forward exactly one upload to the store" + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_a_reported_failure_is_refused_and_names_the_error(partner, tmp_path): + """The defect this port exists for: uploaded file, failed metadata, success=False.""" + port, _ = _port(partner, _Result(success=False, error="metadata storage failed")) + with pytest.raises(RuntimeError) as excinfo: + _upload(port, tmp_path) + message = str(excinfo.value) + assert "metadata storage failed" in message, ( + "the refusal must carry the store's own error — an operator debugging a partial " + "upload should not have to go find it" + ) + assert "orphan" in message, ( + "the refusal must say what the failure MEANS: a file with no metadata document, " + "which is invisible to the consumer rather than absent" + ) + + +@pytest.mark.parametrize( + "result, why", + [ + (None, "the store returned nothing at all"), + (_Result(success=None), "success is None — the fail-open case C-79 named"), + (_Result(success="ok"), "success is a truthy non-bool"), + (object(), "the result has no success attribute"), + ], +) +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_an_unrecognised_result_is_refused_rather_than_assumed_good( + partner, result, why, tmp_path +): + """Fail CLOSED. This is the polarity fix, and it is the whole of C-79. + + The check was ``if success is False``, so every case above sailed through as though + the upload had worked. Today ``upload_data`` has one return path and ``success`` is + a ``bool``, so the two polarities agree — but "today" is doing a lot of work in that + sentence, and this entry's own trigger is *when pipeline-core changes what + ``upload_data`` returns*. Fail-open is the wrong side to be on when the question is + whether a partner delivery actually landed. + """ + port, _ = _port(partner, result) + with pytest.raises(RuntimeError, match="did not fully succeed"): + _upload(port, tmp_path) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_refusal_names_what_it_actually_got(partner, tmp_path): + """A refusal an operator cannot act on is barely better than no refusal. + + ``success=None`` and ``success=False`` are different faults — one is a store that + reported a failure, the other a store whose contract moved. The message has to + separate them or the next person re-runs the delivery instead of reading a changelog. + """ + port, _ = _port(partner, _Result(success=None)) + with pytest.raises(RuntimeError) as excinfo: + _upload(port, tmp_path) + assert "success=None" in str(excinfo.value) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_port_forwards_every_declared_field(partner, tmp_path): + """The port is a four-method seam; dropping a field here loses it silently. + + ``name`` in particular is what the consumer filters on (register C-77) — an upload + that arrives without it is stored, billed, and unretrievable. + """ + port, store = _port(partner, _Result(success=True)) + _upload(port, tmp_path) + forwarded = store.calls[0] + # Note `doc_type` -> `type`: the port renames it on the way through, because + # `type` is the store's field name and a builtin here. That rename is the kind of + # thing a seam quietly loses, which is why the field list is asserted rather than + # assumed. + for field in ("file", "filename", "name", "type", "category", "loa", "targets", + "description"): + assert field in forwarded, f"the port dropped {field!r} on its way to the store" + assert forwarded["name"] == "un_fao" + assert forwarded["category"] == "historical" + assert forwarded["type"] == "model", "doc_type must arrive as the store's `type`" diff --git a/views_postprocessing/contract/launch_config.py b/views_postprocessing/contract/launch_config.py index 2b88f56..8ebcafb 100644 --- a/views_postprocessing/contract/launch_config.py +++ b/views_postprocessing/contract/launch_config.py @@ -65,6 +65,46 @@ def assert_contract_mode(configs: dict) -> None: raise LaunchConfigError(err_msg) +def assert_queryset_was_importable(queryset: object | None) -> None: + """Raise if the queryset could not be read at all — before asking what it declares. + + **Register C-83: three correct behaviours composing into a lie.** pipeline-core's + ``ModelPathManager.get_queryset()`` catches any exception from importing + ``config_queryset.py``, logs it, and returns ``None``. ``declared_data_format(None)`` + then returns ``'dataframe'`` — the documented default for a non-dict. And + ``assert_frame_native_historical('dataframe')`` says: + + the queryset declares data_format='dataframe' … Set data_format: + 'feature_frame' in the postprocessor's config_queryset + + ...pointing at a file that already says ``feature_frame``. A queryset that **failed + to import** was indistinguishable from one that **declared the wrong format**, and + the message arrived on the live FAO path while someone was fixing a failed run. + + Each layer is faithful on its own. What goes wrong is that the *absence* of an + answer is silently given the shape of an answer — ADR-003's rule broken by + composition rather than by anyone inferring anything. + + Args: + queryset: the return of ``model_path.get_queryset()``. + + Raises: + LaunchConfigError: saying the queryset could not be imported, which is a + different problem from a queryset that declares the wrong thing. + """ + if queryset is None: + err_msg = ( + "the postprocessor's config_queryset could not be imported — " + "`get_queryset()` returned None, which it does for ANY exception raised " + "while importing that module (pipeline-core swallows it and logs). This is " + "NOT a declaration problem: do not edit data_format until the module " + "imports. Check the traceback pipeline-core logged just above this, and a " + "missing sibling checkout or dependency first." + ) + logger.error(err_msg) # ADR-008: logged persistently AND raised + raise LaunchConfigError(err_msg) + + def assert_frame_native_historical(data_format: str | None) -> None: """Raise unless the queryset descriptor declares the frame-native historical read. diff --git a/views_postprocessing/crafd/managers/crafd.py b/views_postprocessing/crafd/managers/crafd.py index fccfe0c..87c09d2 100644 --- a/views_postprocessing/crafd/managers/crafd.py +++ b/views_postprocessing/crafd/managers/crafd.py @@ -56,16 +56,24 @@ def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, # 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. - # (Until 2026-08-03 this comment said the store "only LOGS": false, and - # self-defeating — if it only logged, `success` would be True.) + # + # **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 None and hasattr(result, "to_dict"): - success = result.to_dict().get("success") - if success is False: + 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 " - f"orphan without a metadata document): {error}" + 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__})." ) @@ -118,9 +126,13 @@ def _read_historical_data(self): """ # Declaration first: the check reads the queryset, not the loader, so a # refused config must not pay for loader construction. - launch_config.assert_frame_native_historical( - declared_data_format(self._model_path.get_queryset()) - ) + # + # Read ONCE, and distinguish "could not import it" from "it declares the wrong + # thing" (register C-83). Passing None straight into `declared_data_format` + # turns a failed import into a confident, wrong claim about the config. + queryset = self._model_path.get_queryset() + launch_config.assert_queryset_was_importable(queryset) + launch_config.assert_frame_native_historical(declared_data_format(queryset)) self._initialize_data_loader() self._read_historical_frame() diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index cd61b25..36388e2 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -56,16 +56,24 @@ def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, # 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. - # (Until 2026-08-03 this comment said the store "only LOGS": false, and - # self-defeating — if it only logged, `success` would be True.) + # + # **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 None and hasattr(result, "to_dict"): - success = result.to_dict().get("success") - if success is False: + 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 " - f"orphan without a metadata document): {error}" + 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__})." ) @@ -118,9 +126,13 @@ def _read_historical_data(self): """ # Declaration first: the check reads the queryset, not the loader, so a # refused config must not pay for loader construction. - launch_config.assert_frame_native_historical( - declared_data_format(self._model_path.get_queryset()) - ) + # + # Read ONCE, and distinguish "could not import it" from "it declares the wrong + # thing" (register C-83). Passing None straight into `declared_data_format` + # turns a failed import into a confident, wrong claim about the config. + queryset = self._model_path.get_queryset() + launch_config.assert_queryset_was_importable(queryset) + launch_config.assert_frame_native_historical(declared_data_format(queryset)) self._initialize_data_loader() self._read_historical_frame()