diff --git a/README.md b/README.md index b9b74f8..f2e071f 100644 --- a/README.md +++ b/README.md @@ -135,17 +135,24 @@ views-postprocessing/ │ ├── source_metadata.py # producer (datafactory) facts │ ├── store_metadata.py # prediction-store facts │ └── launch_config.py # the delivery mode the launcher must declare - ├── unfao/ # WHO A DELIVERY IS FOR — the only FAO-specific code + ├── unfao/ # WHO A DELIVERY IS FOR — the FAO-specific code │ ├── product.py # targets, consumer name, S_MIN, upload interlock │ ├── appwrite_env.py # the declared store coordinates │ └── managers/unfao.py # UNFAOPostProcessorManager + ├── crafd/ # WHO A DELIVERY IS FOR — the CRAF'd-specific code + │ ├── product.py # same three files, same shape (register C-33 on + │ ├── appwrite_env.py # why the manager is a copy, and what would + │ └── managers/crafd.py # make it time to stop copying) └── data/gaul_lookup.parquet # the precomputed GAUL lookup (ADR-011) ``` -**Dependencies point one way only:** `unfao/` → `contract/` → `delivery/`. Nothing in -`contract/` may import `unfao/` — that is what lets a new partner reuse the machinery -without inheriting FAO, and it is enforced by `tests/test_clone_readiness.py`, not by -convention. See [`docs/CLONING.md`](docs/CLONING.md). +**Dependencies point one way only:** `/` → `contract/` → `delivery/`. Nothing +in `contract/` may import a partner package — that is what lets a new partner reuse the +machinery without inheriting another partner's product, and it is enforced by +`tests/test_clone_readiness.py`, not by convention. The partner list lives in one place +(`tests/conftest.py`) and is itself checked against the filesystem, so a package added +without being declared fails rather than passing quietly. +See [`docs/CLONING.md`](docs/CLONING.md). --- diff --git a/docs/ADRs/012_revised_ontology.md b/docs/ADRs/012_revised_ontology.md index 79dcc3e..433e70a 100644 --- a/docs/ADRs/012_revised_ontology.md +++ b/docs/ADRs/012_revised_ontology.md @@ -40,8 +40,21 @@ packages answer three different questions, and every category below names the on delivery/ what makes a delivery VALID — representation-free invariants contract/ how a delivery is BUILT — partner-neutral machinery unfao/ who a delivery is FOR — one partner's product and manager +crafd/ who a delivery is FOR — another partner's product and manager ``` +**Amended 2026-08-03 (#211): the third row repeats.** `crafd/` joined `unfao/` as a +second partner package. This does not add a fourth category — a partner package is a +partner package, and the closed set is unchanged. What it changes is that *"who a +delivery is FOR"* is answered N times rather than once, and every claim below that said +**"the"** partner or **"one"** module now says how many. + +A partner package is the shape a partner takes **inside this repository**. It is not +the same thing as a partner *repository*: `views-crafdapi` and `views-productionapi` +are consumer APIs cut from `views-faoapi`, and this repository is the single producer +that serves all of them. `docs/CLONING.md` was written before that was settled and +described the cut-a-repo case; it now says which is which. + | Category | Purpose | Authority | Stability | |----------|---------|-----------|-----------| | **Delivery Invariants** | Representation-free rules over primitives that a delivery must satisfy: coverage, no-collapse, gid parity, observed-range, provenance. Live in `delivery/` — **nothing there imports pandas or views_frames**. *Forecast identity was one of these until 2026-07-31 — see the amendment below.* | Authoritative — they define what a valid delivery is | Stable — changes are governance decisions | @@ -51,8 +64,8 @@ unfao/ who a delivery is FOR — one partner's product and manager | **Artifact Builders** | `contract/historical.py` — turns a frame plus the lookup into the partner-facing artifact. | Derived | Evolving | | **External Facts** | Facts read from systems this repo does not own: the producer's (`contract/source_metadata.py` — `last_valid_month_id`, D-07) and the store's (`contract/store_metadata.py`). | Authoritative (the owning system is the source of truth) | Evolving | | **Launch Declarations** | `contract/launch_config.py` — the delivery mode the launcher must declare. Omitting a key is **refused by name**, never inferred (ADR-003, register C-63). | Authoritative | Stable | -| **Partner Product** | `unfao/product.py` (targets, consumer document name, collapse floor, upload interlock) and `unfao/appwrite_env.py` (the store coordinates). **This is what a clone replaces.** | Authoritative — one reason to change: the partner relationship | Evolving | -| **Pipeline Manager** | `unfao/managers/unfao.py` — a concrete pipeline-core postprocessor (Template-Method subclass) that *orchestrates* read/transform/validate/save and **calls** the invariants, never inherits them. **It is the only module in the repository that imports `views_pipeline_core`** — the coupling C-40 describes is one file wide. It is not yet *thin*: 406 lines, down from 636 (#149). | Derived | Evolving | +| **Partner Product** | Per partner: `/product.py` (targets, consumer document name, collapse floor, upload interlock) and `/appwrite_env.py` (the store coordinates). Two exist — `unfao/` and `crafd/` (#211). **This pair plus the Pipeline Manager below is what a new partner supplies** — three files, as `docs/CLONING.md` states them. | Authoritative — one reason to change: that partner relationship | Evolving | +| **Pipeline Manager** | One per partner: `/managers/.py` — a concrete pipeline-core postprocessor (Template-Method subclass) that *orchestrates* read/transform/validate/save and **calls** the invariants, never inherits them. **These are the only modules in the repository that import `views_pipeline_core`**, pinned to an explicit allowlist by `tests/test_doc_accuracy.py` — the coupling C-40 describes is one file per partner. Neither is yet *thin* — each sits just under the 450-line budget `tests/test_doc_accuracy.py` holds them to, down from 636 (#149), and since #211 the second is a near-verbatim copy of the first — deliberate WET with a named extraction trigger, recorded in register **C-33**. | Derived | Evolving | | **Derived Outputs** | Arrow shards, the GAUL sidecar, the run manifest and the historical parquet, produced per run and delivered to the partner store. | Ephemeral | Ephemeral | **Two claims this ADR made until 2026-08-01, both now corrected rather than quietly dropped** @@ -78,9 +91,16 @@ waiting for an audit. - **Screaming architecture:** the categories match the package layout — `delivery/` (invariants), `contract/` (the machinery: `wire/`, the `frame_extraction.py` seam, the - GAUL asset, artifact builders, external-fact readers), `unfao/` (the partner's product and - its manager). A reader can infer responsibilities from the structure, and the one-way - dependency `unfao/ → contract/ → delivery/` is enforced by test, not convention. + GAUL asset, artifact builders, external-fact readers), and one package per partner — + `unfao/`, `crafd/` — each holding that partner's product and its manager. A reader can + infer responsibilities from the structure, and the one-way dependency + `/ → contract/ → delivery/` is enforced by test, not convention — **both legs + of it, for every declared partner, since #211.** Neither was fully true before: + `test_contract_package_does_not_import_any_partner` named `unfao`, so `contract/` was + free to import `crafd`; and the `contract/ → delivery/` leg had no test at all until + `test_the_invariants_do_not_import_the_machinery` was written — a documented arrow + that half existed, which is worse than an undocumented one because a reader stops + checking. - **DIP / OCP:** primitives are the abstraction the invariants depend on; the representation seam is the single point of change for a representation migration (C-40), so the invariants are closed against it. diff --git a/docs/CICs/UNFAOPostProcessorManager.md b/docs/CICs/UNFAOPostProcessorManager.md index 2629c93..b1a4d77 100644 --- a/docs/CICs/UNFAOPostProcessorManager.md +++ b/docs/CICs/UNFAOPostProcessorManager.md @@ -105,7 +105,7 @@ The following **must never** fail silently: - PRIO-GRID geometry details - The internals of how the lookup table was built -This anchors the class within ADR-002 (topology): `unfao/` → `contract/` → `delivery/`, one way only. It is **the repository's only importer of `views_pipeline_core`** (mechanically pinned by `tests/test_doc_accuracy.py`), which is what makes C-40's blast radius one file wide. 406 lines as of epic #148, down from 636 — not yet *thin*, and held under a 450-line budget by the same test. +This anchors the class within ADR-002 (topology): `unfao/` → `contract/` → `delivery/`, one way only — and since #211 the same holds for `crafd/`, the second partner package. It is one of **the repository's only two importers of `views_pipeline_core`** (both mechanically pinned to an allowlist by `tests/test_doc_accuracy.py`), which keeps C-40's blast radius at one file per partner. Not yet *thin*: it came down from 636 lines at epic #148 and now sits just under a **450-line budget**, which `tests/test_doc_accuracy.py` applies to the whole `managers/` directory of each partner rather than to this file alone — a seam that holds its line count by moving 800 lines into a sibling module has not held anything. The exact figure is deliberately not repeated here; the test carries it. --- diff --git a/docs/CLONING.md b/docs/CLONING.md index 40145e8..dd1c20c 100644 --- a/docs/CLONING.md +++ b/docs/CLONING.md @@ -1,7 +1,19 @@ -# Cloning this repository for a new partner - -Read this **before** cutting `views-crafdapi`, `views-productionapi`, or any future -partner delivery. It is short on purpose. +# Adding a new partner delivery + +Read this **before** adding a partner package to this repository, or cutting a partner +API repository that consumes one. It is short on purpose. + +> **Corrected 2026-08-03 (#211). This document used to say "before cutting +> `views-crafdapi`", and that framed the job wrongly.** `views-crafdapi` and +> `views-productionapi` are **consumer** APIs, cut from `views-faoapi`. They do not +> clone *this* repository. This repository is the single **producer** that serves every +> partner, and a new partner is added here as a package alongside `unfao/` and +> `crafd/` — not as a new producer repo. +> +> The three-things-to-supply structure below survives that correction unchanged, because +> it was always describing the same three files. What changes is where they go: into a +> new directory in this repo, not into a new repository. The "Hard rules" section below +> is where the distinction actually mattered, and it is corrected there too. ## What you get for free @@ -16,10 +28,18 @@ partner package arrives with them). ## What you must supply -Three things. They are the only FAO-specific files in the repository, so the shape of -your work is: **replace these three, keep everything else.** +Three things. They are the only partner-specific files in the repository, so the shape +of your work is: **copy these three from an existing partner, change them, keep +everything else.** `crafd/` is the worked example. One caveat if you read it as a +template: it did *not* copy `unfao/managers/README.md`, the operational summary that +sits beside the FAO manager. That was an omission rather than a decision — write one. + +Register them as you add them: `tests/conftest.py` holds the repository's single +declared partner list (`PARTNER_PACKAGES`), and a partner missing from it is exempt +from every guard below. A test asserts that list against the filesystem, so forgetting +fails CI rather than passing quietly — which is what happened when `crafd/` landed. -### 1. Your product — `unfao/product.py` +### 1. Your product — `/product.py` Four declarations, and nothing may be inferred: @@ -32,25 +52,78 @@ Four declarations, and nothing may be inferred: - **`UPLOAD_ENABLED`** — the interlock. **Leave it `False`** until your consumer's selection guard is deployed in production, not merely merged. -### 2. Your store coordinates — `unfao/appwrite_env.py` +### 2. Your store coordinates — `/appwrite_env.py` The env-var names your delivery requires, validated fail-loud before any store is constructed. Names come from the **Appwrite Seam Contract's coordinate registry** (homed in views-appwrite) and are referenced **by URL at a pinned commit, never copied**. The secret stays an operator slot. -### 3. Your manager — `unfao/managers/unfao.py` +### 3. Your manager — `/managers/.py` + +The pipeline-core seam. **The partner managers are the only modules in the repository +that import `views_pipeline_core`**, and a test holds them to an explicit allowlist. +Yours will orchestrate read → transform → validate → save and *call* the invariants — +never inherit them. + +Today the two managers are near-identical. See for yourself rather than trusting a +number here — the number went stale twice while this paragraph was being written: + +``` +diff views_postprocessing/unfao/managers/unfao.py \ + views_postprocessing/crafd/managers/crafd.py +``` + +Sixteen lines differ on each side and **none of them changes behaviour**: the import, +the class name, the two partner-named methods and their two call sites, the four +env-var literals, one line that both selects which env tuple is validated and labels +the store, one runtime refusal message, and four lines of prose. -The pipeline-core seam. **This is the only module in the repository that imports -`views_pipeline_core`**, and a test keeps it that way. Yours will orchestrate -read → transform → validate → save and *call* the invariants — never inherit them. +**That is deliberate** — WET before DRY, and the second copy is what finally showed the +seam is a config object rather than a behavioural one. Register **C-33** carries the +extraction trigger: a **third** in-repo partner, or the first bug that has to be +hand-patched identically in both files. If you are the third, read C-33 before copying +a fourth time. ## Hard rules, and why each exists -**Do not import `views_pipeline_core.modules.{appwrite,datastore}`.** -þing-02 **S24(5)**, binding. This repository's own import of those is how a two-repo -defect became three (register C-40); pipeline-core declines to offer the surface, and -that refusal is deliberate. Write a thin client against the SDK, as views-faoapi did. +**Your manager may import `views_pipeline_core.modules.{appwrite,datastore}`. +Nothing else in this repository may — and you add yourself to the allowlist by hand.** +`tests/test_doc_accuracy.py::test_views_pipeline_core_is_confined_to_the_partner_managers` pins the +importer set to an explicit list of manager files. Adding a partner means editing that +list deliberately. That is the cost of a new partner, not a formality: the coupling is +bounded only because someone has to write the file's name down. + +Pipeline-core declines to export that surface, and this repository's own import of it is +how a two-repo defect became three (register **C-40**). Unwinding it is deferred under +issue **#146** behind a **named trigger — þ01-D8's supply trigger firing on the C-221 +decomposition, explicitly not on this repository's convenience**. Do not read the +deferral as "not done yet"; it is a decision with a condition attached (ADR-014 §4). + +**If you are cutting a consumer API repo, the rule inverts: do *not* import them.** +þing-02 **S24(5)** binds the repositories cut from views-faoapi — `views-crafdapi` +(the þing records call it `un-crafdapi`) and `views-productionapi`. It does not reach a +partner package inside this producer, which is why `crafd/managers/crafd.py` may import +what a consumer API may not. An earlier version of this document cited the verdict as a +flat prohibition and over-claimed it. Write a thin client against the SDK, as +views-faoapi did. + +**Check the store's result. It is load-bearing, not boilerplate.** +`_ContractStorePort.upload` inspects `result.success` and raises. It looks like +defensive noise and is not. When metadata storage fails after the file is already +uploaded, the pipeline-core store logs the error and **returns +`OperationResult(success=False, code="PARTIAL_SUCCESS")`** — it reports the failure +faithfully and simply does not raise. A caller that discards the result therefore +proceeds as though the delivery were complete, leaving a file with no metadata +document: invisible to the consumer, exactly like a wrong document name. That happened +to run-0's historical artifact on 2026-07-27. + +This matters more than it reads, because of a date. þing-02 **D10/S30** required this +repository's legacy path to be guarded or retired **before 2026-11-30**, when the +current key expires — an unguarded path on that day *reports success and ships nothing*. +The legacy path itself was retired in #149 (register C-63), so what the obligation now +amounts to is keeping this guard on the contract path. If you copy a manager you inherit +it; do not tidy it away. **Get your own key before the first run, not after.** Free at t=0, a migration later. One key per identity per environment (the Appwrite Seam Contract diff --git a/docs/architecture/role_and_seams.md b/docs/architecture/role_and_seams.md index 7813b2c..1a4e99a 100644 --- a/docs/architecture/role_and_seams.md +++ b/docs/architecture/role_and_seams.md @@ -14,7 +14,10 @@ metadata, guards their integrity, and delivers them to a partner store** — it **post-forecast delivery layer**, not a spatial-mapping library and not a statistical post-processor. -The only live consumer today is the **UN FAO** delivery (`views_postprocessing/unfao/`). +Two partner deliveries live here: the **UN FAO** one +(`views_postprocessing/unfao/`), delivering to FAO-FSFC since 2026-07-27, and +**CRAF'd** (`views_postprocessing/crafd/`), added 2026-08-03 with its upload interlock +still closed. They are peers — one producer, one partner package each. --- @@ -182,11 +185,17 @@ views_postprocessing/ │ ├── source_metadata.py producer (datafactory) facts, e.g. last_valid_month_id │ ├── store_metadata.py prediction-store facts │ └── launch_config.py the delivery mode the launcher must declare -├── unfao/ WHO A DELIVERY IS FOR — the only FAO-specific code +├── unfao/ WHO A DELIVERY IS FOR — the FAO-specific code, and only that │ ├── product.py targets, consumer document name, S_MIN, upload interlock │ ├── appwrite_env.py the declared store coordinates -│ └── managers/unfao.py UNFAOPostProcessorManager (406 lines; the only importer -│ of views_pipeline_core) +│ └── managers/unfao.py UNFAOPostProcessorManager +├── crafd/ WHO A DELIVERY IS FOR — the CRAF'd-specific code (same three +│ │ files, same shape; register C-33 on why it is a copy) +│ ├── product.py +│ ├── appwrite_env.py +│ └── managers/crafd.py CRAFDPostProcessorManager +│ the two managers are the ONLY importers of +│ views_pipeline_core — one per partner, allowlisted by test └── data/gaul_lookup.parquet the precomputed GAUL lookup (ADR-011) ``` diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 0b0a8e0..787473f 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -4,10 +4,10 @@ |-------------------|--------------------------------------| | Project | views-postprocessing | | Owner | Dylan Pinheiro / PRIO MD&D Team | -| Last Updated | 2026-08-02 | -| Total Concerns | 76 | -| Open Concerns | 17 | -| Resolved Concerns | 59 | +| Last Updated | 2026-08-03 | +| Total Concerns | 79 | +| Open Concerns | 19 | +| Resolved Concerns | 60 | --- @@ -32,6 +32,7 @@ covered a single open entry (see Historical clusters below). ### Cluster G: Inherited pipeline-core surface **Root cause:** this repo *is-a* pipeline-core postprocessor by double inheritance, so it inherits that project's data loader, container, store I/O, and dependency tree — defects in that surface land in FAO delivery without this repo owning the fix. **Entries:** C-40 (root), C-07, C-13, C-26, C-27, C-28, C-29, C-44, C-58, C-62 +**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. **Resolution scope:** Partial — C-26/C-27/C-28 are upstream-owned; de-inheritance makes them visible and testable, not fixed. @@ -316,8 +317,9 @@ See also D-10 (handling decision), C-43 (the *value*-correctness sibling — run | ID | C-33 | | Tier | 2 — two to three additional Appwrite stores are planned imminently; the current design forces copy-pasting a 273-line manager per store | | Source | `expert-code-review` (2026-06-12) | -| Trigger | When the second Appwrite prediction store is configured (issue #97 scoping), verify store identity comes from configuration — the env **names** are now centrally declared, but the three `AppwriteConfig` constructions, the targets list, and the category strings are still inline per-store | -| Location | `views_postprocessing/unfao/managers/unfao.py:148-204` (`_prod_forecasts_datastore`), `:495-517` (`_unfao_datastore`/`_unfao_appwrite_config`), `:528-534` (legacy `_save`); declared names in `views_postprocessing/unfao/appwrite_env.py` | +| Trigger | **Fired 2026-08-03 — see the update below.** The remaining trigger is the *extraction* one, and it is now named: a **third** in-repo partner package, **or** the first bug that must be hand-patched identically in both manager files — whichever comes first. | +| Owner | Whoever adds the third partner package, or hits the first double-patch. Until one of those happens the duplication is the deliberate WET position, not a task anyone is behind on. | +| Location | `views_postprocessing//managers/.py` — `_prod_forecasts_datastore`, `__datastore`, `__appwrite_config`, and the four hardcoded `os.getenv("APPWRITE__*")` literals inside the last of those; declared names in each partner's `appwrite_env.py`. **Symbols, not line numbers** — see the note under the measurement below. | Mitigation: a small `DeliveryProfile` (bucket/collection/database ids, category, targets) passed to the manager — one manager class, N store configs. Scheduled **after** the FAO global delivery ships (D-09); the only immediate action is deleting the commented-out config blocks at lines 80-107, which are a mis-uncomment hazard during deadline work. @@ -327,7 +329,38 @@ 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. -See also C-24 (schema contract per store), D-09 (the deferral, now expired), #97 (second-store scoping). +**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 + + diff views_postprocessing/unfao/managers/unfao.py \ + views_postprocessing/crafd/managers/crafd.py | grep -c '^[<>]' + +**32** — sixteen differing lines on each side. Substitute every form of the partner name (case-insensitively, including `un_fao`/`un_crafd` and `faoapi`) and it falls to **2**: one line per side. + +The sixteen are, by category: one import, one class name, two partner-named method definitions, their two call sites, one refusal-message string, one line that is *both* the `*_ENV` tuple reference and the store label, the four env-name literals, and four lines of prose. + +**None of the difference is behaviour, but "byte-identical" is too strong for one method.** `_read`, `_transform`, `_validate`, `_check_coverage` and `_build_historical_artifact` are byte-identical. `_save_contract` is not: five of the sixteen fall inside it — the datastore call, two comments, and the refusal string. All five are partner-name substitutions; none changes what the method does. + +*(**This paragraph was wrong five times, and how it was wrong is the entry's most useful content.** (1) "roughly ten lines", carried from the review that found it and never measured. (2) A normalised count of 4 and a claim that `_save_contract` was byte-identical, neither checked. (3) A story that #211 "fixed two divergences that already existed" — false: at `9799e87` the second line was **byte-identical in both files**, an inherited inaccuracy rather than a divergence, and rewording CRAF'd's copy is what *created* a divergence there. Only the `:222` pair was real. (4) and (5) An exact list of sixteen line numbers and a line count, invalidated twice within the hour by comment corrections elsewhere in the same file.* + +*The fix was not a sixth careful re-count. **Neither this measurement nor any `Location` field in C-33, C-40, C-77 or C-79 states a line number any more** — they name symbols, which grep can find and which survive an edit above them. There was a sixth failure, and it is why: a draft of this very paragraph announced that the entry "no longer states line numbers" while its own `Location` row still carried six, every one of them shifted by four lines by a comment correction made in the same commit. `tests/test_doc_accuracy.py` had already written the rule down — "The FILE is the claim; the line number is not." A count a reader relies on is a claim under ADR-014 §1, and each of these six was written by someone who believed it.)* + +**The duplication is the right call today, and this is the record that says why.** CLAUDE.md's WET rule asks for a *second incident* before extracting, and this is genuinely it: one implementation showed nothing, two show that the seam is trivial — a partner-config object, not a behavioural one. The two candidate abstractions are both worse than the copy. A shared base class would stack a second, repo-owned Template Method under pipeline-core's imposed one, producing a three-tier inheritance chain to deduplicate ~10 lines; it would also have to live somewhere, and `contract/` is pinned pipeline-core-free by `tests/test_clone_readiness.py`. A factory solves a dispatch problem this system does not have — each partner is wired explicitly by its own launcher config, and nothing selects a manager class at runtime. + +**What was missing was the trigger, and ADR-014 §4 says a deferral without one is not a deferral.** It is now in the Trigger field above. Both halves matter: a *third* package is the point at which "two copies you can hold in your head" becomes sprawl, and the *first double-patched bug* is the point at which the copies start costing correctness rather than bytes. + +**Drift is the live risk, and both directions of it showed up immediately.** + +*A real divergence the copy created.* At `9799e87`, `unfao/managers/unfao.py:222` named the artifact builder as `unfao/historical.py` — a path retired by #153 — while the fresh copy said `contract/historical.py` and was correct. **The clone silently fixed a stale reference in the original and the fix never propagated back.** #211 fixed the original too. + +*An inherited inaccuracy that was not a divergence — until fixing it made one.* Both files carried *"same artifact shape faoapi already ingests"*. Identical, so no diff flagged it; wrong for CRAF'd, whose consumer is not faoapi. #211 reworded CRAF'd's copy, which is correct for both files and **puts that line into the raw diff for the first time**. A copy can therefore drift by being corrected, and a rising count is not by itself evidence of anything going wrong. + +Both were prose, both were harmless, and together they are how a 16-line diff becomes a 40-line one — in under a day, with no contributor doing anything careless. #211 also extended the line-budget guard (`tests/test_doc_accuracy.py`) to cover **both** managers rather than only `unfao.py`, which had left the second copy of the file epic #148 shrank from 636 lines with no regrowth protection at all. + +**What this does not license.** The four env-name literals in `__appwrite_config` duplicate names that `appwrite_env.py` already declares as data, in both files. Removing that is not an abstraction and does not wait for the trigger — it is not encoding the partner's identity twice in the same package. Left as a follow-up rather than folded into #211, which is a partner-addition PR. + +See also C-24 (schema contract per store), C-77 (the fourth home for partner identity, in the duplicated `name=` argument of the historical upload), D-09 (the deferral, now expired), ADR-014 §4 (a deferral needs a trigger and an owner), #97 (second-store scoping), #211. --- @@ -339,7 +372,7 @@ See also C-24 (schema contract per store), D-09 (the deferral, now expired), #97 | Tier | 2 | | Source | `expert-code-review` (2026-06-24) | | Trigger | **(a) Upstream change:** when pipeline-core changes `PGMDataset` / the data loader / the postprocessor base (mid-migration: their #186/#188/#161), verify the inherited surface this repo depends on still holds. **(b) Standing work item:** the input-side de-inheritance (the sink side landed — see the 2026-07-31 update) — schedule it, don't wait for a trigger. | -| Location | `views_postprocessing/unfao/managers/unfao.py:80` (double inheritance); `:148-204`, `:495-534` (inline env/AppwriteConfig/DatastoreModule); `:276-287` (`_append_metadata`), `:300-349` (`_validate`); DIP sink adapter at `:37-78` (`_ContractStorePort`) | +| Location | `views_postprocessing//managers/.py` — the `class PostProcessorManager(PostprocessorManager, ForecastingModelManager)` statement (double inheritance); `_prod_forecasts_datastore`, `__datastore`, `__appwrite_config` (inline env/AppwriteConfig/DatastoreModule); `_validate` and `_check_coverage`; the DIP sink adapter `_ContractStorePort`. **Since 2026-08-03 all of it exists twice** — `unfao` and `crafd` are the same file with the partner name changed (C-33). Symbols rather than lines, deliberately: an earlier version of this row was invalidated by a comment edit four lines long. | `UNFAOPostProcessorManager` subclasses **two concrete** pipeline-core base classes (`PostprocessorManager`, `ForecastingModelManager`) and **interleaves infrastructure** (env reading, `AppwriteConfig` construction, `DatastoreModule`, path resolution) with the FAO **business logic** (GAUL enrichment, the 9-column null gate) inside the lifecycle hooks. Consequences: (a) the FAO logic cannot be instantiated or unit-tested without the full framework + Appwrite env + viewser; (b) **pandas cannot leave the delivery path** because the inherited data loader and `PGMDataset` are pandas — gated on pipeline-core's own DataFrame retirement; (c) **SDP exposure** — heavy *inheritance* coupling to a pipeline-core that is itself unstable (mid-migration), so upstream changes break far from their cause (cf. C-27, C-29); (d) it's the repo's only composition-over-inheritance violation. The dependency itself is correct (`unfao.py` genuinely *is* a pipeline-core postprocessor) — the issue is its **blast radius**. Mitigation (does **not** fight the Template-Method framework): 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). This makes the FAO logic testable standalone and insulates it from pipeline-core churn. @@ -369,7 +402,15 @@ See also C-24 (schema contract per store), D-09 (the deferral, now expired), #97 *Did:* the surrounding surface shrank sharply. The manager is **406 lines** (from 636); it imports neither pandas nor `PGMDataset`; the partner-neutral machinery moved out to `contract/` (#153); and **`views_pipeline_core` is still imported by exactly one module — this one — now pinned mechanically** by `tests/test_doc_accuracy.py` and `tests/test_clone_readiness.py`. That property is what keeps this entry's blast radius one file wide, and it is no longer a claim anyone has to re-check by hand. -*Did not:* the double inheritance at `unfao.py:80` stands, and so do consequences (a) — the FAO logic still cannot be instantiated without the framework — and (c)/(d). **This entry remains open on exactly that scope.** Its remaining fix is gated on views-pipeline-core's 3.0.0 (C-44/C-62), which is a release signal rather than engineering work. +**⚠ Superseded 2026-08-03 (PR #211): "exactly one module" is now exactly TWO.** `views_postprocessing/crafd/managers/crafd.py` is the second, and it imports the same `views_pipeline_core.modules.{appwrite,datastore}` surface at the same lines. The claim above was true when written and is left visible rather than edited away, per ADR-014 §5. + +**What actually changed, and what did not.** The blast radius is no longer *one file wide* — it is **one file, twice**, which is a different and slightly worse property: an upstream change now has two identical landing sites and no mechanism guarantees they are patched together (C-33). What did **not** change is the more important half: the count is still **bounded and pinned**. `test_views_pipeline_core_is_confined_to_the_partner_managers` (renamed in #211 — it had asserted *two* under a name that said *one*) was widened to an explicit allowlist, not deleted, so a *third* importer still fails CI. Every other module in the repository remains pipeline-core-free, including the whole of `contract/` and `delivery/`, and `tests/test_clone_readiness.py` still proves the machinery imports in a subprocess without it. + +**On þing-02 S24(5).** `docs/CLONING.md` cited that verdict as forbidding these imports outright. Reading it directly (`þingit/02_credential_identity_key_ownership/sáttmál.md:240-242` — precondition (5) itself; the section opens at `:232` under the heading *"§5 — The clone (`un-crafdapi`)"* — and `orð_dómr.md:418-441`), it binds *"the clone"* — `un-crafdapi` and `views-productionapi`, repositories **git-cloned from views-faoapi** — and does not reach an in-repo partner package of the producer. CLONING.md over-claimed; PR #211 corrects the citation rather than weakening the rule. This entry's own scope is unaffected: the coupling is a design concern here regardless of what the verdict binds, and issue **#146**'s deferred unwind now covers two files instead of one. + +Tier held at 2. The residual scope — the double inheritance and the framework-bound instantiation — is unchanged, and is still gated on views-pipeline-core 3.0.0 (C-44/C-62). + +*Did not:* the double inheritance (the `class UNFAOPostProcessorManager(...)` statement — this row cited `unfao.py:80` when written, and that number has moved twice since) stands, and so do consequences (a) — the FAO logic still cannot be instantiated without the framework — and (c)/(d). **This entry remains open on exactly that scope.** Its remaining fix is gated on views-pipeline-core's 3.0.0 (C-44/C-62), which is a release signal rather than engineering work. See also C-07/C-27/C-29 (pipeline-core coupling symptoms), C-39 (the dead-mapper cleanup that precedes any unfao restructuring), **#45** (the delivery-side draw carrier — ship `(N, S)` uncollapsed as a native frame, the producer half of this same problem), and **epic #85** (the migration backlog). @@ -578,6 +619,62 @@ Cross-refs: **C-59** and **C-61** (RESOLVED — the invariant block this sits be --- + + +--- + +### 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. + +--- + +### C-77: The historical leg names its document from the model path, not from the declared consumer name — and nothing checks the two agree + +| Field | Value | +|-------|-------| +| ID | C-77 | +| Tier | 2 — structural fragility with a clear trigger, affecting **both** partners. Not Tier 1: the failure is a document the consumer cannot find, not a wrong value inside one. But it is the **F1 invisibility shape** — ADR-013 §4.1a, the defect that left six `orange_ensemble` forecast documents stranded in `unfao_bucket` while forecast serving read empty for months. Nobody notices a delivery that simply is not there. | +| Source | `code-review max` (2026-08-03) — PR #211, cross-checking the crafd producer against the views-crafdapi consumer | +| Trigger | When a postprocessor's directory is renamed in views-models, or a new partner package is added whose directory name differs from its `CONSUMER_DOCUMENT_NAME` — check that the historical artifact is still retrievable by the consumer's filter. The forecast leg will keep working, so a green delivery run is not evidence. | +| Owner | Whoever takes the guard. It is a one-line assertion plus a test, not a design decision — but it must be taken deliberately, because the current agreement is a coincidence nobody has written down. | +| Location | The historical-artifact upload in `views_postprocessing//managers/.py` — the call passing `name=self._model_path.model_name`, in `_save_contract`. For contrast, the correct leg is the `consumer_name=product.CONSUMER_DOCUMENT_NAME` argument a few lines above, which reaches the wire as `common["name"]` in `contract/wire/sink.py::deliver_run`. | + +The forecast leg is right. It threads the declared constant through: the manager passes `consumer_name=product.CONSUMER_DOCUMENT_NAME` into `deliver_run`, which sets `common = {"name": consumer_name, ...}`. One declaration, carried to the wire as a parameter — the shape C-69 credited as already correct. + +**The historical-actuals leg does not use that constant at all.** It passes `name=self._model_path.model_name` — a value that comes from the postprocessor's *directory name* in views-models, not from any declaration in this repository. The consumer filters on exactly the string this repo declares: `filters["name"] = self.model_path.model_name`, where the path manager is constructed as `APIPathManager("un_crafd")`. + +**For FAO the two agree; for CRAF'd nobody can yet say.** `views-models/postprocessors/` contains `un_fao` and nothing else — there is **no `un_crafd` postprocessor directory**, so CRAF'd's historical `name=` has never been resolved, let alone compared against its consumer's filter. That makes this worse rather than better: for the live partner the agreement is a coincidence nobody wrote down, and for the new one it is an assumption that will first be tested by a production run. Whoever creates that directory decides, without knowing it, whether CRAF'd's actuals are retrievable. + +**Nothing in this repository asserts they agree.** `tests/test_product.py` asserts `CONSUMER_DOCUMENT_NAME` for the forecast leg; `tests/test_hop_b_sink_e2e.py` checks `consumer_name` on the forecast leg. Neither touches the historical leg's `name=`. A rename of the views-models directory — an ordinary, plausible act, done in a different repository by someone who has never read this file — silently detaches the historical artifact from the consumer's filter while every test here stays green and every delivery run reports success. + +This is ADR-003's rule broken in the quiet direction: the delivery **infers** its consumer identity from a path instead of reading the declaration that exists three lines away. It is also the fourth home for partner identity, where C-69's 2026-07-31 note counted three and recommended consolidation rather than relocation. Consolidation did not reach this line. + +**Scope note:** the crafd package inherited this unchanged from `unfao`; PR #211 did not introduce it, it doubled it. Registering it against both partners rather than against the PR. + +Cross-refs: **C-01** (RESOLVED — the metadata-completeness gate; same partner, same delivery, different field), **C-69** (RESOLVED — "partner identity has THREE homes"; this is the fourth and the note's consolidation recommendation is the fix), **C-33** (the duplication that turned one instance into two), ADR-013 §4.1a (F1 invisibility), ADR-003 (declarations over inference), #211. + +--- + ## Disagreements ### D-12: Post-Run-0 infrastructure & naming intents — repo rename, internal-store transport, compute co-location @@ -646,6 +743,29 @@ See also C-40 (the inheritance/representation coupling this migration unwinds), ## Resolved Concerns +### C-78: A partner package without an `__init__.py` is invisible to the guard that inventories them — RESOLVED same day + +| Field | Value | +|-------|-------| +| ID | C-78 | +| Tier | 4 — FIXED in the same change that found it; recorded because the *reasoning* is what future guards need, not because work is outstanding. No delivery was ever affected. | +| Source | `code-review max` (2026-08-03) — PR #211 second pass, attacking the new scope guard | +| Trigger | When a future guard inventories the package tree, check what it uses as its "is this a package" test. If it asks for `__init__.py`, it disagrees with every other scan in this suite and with the repository's own root. | +| Owner | Discharged. | +| Location | `tests/test_clone_readiness.py` (the criterion); `views_postprocessing/` (which has no `__init__.py` of its own) | + +`test_the_declared_partner_list_is_the_real_one` was written to stop a partner package going unguarded — the defect that let `crafd/` land exempt from at least seven checks — the four `tests/conftest.py` enumerates, plus the three product pins (`TARGETS`, `S_MIN`, `UPLOAD_ENABLED`) that `tests/test_product.py` held for FAO alone. Its first draft asked for a directory containing `__init__.py`. + +**`views_postprocessing/` has no `__init__.py`.** The distribution root is already a PEP 420 namespace package, so the guard applied to its children a test its own parent fails. Verified by building a partner package without one, carrying three real defects — `UPLOAD_ENABLED = True` (ADR-013 §11.4), a wrong `CONSUMER_DOCUMENT_NAME` (§4.1a), and a live `load_dotenv` (þing-01 #134) — and running the full suite: **green**. Adding one empty `__init__.py` to the identical tree made the guard fire. It imported and ran fine at runtime throughout. + +The criterion is now "contains at least one `.py`, and is not `__pycache__`", which is what the suite's eight other tree scans effectively use (`rglob("*.py")`). The same review found `MACHINERY_PACKAGES` was validated against nothing — a stale name there **pre-classifies** any future package that takes it, and `reconciliation` (C-47's phantom) is exactly such a name. Both lists are now checked against disk. + +**One correction to C-47 while here.** That entry's Tier-4 rationale says the phantom directory was *"not importable (no `__init__.py`, no sources)"*. Under PEP 420 that reasoning is wrong: a directory with no `__init__.py` and no sources still imports as a **namespace package** whose `__path__` points at it — only its submodules fail. Reproduced on a copy of the tree. The directory itself was deleted by #177 on 2026-08-01, so nothing is importable today and the tier stands; what does not stand is the reason given for it. The harm C-47 actually recorded — *"misleading tools that inventory the tree"* — is precisely what this entry is about. + +Cross-refs: **C-47** (the phantom directory, and the corrected rationale above), **C-57** (a guard scoped by name missing the second subject — the same disease, one file over), **C-74** (a guard whose declared roots stopped existing), ADR-014 §2, #211. + +--- + ### C-22: No post-delivery correction process for wrong assignments — RESOLVED (procedure written; the partner-facing step is an open OPERATOR decision) | Field | Value | @@ -833,7 +953,7 @@ A second, smaller instance of the same shape: these checks parse TOML with `toml | Tier | 3 | | Source | `manual` (2026-07-31) — review-rr blind-spot analysis, following the þing-01 verdict (`orð_dómr.md`, ratified as amended 2026-07-28) | | Trigger | When views-appwrite amends `coordinate_registry.toml` — renames a coordinate, retires the legacy secret slot in favour of `APPWRITE_{READ,WRITE,PROVISION}_API_KEY`, or adds a target — verify `views_postprocessing/unfao/appwrite_env.py` still matches. Nothing mechanical will tell you: the registry is deliberately **referenced, never copied**, and the two live in different repositories | -| Location | `views_postprocessing/unfao/appwrite_env.py` (`CONNECTION_ENV`, `PROD_FORECASTS_ENV`, `UNFAO_ENV`); views-appwrite `docs/ADRs/platform/coordinate_registry.toml` (the authority); `tests/test_env_declaration.py` (guards this repo's half only); `docs/ADRs/013_sampled_forecast_wire_contract.md` §7(d) (the URL reference) | +| Location | `views_postprocessing/unfao/appwrite_env.py` (`CONNECTION_ENV`, `PROD_FORECASTS_ENV`, `UNFAO_ENV`) and `views_postprocessing/crafd/appwrite_env.py` (`CRAFD_ENV`) — see the 2026-08-03 amendment; views-appwrite `docs/ADRs/platform/coordinate_registry.toml` (the authority); `tests/test_env_declaration.py` (guards this repo's half only); `docs/ADRs/013_sampled_forecast_wire_contract.md` §7(d) (the URL reference) | The þing-01 assembly (D1) settled that the PLATFORM-001 contract is **homed in views-appwrite and referenced by URL, never by copy** — a deliberate and correct choice: copies were the platform's original disease (sáttmál S6, the copy-chain this repo's own `load_dotenv` borrow was the runtime edge of, killed in #134/PR #137). But referencing-not-copying moves the failure mode rather than removing it: **the registry can now change without this repo noticing.** @@ -845,6 +965,15 @@ Two named changes are already anticipated and will fire this trigger: the **reti **⚠ CORRECTED 2026-08-02, then restored the same day.** The word *already* above overclaimed at the time: **C-74** showed that guard scanning one of its five declared roots, four having pointed at paths #153 moved. **C-74 closed later that day (S10 / #192)** — the roots are re-pointed, the scan covers 17 files, and a declared root that does not exist now fails rather than emptying the scan silently. The sentence above is true again, and the episode is left visible because a claim that was false for two days is worth more as a record than as a correction quietly reverted. +**⚠ AMENDED 2026-08-03 (PR #211) — the detector was built for one partner, and the second partner proved it.** Two corrections to the resolution above, and one of them is the same disease in the cure. + +1. **The pin quoted above is stale.** `SEAM_CONTRACT_VERSION = "1.3.0"` / `SEAM_CONTRACT_COMMIT = "47172af"` was accurate when written on 2026-08-02; the registry then moved twice in under twelve hours — to v1.4.0 (`4a5ab1b`, reaching `main` as `20dfd0f`, 2026-08-02 18:45) and to v1.4.1 (`0da2682`, reaching `main` as `5266b90`, 2026-08-03 02:52) — and `unfao/appwrite_env.py` was re-pinned each time. This repo's current pin, `90fc105`, is **neither** of those commits: it is a later views-appwrite merge that does not touch the registry at all. That is correct and intended — a pin names *an edition of `main` that was read*, not the commit that changed the file — but the two must not be written as though they were the same thing. The values are left above as the worked example they were written to be, but they are no longer what the file says. +2. **The detector was scoped to `unfao` by name and did not follow the second partner.** `tests/test_env_declaration.py` imported only `views_postprocessing.unfao.appwrite_env`; a grep for `crafd` in it returned zero. So when PR #211 added `views_postprocessing/crafd/appwrite_env.py` pinned at **`1.3.0` / `47172af`** — an edition at which all four `APPWRITE_CRAFD_*` coordinates were declared with **no value**, and which predates the very views-appwrite PR #38 that the file's own docstring cites as its justification — **nothing failed.** Had this entry's own **version** check covered crafd, that pin would have failed **locally** the moment it was written. Not CI: the check opens with `require_sibling("views-appwrite")` and skips without a checkout, and the workflow checks out only this repository — which is this entry's own standing Residual, below. Note which half does the work: the *reachability* check would have passed, because `47172af` is a perfectly good ancestor of views-appwrite's `main`. Existence and reachability were both satisfied by a pin that was nonetheless two editions out of date — which is precisely why the version check exists alongside them rather than instead of them. + +This is ADR-014 §2 in its narrow form: a guard's *scope* is part of what has to be mutation-proven, not just its matching. The four checks were each proven to fail on the defect they were written for, against `unfao` — and stayed silent on an identical defect one package over. Same shape as **C-74**, one layer up: there the declared scan roots stopped existing; here the declared scope never grew. + +PR #211 re-pins crafd to `1.4.1` / `90fc105` and parameterises **three** of the four checks over both partner declarations — names-and-class, pinned edition, commit reachability. The fourth, the value-copy scan, was never partner-scoped: it walks `_PKG.rglob("*.py")` and so covered `crafd/` from the day it landed. A third partner is now a one-line addition, and an unguarded one is a failure. This entry stays RESOLVED — the mechanism was right, its reach was not — but the residual below now has a companion: a detector that names its subject is a detector that will miss the next subject. + Cross-refs: C-74 (the guard this paragraph vouched for), C-33 (store identity still hardcoded per store — the same env surface, different concern), C-58 (what happens when a coordinate is wrong rather than missing), C-44 (the pipeline-core version coupling that would carry a registry change), issues #134/#135/#138 (this repo's discharged þing-01 obligations), #104 (README env block placeholders). --- diff --git a/tests/conftest.py b/tests/conftest.py index dfee96c..78493ac 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -32,6 +32,34 @@ _REPO = Path(__file__).resolve().parent.parent +#: The partner packages under ``views_postprocessing/``, DECLARED (ADR-003) — and +#: declared **once**, because the alternative is what #211 exposed. +#: +#: **Eight** separate guards named ``"unfao"``, and when ``crafd/`` landed all eight +#: went on passing over a package they did not cover: +#: +#: the import-purity subprocess · the ADR-002 direction check · the manager line +#: budget · the coordinate-drift check · the þing-01 dotenv guard · and the three +#: product pins (``TARGETS``, ``S_MIN``, ``UPLOAD_ENABLED``) +#: +#: So ``contract/`` could import ``crafd``; the second manager had no budget; a registry +#: pin two editions stale failed nothing; the retired ``load_dotenv`` borrow could be +#: reintroduced in the new manager; and nothing asserted the new partner's consumer +#: document name — the one field whose failure mode is a delivery nobody can find. +#: +#: A partner list per guard is eight places to forget the next partner; this is one. +#: (An earlier version of this comment said four. It was written by counting the guards +#: that had already been fixed.) +#: +#: ``tests/test_clone_readiness.py::test_the_declared_partner_list_is_the_real_one`` +#: checks this against the filesystem, so the declaration cannot quietly go stale +#: either — ADR-014 §2: where a guard's inputs are declared, assert they are real. +PARTNER_PACKAGES = ("unfao", "crafd") + +#: The partner-neutral packages. The split is two-sided: a new top-level package is +#: either a partner or machinery, and the same test refuses to let it be neither. +MACHINERY_PACKAGES = ("contract", "delivery") + #: repo name -> the environment variable that overrides its location. #: Declared, never derived from the name: ``views-datafactory`` → ``VIEWS_DATAFACTORY`` #: happens to be mechanical, but a future sibling need not follow the pattern and @@ -39,6 +67,24 @@ SIBLING_ENV = { "views-datafactory": "VIEWS_DATAFACTORY", "views-appwrite": "VIEWS_APPWRITE", + "views-faoapi": "VIEWS_FAOAPI", + "views-crafdapi": "VIEWS_CRAFDAPI", +} + +#: partner package -> the repository that CONSUMES its delivery. +#: +#: Declared, never derived. ``unfao`` → ``views-faoapi`` and ``crafd`` → +#: ``views-crafdapi`` are not a pattern a rule could produce, and the þing records call +#: the second one ``un-crafdapi`` while the repository on disk is ``views-crafdapi`` — +#: exactly the kind of near-miss that makes guessing expensive. +#: +#: This exists so the consumer-document-name pin can be checked **across the seam** +#: rather than asserted locally. A name this repo declares and the consumer filters on +#: is a fact this repo does not own; declaring it here is right, but only the sibling +#: checkout can confirm it still matches (ADR-014 §1 — the guarantee needs a check). +CONSUMER_REPO = { + "unfao": "views-faoapi", + "crafd": "views-crafdapi", } @@ -61,6 +107,30 @@ def sibling_repo(name: str) -> Path | None: return candidate if candidate.exists() else None +def broken_sibling_overrides() -> dict[str, str]: + """Declared sibling variables that are SET but point at nothing. + + A typo'd override is an operator error, not a normal absence: someone set the + variable because they meant to run those checks, and returning ``None`` turns the + typo into permanent, invisible non-coverage of every cross-repo assertion this repo + has. ``test_no_sibling_override_points_at_a_missing_path`` fails on it. + + **This is deliberately not a raise inside ``sibling_repo``.** It was, for about an + hour. Three modules resolve a sibling at import time (``test_delivery_coverage``, + ``test_datafactory_deploy_readiness``, ``test_gaul_lookup_fidelity``), so raising + there turned a one-character typo in ``VIEWS_DATAFACTORY`` into + ``Interrupted: 3 errors during collection`` and **zero tests run** — trading silent + under-coverage for total loss of the suite. One clean failure says the same thing + and lets the other 360 tests report. + """ + return { + var: value + for var in SIBLING_ENV.values() + for value in [os.environ.get(var)] + if value and not Path(value).exists() + } + + def require_sibling(name: str) -> Path: """``sibling_repo`` or skip, with a message naming what to set. diff --git a/tests/test_clone_readiness.py b/tests/test_clone_readiness.py index 27f2de7..549bb77 100644 --- a/tests/test_clone_readiness.py +++ b/tests/test_clone_readiness.py @@ -14,9 +14,16 @@ **Why it exists at all.** #153 separated partner-neutral machinery (``contract/``) from FAO's product (``unfao/``). Nothing then stops the next contributor adding one convenient import back — and the boundary would be gone with no signal, because -everything would still work *for FAO*. It would only break for views-crafdapi and -views-productionapi, in a repository nobody had cut yet. Register C-69's fix is -one-shot; this is what makes it hold. +everything would still work *for FAO*. Register C-69's fix is one-shot; this is what +makes it hold. + +**Amended 2026-08-03 (#211): there are now two partners, and the guard was scoped to +one by name.** ``crafd/`` joined ``unfao/`` and every assertion here hardcoded the +string ``"views_postprocessing.unfao"``, so ``contract/`` could import ``crafd`` and +nothing objected — verified by adding exactly that import and watching the suite stay +green. The partners are now **declared** in ``_PARTNER_PACKAGES`` and checked against +the filesystem, because a guard that names its subject will miss the next subject +(register C-57's 2026-08-03 amendment, which is the same failure one file over). """ from __future__ import annotations @@ -28,7 +35,14 @@ import pytest +# The partner and machinery package lists are declared once, in tests/conftest.py, and +# imported by every guard that needs them. Four guards each carrying their own copy of +# the string "unfao" is how `crafd` arrived unguarded — see that module's comment. +from tests.conftest import MACHINERY_PACKAGES as _MACHINERY_PACKAGES +from tests.conftest import PARTNER_PACKAGES as _PARTNER_PACKAGES + _REPO = Path(__file__).resolve().parent.parent +_PKG = _REPO / "views_postprocessing" #: Every partner-neutral module a clone is expected to reuse as-is. _MACHINERY = ( @@ -50,19 +64,25 @@ "views_postprocessing.contract.historical", "views_postprocessing.contract.gaul_lookup", "views_postprocessing.contract.gaul_schema", + "views_postprocessing.contract.enrichment", "views_postprocessing.contract.launch_config", "views_postprocessing.contract.source_metadata", "views_postprocessing.contract.store_metadata", ) -def _import_in_subprocess(modules: tuple[str, ...], forbidden_prefix: str) -> subprocess.CompletedProcess: - """Import ``modules`` in a fresh interpreter; report any ``forbidden_prefix`` arrivals.""" +def _import_in_subprocess( + modules: tuple[str, ...], forbidden_prefixes: tuple[str, ...] +) -> subprocess.CompletedProcess: + """Import ``modules`` in a fresh interpreter; report any ``forbidden_prefixes`` arrivals.""" script = textwrap.dedent(f""" import sys for name in {list(modules)!r}: __import__(name) - leaked = sorted(m for m in sys.modules if m.startswith({forbidden_prefix!r})) + leaked = sorted( + m for m in sys.modules + if any(m.startswith(p) for p in {list(forbidden_prefixes)!r}) + ) print("LEAKED:" + ",".join(leaked)) """) return subprocess.run( @@ -71,59 +91,204 @@ def _import_in_subprocess(modules: tuple[str, ...], forbidden_prefix: str) -> su ) -def test_the_machinery_imports_without_the_partner(): +#: The subset of the above that is the bottom of the stack. ADR-002's chain is +#: ``/ -> contract/ -> delivery/``, and the arrow points one way: the +#: invariants must be usable without the machinery that calls them. +_INVARIANTS = tuple( + m for m in _MACHINERY if m.startswith("views_postprocessing.delivery.") +) + + +def _partner_prefixes() -> tuple[str, ...]: + return tuple(f"views_postprocessing.{name}" for name in _PARTNER_PACKAGES) + + +def _modules_on_disk(package: str) -> set[str]: + return { + "views_postprocessing." + f.relative_to(_PKG).with_suffix("").as_posix().replace("/", ".") + for f in (_PKG / package).rglob("*.py") + if f.name != "__init__.py" + } + + +def test_the_machinery_list_is_the_whole_machinery(): + """Assert this guard's inputs are real — the clause it kept applying elsewhere. + + ``_MACHINERY`` is a hand-written tuple, and every purity check here iterates it. A + machinery module missing from it is never imported in the subprocess, so it can + depend on a partner and nothing objects. + + **That was not hypothetical.** ``contract/enrichment.py`` was absent, and nothing + else in the package imports it, so it was never dragged in transitively either: + verified 2026-08-03 that ``from ..crafd import product`` in that file left the whole + suite green. This changeset added a completeness assertion to five other declared + lists (ADR-014 §2) and missed the one in the file that argues for them. + """ + declared = set(_MACHINERY) + # Derived from the declared machinery list, not from two names written here. A + # third machinery package added to MACHINERY_PACKAGES and forgotten here would + # otherwise be exempt from every purity check — which is the defect this whole + # changeset moved four other lists into conftest.py to prevent, reproduced inside + # the test written to enforce it. + on_disk = set().union(*(_modules_on_disk(p) for p in _MACHINERY_PACKAGES)) + + assert not (on_disk - declared), ( + f"machinery modules absent from _MACHINERY: {sorted(on_disk - declared)}. Each " + "is exempt from every purity check in this file — it may import a partner, and " + "nothing here will notice." + ) + assert not (declared - on_disk), ( + f"_MACHINERY names modules that no longer exist: {sorted(declared - on_disk)}. " + "A subprocess that imports a vanished module fails confusingly; one that was " + "quietly dropped from the list stops being checked." + ) + + +def test_the_invariants_import_without_the_machinery(): + """ADR-002's lower arrow, proven the way the upper one is. + + ``delivery/`` sits below ``contract/`` and must stay usable on its own — that is + what makes the invariants *representation-free* rather than merely + representation-light, and it is what lets a partner reuse them without the wire. + + **A regex was written for this first and was not enough.** It caught + ``from views_postprocessing.contract import gaul_schema`` — the one form used to + demonstrate the gap — while missing ``from ..contract import gaul_schema`` and + ``from views_postprocessing import contract``, both plain module-level imports that + execute on import. A fresh interpreter sees all of them, which is why this is the + load-bearing half and the regex in ``test_doc_accuracy.py`` is the supplement that + also covers imports hidden in function bodies. + """ + result = _import_in_subprocess( + _INVARIANTS, ("views_postprocessing.contract",) + _partner_prefixes() + ) + assert result.returncode == 0, ( + f"the invariants failed to import on their own:\n{result.stderr}" + ) + leaked = [m for m in result.stdout.split("LEAKED:")[-1].strip().split(",") if m] + assert not leaked, ( + f"importing delivery/ pulled in the machinery above it: {leaked}. ADR-002's " + "dependency arrow points one way; an invariant that needs the wire to load is " + "no longer an invariant about primitives." + ) + + +def test_the_declared_partner_list_is_the_real_one(): + """Assert the guard's inputs are real (ADR-014 §2). + + Every other test here trusts ``_PARTNER_PACKAGES``. A partner package that exists + on disk but is missing from that tuple is unguarded, and the suite stays green — + which is exactly what happened to ``crafd`` on the day it landed. So the tuple is + checked against the filesystem rather than believed. + + A new top-level package under ``views_postprocessing/`` is either a partner or + machinery. If it is neither, this fails and someone decides which it is, on + purpose, rather than by whichever guard happens not to mention it. + + **Why "contains a .py" and not "contains an ``__init__.py``".** The first draft + asked for ``__init__.py`` — a criterion ``views_postprocessing/`` itself fails, since + the distribution root has no ``__init__.py`` and is already a PEP 420 namespace + package. A partner directory without one imports perfectly at runtime and was + invisible here: verified 2026-08-03 by building a namespace partner carrying three + real defects (``UPLOAD_ENABLED = True``, a wrong consumer name, a live + ``load_dotenv``) and watching the full suite stay green, with one empty + ``__init__.py`` the entire difference. Every other tree scan in this suite uses + ``rglob("*.py")`` and would have seen it; this guard was the odd one out. + + **And ``rglob`` is why, not ``glob``.** The first correction used ``glob("*.py")`` + while its own comment claimed parity with the ``rglob`` scans — so a partner whose + modules sit only in ``managers/`` (which is where a partner's manager actually + lives) was *still* invisible. Verified: ``wfp/managers/wfp.py`` carrying a live + ``load_dotenv`` passed this guard. Two drafts, the same mistake, caught the second + time only because someone checked the sentence against the code. + """ + on_disk = { + p.name for p in _PKG.iterdir() + if p.is_dir() and p.name != "__pycache__" and any(p.rglob("*.py")) + } + classified = set(_PARTNER_PACKAGES) | set(_MACHINERY_PACKAGES) + + for label, declared in ( + ("_PARTNER_PACKAGES", _PARTNER_PACKAGES), + ("_MACHINERY_PACKAGES", _MACHINERY_PACKAGES), + ): + missing = sorted(p for p in declared if p not in on_disk) + assert not missing, ( + f"{label} names packages that do not exist: {missing}. A guard whose " + "declared scope has gone missing scans nothing and reports success — and a " + "stale name here is worse than dead, because it pre-classifies any future " + "package that happens to take it (register C-47: `reconciliation` was " + "exactly such a phantom in this tree)." + ) + + unclassified = sorted(on_disk - classified) + assert not unclassified, ( + f"new top-level package(s) {unclassified} are neither declared partners nor " + "declared machinery. Add each to PARTNER_PACKAGES or MACHINERY_PACKAGES in " + "tests/conftest.py — a partner left out of the first is silently exempt from " + "every check below." + ) + + +def test_the_machinery_imports_without_any_partner(): """The load-bearing assertion of the whole epic. A clone must be able to take `delivery/` and `contract/` and get a working - ADR-013 delivery without inheriting FAO's product, FAO's store coordinates, or - FAO's manager. + ADR-013 delivery without inheriting any partner's product, store coordinates, or + manager. "Any" is the operative word since #211: the machinery serves two + partners now, and being neutral toward one of them is not neutrality. """ - result = _import_in_subprocess(_MACHINERY, "views_postprocessing.unfao") + result = _import_in_subprocess(_MACHINERY, _partner_prefixes()) assert result.returncode == 0, ( f"the machinery failed to import on its own:\n{result.stderr}" ) leaked = [m for m in result.stdout.split("LEAKED:")[-1].strip().split(",") if m] assert not leaked, ( - f"importing the partner-neutral machinery pulled in the partner: {leaked}. " - "A clone (views-crafdapi, views-productionapi) would inherit FAO's product " - "through this path — see register C-69 and views_postprocessing/contract/__init__.py." + f"importing the partner-neutral machinery pulled in a partner: {leaked}. " + "The next partner would inherit that one's product through this path — see " + "register C-69 and views_postprocessing/contract/__init__.py." ) def test_the_machinery_does_not_pull_in_pipeline_core(): - """`views_pipeline_core` is imported by exactly one module — the manager. + """`views_pipeline_core` is imported only by the partner managers. - Pinned while it is true. A clone writes its own manager against its own + Pinned while it is true. Each partner's manager is written against its own framework seam; if the machinery started dragging pipeline-core in, that choice - would be made for it, and register C-40's blast radius would widen from one file - to the whole package. + would be made for every partner at once, and register C-40's blast radius would + widen from two files to the whole package. - ADR-013 §11.4-adjacent: þing-02 **S24(5)** makes this binding for the clone — it - must not import `views_pipeline_core.modules.{appwrite,datastore}`, because this - repo's own import of those is how a two-repo defect became three. + ADR-013 §11.4-adjacent: þing-02 **S24(5)** binds *the cloned repositories* — + `un-crafdapi` and `views-productionapi`, cut from views-faoapi — not to import + `views_pipeline_core.modules.{appwrite,datastore}`. It does not reach an in-repo + partner package of the producer, which is why `crafd/managers/crafd.py` may import + them and `docs/CLONING.md` was corrected. What this test defends is the narrower + and repo-owned rule: the *machinery* stays free of them regardless. """ - result = _import_in_subprocess(_MACHINERY, "views_pipeline_core") + result = _import_in_subprocess(_MACHINERY, ("views_pipeline_core",)) assert result.returncode == 0, result.stderr leaked = [m for m in result.stdout.split("LEAKED:")[-1].strip().split(",") if m] assert not leaked, ( - f"the machinery pulled in views_pipeline_core: {leaked}. It is imported by " - "exactly one module (the manager) and that is what keeps C-40 bounded." + f"the machinery pulled in views_pipeline_core: {leaked}. It is imported only " + "by the partner managers, and that is what keeps C-40 bounded." ) -def test_the_guard_would_actually_catch_a_violation(): +@pytest.mark.parametrize("partner", _PARTNER_PACKAGES) +def test_the_guard_would_actually_catch_a_violation(partner): """A purity test that cannot fail is decoration. - Imports the *partner* deliberately and asserts the detector sees it — so a future - reader knows the two tests above are load-bearing rather than vacuously passing - because the subprocess silently did nothing. + Imports each *partner* deliberately and asserts the detector sees it — so a + future reader knows the tests above are load-bearing rather than vacuously + passing because the subprocess silently did nothing. Parametrised, because a + detector proven against one partner is not proven against the other. """ result = _import_in_subprocess( - ("views_postprocessing.unfao.product",), "views_postprocessing.unfao" + (f"views_postprocessing.{partner}.product",), _partner_prefixes() ) assert result.returncode == 0, result.stderr leaked = [m for m in result.stdout.split("LEAKED:")[-1].strip().split(",") if m] - assert leaked, "the detector reported nothing while importing the partner directly" + assert leaked, f"the detector reported nothing while importing {partner} directly" @pytest.mark.parametrize("doc", ["docs/CLONING.md"]) diff --git a/tests/test_datafactory_deploy_readiness.py b/tests/test_datafactory_deploy_readiness.py index f8ffa8d..acadef8 100644 --- a/tests/test_datafactory_deploy_readiness.py +++ b/tests/test_datafactory_deploy_readiness.py @@ -3,9 +3,13 @@ These guard the cross-repo preconditions that the FAO global-delivery plan (umbrella views-postprocessing#20, region flip views-models#127) depends on. -They are written against a local views-datafactory checkout and FAIL BY DESIGN -until the datafactory deploy candidate is actually releasable and the served -artifact matches the branch. +They are written against a local views-datafactory checkout. + +**"FAIL BY DESIGN" no longer describes this module as a whole, and saying so was +misleading.** The release-gate half is satisfied: the land_gaul work reached a tag, and +that is asserted below. What still fails by design is the *served-artifact* half — the +two ``xfail(strict)`` classes, which flip to failures the moment the served artifact +catches up with the branch. Corrected 2026-08-03. Resolution is the repo's one declared way of finding a sibling checkout (``tests/conftest.sibling_repo``): ``$VIEWS_DATAFACTORY``, else the conventional @@ -40,38 +44,40 @@ def _git(*args: str) -> str: class TestReleaseGate: - """P1 (HARD): the deploy gate (ADR-022) checks out an exact git TAG on the - server. land_gaul / Azores / SHDI are all committed AFTER the latest tag - (v1.2.29) and the version in pyproject is unchanged. Deploying 'to serve' - the current state would serve v1.2.29 — which has none of this work — and - views-models#127's REGION='land_gaul' flip would hit a package without it. + """P1: the deploy gate (ADR-022) checks out an exact git TAG on the server, so + anything this repository depends on must be *inside a tag* — not merely committed. + + **The blocker this class was written for is discharged.** When it was written, + land_gaul / Azores / SHDI were all committed after the latest tag (v1.2.29) with + pyproject unchanged, so deploying "to serve" would have served a package without + land_gaul and views-models#127's ``REGION='land_gaul'`` flip would have hit it. + That is no longer the situation: ``bac163e`` is contained in v1.10.0 and v1.11.0, + and datafactory's latest tag is v1.11.0. + + What survives is the standing check — the commit this repository depends on is in a + release tag — which is the durable form of the same question and does not care what + anyone's version number says today. The prose above described a live cross-repo + blocker for a while after it stopped being one; corrected 2026-08-03. """ - # #224 resolved (v1.4.0), then datafactory TAGGED v1.4.0 (2026-06-24) — so - # pyproject version == latest tag, which this over-strict check reads as a - # "stale version". Re-xfail(strict): the gate flaps with datafactory's release - # cadence; it flips back to a failure (demanding promotion) only when - # datafactory bumps past the tag to the next -dev. - @pytest.mark.xfail( - reason="cross-repo deploy gate flaps with datafactory's release cadence: " - "v1.4.0 is now tagged and == pyproject version; xfail(strict) " - "re-promotes when datafactory bumps past the tag.", - strict=True, - ) - def test_version_bumped_past_latest_tag(self): - version_line = (_DF / "pyproject.toml").read_text() - current = next( - ln.split("=")[1].strip().strip('"') - for ln in version_line.splitlines() - if ln.startswith("version") - ) - tags = _git("tag", "-l").splitlines() - assert f"v{current}" not in tags, ( - f"pyproject version {current} is already tagged (v{current}). The " - f"land_gaul/Azores/SHDI work is committed but UNTAGGED; the tag-based " - f"deploy gate would serve v{current}, which lacks land_gaul. Bump the " - f"version and cut a release before merging to main / deploying." - ) + # **`test_version_bumped_past_latest_tag` was RETIRED here on 2026-08-03.** + # + # It asserted `f"v{current}" not in tags` — that datafactory's pyproject version is + # ahead of its latest tag. That is true only in the window between a version bump + # and the tag that follows it, so the check went green or red on another + # repository's release *timing*, not on anything this repository depends on. It had + # been xfail(strict) for that reason, with a written re-promotion trigger; the + # trigger fired (datafactory reached v1.10.0 tagged / 1.11.0 in pyproject), the + # marker turned the unexpected pass into a failure, and re-reading it showed the + # test was the problem rather than the marker. datafactory's HEAD is literally + # "chore: bump to 1.11.0": the moment it cuts v1.11.0 this would have gone red + # again, in a repository doing everything right. + # + # Nothing is lost. Its stated purpose — "the land_gaul work is committed but + # UNTAGGED, so the tag-based deploy gate would serve a release without it" — is + # exactly what the surviving test below asserts, directly and without reference to + # anyone's release cadence. A gate that flaps gets ignored, and then the thing it + # guarded is unguarded (ADR-014 §3). def test_land_gaul_commit_is_in_a_release_tag(self): # bac163e = "feat: add bundled curated region land_gaul" diff --git a/tests/test_doc_accuracy.py b/tests/test_doc_accuracy.py index ea16dd2..4a29779 100644 --- a/tests/test_doc_accuracy.py +++ b/tests/test_doc_accuracy.py @@ -20,6 +20,10 @@ import re from pathlib import Path +import pytest + +from tests.conftest import PARTNER_PACKAGES as _PARTNER_PACKAGES + _REPO = Path(__file__).resolve().parent.parent _PKG = _REPO / "views_postprocessing" @@ -136,8 +140,19 @@ def test_internal_doc_links_resolve(): # module" while pandas lived in three. Both drifted the same way and neither was # caught by reading. These make the claims fail CI instead. -_MANAGER = _PKG / "unfao" / "managers" / "unfao.py" -_MANAGER_LINE_BUDGET = 450 # epic #148's bound; 406 at close, 636 before #149 +#: Every partner's manager directory, derived from the one declared partner list. +#: +#: **This was a hardcoded 2-tuple until 2026-08-03 and it was the last such list in the +#: suite.** Fifteen guards parametrise over ``PARTNER_PACKAGES``; this one did not, so a +#: third partner with a **906-line** manager passed the budget while failing everything +#: else — and nothing in those failures named the list it was missing from. That is +#: register C-57's shape surviving inside the file that documents C-57's fix. +_MANAGER_DIRS = tuple((_PKG / p / "managers") for p in _PARTNER_PACKAGES) + +#: epic #148's bound; 406 at close, 636 before #149. Applied to the manager *directory*, +#: not the manager file: an 800-line helper module beside a 406-line manager was +#: previously unbudgeted, which is the same regrowth wearing a different filename. +_MANAGER_LINE_BUDGET = 450 def _is_type_checking(test: ast.expr) -> bool: @@ -217,7 +232,7 @@ def test_pandas_is_not_imported_at_runtime_anywhere_in_the_package(): ) -def test_views_pipeline_core_has_exactly_one_importer(): +def test_views_pipeline_core_is_confined_to_the_partner_managers(): """ADR-012's 'Pipeline Manager' row, and C-40's blast-radius claim. This property is what makes the C-40 de-inheritance a bounded job rather than an @@ -228,36 +243,129 @@ def test_views_pipeline_core_has_exactly_one_importer(): for f in _PKG.rglob("*.py") if "views_pipeline_core" in f.read_text() ) - assert importers == ["unfao/managers/unfao.py"], ( - f"views_pipeline_core is imported by {importers}. ADR-012 and register C-40 both " - "state it is one file wide; a second importer widens C-40's blast radius." + assert importers == ["crafd/managers/crafd.py", "unfao/managers/unfao.py"], ( + f"views_pipeline_core is imported by {importers}. ADR-012 and register C-40 state it " + "is confined to the per-partner manager seam — one file per delivery (unfao, crafd). " + "An importer OUTSIDE those managers widens C-40's blast radius; a new partner manager " + "is expected and joins this list." ) -def test_the_manager_stays_within_its_line_budget(): - """ADR-012 no longer calls the manager 'thin' — it states a number. Hold it.""" - lines = len(_MANAGER.read_text().splitlines()) +@pytest.mark.parametrize("managers_dir", _MANAGER_DIRS, ids=lambda p: p.parent.name) +def test_the_manager_stays_within_its_line_budget(managers_dir): + """ADR-012 no longer calls the manager 'thin' — it states a number. Hold it. + + Counts every ``.py`` under the partner's ``managers/`` directory. The bound is on + the *seam*, and a seam that stays at 406 lines by moving 800 into a sibling module + has not stayed anywhere. + """ + assert managers_dir.is_dir(), ( + f"no managers/ directory at {managers_dir.relative_to(_PKG.parent)}. A budget " + "over a path that stopped existing counts nothing and reports success." + ) + sources = sorted(managers_dir.rglob("*.py")) + lines = sum(len(f.read_text().splitlines()) for f in sources) assert lines <= _MANAGER_LINE_BUDGET, ( - f"the manager is {lines} lines, over epic #148's {_MANAGER_LINE_BUDGET} bound. " + f"{managers_dir.parent.name}'s managers/ is {lines} lines across " + f"{[f.name for f in sources]}, over epic #148's {_MANAGER_LINE_BUDGET} bound. " "It was 636 before #149 and is the repo's one known dumping ground — growth " "here is the regression that epic existed to reverse." ) -def test_contract_package_does_not_import_the_partner(): - """The dependency direction ADR-002 declares: unfao/ -> contract/ -> delivery/.""" +def _imported_subpackages(source: str, module_path: Path) -> set[str]: + """Every ``views_postprocessing.`` this module imports — all four spellings. + + **Regexes were tried twice here and escaped twice.** A pattern that matched + ``from views_postprocessing.contract import x`` missed ``from ..contract import x``; + widened for that, it still missed ``from views_postprocessing import contract``, + because the optional ``views_postprocessing.`` group requires the dot. Both are + ordinary module-level imports. A third spelling, ``import views_postprocessing.x``, + needed its own alternative. Meanwhile the pattern fired on a docstring that merely + *spelled* a forbidden import — cry-wolf on prose while missing real code, which is + the worst of both (ADR-014 §2, §3). + + The AST knows what an import is. It resolves relative levels, sees the bare + ``from package import subpackage`` form as what it is, and cannot see prose at all. + This module already had the pattern twice (``_classify_pandas_imports``, + and ``_dotenv_use`` in ``test_env_declaration.py``); the import guards simply had + not caught up. + """ + parts = module_path.relative_to(_PKG).with_suffix("").as_posix().split("/") + package = ["views_postprocessing"] + parts[:-1] # the module's own package + + found: set[str] = set() + + def record(dotted: str) -> None: + bits = dotted.split(".") + if len(bits) >= 2 and bits[0] == "views_postprocessing": + found.add(bits[1]) + + for node in ast.walk(ast.parse(source)): + if isinstance(node, ast.Import): + for alias in node.names: + record(alias.name) + elif isinstance(node, ast.ImportFrom): + if node.level: + base = package[: len(package) - (node.level - 1)] + resolved = ".".join(base + ([node.module] if node.module else [])) + else: + resolved = node.module or "" + record(resolved) + # `from import ` — the imported NAME is the subpackage. + for alias in node.names: + record(f"{resolved}.{alias.name}") + return found + + +def test_the_invariants_do_not_import_the_machinery(): + """The lower leg of ADR-002's chain: ``/ -> contract/ -> delivery/``. + + ``delivery/`` is the bottom of the stack and must stay usable on its own — that is + what makes the invariants *representation-free* rather than merely + representation-light. Its module-level counterpart is + ``test_clone_readiness.py::test_the_invariants_import_without_the_machinery``, which + proves the same thing in a fresh interpreter; this one also sees imports hidden + inside function bodies, which a subprocess never executes. + + **Written 2026-08-03 because the claim existed without it.** ADR-012 said the + one-way dependency was "enforced by test, not convention"; only the + partner-to-machinery leg was. + """ + forbidden = {"contract", *_PARTNER_PACKAGES} offenders = [ - f.relative_to(_PKG).as_posix() - for f in (_PKG / "contract").rglob("*.py") - if re.search(r"^\s*(?:from|import)\s+views_postprocessing\.unfao", f.read_text(), re.M) + f"{f.relative_to(_PKG).as_posix()} -> views_postprocessing.{name}" + for f in (_PKG / "delivery").rglob("*.py") + for name in sorted(_imported_subpackages(f.read_text(), f) & forbidden) ] assert not offenders, ( - f"contract/ imports the partner package: {offenders}. The machinery must be " - "reusable by views-crafdapi / views-productionapi without taking FAO (C-69)." + f"delivery/ imports upward: {offenders}. The invariants sit below the machinery " + "and must stay usable without it — that is what makes them representation-free " + "rather than merely representation-light (ADR-002, ADR-012)." ) -# --- 3. retired cross-repo contract name (S3 / #184, finishing #158) -------------------- +def test_contract_package_does_not_import_any_partner(): + """The upper leg of ADR-002's chain, and register C-69's fix made permanent. + + Scoped to the declared partner list rather than to ``unfao`` by name: the + single-name version passed while ``contract/`` was free to import ``crafd``, proven + by adding that import and watching the suite stay green. + + Uses the same AST walk as its sibling above, for the same reason — the regex here + was absolute-only, so ``from ..unfao import product`` inside a function body was + invisible to it *and* to the subprocess check, which never executes a function body. + """ + offenders = [ + f"{f.relative_to(_PKG).as_posix()} -> views_postprocessing.{name}" + for f in (_PKG / "contract").rglob("*.py") + for name in sorted(_imported_subpackages(f.read_text(), f) & set(_PARTNER_PACKAGES)) + ] + assert not offenders, ( + f"contract/ imports a partner package: {offenders}. The machinery must be " + "reusable by the next partner without taking this one's product (C-69)." + ) + #: Retired 2026-07-31. The registry itself records the retirement: #: ``former_contract_name = "PLATFORM-001" # retired 2026-07-31``. diff --git a/tests/test_env_declaration.py b/tests/test_env_declaration.py index 85bae19..ad34e8e 100644 --- a/tests/test_env_declaration.py +++ b/tests/test_env_declaration.py @@ -7,11 +7,20 @@ verdict retired it — the launcher declares its env sourcing (views-models M3, merged), and this package only validates (verdict D6). -Imports `unfao.appwrite_env` and its sibling `contract.launch_config` — both are -dependency-light by design (no views-pipeline-core, no pandas), which is what -lets them be exercised directly here. The manager module is not: it needs -views-pipeline-core, absent in test environments, so the manager-side facts are -pinned by source scan instead — the repo's standing pattern. +Imports **every partner's** `appwrite_env` and their shared sibling +`contract.launch_config` — all dependency-light by design (no views-pipeline-core, no +pandas), which is what lets them be exercised directly here. The manager modules are +not exercised directly: instantiating one needs a full Appwrite environment and a +views-models path manager, so the manager-side facts are pinned by source scan instead — +the repo's standing pattern. (The older reason given here, that views-pipeline-core is +"absent in test environments", stopped being true: it is a declared dependency, +CI installs it, and the manager module imports fine.) + +**Scoped to `unfao` by name until #211**, at which point `crafd/appwrite_env.py` +landed pinned to a registry edition at which its own coordinates had no values, and +every check here went on passing over a module it was not looking at. The partner list +now comes from `tests/conftest.PARTNER_PACKAGES` and is asserted against the +filesystem, so the next partner cannot be silently exempt (register C-57). """ import ast @@ -21,30 +30,165 @@ import pytest -from tests.conftest import commit_is_on_main, git_output, require_sibling, sibling_repo +from tests.conftest import ( + PARTNER_PACKAGES, + broken_sibling_overrides, + commit_is_on_main, + git_output, + require_sibling, + sibling_repo, +) from views_postprocessing.contract import launch_config +from views_postprocessing.crafd import appwrite_env as crafd_env from views_postprocessing.unfao import appwrite_env _PKG = Path(__file__).resolve().parent.parent / "views_postprocessing" -_MANAGER_SOURCE = ( - Path(__file__).resolve().parent.parent - / "views_postprocessing" - / "unfao" - / "managers" - / "unfao.py" -) +def _manager_source(partner: str) -> Path: + """``/managers/.py``, asserted to exist. + + The layout is **declared** — ``docs/CLONING.md`` §3 states it as the shape a new + partner supplies — but a declaration this function merely assumes is one it cannot + notice going stale. Without the check below a renamed manager gives a bare + ``FileNotFoundError`` from ``read_text``, which is loud but says nothing about + which rule was broken (ADR-014 §2: assert that a guard's inputs are real). + """ + path = _PKG / partner / "managers" / f"{partner}.py" + assert path.exists(), ( + f"no manager at {path.relative_to(_PKG.parent)}. Every check in this file that " + f"scans {partner}'s manager silently covers nothing without it. The layout is " + "declared in docs/CLONING.md §3 — either follow it or teach this function the " + "new one; do not leave the scan pointing at a path that stopped existing." + ) + return path + + +_SHARED_CLASS = { + "APPWRITE_ENDPOINT": "connection", + "APPWRITE_DATASTORE_PROJECT_ID": "connection", + "APPWRITE_DATASTORE_API_KEY": "secret", + "APPWRITE_PROD_FORECASTS_BUCKET_ID": "target", + "APPWRITE_PROD_FORECASTS_BUCKET_NAME": "target", + "APPWRITE_PROD_FORECASTS_COLLECTION_ID": "target", + "APPWRITE_PROD_FORECASTS_COLLECTION_NAME": "target", + "APPWRITE_METADATA_DATABASE_ID": "target", + "APPWRITE_METADATA_DATABASE_NAME": "target", +} + +#: Per partner: the module, the tuple naming its own outbound store, and how this test +#: treats every name that partner declares. +#: +#: **Parametrised since #211, and that is the point.** Every check below imported +#: ``unfao.appwrite_env`` by name. When ``crafd/appwrite_env.py`` landed pinned two +#: registry editions stale — at a commit where its own four ``APPWRITE_CRAFD_*`` +#: coordinates still carried no values — not one of them fired, because none of them +#: was looking. A drift detector that names its subject cannot detect drift in the +#: subject it does not name (register C-57, amended 2026-08-03). +_PARTNER_ENV = { + "unfao": ( + appwrite_env, + appwrite_env.UNFAO_ENV, + _SHARED_CLASS | { + "APPWRITE_UNFAO_BUCKET_ID": "target", + "APPWRITE_UNFAO_BUCKET_NAME": "target", + "APPWRITE_UNFAO_COLLECTION_ID": "target", + "APPWRITE_UNFAO_COLLECTION_NAME": "target", + }, + ), + "crafd": ( + crafd_env, + crafd_env.CRAFD_ENV, + _SHARED_CLASS | { + "APPWRITE_CRAFD_BUCKET_ID": "target", + "APPWRITE_CRAFD_BUCKET_NAME": "target", + "APPWRITE_CRAFD_COLLECTION_ID": "target", + "APPWRITE_CRAFD_COLLECTION_NAME": "target", + }, + ), +} + +_PARTNERS = tuple(_PARTNER_ENV) + + +#: Function names that belong to python-dotenv and to essentially nothing else. +#: +#: ``set_key``/``get_key``/``unset_key`` are deliberately **absent**. They are dotenv +#: members, but they are also ordinary method names — a first draft included ``set_key`` +#: and flagged ``draft.set_key("run_id", run_id)`` in ``contract/store_metadata.py`` as +#: *"python-dotenv is back in this package"*. That is ADR-014 §3: a guard that cries +#: wolf gets deleted, and then the rule it carried is unguarded. Reaching them requires +#: importing ``dotenv``, which the import half below catches outright. +_DOTENV_CALLS = {"load_dotenv", "find_dotenv", "dotenv_values"} + + +def _dotenv_use(source: str) -> list[str]: + """Real imports of, and calls into, python-dotenv — parsed, not grepped. + + **Prose is not a violation and must not be treated as one.** ``appwrite_env.py``'s + own docstring explains what the retired borrow was, spelling it exactly; a token + scan over the package flags that sentence and gets deleted, after which the rule it + carried is unguarded (ADR-014 §3 — when a guard cries wolf, the matching is wrong + before the scope is). An AST walk sees imports and calls and never sees a docstring + or a comment, so the guard can cover the whole package without lying about prose. + """ + found = [] + for node in ast.walk(ast.parse(source)): + if isinstance(node, ast.Import): + found += [a.name for a in node.names if a.name.split(".")[0] == "dotenv"] + elif isinstance(node, ast.ImportFrom): + if (node.module or "").split(".")[0] == "dotenv": + found.append(f"from {node.module}") + elif isinstance(node, ast.Call): + fn = node.func + name = fn.id if isinstance(fn, ast.Name) else getattr(fn, "attr", None) + if name in _DOTENV_CALLS: + found.append(f"{name}()") + return sorted(set(found)) + + +def test_no_sibling_override_points_at_a_missing_path(): + """Assert the cross-repo checks' inputs are real (ADR-014 §2), once for all of them. + + Every gated check in this repository resolves a sibling checkout and skips when it + is absent. Absent is normal — CI checks out only this repo. A variable that is + *set* and wrong is not normal, and skipping on it means an operator who asked for + the cross-repo assertions silently got none of them, for as long as the typo lives. + """ + broken = broken_sibling_overrides() + assert not broken, ( + f"sibling override(s) set but pointing at nothing: {broken}. Every check gated " + "on those repositories is skipping — you asked for them and are getting none. " + "Fix the path or unset the variable." + ) def test_the_dotenv_borrow_is_dead(): - # Prose may mention the dead borrow; importing or calling it may not. - text = _MANAGER_SOURCE.read_text() - for token in ("load_dotenv", "from dotenv", "import dotenv", "find_dotenv"): - assert token not in text, ( - f"{token!r} is back in the manager (þing-01 #134 killed the borrow): " - "the launcher declares the environment; this package must never load one." - ) + """þing-01 #134's verdict, held over the whole package rather than two files. + + **This guard has been too narrow twice.** It was scoped to ``unfao``'s manager, and + a live ``load_dotenv`` in ``crafd/managers/crafd.py`` left the suite green. Widened + to both managers, it still covered 2 files of 30: verified 2026-08-03 that + ``load_dotenv(find_dotenv())`` at module scope in ``unfao/appwrite_env.py`` — *the + entry validator whose own docstring says the borrow is dead* — ran on import with + the suite green, as did the same line in ``product.py`` and ``launch_config.py``. + + The rule was never about managers. þing-01 D6 says **this package** validates the + environment and never loads one, so the scan is the package. Register **C-74**'s + shape twice over: a guard narrower than the sentence describing it. + """ + offenders = { + source.relative_to(_PKG).as_posix(): used + for source in sorted(_PKG.rglob("*.py")) + for used in [_dotenv_use(source.read_text())] + if used + } + assert not offenders, ( + f"python-dotenv is back in this package: {offenders}. þing-01 #134 killed the " + "borrow — the launcher declares the environment (verdict D6) and this package " + "validates it fail-loud. A module that loads a .env reintroduces the copy-chain " + "the assembly retired, and does it at import time, before any validation runs." + ) def test_missing_env_raises_naming_every_missing_variable(monkeypatch): @@ -89,18 +233,27 @@ def test_empty_string_counts_as_missing(monkeypatch): ) -def test_declared_names_match_the_manager_reads(): +@pytest.mark.parametrize("partner", _PARTNERS) +def test_declared_names_match_the_manager_reads(partner): # Declaration and use must not drift: every APPWRITE_* name the manager # actually reads is declared, and both store paths validate before building. - text = _MANAGER_SOURCE.read_text() + # + # Parametrised since #211. The crafd manager reads its own four coordinates by + # hardcoded literal (`crafd.py:363-366`) exactly as the FAO one does, so it can + # drift from its own declaration in exactly the same way — and did not have a + # check saying otherwise. + module, own_store, _ = _PARTNER_ENV[partner] + text = _manager_source(partner).read_text() read_names = set(re.findall(r'os\.getenv\("(APPWRITE_[A-Z_]+)"\)', text)) - declared = set( - appwrite_env.CONNECTION_ENV + appwrite_env.PROD_FORECASTS_ENV + appwrite_env.UNFAO_ENV + declared = set(module.CONNECTION_ENV + module.PROD_FORECASTS_ENV + own_store) + assert read_names == declared, ( + f"[{partner}] the manager reads {sorted(read_names - declared)} without " + f"declaring them, and declares {sorted(declared - read_names)} without " + "reading them" ) - assert read_names == declared # One validation per store construction. Was 3 until #149 retired the legacy # `_save`, whose Appwrite config duplicated `_unfao_appwrite_config` verbatim; - # the two survivors are the production_forecasts read and the unfao_bucket write. + # the two survivors are the production_forecasts read and the partner-bucket write. assert text.count("appwrite_env.assert_env_declared(") == 2 @@ -223,36 +376,37 @@ def test_secret_env_names_follow_the_seam_contract_naming_rule(): #: inferred from a name's prefix"), reproduced inside the test written to enforce it. #: It happened to be correct, which is what makes the habit worth breaking rather than #: excusing. Adding a name without classifying it now fails below. -_EXPECTED_CLASS = { - "APPWRITE_ENDPOINT": "connection", - "APPWRITE_DATASTORE_PROJECT_ID": "connection", - "APPWRITE_DATASTORE_API_KEY": "secret", - "APPWRITE_PROD_FORECASTS_BUCKET_ID": "target", - "APPWRITE_PROD_FORECASTS_BUCKET_NAME": "target", - "APPWRITE_PROD_FORECASTS_COLLECTION_ID": "target", - "APPWRITE_PROD_FORECASTS_COLLECTION_NAME": "target", - "APPWRITE_UNFAO_BUCKET_ID": "target", - "APPWRITE_UNFAO_BUCKET_NAME": "target", - "APPWRITE_UNFAO_COLLECTION_ID": "target", - "APPWRITE_UNFAO_COLLECTION_NAME": "target", - "APPWRITE_METADATA_DATABASE_ID": "target", - "APPWRITE_METADATA_DATABASE_NAME": "target", -} +def test_every_partner_package_has_its_environment_checked_here(): + """Assert this module's declared scope is the real one (ADR-014 §2). + + ``_PARTNER_ENV`` is what every parametrised check below iterates. A partner + package missing from it is a partner whose coordinates nobody compares against the + registry — and the suite stays green, which is exactly how ``crafd`` arrived. So + the keys are checked against the repository's single declared partner list rather + than maintained by hand and hoped over. + """ + assert set(_PARTNERS) == set(PARTNER_PACKAGES), ( + f"partner packages without an environment check: " + f"{sorted(set(PARTNER_PACKAGES) - set(_PARTNERS))}; " + f"checked but no longer a partner: {sorted(set(_PARTNERS) - set(PARTNER_PACKAGES))}. " + "Add the partner to _PARTNER_ENV — an unlisted one is silently exempt from " + "every registry comparison in this file." + ) -def test_every_declared_name_is_classified_here(): - """The map above must cover the module exactly — no silent gaps, no strays. +@pytest.mark.parametrize("partner", _PARTNERS) +def test_every_declared_name_is_classified_here(partner): + """The map above must cover each module exactly — no silent gaps, no strays. Without this, adding a name to one of the ENV tuples would simply not be checked against the registry, and the drift test would keep passing while covering less. That is register **C-74**'s shape: a guard quietly narrower than it claims. """ - declared = set( - appwrite_env.CONNECTION_ENV + appwrite_env.PROD_FORECASTS_ENV + appwrite_env.UNFAO_ENV - ) - assert set(_EXPECTED_CLASS) == declared, ( - f"unclassified: {sorted(declared - set(_EXPECTED_CLASS))}; " - f"stale: {sorted(set(_EXPECTED_CLASS) - declared)}" + module, own_store, expected = _PARTNER_ENV[partner] + declared = set(module.CONNECTION_ENV + module.PROD_FORECASTS_ENV + own_store) + assert set(expected) == declared, ( + f"[{partner}] unclassified: {sorted(declared - set(expected))}; " + f"stale: {sorted(set(expected) - declared)}" ) @@ -274,47 +428,55 @@ def _declared_classes(registry: dict) -> dict[str, str]: } -def test_every_declared_name_exists_in_the_registry_with_the_class_we_treat_it_as(): +@pytest.mark.parametrize("partner", _PARTNERS) +def test_every_declared_name_exists_in_the_registry_with_the_class_we_treat_it_as(partner): """C-57: a rename or reclassification upstream must not be silent here.""" repo = require_sibling("views-appwrite") declared = _declared_classes(_load_registry(repo)) + _, _, expected_class = _PARTNER_ENV[partner] - missing = sorted(n for n in _EXPECTED_CLASS if n not in declared) + missing = sorted(n for n in expected_class if n not in declared) assert not missing, ( - f"names this package requires are absent from the Appwrite Seam Contract's " - f"registry: {missing}. Either the registry retired them or this module invented " - "them; the registry is the authority." + f"[{partner}] names this package requires are absent from the Appwrite Seam " + f"Contract's registry: {missing}. Either the registry retired them or this " + "module invented them; the registry is the authority." ) misclassified = { n: (expected, declared[n]) - for n, expected in _EXPECTED_CLASS.items() + for n, expected in expected_class.items() if declared[n] != expected } assert not misclassified, ( - f"class mismatch (expected, registry) {misclassified}. Class is DECLARED by the " - "registry, never inferred from a name's prefix — a coordinate treated as a " - "secret (or the reverse) is a redaction bug waiting to happen." + f"[{partner}] class mismatch (expected, registry) {misclassified}. Class is " + "DECLARED by the registry, never inferred from a name's prefix — a coordinate " + "treated as a secret (or the reverse) is a redaction bug waiting to happen." ) -def test_the_pinned_contract_edition_still_matches_the_registry(): +@pytest.mark.parametrize("partner", _PARTNERS) +def test_the_pinned_contract_edition_still_matches_the_registry(partner): """The check that catches everything the other one cannot — including additions. Names and classes catch a rename. The edition catches **any** other change: a new target this repo ought to adopt, a retired secret slot, a reworded rule. It fails loudly and tells you what to do rather than what broke. + + It is also the check that would have caught #211's stale crafd pin on the day it + was written, had it been looking at crafd. It was not. It is now. """ repo = require_sibling("views-appwrite") + module = _PARTNER_ENV[partner][0] actual = _load_registry(repo)["meta"]["version"] - assert actual == appwrite_env.SEAM_CONTRACT_VERSION, ( - f"the Appwrite Seam Contract's registry moved to v{actual}; this repo declares " - f"v{appwrite_env.SEAM_CONTRACT_VERSION}. Re-verify appwrite_env's declaration " - f"against v{actual}, then bump SEAM_CONTRACT_VERSION and SEAM_CONTRACT_COMMIT " - "together. Do not bump one alone — the pair is the claim." + assert actual == module.SEAM_CONTRACT_VERSION, ( + f"the Appwrite Seam Contract's registry moved to v{actual}; " + f"{partner}/appwrite_env.py declares v{module.SEAM_CONTRACT_VERSION}. Re-verify " + f"that module's declaration against v{actual}, then bump SEAM_CONTRACT_VERSION " + "and SEAM_CONTRACT_COMMIT together. Do not bump one alone — the pair is the claim." ) -def test_the_pinned_commit_is_reachable_from_the_contract_repos_main(): +@pytest.mark.parametrize("partner", _PARTNERS) +def test_the_pinned_commit_is_reachable_from_the_contract_repos_main(partner): """Existence is not reachability, and that distinction cost a merged PR (#196). S3 pinned a commit resolved with ``rev-parse HEAD`` on a views-appwrite checkout @@ -323,44 +485,60 @@ def test_the_pinned_commit_is_reachable_from_the_contract_repos_main(): ``main``, declared a version that was never ratified, and was withdrawn. """ repo = require_sibling("views-appwrite") - commit = appwrite_env.SEAM_CONTRACT_COMMIT + commit = _PARTNER_ENV[partner][0].SEAM_CONTRACT_COMMIT if not git_output(repo, "cat-file", "-t", commit): pytest.skip( f"{commit} is not in the local views-appwrite checkout — run `git fetch` " "there; a stale clone cannot answer whether the pin reached main" ) assert commit_is_on_main(repo, commit), ( - f"the pinned commit {commit!r} is not an ancestor of views-appwrite's main. A " + f"[{partner}] the pinned commit {commit!r} is not an ancestor of " + "views-appwrite's main. A " "pin taken from a working copy's HEAD can land on an unmerged branch — that is " "#196, verbatim. Re-pin from `git rev-parse --short origin/main`." ) -def test_the_drift_check_would_catch_a_rename(tmp_path): +@pytest.mark.parametrize("partner", _PARTNERS) +def test_the_drift_check_would_catch_a_rename(partner): """A gated test that cannot fail is decoration — so prove this one bites in CI. Runs with **no** views-appwrite checkout: a synthetic registry with one name renamed and one reclassified, fed to the same comparison the gated tests use. + + Parametrised over partners for the reason the whole file now is: a detector proven + against one partner's coordinates is not proven against another's, and the gated + checks skip on any machine without a views-appwrite checkout — so this is the only + proof that runs everywhere. """ + module, own_store, expected_class = _PARTNER_ENV[partner] + # The partner's own outbound bucket id — a `target` in the real registry, which is + # what makes reclassifying it to `secret` the meaningful mutation. + canary = own_store[0] + registry = { - "meta": {"version": appwrite_env.SEAM_CONTRACT_VERSION}, + "meta": {"version": module.SEAM_CONTRACT_VERSION}, "connection": {"APPWRITE_ENDPOINT": {"class": "connection"}}, - "target": {"APPWRITE_UNFAO_BUCKET_ID": {"class": "secret"}}, # reclassified + "target": {canary: {"class": "secret"}}, # reclassified "secret": {"APPWRITE_DATASTORE_API_KEY": {"class": "secret"}}, } declared = _declared_classes(registry) + assert expected_class[canary] == "target", ( + f"{canary} is not classified as a target here, so reclassifying it below is " + "not the mutation this test believes it is" + ) assert "APPWRITE_DATASTORE_PROJECT_ID" not in declared, "fixture should omit it" - missing = sorted(n for n in _EXPECTED_CLASS if n not in declared) + missing = sorted(n for n in expected_class if n not in declared) assert missing, "the detector reported no missing names against a registry that omits most" mismatched = [ - n for n, expected in _EXPECTED_CLASS.items() + n for n, expected in expected_class.items() if n in declared and declared[n] != expected ] - assert "APPWRITE_UNFAO_BUCKET_ID" in mismatched, ( - "a target reclassified as a secret went unnoticed — that is the case where " - "getting it wrong leaks or hides a value" + assert canary in mismatched, ( + f"[{partner}] a target reclassified as a secret went unnoticed — that is the " + "case where getting it wrong leaks or hides a value" ) diff --git a/tests/test_frame_extraction.py b/tests/test_frame_extraction.py index 947aae1..210d628 100644 --- a/tests/test_frame_extraction.py +++ b/tests/test_frame_extraction.py @@ -1,4 +1,4 @@ -"""Tests for the representation seam (`unfao/frame_extraction.py`). +"""Tests for the representation seam (`contract/frame_extraction.py`). **These were parity tests until #151.** They proved that the same data expressed as a pandas MultiIndex frame and as a views-frames `PredictionFrame` yielded *identical* diff --git a/tests/test_product.py b/tests/test_product.py index ab4b0a8..8cdab80 100644 --- a/tests/test_product.py +++ b/tests/test_product.py @@ -1,22 +1,169 @@ -"""The declared FAO product (unfao/product.py) — ADR-013 §4.2a/§6/§4.1a/§11.4 pins.""" +"""Each partner's declared product — ADR-013 §4.2a/§6/§4.1a/§11.4 pins. -from views_postprocessing.unfao import product +**Scoped to the FAO product until #211.** When `crafd/product.py` landed, every pin +here still read `unfao.product`: nothing asserted CRAF'd's consumer document name, +its target vocabulary, or that its upload interlock defaulted off. The values were all +correct — but "correct and unchecked" is the state a product declaration is in right +up until the day it is not, and §4.1a's failure mode is silence, not an error. +The consumer-name pin is the one that matters most and the one a partner-scoped test +could never catch: a document written under a name the consumer does not filter for is +delivered, stored, billed, and invisible. That is not hypothetical here — six +`orange_ensemble` forecast documents sat in `unfao_bucket` for months while FAO's +forecast serving read empty (ADR-013 §11.4 post-adoption record, register C-01). -def test_targets_are_the_pinned_wire_vocabulary(): +Parametrised over `tests/conftest.PARTNER_PACKAGES`, so a third partner is caught by +`test_clone_readiness.py::test_the_declared_partner_list_is_the_real_one` rather than +quietly skipped. +""" + +from __future__ import annotations + +import importlib +import re +from pathlib import Path + +import pytest + +from tests.conftest import CONSUMER_REPO, PARTNER_PACKAGES, SIBLING_ENV, require_sibling + +#: partner -> the document name its consumer filters on, DECLARED here rather than +#: read back from the module under test. +#: +#: A test that asserts ``product.CONSUMER_DOCUMENT_NAME == product.CONSUMER_DOCUMENT_NAME`` +#: passes for every possible value, which is the shape of the replica defect epic #181 +#: spent a story retiring. So the expected value is written out. +#: +#: **But a literal transcribed from another repository is a guarantee, and ADR-014 §1 +#: says a guarantee needs a check.** On its own this pin catches drift authored *here* — +#: which was never the danger. If views-crafdapi changed its filter tomorrow, this file +#: would stay green, the delivery would upload, and the document would be invisible: +#: ADR-013 §4.1a, the defect that stranded six `orange_ensemble` documents in +#: `unfao_bucket` while FAO's forecast serving read empty for months. +#: +#: ``test_the_declared_consumer_name_still_matches_the_consumer`` closes that half +#: against the sibling checkout, following the pattern +#: ``tests/test_env_declaration.py`` already uses for the coordinate registry: declared +#: locally, verified across the seam when the other repo is on disk. +_CONSUMER_DOCUMENT_NAME = { + "unfao": "un_fao", + "crafd": "un_crafd", +} + +#: Where the consumer's name lives, and the mechanism that consumes it. Both are +#: pinned: a consumer that kept the string but started filtering on a different field +#: would strand a delivery just as thoroughly as one that renamed it. +_CONSUMER_PATH_MANAGER = re.compile(r'APIPathManager\(\s*"([a-z0-9_]+)"') +_CONSUMER_FILTER = 'filters["name"] = self.model_path.model_name' + + +def _product(partner: str): + return importlib.import_module(f"views_postprocessing.{partner}.product") + + +def _consumer_package(repo: Path) -> Path: + """``src/views_api`` inside a consumer checkout.""" + candidates = sorted((repo / "src").glob("views_*api")) + assert len(candidates) == 1, ( + f"expected exactly one package under {repo}/src, found {candidates}. The " + "consumer's layout changed; this test's assumption about where to look is " + "part of what it asserts." + ) + return candidates[0] + + +def test_every_partner_has_its_consumer_name_pinned(): + """Assert this file's declared scope is the real one (ADR-014 §2).""" + assert set(_CONSUMER_DOCUMENT_NAME) == set(PARTNER_PACKAGES), ( + f"partners with no pinned consumer name: " + f"{sorted(set(PARTNER_PACKAGES) - set(_CONSUMER_DOCUMENT_NAME))}; " + f"pinned but no longer a partner: " + f"{sorted(set(_CONSUMER_DOCUMENT_NAME) - set(PARTNER_PACKAGES))}. An unpinned " + "partner can be renamed into invisibility without a single test failing." + ) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_targets_are_the_pinned_wire_vocabulary(partner): # §7a: wire names, never internal model names; order stable for manifests. - assert product.TARGETS == ("lr_ged_sb", "lr_ged_ns", "lr_ged_os") + assert _product(partner).TARGETS == ("lr_ged_sb", "lr_ged_ns", "lr_ged_os") -def test_s_min_is_the_walking_skeleton_floor(): - assert product.S_MIN == 2 +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_s_min_is_the_walking_skeleton_floor(partner): + # §6's walking-skeleton value. The production floor is still an open maintainer + # item in ADR-013's post-adoption record; both partners inherit it, deliberately. + assert _product(partner).S_MIN == 2 -def test_consumer_document_name_is_the_faoapi_pin(): - # §4.1a: any other name is invisible to faoapi's unconditional name filter. - assert product.CONSUMER_DOCUMENT_NAME == "un_fao" +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_consumer_document_name_is_the_pin_its_consumer_filters_on(partner): + # §4.1a: any other name is invisible to the consumer's unconditional name filter. + assert _product(partner).CONSUMER_DOCUMENT_NAME == _CONSUMER_DOCUMENT_NAME[partner] -def test_upload_interlock_defaults_off(): +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_upload_interlock_defaults_off(partner): # §11.4: the default configuration must be unable to touch the live bucket. - assert product.UPLOAD_ENABLED is False + assert _product(partner).UPLOAD_ENABLED is False + + +# ── across the seam: the pin above, checked against the repo that owns the fact ── + + +def test_every_partner_has_a_declared_consumer_repository(): + """The gated check below iterates this map; assert it is real (ADR-014 §2).""" + assert set(CONSUMER_REPO) == set(PARTNER_PACKAGES), ( + f"partners with no declared consumer repository: " + f"{sorted(set(PARTNER_PACKAGES) - set(CONSUMER_REPO))}. Without one, that " + "partner's consumer-name pin is never checked against the consumer." + ) + undeclared = sorted(r for r in CONSUMER_REPO.values() if r not in SIBLING_ENV) + assert not undeclared, ( + f"consumer repositories with no SIBLING_ENV entry: {undeclared}. " + "require_sibling() raises KeyError rather than skipping for those, so the " + "check would fail confusingly instead of skipping cleanly." + ) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_declared_consumer_name_still_matches_the_consumer(partner): + """§4.1a across the seam — the half a local literal cannot carry. + + The consumer decides what it filters on. This repository declares what it writes. + Agreement between the two is the whole of §4.1a, and it is not a fact this + repository owns — so it is checked against the consumer's checkout, exactly as the + coordinate registry is checked against views-appwrite's. + + Gated: skips without the sibling, naming the variable to set. The always-on pin + above still runs everywhere, so CI is never left with nothing. + """ + repo = require_sibling(CONSUMER_REPO[partner]) + pkg = _consumer_package(repo) + + api = (pkg / "managers" / "api.py").read_text() + found = _CONSUMER_PATH_MANAGER.findall(api) + assert len(found) == 1, ( + f"expected exactly one APIPathManager(...) construction in " + f"{CONSUMER_REPO[partner]}'s managers/api.py, found {found}. More than one " + "means the consumer serves several document names and this check no longer " + "knows which one is ours." + ) + assert found[0] == _CONSUMER_DOCUMENT_NAME[partner], ( + f"{CONSUMER_REPO[partner]} filters on {found[0]!r}; this repository declares " + f"{_CONSUMER_DOCUMENT_NAME[partner]!r} and writes it as " + f"{partner}/product.py's CONSUMER_DOCUMENT_NAME. A delivery under a name the " + "consumer does not ask for is uploaded, stored, and invisible (ADR-013 §4.1a)." + ) + assert found[0] == _product(partner).CONSUMER_DOCUMENT_NAME, ( + "the declared pin in this file and the module's constant disagree — the " + "always-on test above should have caught this first" + ) + + manager = (pkg / "managers" / "prediction" / "manager.py").read_text() + assert _CONSUMER_FILTER in manager, ( + f"{CONSUMER_REPO[partner]} no longer selects by " + f"{_CONSUMER_FILTER!r}. The name may still match while the consumer filters on " + "something else entirely — same invisibility, different cause. Re-read its " + "selection path before assuming this repository's deliveries are reachable." + ) diff --git a/tests/test_views_frames_conformance.py b/tests/test_views_frames_conformance.py index e17a3fe..5e94c74 100644 --- a/tests/test_views_frames_conformance.py +++ b/tests/test_views_frames_conformance.py @@ -1,4 +1,4 @@ -"""Conformance + parity tests for the views-frames constructors (`unfao/frames.py`). +"""Conformance + parity tests for the views-frames constructors (`contract/frames.py`). `build_prediction_frame` / `build_target_frame` take **declared primitives** — a 2-D `(N, S)` value array + `(time, unit)` arrays — and build a views-frames value object. These diff --git a/views_postprocessing/contract/__init__.py b/views_postprocessing/contract/__init__.py index b1bb6a5..421d0b1 100644 --- a/views_postprocessing/contract/__init__.py +++ b/views_postprocessing/contract/__init__.py @@ -4,7 +4,10 @@ delivery/ what makes a delivery VALID — representation-free invariants contract/ how a delivery is BUILT — this package - unfao/ who a delivery is FOR — one partner's product and manager + unfao/ who a delivery is FOR — the FAO product and its manager + crafd/ who a delivery is FOR — the CRAF'd product and its manager + +There is one package per partner and this package must import none of them. **Why this package exists (register C-69, #153).** Until now ~800 of `unfao/`'s 1,001 lines were partner-neutral: the whole ADR-013 wire, the frame seam, the GAUL @@ -17,9 +20,10 @@ coming UN-agency deliveries reuse these"* and nothing in it names FAO. This package extends that property to the machinery. -**Nothing here may import `unfao`.** That is the whole point, and it is enforced -mechanically rather than by convention — see `tests/test_clone_readiness.py` (#155). +**Nothing here may import a partner package.** That is the whole point, and it is +enforced mechanically rather than by convention — see `tests/test_clone_readiness.py` +(#155), which since #211 checks every declared partner rather than only `unfao`. -What a clone supplies for itself: its product declarations, its store coordinates, +What a partner supplies for itself: its product declarations, its store coordinates, and its manager. See `docs/CLONING.md`. """ diff --git a/views_postprocessing/contract/wire/__init__.py b/views_postprocessing/contract/wire/__init__.py index 1b0b3c6..ed88c67 100644 --- a/views_postprocessing/contract/wire/__init__.py +++ b/views_postprocessing/contract/wire/__init__.py @@ -2,7 +2,8 @@ One closure: this package changes when the wire contract (``docs/ADRs/013_sampled_forecast_wire_contract.md``) changes, and for no other -reason. The FAO *product* declaration lives outside it (``unfao/product.py`` — the partner layer; -different reason to change); representation-free *rules* live below it +reason. Each partner's *product* declaration lives outside it (``unfao/product.py``, +``crafd/product.py`` — the partner layer, a different reason to change); +representation-free *rules* live below it (``delivery/``); the manager above composes it. """ diff --git a/views_postprocessing/crafd/__init__.py b/views_postprocessing/crafd/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/views_postprocessing/crafd/appwrite_env.py b/views_postprocessing/crafd/appwrite_env.py new file mode 100644 index 0000000..68c42ac --- /dev/null +++ b/views_postprocessing/crafd/appwrite_env.py @@ -0,0 +1,105 @@ +"""The Appwrite-seam environment this package requires, DECLARED (þing-01 P1, #134). +Clone of ``unfao/appwrite_env.py`` for the CRAF'd delivery (its own outbound bucket). + +Names follow **the Appwrite Seam Contract**'s coordinate registry (connection + +target classes) plus the one operator-issued secret slot. The contract is homed in +views-appwrite and referenced by pinned URL, never copied — copies were the +platform's original failure (þing-01 sáttmál S6): + + https://github.com/views-platform/views-appwrite/blob/90fc105/docs/ADRs/platform/coordinate_registry.toml + +That pin is registry **v1.4.1** — declared below as ``SEAM_CONTRACT_VERSION`` / +``SEAM_CONTRACT_COMMIT`` so the pin is a value a test can check rather than a fact +buried in prose. A pinned URL does not rot, but it does go stale, and nothing in this +repository could previously tell you it had (register C-57). + +**The first pin taken here predated its own coordinates having values.** At commit +``47172af`` the four ``APPWRITE_CRAFD_*`` names were reserved slots with **no values**. +The operator filled them in views-appwrite PR #38 (merged ``12eb6c6``, 2026-08-02), +which landed *within* v1.3.0 without bumping the version; v1.4.0 came two hours later +in PR #40. So the stale pin was stale by commit, not only by version — and no test +could see it, because the drift check was scoped to the FAO package by name (register +C-57, amended 2026-08-03). It is now scoped to both. + +**Pin from the tip of `main`, never from a sibling checkout's `HEAD`** (#196). + +The LAUNCHER assembles the environment +(views-models M3: run.sh reads the owned registry; the secret stays the operator +slot) — this package loads no dotenv and validates fail-loud instead (verdict D6). + +Deliberately dependency-light: no pipeline-core imports, so the declaration is +importable (and testable) everywhere. +""" + +from __future__ import annotations + +import logging +import os + +logger = logging.getLogger(__name__) + +#: The Appwrite Seam Contract edition these names were verified against, and the commit +#: this repo cites. **A version string and a sha are not coordinate values** — the +#: registry forbids copying its values, and nothing here copies one. What is recorded is +#: *which edition was read*, which is exactly what makes drift detectable. +#: +#: Bumping these is not bookkeeping: it asserts that someone re-checked this module's +#: declaration against that edition of the registry. ``tests/test_env_declaration.py`` +#: enforces the pair — for this package and for ``unfao`` alike — against a local +#: views-appwrite checkout when one is present. +SEAM_CONTRACT_VERSION = "1.4.1" +SEAM_CONTRACT_COMMIT = "90fc105" + +CONNECTION_ENV = ( + "APPWRITE_ENDPOINT", + "APPWRITE_DATASTORE_PROJECT_ID", + "APPWRITE_DATASTORE_API_KEY", # secret slot — the value must never be logged +) +PROD_FORECASTS_ENV = ( + "APPWRITE_PROD_FORECASTS_BUCKET_ID", + "APPWRITE_PROD_FORECASTS_BUCKET_NAME", + "APPWRITE_PROD_FORECASTS_COLLECTION_ID", + "APPWRITE_PROD_FORECASTS_COLLECTION_NAME", + "APPWRITE_METADATA_DATABASE_ID", + "APPWRITE_METADATA_DATABASE_NAME", +) +CRAFD_ENV = ( + "APPWRITE_CRAFD_BUCKET_ID", + "APPWRITE_CRAFD_BUCKET_NAME", + "APPWRITE_CRAFD_COLLECTION_ID", + "APPWRITE_CRAFD_COLLECTION_NAME", + "APPWRITE_METADATA_DATABASE_ID", + "APPWRITE_METADATA_DATABASE_NAME", +) + + +def assert_env_declared(names: tuple, *, store: str) -> None: + """Entry validation (þing-01 D6): every required name resolved and non-empty, + or raise naming ALL missing variables — never a partial config that half-works. + + Logs at ERROR before raising (ADR-008): the refusal must survive in the run's + log, not only in a traceback the operator no longer has. Names only — the + resolved value of a variable is never read, and never logged. + + Args: + names: the environment-variable names this store requires. + store: the store being configured, for the message. + + Raises: + EnvironmentError: naming every missing variable. + """ + missing = [name for name in names if not os.getenv(name)] + if missing: + # `missing` holds NAMES, never values: membership is decided by + # `os.getenv(name)` being falsy, and the resolved value is never read. + # That is what makes this loggable at all — CONNECTION_ENV carries the + # APPWRITE_DATASTORE_API_KEY secret slot. + err_msg = ( + f"{store}: the launcher did not assemble the required environment — " + f"missing {missing}. Coordinates come from the Appwrite Seam Contract's " + "coordinate registry, homed in views-appwrite (views-models run.sh " + "declares its sourcing); the secret is the operator slot. This package " + "no longer loads any dotenv (#134)." + ) + logger.error(err_msg) # ADR-008: logged persistently AND raised + raise EnvironmentError(err_msg) diff --git a/views_postprocessing/crafd/managers/__init__.py b/views_postprocessing/crafd/managers/__init__.py new file mode 100644 index 0000000..fe29bf0 --- /dev/null +++ b/views_postprocessing/crafd/managers/__init__.py @@ -0,0 +1 @@ +from .crafd import CRAFDPostProcessorManager as CRAFDPostProcessorManager \ No newline at end of file diff --git a/views_postprocessing/crafd/managers/crafd.py b/views_postprocessing/crafd/managers/crafd.py new file mode 100644 index 0000000..12119f5 --- /dev/null +++ b/views_postprocessing/crafd/managers/crafd.py @@ -0,0 +1,410 @@ +from views_pipeline_core.managers.postprocessor.postprocessor import ( + PostprocessorManager, + PostprocessorPathManager, +) +import logging + +from views_pipeline_core.modules.appwrite.file import AppwriteConfig +from views_pipeline_core.modules.datastore import DatastoreModule +from views_pipeline_core.managers.model import ForecastingModelManager + +from views_pipeline_core.managers.ensemble import EnsemblePathManager +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.crafd import appwrite_env, product +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 +from pathlib import Path + +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. + # (Until 2026-08-03 this comment said the store "only LOGS": false, and + # self-defeating — if it only logged, `success` would be True.) + success = getattr(result, "success", None) + if success is None and hasattr(result, "to_dict"): + success = result.to_dict().get("success") + if success is False: + 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}" + ) + + +class CRAFDPostProcessorManager(PostprocessorManager, ForecastingModelManager): + def __init__( + self, + model_path: PostprocessorPathManager, + wandb_notifications: bool = True, + use_prediction_store: bool = False, + ) -> None: + super().__init__(model_path, wandb_notifications, use_prediction_store) + + # Add your custom initialization below + logger.info(f"Initializing {self.__class__.__name__}") + self._forecast_resolution = None # {target: TargetLease}, set by _read + self._historical_frame = None # views_frames.FeatureFrame, set by _read + self.ensemble_path_manager = None + + def _read_historical_frame(self): + """#126: historical actuals as a views_frames.FeatureFrame — the first + production consumer of pipeline-core's frame-native fetch. Same clip + policy as the legacy path (producer-sourced boundary, degrade-open).""" + frame = self._data_loader.get_feature_frame( + partition="forecasting", use_saved=False, level="pgm", validate=True + ) + try: + lv = source_metadata.last_valid_month_id(self.configs.get("zarr_url")) + except Exception: + logger.warning("last_valid_month_id unavailable; skipping clip (degrade-open, C-26).", exc_info=True) + lv = None + if lv is None: + self._historical_frame = frame + return + fabricated = observed_range.fabricated_months(frame_extraction.months_of(frame), lv) + if len(fabricated): + logger.warning( + "Dropping %d fabricated (unobserved) month(s) above last_valid_month_id=%d.", + len(fabricated), lv, + ) + frame = frame_extraction.drop_months_above(frame, lv) + self._historical_frame = frame + + def _read_historical_data(self): + """Historical actuals, frame-native only (#126). + + The queryset must DECLARE ``data_format: feature_frame`` — pipeline-core's + ``declared_data_format`` is the one gate, and a queryset that declares + anything else is refused rather than quietly read through a retired pandas + path (register C-63, #149). + """ + # 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()) + ) + self._initialize_data_loader() + self._read_historical_frame() + + def _prod_forecasts_datastore(self) -> DatastoreModule: + """The shared internal store (ADR-013's 'shared shelf'), configured from the + launcher-assembled environment (validated fail-loud; þing-01 #134 — no dotenv + is loaded here). + + pipeline-core's ``DatastoreModule.get_predictions_by_metadata`` injects an + automatic ``name == model_name`` filter on every lookup. The contract read + must **not** have it: ADR-013 artifacts are named + ``{run_id}__{target}__m{month}.arrow.parquet`` (never the bare ensemble + name), so an injected ``name == "rusty_bucket"`` matches nothing and also + clobbers the wire layer's own run-id / target / name filters. Suppressed + unconditionally below — the retired legacy reader was the only caller that + needed it on (#149).""" + ensemble_name = self.configs.get("ensemble", None) + if not ensemble_name: + err_msg = "Ensemble name must be provided in configs with the `ensemble` key for forecasting. Cannot proceed." + logger.error(err_msg) + raise ValueError(err_msg) + self.ensemble_path_manager = EnsemblePathManager(ensemble_name_or_path=ensemble_name, validate=False) + # ensemble_configs = EnsembleManager( + # ensemble_path=self.ensemble_path_manager, + # ).configs + + # loa = ensemble_configs.get("level", None) + loa = "pgm" + if not loa: + err_msg = "level must be defined in the ensemble configurations (e.g, pgm, cm). Cannot proceed." + logger.error(err_msg) + raise ValueError(err_msg) + + appwrite_env.assert_env_declared( + appwrite_env.CONNECTION_ENV + appwrite_env.PROD_FORECASTS_ENV, + store="production_forecasts datastore", + ) + appwrite_config = AppwriteConfig( + path_manager=self.ensemble_path_manager, + endpoint=os.getenv("APPWRITE_ENDPOINT"), + project_id=os.getenv("APPWRITE_DATASTORE_PROJECT_ID"), + credentials=os.getenv("APPWRITE_DATASTORE_API_KEY"), + auth_method="api_key", + cache_ttl_hours=24, + bucket_id=os.getenv("APPWRITE_PROD_FORECASTS_BUCKET_ID"), + bucket_name=os.getenv("APPWRITE_PROD_FORECASTS_BUCKET_NAME"), + collection_id=os.getenv("APPWRITE_PROD_FORECASTS_COLLECTION_ID"), + collection_name=os.getenv("APPWRITE_PROD_FORECASTS_COLLECTION_NAME"), + database_id=os.getenv("APPWRITE_METADATA_DATABASE_ID"), + database_name=os.getenv("APPWRITE_METADATA_DATABASE_NAME"), + ) + datastore = DatastoreModule(appwrite_file_manager_config=appwrite_config) + # Suppress the automatic name==model_name filter (see docstring). model_path + # is used by DatastoreModule only for that injection and for uploads; the + # contract read neither uploads nor performs any model-scoped lookup. + datastore.model_path = None + return datastore + + def _read_forecast_data_contract(self): + """ADR-013 contract inbound (epic #105; streaming since the run-0 OOM fix): + RESOLVE the newest fully-manifested run — manifests + pinned shard + file_ids only, no heavy bytes. Frames materialize one target at a time + inside the sink at _save (each lease loads → verifies → curates → is + released). The `wire/` package owns the policy; this method only adapts + the store (DIP) and declares the product facts (region curation + + coverage expectations live in the lease, where frames exist).""" + port = _ContractStorePort(self._prod_forecasts_datastore()) + region = self.configs.get("region") + self._forecast_resolution = source_selection.resolve_run( + port, + expected_targets=product.TARGETS, + expected_ensemble=self.configs["ensemble"], + excluded_gids=coverage.excluded_for(region), + expected_cells=coverage.expected_for(region), + ) + run_id = next(iter(self._forecast_resolution.values())).run_id + logger.info( + "Contract inbound resolved: run %s (%d targets leased; region=%r; " + "heavy fetch deferred to delivery).", + run_id, + len(self._forecast_resolution), + region, + ) + + def _read_forecast_data(self): + """Forecast inbound — ADR-013 contract only. + + The launcher must DECLARE ``wire_contract: True``. The pandas reader this + key used to select was retired in #149; omitting the key is a refusal, not + a fallback (register C-63). + """ + launch_config.assert_contract_mode(self.configs) + self._read_forecast_data_contract() + + def _read(self) -> any: + self._read_historical_data() + self._read_forecast_data() + + def _transform(self) -> None: + """No-op by design. + + Geography is not joined into either payload: the historical artifact + attaches it at build time (``contract/historical.py``) and the forecast ships + it as the §5 GAUL sidecar, built in the sink at ``_save``. The hook stays + so the Template Method's phases remain truthful. + """ + return + + def _validate(self) -> None: + """Assert the read produced what the save needs, then check coverage. + + Neither payload is null-gated here. The historical artifact's metadata + null-gate fires at build time (``historical.assert_metadata_complete``); + the forecast's guarantees are the wire's own verified chain — content + hashes, header/payload asserts, and per-target coverage inside each + lease's ``load()``, plus the §6 no-collapse gate and gid parity at + ``_save``. This phase asserts the RESOLUTION happened, keeping the + Template Method's phases truthful: ``_read`` resolves, ``_save`` + materializes. + """ + if self._historical_frame is None: + raise ValueError("no historical frame — _read did not run.") + if self._forecast_resolution is None: + raise ValueError("no resolved forecast run — _read did not resolve.") + logger.info( + "Historical is frame-native: the metadata null-gate is enforced at " + "artifact build (historical.assert_metadata_complete)." + ) + self._check_coverage() + + def _check_coverage(self) -> None: + """Log delivered cell counts and enforce the region coverage contract (S1/C-34). + + Orchestration only: extract primitives via ``frame_extraction``, then call + the representation-free ``delivery.coverage`` invariant — the rule is + *called*, not embedded. The count-gate fires only for regions pinned in + ``coverage.EXPECTED_CELLS``; an unpinned/unresolved region logs a skipped gate + rather than guessing. + + Only the historical leg is checked here. Forecast coverage is asserted + inside each lease's ``load()``, where the frame actually exists — see + ``wire/source_selection``. + """ + region = self.configs.get("region") + expected = coverage.expected_for(region) + excluded = coverage.excluded_for(region) + for label, cells, n_rows in [self._historical_coverage_source()]: + logger.info( + "%s delivery coverage: %d distinct cells, %d rows.", + label, + len(cells), + n_rows, + ) + # GAUL-uncovered cells the curated region must drop (S4/C-30) — checked + # before the count gate so a leaked island names itself, not "over-coverage + # by 1". Empty for unpinned regions (e.g. africa_me_legacy keeps its ocean + # cells), so this is a no-op there. + if excluded: + coverage.assert_no_excluded_cells(cells, excluded, label=label) + if expected is not None: + coverage.assert_complete_coverage(cells, expected, label=label) + else: + logger.warning( + "Coverage count-gate skipped for %s: region %r is not pinned in " + "delivery.coverage.EXPECTED_CELLS — verify and pin before relying on it.", + label, + region, + ) + + def _save_contract(self) -> dict: + """ADR-013 contract outbound (epic #105): the composed sink delivers the run. + + The §11.4 interlock is enforced by ``wire.sink`` itself: with the default + ``product.UPLOAD_ENABLED=False`` (overridable only by the explicit + ``wire_upload_enabled`` launch-config key), artifacts are staged locally and + ZERO store calls occur. First live enablement is gated on C-161 closure. + """ + if self._forecast_resolution is None: + raise ValueError( + "contract _save called without a resolved run — _read must run first." + ) + # ONE read of the 888 KB lookup per delivery (#152/C-66), threaded to both + # consumers — each takes it as a parameter (DIP), so neither reaches for the + # file itself. + lookup = gaul_lookup.load() + upload_enabled = bool(self.configs.get("wire_upload_enabled", product.UPLOAD_ENABLED)) + store = _ContractStorePort(self._crafd_datastore()) if upload_enabled else None + # The wire is partner-neutral (#153): the manager supplies CRAF'd's product + # facts explicitly rather than the mechanism reaching for them. + summary = wire_sink.deliver_run( + self._forecast_resolution, + lookup=lookup, + staging_dir=Path(self._model_path.data_generated) / "wire_contract", + consumer_name=product.CONSUMER_DOCUMENT_NAME, + s_min=product.S_MIN, + store=store, + upload_enabled=upload_enabled, + ) + # Historical leg (#126): the CRAF'd product ships actuals alongside the wire — + # frame-built, same artifact shape the FAO delivery already ships, same interlock. + if self._historical_frame is None: + raise ValueError( + "contract _save: no historical frame — the un_crafd descriptor must " + "declare data_format: feature_frame (#126)." + ) + hist_path, hist_description, _ = self._build_historical_artifact( + Path(summary["staging_dir"]), lookup + ) + if upload_enabled: + store.upload( + hist_path, + filename=hist_path.name, + name=self._model_path.model_name, + doc_type="model", + category="historical", + loa="pgm", + targets=list(self.configs.get("targets", [])), + description=hist_description, + ) + logger.info("uploaded %s (historical, run %s)", hist_path.name, summary["run_id"]) + else: + logger.info( + "Interlock holding: historical artifact staged at %s (no store calls).", + hist_path, + ) + summary["historical"] = hist_path.name + return summary + + def _crafd_datastore(self) -> DatastoreModule: + """The CRAF'd-facing store (`crafd_bucket`).""" + return DatastoreModule(appwrite_file_manager_config=self._crafd_appwrite_config()) + + def _crafd_appwrite_config(self) -> AppwriteConfig: + appwrite_env.assert_env_declared( + appwrite_env.CONNECTION_ENV + appwrite_env.CRAFD_ENV, store="crafd_bucket datastore" + ) + return AppwriteConfig( + path_manager=self._model_path, + endpoint=os.getenv("APPWRITE_ENDPOINT"), + project_id=os.getenv("APPWRITE_DATASTORE_PROJECT_ID"), + credentials=os.getenv("APPWRITE_DATASTORE_API_KEY"), + auth_method="api_key", + cache_ttl_hours=24, + bucket_id=os.getenv("APPWRITE_CRAFD_BUCKET_ID"), + bucket_name=os.getenv("APPWRITE_CRAFD_BUCKET_NAME"), + collection_id=os.getenv("APPWRITE_CRAFD_COLLECTION_ID"), + collection_name=os.getenv("APPWRITE_CRAFD_COLLECTION_NAME"), + database_id=os.getenv("APPWRITE_METADATA_DATABASE_ID"), + database_name=os.getenv("APPWRITE_METADATA_DATABASE_NAME"), + ) + + def _save(self) -> dict: + """Deliver the run — ADR-013 contract only (#149).""" + return self._save_contract() + + def _historical_coverage_source(self): + """(label, cells, n_rows) for the historical frame.""" + return ( + "historical", + frame_extraction.cells_of(self._historical_frame), + self._historical_frame.n_rows, + ) + + def _historical_frame_description(self, table, timestamp: str) -> str: + """The C-15 provenance description for the frame-built historical artifact.""" + region = self.configs.get("region") + prov = provenance.build_provenance( + lookup_version=gaul_lookup.version(), + region=region, + expected_cell_count=coverage.expected_for(region), + actual_cell_count=len(frame_extraction.cells_of(self._historical_frame)), + unmapped_count=historical.unmapped_cell_count(table), + ) + return provenance.compact_description(prov) + + def _build_historical_artifact(self, directory, lookup) -> tuple: + """Frame-built historical artifact staged into ``directory``; returns + (path, description, timestamp). Null-gate enforced here (fail loud). + + ``lookup`` is the already-read GAUL table (injected, not fetched — #152). + """ + timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") + table = historical.build_historical_table(self._historical_frame, lookup) + historical.assert_metadata_complete(table) + path = Path(directory) / f"historical_dataset_{timestamp}.parquet" + historical.write_historical_artifact(table, path) + return path, self._historical_frame_description(table, timestamp), timestamp diff --git a/views_postprocessing/crafd/product.py b/views_postprocessing/crafd/product.py new file mode 100644 index 0000000..b378a9a --- /dev/null +++ b/views_postprocessing/crafd/product.py @@ -0,0 +1,42 @@ +"""The declared CRAF'd delivery product (ADR-013 §4.2a's "views-postprocessing +configuration"). Clone of ``unfao/product.py`` for the second external partner — +the Complex Risk Analytics Fund (CRAF'd), served by views-crafdapi. + +Everything here is a **declaration**, never an inference: the delivery refuses to +ship until reality matches these constants, and changing the product is a human +decision plus an edit here — reviewed, git-historied, fail-loud. One reason to +change: the CRAF'd partner relationship. + +CRAF'd is FAO *extended* (same VIEWS forecasts, same PRIO-GRID geography, same +cadence): for now the same three conflict-fatality series, delivered to CRAF'd's +own bucket. The uncertainty *surface* CRAF'd adds — exceedance probabilities +alongside HDI/MAP — lives in the consumer (views-crafdapi ADR-034), not here; the +producer ships the same posterior-sample wire the FAO producer ships. Additional +targets, when CRAF'd names them, are an Amendment A1 edit to ``TARGETS`` below. + +The constants and their contract homes: + +- ``TARGETS`` — the expected target set (§4.2a): a run is *complete* only when every + target listed here has a manifested Hop-A leg. Adding a target follows Amendment + A1 (§7a): maintainer names it, producer's mapping gains an entry, this tuple + gains an entry. Wire vocabulary only — never internal model names. +- ``S_MIN`` — the §6 no-collapse floor passed to ``delivery.draws``. +- ``CONSUMER_DOCUMENT_NAME`` — the §4.1a store-document ``name`` pin. views-crafdapi's + query layer filters on it unconditionally; a document under any other name is + invisible to the consumer. Config-owned by views-crafdapi (ADR-034 §6); changing + it is a contract amendment. +- ``UPLOAD_ENABLED`` — the §11.4 upload interlock: ``False`` means the sink writes + artifacts locally and never calls the store. Overriding requires an explicit + launch-config declaration, and the first live enablement is gated on the + views-crafdapi consumer's selection guard being deployed in production. +""" + +from __future__ import annotations + +TARGETS: tuple[str, ...] = ("lr_ged_sb", "lr_ged_ns", "lr_ged_os") + +S_MIN: int = 2 + +CONSUMER_DOCUMENT_NAME: str = "un_crafd" + +UPLOAD_ENABLED: bool = False diff --git a/views_postprocessing/unfao/managers/README.md b/views_postprocessing/unfao/managers/README.md index abbd524..b3aa405 100644 --- a/views_postprocessing/unfao/managers/README.md +++ b/views_postprocessing/unfao/managers/README.md @@ -17,13 +17,16 @@ and uploads the result to the FAO Appwrite store. rules live in `views_postprocessing/delivery/` and are **called** by the manager (via the `contract/frame_extraction.py` seam), never inherited into it. -It is **406 lines**, against a 450-line budget enforced by +It sits just under a **450-line budget** — applied to this whole directory, not to this file alone — enforced by `tests/test_doc_accuracy.py::test_the_manager_stays_within_its_line_budget`. It was 636 before #149. ADR-012 deliberately stopped calling it "thin" and states a number instead — a word nobody can check became a bound a test can. -It is also **the only module in this repository that imports `views_pipeline_core`**, and a -test keeps it that way (register C-40). +It is also one of **only two modules in this repository that import +`views_pipeline_core`** — this one and its CRAF'd counterpart, `crafd/managers/crafd.py`, +added in #211. A test holds that to an explicit allowlist, so a *third* importer fails +CI (register C-40). The two files are near-identical by design; register **C-33** names +what would make it time to stop copying. It does **not** transform prediction values (no collapse, no reconciliation — those are downstream). It joins metadata, guards integrity, and delivers. diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index 11782d2..bf4e91c 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -51,9 +51,13 @@ def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, targets=targets, description=description, ) - # The store module degrades gracefully (its ADR-046 policy) and only LOGS - # metadata failures — which strands an invisible orphan file (run-0 - # historical, 2026-07-27). The delivery fails loud instead. + # 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. + # (Until 2026-08-03 this comment said the store "only LOGS": false, and + # self-defeating — if it only logged, `success` would be True.) success = getattr(result, "success", None) if success is None and hasattr(result, "to_dict"): success = result.to_dict().get("success") @@ -219,7 +223,7 @@ def _transform(self) -> None: """No-op by design. Geography is not joined into either payload: the historical artifact - attaches it at build time (``unfao/historical.py``) and the forecast ships + attaches it at build time (``contract/historical.py``) and the forecast ships it as the §5 GAUL sidecar, built in the sink at ``_save``. The hook stays so the Template Method's phases remain truthful. """