Skip to content

fix(delivery): C-79 and C-83 — two refusals on the live FAO path #222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Polichinel merged 1 commit into from
Aug 5, 2026
Merged

fix(delivery): C-79 and C-83 — two refusals on the live FAO path #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 72 additions & 54 deletions reports/technical_risk_register.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |

---

Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 |
Expand Down
60 changes: 60 additions & 0 deletions tests/test_env_declaration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
Loading
Loading