From 0d31e182c345f43ec155f73fbfd199b257908f5b Mon Sep 17 00:00:00 2001 From: Polichinl Date: Sun, 30 Aug 2026 17:01:55 +0200 Subject: [PATCH] docs: the orientation documents froze on 2026-08-04, and CRAF'd had no contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /review-base-docs found the mechanical layer healthy — validate_docs.sh passes, every doc a test can reach is accurate — and the human layer stale in exactly the places nothing reads. Five findings, four fixed here. **The CRAF'd manager had an intent contract in no form at all.** No CIC file and no class docstring, while its line-for-line twin has a 189-line contract. ADR-006 mandates one for orchestration and state-owning classes; the CIC README names both categories. The partner package was cloned and the contract was not. It is written as a DELTA, deliberately. Two 189-line documents describing one behaviour is not redundancy, it is two things that can disagree — which is C-75 verbatim, where this directory's files asserted opposite things about the same call for weeks. The new file states purpose, what it inherits, and a table of what differs. If the managers ever diverge behaviourally, C-33's extraction trigger has fired and the form should be revisited with the code. Registering it in _CIC_SUBJECT made the collaborator guard fire immediately: the document named the sibling CLASS in backticks, which crafd.py never references. The guard was right and the backticks were wrong — a markdown link to a sibling DOCUMENT is not a claim that the class reaches for a collaborator. Fixed by saying what was meant, not by exempting the file. **Both orientation documents omitted delivery/findability.py**, which shipped 2026-08-18. README and role_and_seams.md — the two files a new contributor opens first — both last touched 2026-08-04. Now listed in both, and a new guard asserts delivery/ stays complete in both. Mutation-proven: deleting the line reddens it. Scoped to delivery/ because it is the one directory both docs enumerate exhaustively; firing on the README's deliberate contract/wire/ abbreviation would be crying wolf (ADR-014 §3). **The sibling CIC was four days stale** — reviewed 2026-08-21, class changed 2026-08-25. It covered findability and TornRunError but not observed_through, the #297 boundary stamp. That gap was mine. Recorded, review date moved. **CLONING.md's "sixteen lines differ" is now fourteen** — the third time that number went stale, in the paragraph that warns it will. Removed rather than corrected; the diff command beside it is the source of truth and a number next to it is a liability. Two audit findings NOT fixed, and one of them was my error: - **ADR-004 does have trigger conditions.** The audit reported none; it has five. What it does not have is an owner. Reading them produced a better finding: at least three have FIRED — downstream systems depend on this repo, breaking changes now incur real coordination cost (views-models#403 took eight days; #439 is open), and ADR-013 §10 pins bytes across time. Recorded on the ADR with evidence. Writing the successor is an architectural decision, not a docs fix. - **pyproject's python range stays wrong.** Narrowing >=3.11,<3.15 to <3.12 is one line and correct, but it invalidates poetry.lock and forces a full re-resolve that would move pyarrow off the 16.1.0 the ADR-013 §10 fixtures pin (C-72). Attempted, measured, reverted. #295 owns it; the README now states the truth, which was the safe half. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 36 +++++- .../004_rules_for_evolution_and_stability.md | 20 ++++ docs/CICs/CRAFDPostProcessorManager.md | 103 ++++++++++++++++++ docs/CICs/UNFAOPostProcessorManager.md | 4 +- docs/CLONING.md | 4 +- docs/architecture/role_and_seams.md | 5 +- tests/test_doc_accuracy.py | 31 ++++++ 7 files changed, 194 insertions(+), 9 deletions(-) create mode 100644 docs/CICs/CRAFDPostProcessorManager.md diff --git a/README.md b/README.md index 361fbd4..2413100 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # views-postprocessing -[![Python 3.11+](https://img.shields.io/badge/python-3.11+-blue.svg)](https://www.python.org/downloads/) +[![Python 3.11](https://img.shields.io/badge/python-3.11-blue.svg)](https://www.python.org/downloads/) [![Poetry](https://img.shields.io/badge/dependency%20management-poetry-blueviolet)](https://python-poetry.org/) [![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](https://opensource.org/licenses/MIT) @@ -43,7 +43,18 @@ poetry install pip install views-postprocessing ``` -Requires **Python 3.11–3.14**. +Requires **Python 3.11**, and only 3.11. + +`pyproject.toml` still declares `>=3.11,<3.15`. **That declaration is wrong** and is +tracked as **#295**: the lockfile resolves on 3.11 alone, because `ingester3` caps +`levenshtein >=0.20,<0.21` and no release in that range publishes a 3.12+ wheel. +Measured 2026-08-25 against the index. CI and the delivery's own environment both run +3.11, so nothing in production is affected — the cost is that a contributor arriving on +3.12 or 3.13 is told the project supports them and then cannot install it. + +Narrowing the declaration is a one-line edit that invalidates `poetry.lock` and forces a +full re-resolve, which would move `pyarrow` off the 16.1.0 the ADR-013 §10 byte-parity +fixtures are pinned to (C-72). So it is #295's own change, not a documentation fix. ### Dependencies @@ -89,6 +100,21 @@ in #149; their rules survive as called invariants under `delivery/`. See the affected, confirm the fault offline, and supersede on the wire. The contract has no retraction primitive; a correction is a new complete run, manifest last. +### If a delivery fails loudly + +Since **1.2.0** a run can stop in ways it previously would not, and each one replaces a +silent failure with a refusal. A launcher may see: + +| exception | what it means | +|---|---| +| `DeliveryNotFindableError` | the upload succeeded but the consumer's own query does not find **this** run — the failure where every call reports success and the partner sees nothing | +| `FindabilityUnverifiedError` | the check above could not run; *"could not ask"* is deliberately not *"asked and got nothing"* | +| `ProducerClientUnavailable` | the producer client would not load, so the observed-data boundary is unknown. Refuses rather than shipping the unobserved tail as observed history | +| `TornRunError` | a run failed partway through uploading. Names every object confirmed uploaded and the one that failed; deletes nothing | + +If one fires after an upgrade it is reporting a condition that was already wrong and +already invisible. [`CHANGELOG.md`](CHANGELOG.md) carries the detail. + ### Output schema (geographic metadata columns) These 9 columns are the delivered geography contract, declared in @@ -132,7 +158,8 @@ views-postprocessing/ │ ├── draws.py # the §6 no-collapse gate │ ├── parity.py # sidecar covers exactly the forecast's cells │ ├── observed_range.py # fabricated-month decision - │ └── provenance.py # structured upload provenance + │ ├── provenance.py # structured upload provenance + │ └── findability.py # does the consumer's query find THIS run? ├── contract/ # HOW A DELIVERY IS BUILT — partner-neutral │ ├── wire/ # the ADR-013 contract (header, shard, sidecar, │ │ # run_manifest, sink, source_selection, naming) @@ -219,7 +246,8 @@ only `.py`; it now scans markdown too.)* |-----|----------------| | [`docs/architecture/role_and_seams.md`](docs/architecture/role_and_seams.md) | **Start here** — role vs the sibling repos + internal seams | | [`docs/ADRs/`](docs/ADRs/) | Architecture decisions (esp. ADR-011 mapper→lookup; ADR-012 ontology) | -| [`docs/CICs/`](docs/CICs/) | Class intent contracts (`UNFAOPostProcessorManager`) | +| [`docs/CICs/`](docs/CICs/) | Class intent contracts — one per partner manager; the CRAF'd one is stated as a delta against the UN-FAO one | +| [`CHANGELOG.md`](CHANGELOG.md) | **What changed for a consumer**, per release — behaviour a launcher can observe, failure modes first | | `reports/technical_risk_register.md` | Tracked risks — C-40 (the remaining pipeline-core inheritance), C-30/C-15 (delivery guards), C-43 (enrichment value verification) | --- diff --git a/docs/ADRs/004_rules_for_evolution_and_stability.md b/docs/ADRs/004_rules_for_evolution_and_stability.md index 942f247..f13f820 100644 --- a/docs/ADRs/004_rules_for_evolution_and_stability.md +++ b/docs/ADRs/004_rules_for_evolution_and_stability.md @@ -66,6 +66,26 @@ This ADR should be revisited when one or more of the following become true: At that point, a new ADR should supersede this one. +> **Audit note, 2026-08-26: at least three of those conditions are now true, and nothing +> noticed.** Recorded here rather than acted on, because writing the successor is an +> architectural decision and this was a documentation audit. +> +> - *"External users or downstream systems depend on this repository"* — views-models pins +> it in two launchers, views-faoapi and views-crafdapi consume its artifacts, and it has +> been published on PyPI since 1.0.0. +> - *"Breaking changes begin to incur real coordination or migration costs"* — the 1.1.0 → +> 1.1.1 pin move took eight days and a direct request (views-models#403); 1.2.0's is open +> as views-models#439. Register **C-111** and **C-112** exist for the two halves of that +> gap: consumers are told nothing but a version number, and nothing here can see what +> production actually runs. +> - *"Reproducibility across time becomes a contractual requirement"* — ADR-013 §10 pins +> delivery bytes against a golden fixture, and C-72 records what a toolchain bump costs. +> +> The deferral itself was sound and remains honest; what has changed is the world it was +> deferring against. **Whoever picks this up should write the successor rather than amend +> this document** — that is what its own last line asks for. The owner field above is still +> `—`, which is the part ADR-014 §4 would actually object to. + --- ## Non-Decisions (Explicitly Out of Scope for Now) diff --git a/docs/CICs/CRAFDPostProcessorManager.md b/docs/CICs/CRAFDPostProcessorManager.md new file mode 100644 index 0000000..1cba825 --- /dev/null +++ b/docs/CICs/CRAFDPostProcessorManager.md @@ -0,0 +1,103 @@ +# Class Intent Contract: CRAFDPostProcessorManager + +**Status:** Active +**Owner:** PRIO MD&D Team +**Last reviewed:** 2026-08-26 +**Related ADRs:** ADR-002, ADR-006, ADR-008, ADR-009, ADR-013 +**Sibling contract:** [UNFAOPostProcessorManager.md](UNFAOPostProcessorManager.md) + +--- + +> **This contract is stated as a delta, deliberately.** `CRAFDPostProcessorManager` is a +> clone of the UN-FAO manager (`docs/CLONING.md`, register **C-33**), and the two +> managers differ by fourteen lines on each side, none of which changes behaviour. Verify +> that rather than trusting this sentence: +> +> ``` +> diff views_postprocessing/unfao/managers/unfao.py \ +> views_postprocessing/crafd/managers/crafd.py +> ``` +> +> A second full contract was considered and rejected. Two 189-line documents describing +> one behaviour is not redundancy, it is **two things that can disagree** — and this +> repository has already paid for exactly that: register **C-75**, where the sibling CIC +> and this directory's other file asserted opposite things about the same call for weeks. +> One contract, one delta. If the managers ever diverge behaviourally, C-33's extraction +> trigger has fired and this document's form should be revisited along with the code. + +## 1. Purpose + +`CRAFDPostProcessorManager` orchestrates the end-to-end postprocessing pipeline that +delivers VIEWS conflict predictions to the **Complex Risk Analytics Fund (CRAF'd)**, +served by views-crafdapi. + +It is the single entrypoint for producing and delivering CRAF'd-formatted prediction data. + +## 2. What this contract inherits + +**Sections 2–11 of [UNFAOPostProcessorManager.md](UNFAOPostProcessorManager.md) apply +to this class unchanged**, substituting the partner identity below. That includes the +non-goals, the four-stage pipeline guarantee, the inputs and assumptions, the failure +modes and their loudness, the boundaries (ADR-002 topology: `crafd/` → `contract/` → +`delivery/`, one way only), and the test-alignment position. + +Two of those are worth naming here because they are the ones a reader most often assumes +differ, and they do not: + +- **The wire is partner-neutral** (#153). CRAF'd receives the same ADR-013 artifacts + built by the same `contract/` code. Only the *product* differs. +- **This class is one of the repository's only two importers of `views_pipeline_core`**, + mechanically pinned to an allowlist by `tests/test_doc_accuracy.py`. That is what keeps + C-40's blast radius at one file per partner. + +## 3. What differs — the whole of it + +| | `unfao` | `crafd` | +|---|---|---| +| consumer document `name` | `un_fao` | `un_crafd` | +| partner store | `unfao_bucket` | `crafd_bucket` | +| env tuple validated | `appwrite_env.UNFAO_ENV` | `appwrite_env.CRAFD_ENV` | +| consumer repository | views-faoapi | views-crafdapi (its ADR-034) | +| §11.4 interlock history | precondition met 2026-07-20 (faoapi C-161) | precondition met 2026-08-12 (views-crafdapi#53) | + +`TARGETS` and `S_MIN` are **the same values today** (`lr_ged_sb`, `lr_ged_ns`, +`lr_ged_os`; `S_MIN = 2`) but are independently declared per partner and may diverge +without either being wrong — CRAF'd naming an additional target is an Amendment A1 edit +to `crafd/product.py` alone. + +**Both partners are gated closed.** `product.UPLOAD_ENABLED` is `False` in each, and the +gate that holds is the same one for both: **views-appwrite#171**, the non-production +Appwrite project decision, without which the upload path cannot be rehearsed against a +real store (#18). The partner-specific preconditions in the table above are satisfied and +no longer gate anything. + +## 4. The uncertainty surface is the consumer's, not this class's + +CRAF'd is FAO *extended*: same forecasts, same PRIO-GRID geography, same cadence. The +additional surface CRAF'd wants — exceedance probabilities alongside HDI/MAP — is +computed in **views-crafdapi** (their ADR-034), not here. This producer ships the same +posterior-sample wire the FAO producer ships. + +This is a **non-goal** and belongs in a contract because the alternative is attractive and +wrong: a manager that starts summarising draws for one partner has taken a consumer +concern into the delivery, and the §6 no-collapse gate exists to prevent exactly that. + +## 5. Test alignment + +Covered by the same source-scan and seam tests as its sibling, parametrised over both +partners via `PARTNER_PACKAGES` in `tests/conftest.py` — including +`tests/test_clone_readiness.py`, which asserts the two partner packages are **independent** +(neither imports the other), and the line budgets in `tests/test_doc_accuracy.py`. + +**A note on the budget, current at this review:** `crafd/` sits at **699 of 700** lines. +The next line added anywhere in that package fails the guard, which is what the guard is +for. It is not a defect and is not scheduled; it is recorded so the next contributor meets +it here rather than in a red build. + +## End of Contract + +This document defines the **intended meaning** of `CRAFDPostProcessorManager`. + +Changes to behavior that violate this intent are bugs. +Changes to intent must update this contract — and, where the intent is inherited, its +sibling. diff --git a/docs/CICs/UNFAOPostProcessorManager.md b/docs/CICs/UNFAOPostProcessorManager.md index 1e1e242..84e715a 100644 --- a/docs/CICs/UNFAOPostProcessorManager.md +++ b/docs/CICs/UNFAOPostProcessorManager.md @@ -3,7 +3,7 @@ **Status:** Active **Owner:** PRIO MD&D Team -**Last reviewed:** 2026-08-19 +**Last reviewed:** 2026-08-26 **Related ADRs:** ADR-001, ADR-002, ADR-008, ADR-009 --- @@ -95,7 +95,7 @@ Assumptions that are not met **must cause failure**, not fallback behavior. The - **Launch config incomplete:** raises `LaunchConfigError` naming the missing key. A launcher that omits `wire_contract` or declares a `data_format` other than `feature_frame` is **refused**, never quietly routed into a fallback (ADR-003, register C-63) - **Region coverage mismatch:** Raises `CoverageError` in `_check_coverage()` (called from `_validate()`) if a pinned region's delivered cell count is wrong (S1/C-34) or a GAUL-uncovered excluded cell leaks into the delivery (S4/C-30) - **Fabricated historical tail:** `_read_historical_frame()` drops months beyond the producer's `last_valid_month_id` at the read (`_clip_observed_history` was the pandas equivalent, retired with that path in #149) so unobserved zero-padding is not shipped as observed history (S2/C-26). Two outcomes when the boundary is unavailable, and they are different on purpose (C-103, 2026-08-17): if the producer simply publishes no boundary — or the read fails — it **degrades open**, skipping the clip with a WARNING that states the unobserved tail will ship; if the producer client cannot be imported at all it **refuses** (`source_metadata.ProducerClientUnavailable`), because a broken environment is not a producer fact -- **Upload provenance:** the historical artifact's `description` carries structured provenance (lookup version, region, expected/actual cell counts, unmapped count) built by `delivery/provenance.py` (`build_provenance` → `compact_description`) via the manager's `_historical_frame_description()` (S5/C-15). The **forecast** side carries no such description: its guarantee is the wire's verified chain — per-shard content hashes recorded in the §4.2 run manifest, header asserts on load, and manifest-last commit ordering. That is identity and integrity, not the C-15 provenance field set; the §4.2 manifest's keys are exactly `contract_version`, `run_id`, `targets`, `shards`, `expected_months`, `expected_cell_count`, `sidecar` — and it carries **no** `lookup_version`, `region` or `unmapped_count`. `_delivery_description()` was the pandas-path equivalent and was deleted with it in #149 +- **Upload provenance:** the historical artifact's `description` carries structured provenance (lookup version, **the observed-range boundary this run clipped against**, region, expected/actual cell counts, unmapped count) built by `delivery/provenance.py` (`build_provenance` → `compact_description`) via the manager's `_historical_frame_description()` (S5/C-15). The **forecast** side carries no such description: its guarantee is the wire's verified chain — per-shard content hashes recorded in the §4.2 run manifest, header asserts on load, and manifest-last commit ordering. That is identity and integrity, not the C-15 provenance field set; the §4.2 manifest's keys are exactly `contract_version`, `run_id`, `targets`, `shards`, `expected_months`, `expected_cell_count`, `sidecar` — and it carries **no** `lookup_version`, `region` or `unmapped_count`. The `observed_through` field is **required and always emitted** (#297, 2026-08-25): an integer when the clip ran, and an explicit `null` when the producer boundary could not be read and the clip was therefore *skipped*, which is the case that most needs recording. A third state — never read — refuses at build time rather than collapsing to `null`, because reporting "clip skipped" for a run whose clip ran is the C-103 conflation one layer down. Added because #297 took a day to answer a question the artifact should have carried: *observed through when, and decided against what?* `_delivery_description()` was the pandas-path equivalent and was deleted with it in #149 The following **must never** fail silently: - Missing or None environment variables for Appwrite diff --git a/docs/CLONING.md b/docs/CLONING.md index 79844a7..11fe1e7 100644 --- a/docs/CLONING.md +++ b/docs/CLONING.md @@ -74,7 +74,9 @@ 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 differing lines **change no behaviour**, and the count is deliberately not +repeated here — it was sixteen when this paragraph was written and is not now. What +differs is: 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. diff --git a/docs/architecture/role_and_seams.md b/docs/architecture/role_and_seams.md index 9af8da0..20624e3 100644 --- a/docs/architecture/role_and_seams.md +++ b/docs/architecture/role_and_seams.md @@ -106,7 +106,7 @@ The input-integrity guards are split into **two homes** on purpose: - `views_postprocessing/delivery/` — **representation-free invariants**. Primitives only (sets of ints, numpy arrays, scalars, dicts). **No pandas, no views_frames.** Each is a pure rule that raises or passes: `coverage.py`, `draws.py`, `parity.py`, - `observed_range.py`, `provenance.py`. + `observed_range.py`, `provenance.py`, `findability.py`. - `views_postprocessing/contract/frame_extraction.py` — **the representation seam**. It turns a `views_frames` frame into the primitives the invariants consume. @@ -171,7 +171,8 @@ views_postprocessing/ │ ├── draws.py the §6 no-collapse gate │ ├── parity.py sidecar covers exactly the forecast's cells │ ├── observed_range.py fabricated-month decision -│ └── provenance.py structured upload provenance +│ ├── provenance.py structured upload provenance +│ └── findability.py does the consumer's own query find THIS run? (C-94) ├── contract/ HOW A DELIVERY IS BUILT — partner-neutral, reusable by a clone │ ├── wire/ the ADR-013 contract (header, shard, sidecar, run_manifest, │ │ sink, source_selection, naming) diff --git a/tests/test_doc_accuracy.py b/tests/test_doc_accuracy.py index 7f9eeea..fea905c 100644 --- a/tests/test_doc_accuracy.py +++ b/tests/test_doc_accuracy.py @@ -140,6 +140,7 @@ def _governance_docs() -> list[Path]: #: `unfao/managers/unfao.py`, and no rule turns one into the other. _CIC_SUBJECT = { "UNFAOPostProcessorManager.md": ("unfao", "managers", "unfao.py"), + "CRAFDPostProcessorManager.md": ("crafd", "managers", "crafd.py"), } @@ -950,3 +951,33 @@ def kinds(src: str) -> list[str]: assert kinds( "from typing import TYPE_CHECKING\nif TYPE_CHECKING:\n import pandas\n" ) == ["type-only"], "a legitimate type-only import was flagged — the guard cries wolf" + + +@pytest.mark.parametrize( + "doc", ["README.md", "docs/architecture/role_and_seams.md"] +) +def test_the_orientation_docs_list_every_delivery_invariant(doc): + """Both orientation documents present `delivery/` as a complete list. Nothing checked + that it was. + + `delivery/findability.py` shipped on 2026-08-18 and was absent from both files until + 2026-08-26 — the README frozen since 2026-08-04, `role_and_seams.md` the same day. + These are the two files a new contributor opens first, and a *complete* list missing + an entry is worse than no list: it is read as exhaustive. + + Scoped to `delivery/` on purpose. It is the one directory both documents enumerate + exhaustively — the README collapses `contract/wire/` to a parenthetical, and a guard + that fired on that would be crying wolf about a deliberate abbreviation (ADR-014 §3). + """ + modules = { + f.stem for f in (_PKG / "delivery").glob("*.py") if f.stem != "__init__" + } + assert modules, "no delivery modules found — this guard is scanning the wrong place" + + text = (_REPO / doc).read_text() + missing = sorted(m for m in modules if f"{m}.py" not in text) + assert not missing, ( + f"{doc} presents delivery/ as a complete list but omits {missing}. Either add " + "them or stop presenting the list as exhaustive — findability.py was absent from " + "both orientation documents for eight days after it shipped." + )