diff --git a/.github/workflows/run_pytest.yml b/.github/workflows/run_pytest.yml index f259bf3..a782614 100644 --- a/.github/workflows/run_pytest.yml +++ b/.github/workflows/run_pytest.yml @@ -18,6 +18,14 @@ jobs: steps: - name: Checkout repository uses: actions/checkout@v3 + with: + # `fetch-depth: 0` for the TAGS, not the history. The default shallow checkout + # fetches none, so `git tag -l` is empty and BOTH guards in + # tests/test_release_version.py skip — the pair written because tag 1.1.0 was cut + # while pyproject.toml still said 1.0.0. A guard against mis-cutting a release + # that only ever runs on the maintainer's laptop is half a guard, and the half + # that is missing is the one watching the moment it matters. + fetch-depth: 0 # ── sibling checkouts: what CI can verify that a bare checkout cannot ──────── # diff --git a/pyproject.toml b/pyproject.toml index 043be9d..f42e9a5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "views-postprocessing" -version = "1.1.0" +version = "1.1.1" description = "" authors = [ "Dylan Pinheiro ", diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 640e686..f90ef8d 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-12 | -| Total Concerns | 98 | -| Open Concerns | 21 | -| Resolved Concerns | 77 | +| Last Updated | 2026-08-15 | +| Total Concerns | 102 | +| Open Concerns | 22 | +| Resolved Concerns | 80 | --- @@ -35,7 +35,7 @@ covered a single open entry (see Historical clusters below). **Update 2026-08-03 — the cluster halved at the 3.0.0 bump.** Six of its ten entries closed with the pin: the inherited surface stopped being a liability for timeouts (C-13), silent provisioning (C-58), the transitive drag (C-62) and the undeclared SDK (C-07). What remains is the root — the double inheritance itself — and the three genuinely upstream-owned data concerns (C-26, C-27, C-28). The cluster's thesis held: fixing the surface upstream fixed them here with a pin and no code. **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. +**Fix strategy:** the thin-shell de-inheritance C-40 prescribes — and which is **half-built**: the sink side landed (`_ContractStorePort`, moved to `/store_port.py` 2026-08-14 by C-99) 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. ### Cluster H: Go-global verification debt — discharged unevenly by run-0 @@ -183,6 +183,26 @@ This is the same shape as vpp_017 §7a, arrived at from the other side: a check --- +### C-100: The "four-method port" has three used methods and a dead third module behind it + +| Field | Value | +|-------|-------| +| ID | C-100 | +| Tier | 4 — no correctness impact; the code is unreachable, not wrong. Registered because deleting it is a decision (the second store, #97) rather than a cleanup, and because an unreachable method inside a seam four documents describe is the kind of thing that gets maintained forever by accident. | +| Source | Reading the whole port while fixing C-99, 2026-08-14 | +| Trigger | The second partner store (#97) is scoped, or anyone proposes deleting `contract/store_metadata.py` — at which point this entry says what it costs and what moves with it. | +| Location | `views_postprocessing/{unfao,crafd}/store_port.py` (`file_metadata`); `views_postprocessing/contract/store_metadata.py` | + +`_ContractStorePort.file_metadata` has **no caller in the package**. Measured: `latest_file_id` is called three times and `download` three times, both in `contract/wire/source_selection.py`; `upload` three times — `contract/wire/sink.py:164` plus each partner's historical artifact at `managers/.py:325`; `file_metadata` is called by nothing. Its only body is a call to `contract/store_metadata.py:file_metadata`, whose own module docstring says *"the one caller is `_ContractStorePort.file_metadata`"* — true, and the chain terminates there. The module has tests (`tests/test_store_metadata.py`) and no production reader. + +The "four methods" the docs describe (`docs/ADRs/015_the_pipeline_core_appwrite_import.md:70`, both `store_port.py:5`, `tests/test_store_port.py:20`) are not wrong — the port really does define four. What none of them says, because nobody had counted, is that three of them run and the fourth is reachable only from a test. + +**Not fixed here on purpose.** C-99's change was a correctness fix on a live delivery path; deleting a public-ish port method and a contract module in the same commit would have mixed a refusal with a removal. It is also not obviously a deletion: the second prediction store (#97) is scoped to be sample-bearing and multi-target, and reading a selected file's identity metadata is the kind of thing that partner may need. The decision is "delete it or give it a caller", and it belongs with #97 rather than with a download bug. + +Cross-refs: **C-99** (the fix that surfaced it), **C-97**, **C-33** (the same symbol exists twice by design). + +--- + ### C-97: Coordinate values sit in docstrings and comments, where the scan deliberately does not look | Field | Value | @@ -350,7 +370,13 @@ What actually keeps it open is neither of those — it is the two deferrals belo - Deferral 1's trigger is *"when #243 finishes touching `tests/test_env_declaration.py`"*. #243 finished. The leak guards are still in that file; `tests/test_redaction_guard.py` is only cross-referenced. - Deferral 2 was *"routed to #243"* — and #243 closed without it. `test_the_drift_check_would_catch_a_rename` still rebuilds its subject's comparison with its own comprehension rather than driving the checked function. -Both were unowned, which is worse than deferred — **now filed as #265**, with acceptance criteria and the reason each is not urgent. That issue closing is what closes this entry: its stated condition was already met by #243. *(An earlier draft of this amendment named the problem and left it there, which under ADR-014 §4 converts two compliant deferrals into two non-compliant items. Naming is not rehoming.)* +Both were unowned, which is worse than deferred — filed as **#265**, and **both discharged 2026-08-14**. *(An earlier draft of this amendment named the problem and left it there, which under ADR-014 §4 converts two compliant deferrals into two non-compliant items. Naming is not rehoming.)* + +**Deferral 2 is fixed as filed.** The comparison the gated check ran inline is now `_name_and_class_drift`, called by both it and the proof, so the proof drives its subject instead of a copy of it. Mutation-proven: making that function return `([], {})` now fails the proof for both partners, where before it stayed green. *(The sibling defects were repaired by driving the real check under `monkeypatch` instead; here the comparison is a pure function of two dicts that both callers want whole, so sharing it is the same guarantee with less machinery.)* It also gained an assertion that the report names *both* sides of a mismatch — what this package expects and what the registry declares — since a reader who cannot tell which moved cannot act on it. + +**Deferral 1 is answered "no", with the reason recorded rather than deferred a third time.** The coordinate-value scan **stays in `tests/test_env_declaration.py`**. Its subject is `_EXPECTED_NAMES`, derived from `_PARTNER_ENV` — the declaration of what each partner reads, which is the substance of that module. Moving a guard away from the declaration it guards, so that a filename reads better, trades a real coupling (CCP) for a filing convenience, and would require exporting a private name from one test module into another. + +What *was* misfiled moved instead: `registry_at` / `registry_current` / `rows` are the shared **reader**, not this package's environment declarations, and their five refusal tests plus `_scratch_repo` are now `tests/test_seam_registry.py`. That is the boundary that was actually wrong: **109 lines of test code moved out**, no shared private state left behind — the new module imports only from `tests/seam_registry.py`. `test_env_declaration.py` is 1406 lines against 1503 before this change — the split removed more than that and the shared comparison above put some back. **Two deferrals, both with triggers (ADR-014 §4).** @@ -818,6 +844,16 @@ 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. +**Update 2026-08-14 — the extraction trigger has now fired, twice, and this is the record of it.** + +This entry's remaining trigger reads: *"a **third** in-repo partner package, **or** the first bug that must be hand-patched identically in both manager files — whichever comes first."* The second arm has fired twice. C-79 (2026-08-05) fixed `_ContractStorePort.upload`'s fail-open result check by hand in both partners. C-99 (2026-08-14) fixed the identical fault in `download`, again by hand in both. + +**The decision is still to duplicate, and the reason has changed.** It is no longer "no second incident has shown the shape" — one has. It is that the shape the incidents showed is not the one this entry proposes extracting. C-33's mitigation is a `DeliveryProfile` carrying store identity, and neither C-79 nor C-99 was about store identity; both were a result-shape check that happens to live in a duplicated file. Extracting a `DeliveryProfile` would not have prevented either. + +**What has changed is that the duplication is now mechanically held.** `tests/test_store_port.py::test_the_two_partners_ports_have_not_drifted` fails if the two `store_port.py` files differ. Note precisely what that does *not* buy: it would not have caught C-79 or C-99, because both files stayed byte-identical throughout while carrying the defect in the untreated method. It closes the partner-vs-partner axis; the method-vs-method axis is closed by `tests/test_store_port.py` covering all four methods, which it now does for three and records the fourth as C-100. + +The `DeliveryProfile` extraction stays where D-09 put it: with the second store's scoping (#97). What is discharged here is the pretence that nobody had hit the trigger. + **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 @@ -878,7 +914,7 @@ See also C-24 (schema contract per store), C-77 (the fourth home for partner ide **Wire contract posted (2026-07-03) — the S6/#45 circular wait is dissolved.** A three-way audit (pipeline-core / producers / consumer+substrate, all on `origin/development` + maintainer-authored issues) established: (i) there are **two wire hops** (producer→store; vpp→faoapi) and the roadmap's arrow work covered only the second; (ii) **no publish path from PFE to the prediction store exists at all** — models#143's "no pipeline-core change required" is **falsified** (PFE's `use_prediction_store` is stored then only logged, `prediction_frame_ensemble.py:141/:799`; `PredictionIOManager._upload_to_prediction_store` raises `NotImplementedError`, `io.py:117`); (iii) full global draws ≈ **9.5 GB/target**, so the wire mandates per-month sharding; (iv) the "platform ADR-046" cited as the format authority **does not exist** (phantom). **ADOPTED 2026-07-15 as ADR-013** *(post-adoption: F1 invisibility confirmed live — six stranded orange_ensemble forecast docs in unfao_bucket, forecast serving has been empty all along; both §11.4 legacy guards merged same day, Hop-B guard must reach production before vpp's first contract upload — **views-faoapi C-161**)* after five reviewed iterations (two seat reviews, reconciliation, owner-ratified F1) — maintainer sign-off on views-models#149. The v1 proposal history: Hop A = Track A zip archive per (run,target,month) + manifest-last commit marker (new **pipeline-core#269**); Hop B = per-month `views_frames.io.arrow` (#91/faoapi#100); interior = per-target 2-D `PredictionFrame`; the 9 GAUL columns move to a **gid-keyed sidecar**; the **#149 no-collapse boundary is named: vpp `delivery/draws.py`** (a new invariant, sibling of coverage/identity — follow-on vpp work with the durable vpp ADR after explicit sign-off); target vocabulary **decided: `lr_ged_sb/ns/os`**, producers rename at publish (models#146). -**Update 2026-07-31 (review-rr — the prescribed DIP mitigation has half landed, uncredited).** This entry's mitigation was: "*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).*" The **sink half exists**: `_ContractStorePort` (`unfao.py:37-78`) wraps `DatastoreModule` behind a four-method port (`latest_file_id` / `file_metadata` / `download` / `upload`), and the contract delivery path drives the store through it. The **invariant half also largely exists** as pipeline-core-free modules the manager calls: `delivery/coverage.py`, `identity.py`, `draws.py`, `parity.py`, `provenance.py`, `observed_range.py` (the package docstring pins them representation-free), plus `unfao/historical.py` and `unfao/wire/`. **Residual scope of this entry is now the input side and the shell itself:** the double inheritance at `:80` (consequences a/c/d), the inherited `ViewsDataLoader`/`PGMDataset` on the legacy branch, and the fact that the FAO logic still cannot be instantiated without the framework. Tier held at 2 — the blast radius argument is unchanged for what remains. This is the root of **Cluster G**. +**Update 2026-07-31 (review-rr — the prescribed DIP mitigation has half landed, uncredited).** This entry's mitigation was: "*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).*" The **sink half exists**: `_ContractStorePort` (`/store_port.py` since 2026-08-14 — it lived at `unfao.py:37-78` when this was written) wraps the store client behind a four-method port (`latest_file_id` / `file_metadata` / `download` / `upload`), and the contract delivery path drives the store through it. The **invariant half also largely exists** as pipeline-core-free modules the manager calls: `delivery/coverage.py`, `identity.py`, `draws.py`, `parity.py`, `provenance.py`, `observed_range.py` (the package docstring pins them representation-free), plus `unfao/historical.py` and `unfao/wire/`. **Residual scope of this entry is now the input side and the shell itself:** the double inheritance at `:80` (consequences a/c/d), the inherited `ViewsDataLoader`/`PGMDataset` on the legacy branch, and the fact that the FAO logic still cannot be instantiated without the framework. Tier held at 2 — the blast radius argument is unchanged for what remains. This is the root of **Cluster G**. **Update 2026-07-31 (`repo-assimilation`, clone-readiness pass — the coupling is CONTAINED, and this file is the only clone blocker).** Two measurements that change how this entry should be read: @@ -1087,6 +1123,111 @@ See also C-40 (the inheritance/representation coupling this migration unwinds), ## Resolved Concerns +### C-102: A guard that has never run is unproven, however carefully it was written — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-102 | +| Tier | 4 — caught before it ran anywhere, so nothing was affected. Registered for the shape, which this repository keeps rediscovering. | +| Source | `/code-review medium` on PR #275, 2026-08-15 | +| Trigger | *(closed)* Any dormant check being switched on — a skipped test made live, a gate moved from advisory to required, a guard whose environment finally satisfies its precondition. | +| Location | `tests/test_release_version.py` (`test_the_newest_release_tag_is_not_ahead_of_the_declared_version`); `.github/workflows/run_pytest.yml` | + +`tests/test_release_version.py` was written on 2026-08-13 after tag `1.1.0` was cut while `pyproject.toml` still said `1.0.0`. It was correct, mutation-proven, and **had never executed anywhere except a maintainer's laptop**: CI checked out with no `fetch-depth`, so no tags were fetched and both of its tests skipped. Verified in a real CI log — run `31844578627` shows `tests/test_release_version.py ss`. + +PR #275 made it live by fetching tags. Review then found that switching it on would have **broken honest branches on its first day**: the check read `git tag -l`, which lists tags on every branch, so it asked *"has a release been cut anywhere"* rather than *"has one been cut from this line of history without the bump"*. With `1.1.1` tagged on the release line, a branch still declaring `1.1.0` — a long-lived feature branch, a hotfix cut from `1.1.0` — fails with *"the newest release tag is 1.1.1 but pyproject.toml declares 1.1.0"*, having done nothing wrong. + +Reproduced in a clone before changing anything: tags anywhere `1.0.0 1.1.0 1.1.1`, tags reachable from the honest branch `1.0.0 1.1.0`, old check fails, `git tag --merged HEAD` passes — while on the tagged line both guards still bite when `pyproject.toml` is set back. + +**The lesson is not about tags.** Mutation-proving establishes that a guard *can* fail; it says nothing about whether the guard has ever been *asked*. This one's logic was fine in the only environment it had run in, and its defect lived entirely in the environment it had never seen. ADR-014 §2 requires a guard to be mutation-proven; this entry adds that a guard which has only ever run in one environment is proven in one environment. C-98 is the same family from the other side — a guard that ran, but watched a proxy. + +**Fixed in the same change**, and the check now asks about reachable history. ADR-014 §3 was the deciding rule: a guard that reddens honest work is one someone deletes, so a false negative is the better failure. + +Cross-refs: **C-98** (a guard measuring a proxy), **C-89**, **C-90**, **C-93** (guards that could not fail), ADR-014 §2 and §3. + +--- + +### C-101: Assembling a target held three copies of it — measured, then bounded — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-101 | +| Tier | 2 — no incorrect output, but the forecast leg needed roughly three times the memory its product occupies, and the failure mode is an OOM kill mid-delivery rather than a refusal. | +| Source | views-postprocessing#269, filed from the views-crafdapi seat 2026-08-14 after the first `un_crafd` delivery attempt | +| Trigger | *(closed)* Any run large enough that three copies of one target's frame did not fit — which on 2026-08-13 meant a machine with 15 GB already in use. | +| Location | `views_postprocessing/contract/track_a_source.py` (`frames_for_target`); `views_postprocessing/contract/wire/source_selection.py` (`TargetLease.load`) | + +**Measured before anything was changed**, because the issue's own diagnosis named one cause and there turned out to be three. A synthetic run through the real `TargetLease.load` — producer-format `.tap.zip` shards, real `read_shard`, `tracemalloc` and peak RSS agreeing to within 2% — at 36 shards x 20,000 cells x 200 samples: + +| | before | after | +|---|---|---| +| peak, tracemalloc | **3.06x** the delivered frame | **1.13x** | +| peak, RSS delta | 3.02x | 1.06x | + +The 3x was **three roughly equal thirds**, and the issue named only the first: + +1. every shard's bytes, resident together — the dict comprehension completed before the first shard was decoded; +2. every decoded per-shard frame, held for the stack; +3. the `np.concatenate` result, allocated while (2) was still alive. + +The ratio held at 12 and 36 shards, so it is the shape and not the scale. Fixing only (1), as the issue proposed, would have taken 3.06x to about 2x. + +**The fix is one loop.** `frames_for_target` now takes `fetch_shard_bytes(name)` instead of a filled dict, drops each shard's bytes the moment they are decoded, and writes each shard into a manifest-sized buffer instead of stacking and concatenating. Peak is now the finished frame plus a **fixed overhead of roughly 4.5 shard-widths** — the raw shard, its decoded array, and the intermediate copies `read_shard` makes unzipping and `np.load`-ing it. Because that overhead is constant while the frame grows with the shard count, the *ratio* falls as 1/n: measured 1.36x at 12 shards, 1.19x at 24, 1.13x at 36, with the absolute overhead steady at about 70 MB throughout. *(An earlier draft of this entry called the residual `2/n_shards`; review pointed out that fits none of its own numbers — 2/12 is 0.17 against a measured 0.36. Re-measured at three shard counts to get the constant above.)* + +*What makes the buffer safe.* Its slots are sized from `expected_cell_count`, a declaration this function already enforced per shard, and the enforcement runs **before** anything is written — so a shard whose row count disagrees is refused rather than straddling two months' slots. + +*What proves the product did not change — corrected, because the first answer was wrong.* This entry originally cited `tests/test_wire_fixture.py`. **That file does not reference the assembly at all**: it round-trips static artifacts against checked-in bytes and never calls `frames_for_target` or `TargetLease.load`. The real end-to-end proof is `tests/test_hop_b_sink_e2e.py::test_e2e_byte_parity_with_the_fixture`, which drives the whole inbound chain and compares delivered bytes to the golden fixture — but **its fixture has one shard**, so `position * expected_cell_count` never ran with a position above zero. + +The core of the rewrite was therefore unverified, and the interleaving guard could not have caught it either: its three shards are byte-identical copies, so any ordering bug would survive. `test_a_multi_shard_run_assembles_in_manifest_order_with_every_row_written` now assembles three shards with distinct values, months and units and asserts the result equals `np.concatenate` in manifest order. Mutation-proven three ways: reversing the slot index, an off-by-one in `stop`, and leaving the identifiers unwritten all fail it. + +**At production scale.** 64,742 cells x 36 months, at ADR-013 **§0**'s *"~1000 samples per cell"* — the sample count is the one input here taken from the contract rather than measured — one target's frame is **8.68 GB**, so peak fell from about **26.6 GB to 9.8 GB per target**, roughly **16.8 GB** saved. For scale, run-0's OOM kill recorded `anon-rss:23778224kB` (#126); that incident's root cause was pandas on the *historical* leg and is not this, but the magnitude says this leg alone would have exhausted the same box. + +**The manager's historical frame is not the elephant, so it is not being chased.** #269 notes `_historical_frame` is held from `_read` through `_save`. By its own declared dimensions — 64,742 cells x 438 months = 28,356,996 rows — that is about **108 MB** at one float32 column, **1.2%** of a single forecast target frame. **Filed as #273**, carrying the measurement so it cannot be picked up under the impression that it is comparable — and because #269 listed *"the historical frame is released, or not held"* as an acceptance criterion of its own, which this change does not meet. Closing #269 while quietly leaving that unmet was the alternative, and it is not one. + +*One refusal added, because the fix moved a constraint.* The buffer's width is fixed by the first shard, so a run whose shards disagree on draws per cell is now this function's constraint rather than an incidental one. Left to the assignment it surfaced as `could not broadcast input array from shape (6,2) into shape (6,4)` — no shard named, no mention of draws. The stacking it replaced was no better, only wordier. It now refuses in its own words, mutation-proven by deleting the check and watching the bare numpy error return. + +*Guarded.* `tests/test_track_a_source.py::test_shards_are_fetched_one_at_a_time_not_all_up_front` asserts the fetch/decode interleaving rather than a byte count — a memory threshold in a test is a flake on a busy machine, while "fetch, decode, fetch, decode" is exactly the property that bounds the peak. Mutation-proven: restoring the up-front dict produces `['fetch','fetch','fetch','decode','decode','decode']` and it fails. + +Cross-refs: **C-99** (the other defect the same delivery attempt found), **C-75** (the pandas retirement that #126 landed on the historical leg), views-postprocessing#269, views-postprocessing#126. + +--- + +### C-99: `_ContractStorePort.download` failed open where `upload` refuses — C-79's untreated sibling — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-99 | +| Tier | 2 — no silent corruption, but an unreadable failure on the live FAO delivery leg, in the one place that knows which file it was. | +| Source | views-postprocessing#268, filed from the views-crafdapi seat 2026-08-14 after the first `un_crafd` delivery attempt | +| Trigger | *(closed)* Any failed download — a yanked file, an expired key, a rate limit, a network blip — on either partner's contract path. | +| Location | `views_postprocessing/{unfao,crafd}/store_port.py` (`download`); previously `managers/{unfao,crafd}.py:38-41` | + +`download` chained `.get()` onto an unvalidated store result: + +```python +self._dsm.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None) +``` + +When `data` is **present and null**, the `{}` default never applies and the next `.get` raises `AttributeError: 'NoneType' object has no attribute 'get'` — from inside a dict comprehension over pinned ids in `TargetLease.load`, three frames from the port, naming neither the `file_id` nor the fact that a download had failed. views-crafdapi spent an evening ruling out an OOM kill (there was one in `dmesg`, three minutes later, on a different pid) before finding it. + +**It was C-79 with the method name changed.** C-79 fixed exactly this polarity on `upload`, in the same class, on 2026-08-05, and recorded the specification in its own resolution note: *"an unrecognised result should be refused and named, not adapted to silently."* That note was never applied a second time. Nine days later the untreated method cost another repo an evening. + +*Why nothing caught it.* `tests/test_store_port.py` was written for C-79 with five parametrised tests across both partners, including `test_an_unrecognised_result_is_refused_rather_than_assumed_good`. It mentioned `download` **zero times**. And `contract/store_metadata.py` already wrote `.get("data", {}) or {}` — the guard `download` lacked, one file away, unapplied. + +**Fixed 2026-08-14.** `download` now refuses anything that is not non-empty bytes, naming the `file_id`, that a *download* failed, and the types it actually got. Empty bytes are refused with the rest: no shard, sidecar or manifest is ever zero-length, so `b""` is a failed download wearing a valid type. Byte-identical in both partners, as C-79 chose for `upload` (C-33). Mutation-proven on three mutants — restoring the original one-liner fails 18 of the module's tests, accepting empty bytes fails exactly 2, dropping the `file_id` from the message fails exactly 2. + +**It also moved.** The refusal pushed `managers/` to 469 lines against epic #148's 450 bound, and that guard's instruction is to move something out rather than raise the number. `_ContractStorePort` is not the manager, so it went to `{partner}/store_port.py` — 388 lines now, 62 of headroom. The port stopped naming `DatastoreModule` in its constructor on the way: a DIP seam whose stated purpose is that nothing downstream sees the client's types should not name one, and a new module that mentioned `views_pipeline_core` would have widened C-40's blast radius past the two files `test_views_pipeline_core_is_confined_to_the_partner_managers` pins. + +**Amendment, same day — the move was right and the way it was reported was not.** The first version of this change moved `_ContractStorePort` out of `managers/` and recorded "388 lines, 62 of headroom" as though the budget had been satisfied. Review measured what actually happened: the counted number fell from **441 to 388** while each partner package grew from **441 to 488**. The guard counts `managers/`, and the code moved to a sibling *of* `managers/`, so 47 lines left the budget's view rather than the codebase. + +The budget's own docstring had already named this failure — *"an 800-line helper module beside a 406-line manager was previously unbudgeted, which is the same regrowth wearing a different filename"* — and had closed it one level in. The evasion simply happened one level out. This is C-98's shape again: a guard that watches a proxy reports on the proxy, and the number it prints is true and irrelevant. + +`test_the_partner_package_stays_within_its_line_budget` now bounds the whole partner package at 700 (measured 2026-08-14: unfao 626, crafd 635), mutation-proven by dropping a 200-line module beside the manager — the exact evasion — and watching it fire. The extraction itself stands: a store adapter is not the manager, and the inner budget's instruction is to move something out. + +Cross-refs: **C-79** (the same defect on `upload`, resolved), **C-100** (the dead fourth method, found while reading this one), **C-33**, **C-40**. + +--- + ### C-27: Loader construction failures swallowed — surface as remote AttributeError — RESOLVED | Field | Value | @@ -1554,7 +1695,7 @@ Cross-refs: **C-44** (the bump that carried this), views-postprocessing#172, pip | Tier | 2 | | Source | `expert-review` (2026-06-02) | | Trigger | When configuring Appwrite connection parameters — in `_ContractStorePort` (contract path) or `_save`/`_read_forecast_data` (legacy path) — verify that timeout parameters are set on the underlying HTTP client; currently no timeout exists and a hung endpoint blocks the pipeline indefinitely | -| Location | `views_postprocessing/unfao/managers/unfao.py:37-64` (`_ContractStorePort` — all four contract-path store calls), `:247` (legacy selection), `:560`, `:571` (legacy uploads) | +| Location | `views_postprocessing/unfao/store_port.py` (`_ContractStorePort` — all four contract-path store calls; it was `managers/unfao.py:37-64` until 2026-08-14), `:247` (legacy selection), `:560`, `:571` (legacy uploads) | `prediction_store_manager.download_latest_file()` (line 131) and `dsm.upload_data()` (lines 262, 272) make network calls to Appwrite with no configured timeout. If the endpoint hangs (DNS resolution stalls, connection accepted but response never arrives, TLS handshake blocks), the pipeline blocks indefinitely. There is no watchdog timer, no circuit breaker, and no automated alert for a run that never completes. The only detection is manual observation that a scheduled run didn't finish. diff --git a/tests/test_doc_accuracy.py b/tests/test_doc_accuracy.py index 1c1c87d..7f9eeea 100644 --- a/tests/test_doc_accuracy.py +++ b/tests/test_doc_accuracy.py @@ -367,6 +367,16 @@ def test_internal_doc_links_resolve(): #: previously unbudgeted, which is the same regrowth wearing a different filename. _MANAGER_LINE_BUDGET = 450 +#: The same rule one level out, added 2026-08-14 because the directory bound was not +#: enough. C-99's fix pushed `managers/` to 469 and the response was to move +#: `_ContractStorePort` to `/store_port.py` — a sibling of `managers/`, not a +#: sibling inside it. The counted number fell 441 -> 388 while the partner package grew +#: by 47 lines, and the PR reported "62 of headroom" against a guard that could no +#: longer see the code. The move was right; reporting it as compliance was not. +#: Measured 2026-08-14: unfao 626, crafd 635. A ratchet, like the class budget — the +#: response to it binding is to move something OUT OF THE PACKAGE, not to raise it. +_PARTNER_PACKAGE_LINE_BUDGET = 700 + #: The manager CLASS, separately (C-40). 351 before the 2026-08-05 extraction, 272 after. #: A ratchet — see `test_the_manager_class_itself_stays_thin` for why it is not a target. _MANAGER_CLASS_BUDGET = 300 @@ -492,6 +502,39 @@ def test_the_manager_stays_within_its_line_budget(managers_dir): ) +@pytest.mark.parametrize("partner", _PARTNER_PACKAGES) +def test_the_partner_package_stays_within_its_line_budget(partner): + """The directory bound, one level out — because moving code past it is not shrinking. + + The budget above deliberately counts the manager *directory* rather than the manager + file, so that a helper module beside a thin manager could not go unbudgeted. On + 2026-08-14 the same evasion happened one directory further out and the guard did not + see it: `_ContractStorePort` moved from `managers/.py` to + `/store_port.py`, the counted number fell from 441 to 388, and the partner + package grew from 441 to 488 lines. + + That move was the right call — a store adapter is not the manager, and the budget's + own instruction is to move something out rather than raise the number. What was + wrong was calling the result "62 of headroom" when the guard had simply stopped + measuring the code. This test is what makes that sentence checkable, and it is the + same lesson as register C-98: a guard that watches a proxy reports on the proxy. + + A ratchet, not a target. If it binds, move something out of the partner package — + to `contract/` or `delivery/`, where the machinery lives — or say in the commit + message why the package genuinely needs to be bigger. + """ + package = _PKG / partner + sources = sorted(package.rglob("*.py")) + lines = sum(len(f.read_text().splitlines()) for f in sources) + assert lines <= _PARTNER_PACKAGE_LINE_BUDGET, ( + f"{partner}/ is {lines} lines across {len(sources)} files " + f"({[f.relative_to(package).as_posix() for f in sources]}), over the " + f"{_PARTNER_PACKAGE_LINE_BUDGET} bound. Moving code from managers/ into a " + "sibling module does not reduce the seam — it only moves it out of the inner " + "budget's view, which is what this outer one exists to notice." + ) + + @pytest.mark.parametrize("partner", _PARTNER_PACKAGES) def test_the_manager_class_itself_stays_thin(partner): """The directory budget above is anti-regrowth. This one is anti-*fusion*. diff --git a/tests/test_env_declaration.py b/tests/test_env_declaration.py index 9632514..8964300 100644 --- a/tests/test_env_declaration.py +++ b/tests/test_env_declaration.py @@ -35,15 +35,10 @@ from tests.seam_registry import ( ABSENT as _ABSENT, - REGISTRY_RELPATH, REGISTRY_RELPATH as _REGISTRY_RELPATH, - RegistryReadError, RegistryReadError as _RegistryReadError, - registry_at, registry_at as _registry_at, - registry_current, registry_current as _registry_current, - rows, rows as _rows, ) from tests.conftest import ( @@ -629,6 +624,32 @@ def _declared_classes(registry: dict) -> dict[str, str]: return {n: row[1] for n, row in _rows(registry, _CONSUMED_TABLES).items()} +def _name_and_class_drift(expected_class: dict, declared: dict) -> tuple[list, dict]: + """What this package expects vs what the registry declares. + + Returns ``(names the registry does not carry, {name: (expected, declared)})``. + + Extracted 2026-08-14 (issue #265) so the gated check below and the ungated proof of + it further down run the **same** comparison. They did not: the proof rebuilt this + with its own comprehensions, so blanking the assertions here left it green. + + The two sibling defects were repaired differently — by adding a test that drives the + real check under ``monkeypatch`` (``test_the_drift_check_fires_when_a_row_this_partner_reads_rotates``). + That works there because the check reads a registry the test can substitute. Here the + comparison is a pure function of two dicts and both callers want exactly it, so + sharing the function is the same guarantee with less machinery. Extracting is + justified by *this* being the second time the pattern has bitten, not by a rule about + line counts. + """ + missing = sorted(n for n in expected_class if n not in declared) + misclassified = { + n: (expected, declared[n]) + for n, expected in expected_class.items() + if n in declared and declared[n] != expected + } + return missing, misclassified + + @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.""" @@ -636,17 +657,12 @@ def test_every_declared_name_exists_in_the_registry_with_the_class_we_treat_it_a declared = _declared_classes(_registry_current(repo)) _, _, expected_class = _PARTNER_ENV[partner] - missing = sorted(n for n in expected_class if n not in declared) + missing, misclassified = _name_and_class_drift(expected_class, declared) assert not missing, ( 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() - if declared[n] != expected - } assert not misclassified, ( f"[{partner}] class mismatch (expected, registry) {misclassified}. Class is " "DECLARED by the registry, never inferred from a name's prefix — a coordinate " @@ -1140,17 +1156,20 @@ def test_the_drift_check_would_catch_a_rename(partner): "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) - assert missing, "the detector reported no missing names against a registry that omits most" - mismatched = [ - n for n, expected in expected_class.items() - if n in declared and declared[n] != expected - ] - assert canary in mismatched, ( + # The gated check's own comparison, not a copy of it (issue #265). This rebuilt the + # logic with its own comprehensions until 2026-08-14, which made it a proof of a + # reimplementation: blank the real check's assertions and it stayed green. + missing, misclassified = _name_and_class_drift(expected_class, declared) + assert missing, "the detector reported no missing names against a registry that omits most" + assert canary in misclassified, ( f"[{partner}] a target reclassified as a secret went unnoticed — that is the " "case where getting it wrong leaks or hides a value" ) + assert misclassified[canary] == ("target", "secret"), ( + "the detector must report BOTH sides of the mismatch — what this package " + "expects and what the registry declares — or a reader cannot tell which moved" + ) def _docstring_nodes(tree: ast.AST) -> set[int]: @@ -1391,113 +1410,3 @@ def test_the_scan_understands_every_assignment_form_this_repo_writes(): "document that introduced it — that is the stopping rule, and it is why the " "form list is derived from this repository's own corpus rather than invented." ) - - -def _scratch_repo(tmp_path: Path): - """A throwaway git repo whose registry differs on `main`, on `origin/main`, and on disk. - - `-c` rather than `git config`: a contributor's global `commit.gpgsign` or - `core.hooksPath` would otherwise reach in and either fail opaquely or block on - pinentry with no timeout. - """ - def git(*args): - return subprocess.run( - ["git", "-C", str(tmp_path), "-c", "commit.gpgsign=false", - "-c", "core.hooksPath=/dev/null", *args], - capture_output=True, text=True, check=True, timeout=30, - ) - - target = tmp_path / REGISTRY_RELPATH - target.parent.mkdir(parents=True) - - def edition(marker: str) -> str: - return f'[meta]\nversion = "{marker}"\n\n[connection.X]\nclass = "connection"\n' - - git("init", "-q", "-b", "main") - git("config", "user.email", "t@t") - git("config", "user.name", "t") - target.write_text(edition("on-main")) - git("add", "-A") - git("commit", "-q", "-m", "main") - - # a remote-tracking ref that is AHEAD of main, so preferring one over the other shows - git("checkout", "-q", "-b", "upstream") - target.write_text(edition("on-origin-main")) - git("add", "-A") - git("commit", "-q", "-m", "origin") - git("update-ref", "refs/remotes/origin/main", "HEAD") - git("checkout", "-q", "main") - - # and a dirty working tree, which is what #196 was about - target.write_text(edition("in-the-working-tree")) - return tmp_path - - -def test_registry_current_reads_origin_main_not_the_working_tree(tmp_path): - """The reason `tests/seam_registry.py` exists, and until now the only untested part. - - A sibling clone sits on whatever branch its own agent last worked on. Comparing - against that grades this repository on unreviewed content — issue #196, which cost a - withdrawn pull request. Replacing this function with a working-tree or `HEAD` read - used to leave the whole suite green. - """ - repo = _scratch_repo(tmp_path) - assert registry_current(repo)["meta"]["version"] == "on-origin-main", ( - "registry_current read something other than origin/main. A working-tree read is " - "#196 verbatim; a bare `main` read misses that the sibling's remote has moved." - ) - - -def test_registry_current_refuses_a_repo_with_neither_ref(tmp_path): - """No `origin/main` and no `main` must say so, not return an empty registry.""" - subprocess.run(["git", "init", "-q", str(tmp_path)], - capture_output=True, text=True, check=True, timeout=30) - with pytest.raises(RegistryReadError, match="neither origin/main nor main"): - registry_current(tmp_path) - - -def test_registry_at_refuses_a_commit_whose_registry_is_missing_or_unparseable(tmp_path): - """`git show` failing, and a blob that is not TOML — two refusal branches nothing reached.""" - def git(*args): - return subprocess.run( - ["git", "-C", str(tmp_path), "-c", "commit.gpgsign=false", - "-c", "core.hooksPath=/dev/null", *args], - capture_output=True, text=True, check=True, timeout=30, - ) - git("init", "-q", "-b", "main") - git("config", "user.email", "t@t") - git("config", "user.name", "t") - - (tmp_path / "unrelated.txt").write_text("no registry here\n") - git("add", "-A") - git("commit", "-q", "-m", "no registry") - absent = git("rev-parse", "--short", "HEAD").stdout.strip() - - target = tmp_path / REGISTRY_RELPATH - target.parent.mkdir(parents=True) - target.write_text("this is not toml = = =\n") - git("add", "-A") - git("commit", "-q", "-m", "not toml") - garbage = git("rev-parse", "--short", "HEAD").stdout.strip() - - with pytest.raises(RegistryReadError, match="cannot read the registry"): - registry_at(tmp_path, absent) - with pytest.raises(RegistryReadError, match="did not parse as TOML"): - registry_at(tmp_path, garbage) - - -def test_rows_refuses_a_section_whose_entries_are_not_tables(): - """`[test_environment]` on the live registry is scalars, not sub-tables. - - Nothing breaks today because that table is IGNORED — but the partition check's own - remediation message tells a maintainer to classify a new table CONSUMED, and doing - that for one written this way used to return an `AttributeError` from a dict - comprehension. Register C-91. - """ - scalars = {"test_environment": {"status": "none", "fact": "a sentence"}} - with pytest.raises(RegistryReadError, match=r"\[test_environment\]\.(status|fact) is a bare str"): - rows(scalars, ("test_environment",)) - - # and the ordinary shape still works, or the refusal above proves nothing - tables = {"target": {"APPWRITE_X": {"class": "target", "value": "v"}}} - assert rows(tables, ("target",)) == {"APPWRITE_X": ("target", "target", "v")} diff --git a/tests/test_release_version.py b/tests/test_release_version.py index 731bc69..3688e18 100644 --- a/tests/test_release_version.py +++ b/tests/test_release_version.py @@ -7,8 +7,14 @@ Measured 2026-08-13: tag `1.1.0` was cut at `main` while `pyproject.toml` still said `1.0.0`, so an install from that tag reported the previous release. Caught before any -consumer pinned. Third time in this arc a version has been declared twice with a guard on -one copy (register C-80, C-82). +consumer pinned, and the tag was re-pointed. + +*(This docstring used to add "third time in this arc a version has been declared twice +with a guard on one copy (register C-80, C-82)". Corrected 2026-08-15: neither entry says +that. C-80 is the doc-accuracy scan exempting ADRs and CICs; C-82 is governance prose +carrying numbers nothing checks — the same **class** as this, a number with no guard, but +about the register and CIC front matter, not about a version declared twice. The claim was +repeated into a release PR before anyone read the entries it cited.)* """ import re @@ -63,9 +69,18 @@ def test_the_newest_release_tag_is_not_ahead_of_the_declared_version(): This is the direction that actually bit: the tag moved, the file did not. Runs on every commit, not only tagged ones, so the gap is visible the moment it opens. + + **Tags reachable from HEAD, not every tag in the repository.** ``git tag -l`` lists + tags on every branch, which asks the wrong question: whether a release was cut + *anywhere*, rather than whether one was cut from *this line of history* without the + bump. The difference was invisible while CI fetched no tags at all, and would have + become a false alarm the moment it started: tag `1.1.1` on `main` would redden every + branch still declaring `1.1.0` — a long-lived feature branch, a hotfix cut from + `1.1.0` — none of which has done anything wrong. ADR-014 §3 prefers a false negative + to a false alarm, and a guard that reddens honest branches is one someone deletes. """ out = subprocess.run( - ["git", "-C", str(_REPO), "tag", "-l", "--sort=-v:refname"], + ["git", "-C", str(_REPO), "tag", "--merged", "HEAD", "--sort=-v:refname"], capture_output=True, text=True, check=False, timeout=30, ) releases = [t for t in out.stdout.split() if re.fullmatch(r"\d+\.\d+\.\d+", t)] diff --git a/tests/test_seam_registry.py b/tests/test_seam_registry.py new file mode 100644 index 0000000..039b327 --- /dev/null +++ b/tests/test_seam_registry.py @@ -0,0 +1,140 @@ +"""Refusals of the shared seam-registry reader (`tests/seam_registry.py`). + +These moved out of `tests/test_env_declaration.py` on 2026-08-14 (issue #265). Their +subject is the *reader* — `registry_at`, `registry_current`, `rows` — not what this +package declares about its environment, and a 1503-line module that had become the home +for both was the clearest signal in the repo that a boundary was wrong. + +What deliberately did **not** move with them: the coordinate-value scan. Its subject is +`_EXPECTED_NAMES`, derived from the partner declarations that are the substance of +`test_env_declaration.py`, and separating a guard from the declaration it guards to make +a filename read better trades a real coupling for a filing convenience. The reasoning is +in register C-89. + +Every refusal here is a real failure someone hit: an empty pin, a commit this clone does +not have, a ref that is not a commit, a registry that will not parse, and a section whose +rows are scalars rather than tables (C-91). +""" + +import subprocess +from pathlib import Path + +import pytest + +from tests.seam_registry import ( + REGISTRY_RELPATH, + RegistryReadError, + registry_at, + registry_current, + rows, +) + + +def _scratch_repo(tmp_path: Path): + """A throwaway git repo whose registry differs on `main`, on `origin/main`, and on disk. + + `-c` rather than `git config`: a contributor's global `commit.gpgsign` or + `core.hooksPath` would otherwise reach in and either fail opaquely or block on + pinentry with no timeout. + """ + def git(*args): + return subprocess.run( + ["git", "-C", str(tmp_path), "-c", "commit.gpgsign=false", + "-c", "core.hooksPath=/dev/null", *args], + capture_output=True, text=True, check=True, timeout=30, + ) + + target = tmp_path / REGISTRY_RELPATH + target.parent.mkdir(parents=True) + + def edition(marker: str) -> str: + return f'[meta]\nversion = "{marker}"\n\n[connection.X]\nclass = "connection"\n' + + git("init", "-q", "-b", "main") + git("config", "user.email", "t@t") + git("config", "user.name", "t") + target.write_text(edition("on-main")) + git("add", "-A") + git("commit", "-q", "-m", "main") + + # a remote-tracking ref that is AHEAD of main, so preferring one over the other shows + git("checkout", "-q", "-b", "upstream") + target.write_text(edition("on-origin-main")) + git("add", "-A") + git("commit", "-q", "-m", "origin") + git("update-ref", "refs/remotes/origin/main", "HEAD") + git("checkout", "-q", "main") + + # and a dirty working tree, which is what #196 was about + target.write_text(edition("in-the-working-tree")) + return tmp_path + + +def test_registry_current_reads_origin_main_not_the_working_tree(tmp_path): + """The reason `tests/seam_registry.py` exists, and until now the only untested part. + + A sibling clone sits on whatever branch its own agent last worked on. Comparing + against that grades this repository on unreviewed content — issue #196, which cost a + withdrawn pull request. Replacing this function with a working-tree or `HEAD` read + used to leave the whole suite green. + """ + repo = _scratch_repo(tmp_path) + assert registry_current(repo)["meta"]["version"] == "on-origin-main", ( + "registry_current read something other than origin/main. A working-tree read is " + "#196 verbatim; a bare `main` read misses that the sibling's remote has moved." + ) + + +def test_registry_current_refuses_a_repo_with_neither_ref(tmp_path): + """No `origin/main` and no `main` must say so, not return an empty registry.""" + subprocess.run(["git", "init", "-q", str(tmp_path)], + capture_output=True, text=True, check=True, timeout=30) + with pytest.raises(RegistryReadError, match="neither origin/main nor main"): + registry_current(tmp_path) + + +def test_registry_at_refuses_a_commit_whose_registry_is_missing_or_unparseable(tmp_path): + """`git show` failing, and a blob that is not TOML — two refusal branches nothing reached.""" + def git(*args): + return subprocess.run( + ["git", "-C", str(tmp_path), "-c", "commit.gpgsign=false", + "-c", "core.hooksPath=/dev/null", *args], + capture_output=True, text=True, check=True, timeout=30, + ) + git("init", "-q", "-b", "main") + git("config", "user.email", "t@t") + git("config", "user.name", "t") + + (tmp_path / "unrelated.txt").write_text("no registry here\n") + git("add", "-A") + git("commit", "-q", "-m", "no registry") + absent = git("rev-parse", "--short", "HEAD").stdout.strip() + + target = tmp_path / REGISTRY_RELPATH + target.parent.mkdir(parents=True) + target.write_text("this is not toml = = =\n") + git("add", "-A") + git("commit", "-q", "-m", "not toml") + garbage = git("rev-parse", "--short", "HEAD").stdout.strip() + + with pytest.raises(RegistryReadError, match="cannot read the registry"): + registry_at(tmp_path, absent) + with pytest.raises(RegistryReadError, match="did not parse as TOML"): + registry_at(tmp_path, garbage) + + +def test_rows_refuses_a_section_whose_entries_are_not_tables(): + """`[test_environment]` on the live registry is scalars, not sub-tables. + + Nothing breaks today because that table is IGNORED — but the partition check's own + remediation message tells a maintainer to classify a new table CONSUMED, and doing + that for one written this way used to return an `AttributeError` from a dict + comprehension. Register C-91. + """ + scalars = {"test_environment": {"status": "none", "fact": "a sentence"}} + with pytest.raises(RegistryReadError, match=r"\[test_environment\]\.(status|fact) is a bare str"): + rows(scalars, ("test_environment",)) + + # and the ordinary shape still works, or the refusal above proves nothing + tables = {"target": {"APPWRITE_X": {"class": "target", "value": "v"}}} + assert rows(tables, ("target",)) == {"APPWRITE_X": ("target", "target", "v")} diff --git a/tests/test_store_port.py b/tests/test_store_port.py index cc99094..46c82a4 100644 --- a/tests/test_store_port.py +++ b/tests/test_store_port.py @@ -1,4 +1,4 @@ -"""The store port's refusal, tested — register C-79. +"""The store port's refusals, tested — register C-79 (``upload``) and C-99 (``download``). **This code had zero tests until 2026-08-05**, while the comment beside it called it "the whole mechanism". It is: pipeline-core's store, on a metadata failure *after* the @@ -7,6 +7,13 @@ invisible to the consumer, which is what happened to run-0's historical artifact on 2026-07-27. This port is the thing that turns that into a refusal. +``download`` is the same fault in the method next door, and it went untested here for +another nine days: it chained ``.get()`` onto an unvalidated result, so a store result +whose ``data`` was null raised ``AttributeError`` three frames away instead of naming +the file that failed. views-crafdapi lost an evening to it on 2026-08-13. C-79's own +resolution note — *an unrecognised result should be refused and named, not adapted to +silently* — was already the specification; it had simply never been applied twice. + Testable now for a reason worth stating: the standing excuse for source-scanning manager-side facts is that the managers need Appwrite env and a views-models path manager to instantiate. ``_ContractStorePort`` needs neither — it takes a store object @@ -19,8 +26,12 @@ import pytest +from pathlib import Path + from tests.conftest import PARTNER_PACKAGES +_PKG = Path(__file__).resolve().parent.parent / "views_postprocessing" + @dataclass class _Result: @@ -30,25 +41,44 @@ class _Result: error: str | None = None +@dataclass +class _Downloaded: + """Shaped like the store's download result: ``.to_dict()["data"]["file_bytes"]``. + + ``data`` is declared as ``object`` rather than ``dict`` on purpose — the whole of + C-99 is what happens when it is not a dict. + """ + + data: object + + def to_dict(self): + return {"data": self.data} + + class _FakeStore: - """Records the upload and returns whatever result the test declares.""" + """Records the upload and returns whatever results the test declares.""" - def __init__(self, result): + def __init__(self, result, downloaded=None): self.result = result + self.downloaded = downloaded self.calls = [] + self.downloads = [] def upload_data(self, **kwargs): self.calls.append(kwargs) return self.result + def download_prediction(self, file_id): + self.downloads.append(file_id) + return self.downloaded + -def _port(partner: str, result): +def _port(partner: str, result, downloaded=None): """The partner's port, wrapping a fake store. Needs no Appwrite environment.""" - pytest.importorskip("views_pipeline_core", reason="the port wraps its DatastoreModule") module = __import__( - f"views_postprocessing.{partner}.managers.{partner}", fromlist=["_ContractStorePort"] + f"views_postprocessing.{partner}.store_port", fromlist=["_ContractStorePort"] ) - store = _FakeStore(result) + store = _FakeStore(result, downloaded) return module._ContractStorePort(store), store @@ -153,3 +183,113 @@ def test_the_port_forwards_every_declared_field(partner, tmp_path): assert forwarded["name"] == "un_fao" assert forwarded["category"] == "historical" assert forwarded["type"] == "model", "doc_type must arrive as the store's `type`" + + +# --------------------------------------------------------------------------- +# `download` — the same polarity, on the method C-79 missed (register C-99). +# --------------------------------------------------------------------------- + +_FILE_ID = "68b0f2c19a4e7d3c5a11" + + +def _download(port): + return port.download(_FILE_ID) + + +@pytest.mark.parametrize("payload", [b"shard-bytes", bytearray(b"shard-bytes")]) +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_a_downloaded_artifact_is_returned_as_bytes(partner, payload): + """The happy path, and the one conversion the port is allowed to make. + + ``bytearray`` is accepted and normalised because it is bytes by any useful + definition; everything else is refused below. If this test did not exist the + refusal could be tightened until nothing passed and the suite would not notice. + """ + port, store = _port(partner, _Result(success=True), _Downloaded({"file_bytes": payload})) + assert _download(port) == b"shard-bytes" + assert store.downloads == [_FILE_ID], "the port must forward the pinned id unchanged" + + +@pytest.mark.parametrize( + "downloaded, why", + [ + (_Downloaded(None), "data is present and null — the crash of 2026-08-13"), + (_Downloaded({}), "data carries no file_bytes at all"), + (_Downloaded({"file_bytes": None}), "file_bytes is present and null"), + (_Downloaded({"file_bytes": ""}), "file_bytes is a str, not bytes"), + (_Downloaded({"file_bytes": b""}), "file_bytes is bytes but empty"), + (_Downloaded("not-a-dict"), "data is not a mapping"), + (None, "the store returned nothing at all"), + (object(), "the result has no to_dict()"), + ], +) +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_a_download_that_is_not_bytes_is_refused_rather_than_returned_as_none( + partner, downloaded, why +): + """Fail CLOSED. This is C-79's polarity applied to the method next door. + + The original ``.get("data", {}).get("file_bytes", None)`` handled exactly one of + these — a *missing* ``data`` key. Every other row here either returned ``None`` to a + caller that could not tell it from an empty artifact, or raised ``AttributeError`` + from inside a dict comprehension three frames away. + + ``b""`` is refused with the rest deliberately: no shard, sidecar or manifest is ever + zero-length, so an empty payload is a failed download wearing a valid type, and + returning it only moves the same crash to the parser. + """ + port, _ = _port(partner, _Result(success=True), downloaded) + with pytest.raises(RuntimeError, match="did not return usable bytes"): + _download(port) + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_download_refusal_names_the_file_id_and_what_it_got(partner): + """The defect was never that it failed — it was that the failure said nothing. + + The crash an operator actually saw was ``'NoneType' object has no attribute 'get'``, + raised inside a dict comprehension over pinned ids. It named no file, did not say a + download had failed, and sent views-crafdapi looking for an OOM kill that turned out + to be a different process. The id is in hand at this point; a refusal that drops it + is barely better than the crash. + """ + port, _ = _port(partner, _Result(success=True), _Downloaded(None)) + with pytest.raises(RuntimeError) as excinfo: + _download(port) + message = str(excinfo.value) + assert _FILE_ID in message, "the refusal must name the file_id it was given" + assert "download" in message, "the refusal must say that a DOWNLOAD failed" + assert "NoneType" in message, ( + "the refusal must name what it actually got, or the reader cannot tell a store " + "that returned nothing from one whose result shape moved" + ) + + +def test_the_two_partners_ports_have_not_drifted(): + """The duplication C-33 blesses is only safe while the copies stay equal. + + Both partners carry this file byte for byte, which is the standing per-partner-track + decision (C-33), not an accident. What makes that decision cheap is that a reader can + treat one file as the truth; what makes it dangerous is a fix applied to one copy and + not the other, which nothing in this repository would have noticed until now. + + **Be precise about what this does not catch.** It would *not* have caught C-99. That + drift was between two METHODS of the same class — ``upload`` was fixed in both + partners on 2026-08-05 and ``download`` in neither — so both files stayed perfectly + identical while carrying the defect for nine days. This guard closes the other axis, + the partner-vs-partner one, which is real but was never the thing that bit. + """ + sources = { + partner: (_PKG / partner / "store_port.py").read_text() + for partner in PARTNER_PACKAGES + } + first, *rest = sorted(sources) + for other in rest: + assert sources[first] == sources[other], ( + f"{first}/store_port.py and {other}/store_port.py have diverged. The port is " + "duplicated per partner on purpose (C-33), and the copies carry no " + "partner-specific content at all — so a difference here is a fix that landed " + "in one partner and not the other, which is how the same delivery bug ships " + "twice. Apply it to both, or if the divergence is deliberate, say so in " + "C-33 and replace this check with one that allows it." + ) diff --git a/tests/test_track_a_source.py b/tests/test_track_a_source.py index 853f8b5..d96ae68 100644 --- a/tests/test_track_a_source.py +++ b/tests/test_track_a_source.py @@ -68,7 +68,7 @@ def test_read_shard_round_trips_the_fixture(): def test_frames_for_target_assembles_the_run(): - frame, headers = tas.frames_for_target(MANIFEST, {SHARD_NAME: SHARD}) + frame, headers = tas.frames_for_target(MANIFEST, {SHARD_NAME: SHARD}.__getitem__) assert frame.n_rows == 6 and frame.sample_count == 4 # headers ride along in manifest shard order (provenance pass-through, §10.2) assert [h["time_id"] for h in headers] == [543] @@ -154,14 +154,14 @@ def test_minor_version_drift_accepted(): def test_missing_shard_bytes_rejected(): with pytest.raises(tas.TrackASourceError, match="not provided"): - tas.frames_for_target(MANIFEST, {}) + tas.frames_for_target(MANIFEST, {}.__getitem__) def test_shard_target_disagreeing_with_manifest_rejected(): bad = _retouched_shard(**{"metadata.json": _header(target="lr_ged_ns")}) manifest = {**MANIFEST, "shards": [{"name": SHARD_NAME, "sha256": _sha(bad)}]} with pytest.raises(tas.TrackASourceError, match="target"): - tas.frames_for_target(manifest, {SHARD_NAME: bad}) + tas.frames_for_target(manifest, {SHARD_NAME: bad}.__getitem__) def test_wrong_month_coverage_rejected(): @@ -170,21 +170,183 @@ def test_wrong_month_coverage_rejected(): # month 544's shard is absent entirely — caught at the bytes gate tas.frames_for_target( {**manifest, "shards": MANIFEST["shards"] + [{"name": "m544", "sha256": "0" * 64}]}, - {SHARD_NAME: SHARD}, + {SHARD_NAME: SHARD}.__getitem__, ) bad = _retouched_shard(**{"metadata.json": _header(time_id=999)}) manifest = {**MANIFEST, "shards": [{"name": SHARD_NAME, "sha256": _sha(bad)}]} with pytest.raises(tas.TrackASourceError, match="months covered"): - tas.frames_for_target(manifest, {SHARD_NAME: bad}) + tas.frames_for_target(manifest, {SHARD_NAME: bad}.__getitem__) def test_wrong_cell_count_rejected(): manifest = {**MANIFEST, "expected_cell_count": 7} with pytest.raises(tas.TrackASourceError, match="cells"): - tas.frames_for_target(manifest, {SHARD_NAME: SHARD}) + tas.frames_for_target(manifest, {SHARD_NAME: SHARD}.__getitem__) def test_manifest_missing_required_field_rejected(): truncated = {k: v for k, v in MANIFEST.items() if k != "expected_months"} with pytest.raises(tas.TrackASourceError, match="expected_months"): tas.read_manifest(json.dumps(truncated).encode()) + + +def test_shards_are_fetched_one_at_a_time_not_all_up_front(monkeypatch): + """The bound this function's memory shape depends on — register C-101. + + ``frames_for_target`` took a filled dict until 2026-08-14, so every shard of a + target was resident before the first was decoded. Measured at 36 shards, that plus + stacking with ``np.concatenate`` put peak at **3.06x the delivered frame**, in three + roughly equal thirds: the raw bytes, the per-shard frames, and the concatenated + copy. Fetching per shard and filling a manifest-sized buffer took it to **1.13x**. + + This asserts the *interleaving*, not a byte count, and deliberately so: a memory + threshold in a test is a flake on a busy machine, whereas "fetch, decode, fetch, + decode" is the property that actually bounds the peak and it is exactly observable. + Reverting to a pre-built dict makes the sequence fetch-fetch-decode-decode and this + fails; nothing else in the suite would notice. + """ + manifest = { + **MANIFEST, + "shards": [{"name": f"shard-{i}", "sha256": SHARD_SHA} for i in range(3)], + "expected_months": [543, 543, 543], + } + events = [] + real_read_shard = tas.read_shard + + def spy(shard_bytes, *, expected_sha256): + events.append("decode") + return real_read_shard(shard_bytes, expected_sha256=expected_sha256) + + monkeypatch.setattr(tas, "read_shard", spy) + + def fetch(name): + events.append("fetch") + return SHARD + + tas.frames_for_target(manifest, fetch) + + assert events == ["fetch", "decode"] * 3, ( + f"shards are not being fetched one at a time: {events}. Every 'fetch' that " + "precedes another 'fetch' is a shard's bytes held while the next is downloaded " + "— at 36 shards that was a third of the peak." + ) + + +def test_shards_with_different_draw_counts_are_refused_in_our_own_words(): + """A run whose shards disagree on S is not one forecast — say so, do not let numpy. + + The assembly buffer's width is fixed by the first shard, which makes a draw-count + disagreement this function's constraint rather than an incidental one. Left to the + assignment it surfaces as ``could not broadcast input array from shape (6,2) into + shape (6,4)`` — no shard named, no mention of draws, three frames from anything a + reader recognises. The stacking it replaced was no better, only wordier; neither is + a refusal, which is the whole of C-99's lesson applied before it could bite again. + """ + narrow_values = io.BytesIO() + np.save(narrow_values, np.zeros((6, 2), dtype=np.float32)) + narrow = _retouched_shard(**{ + "y_pred.npy": narrow_values.getvalue(), + "metadata.json": _header(sample_count=2, time_id=544), + }) + manifest = { + **MANIFEST, + "shards": [ + {"name": SHARD_NAME, "sha256": SHARD_SHA}, + {"name": "narrow", "sha256": _sha(narrow)}, + ], + "expected_months": [543, 544], + } + with pytest.raises(tas.TrackASourceError, match="draws per cell"): + tas.frames_for_target(manifest, {SHARD_NAME: SHARD, "narrow": narrow}.__getitem__) + + +def _shard_with(*, values, time_id, unit_start): + """A fixture-shaped shard carrying declared values/ids — distinct per month.""" + payload, ids = io.BytesIO(), io.BytesIO() + np.save(payload, values) + t, u = io.BytesIO(), io.BytesIO() + np.save(t, np.full(values.shape[0], time_id, dtype=np.int64)) + np.save(u, np.arange(unit_start, unit_start + values.shape[0], dtype=np.int64)) + with zipfile.ZipFile(ids, "w", zipfile.ZIP_STORED) as zf: + zf.writestr("time.npy", t.getvalue()) + zf.writestr("unit.npy", u.getvalue()) + return _retouched_shard(**{ + "y_pred.npy": payload.getvalue(), + "identifiers.npz": ids.getvalue(), + "metadata.json": _header(sample_count=values.shape[1], time_id=time_id), + }) + + +def test_a_multi_shard_run_assembles_in_manifest_order_with_every_row_written(): + """The slot arithmetic, against the stacking it replaced — register C-101. + + Until this existed, **nothing in the suite assembled more than one shard**. The + single-shard fixture drives the whole e2e byte-parity chain + (`tests/test_hop_b_sink_e2e.py`), so `position * expected_cell_count` never ran with + a position above zero, and the interleaving guard's three shards are byte-identical + copies that would survive any ordering bug. The rewrite's core was unverified. + + Three shards with values, months and units that are distinct per shard, asserted + against exactly what `np.concatenate` in manifest order produces. That is the claim + the change makes: same product, less memory. It catches a transposed slot, an + off-by-one in `start`/`stop`, an unwritten row left as `np.empty` garbage, and + identifiers assembled out of step with the values they label. + """ + cells, draws = 4, 3 + blocks = [ + np.full((cells, draws), fill, dtype=np.float32) for fill in (1.5, 2.5, 3.5) + ] + shards, entries, months = {}, [], [] + for i, block in enumerate(blocks): + time_id = 543 + i + raw = _shard_with(values=block, time_id=time_id, unit_start=100_000 + 10 * i) + name = f"m{time_id}" + shards[name] = raw + entries.append({"name": name, "sha256": _sha(raw)}) + months.append(time_id) + + manifest = { + **MANIFEST, + "shards": entries, + "expected_months": months, + "expected_cell_count": cells, + } + frame, headers = tas.frames_for_target(manifest, shards.__getitem__) + + np.testing.assert_array_equal(frame.values, np.concatenate(blocks, axis=0)) + np.testing.assert_array_equal( + np.asarray(frame.index.time), + np.concatenate([np.full(cells, m, dtype=np.int64) for m in months]), + ) + np.testing.assert_array_equal( + np.asarray(frame.index.unit), + np.concatenate([ + np.arange(100_000 + 10 * i, 100_000 + 10 * i + cells, dtype=np.int64) + for i in range(len(blocks)) + ]), + ) + assert frame.n_rows == cells * len(blocks) + assert [h["time_id"] for h in headers] == months, "headers ride in manifest order" + + +@pytest.mark.parametrize( + "declared, why", + [ + (6.0, "a JSON float compares equal to 6 and used to pass"), + (0, "zero cells sizes an empty frame nothing can be checked against"), + (-1, "negative would raise deep inside numpy"), + (True, "bool is an int subclass and would size a one-row frame"), + ], +) +def test_expected_cell_count_must_be_a_positive_integer(declared, why): + """It sizes an array now; it used to sit on one side of a ``!=``. + + ``6.0 == 6`` is True, so a manifest carrying a JSON float passed the old row-count + check and assembled correctly. The rewrite hands the same value to ``np.empty``, + where it raises ``TypeError: 'float' object cannot be interpreted as an integer`` — + bare, naming neither the field nor the manifest, from a document that crossed a + repository boundary. ``read_manifest`` only checks that the key is present. + """ + manifest = {**MANIFEST, "expected_cell_count": declared} + with pytest.raises(tas.TrackASourceError, match="expected_cell_count"): + tas.frames_for_target(manifest, {SHARD_NAME: SHARD}.__getitem__) diff --git a/tests/test_wire_source_selection.py b/tests/test_wire_source_selection.py index accac29..0d84e23 100644 --- a/tests/test_wire_source_selection.py +++ b/tests/test_wire_source_selection.py @@ -141,3 +141,41 @@ def test_selection_filters_are_golden_strings(): assert sel.HOP_A_SHARD_FILTERS == {"category": "forecast", "type": "sampled_forecast_shard"} assert sel.HOP_A_MANIFEST_FILTERS == {"category": "forecast", "type": "sampled_forecast_manifest"} assert sel.HOP_A_MANIFEST_NAME_TEMPLATE == "{run_id}__{target}__manifest.json" + + +def test_a_store_that_raises_keyerror_is_not_blamed_on_the_manifest(): + """A store fault must not be relabelled as a missing manifest entry. + + ``frames_for_target`` reads a ``KeyError`` from the fetch callback as *"this shard + was never pinned"*. The lease's callback calls into the store, so before this guard + a ``KeyError`` thrown anywhere inside the client — a response shape indexed with + ``[]`` rather than ``.get()``, which is how C-99 happened one layer down — would + surface as *"manifest lists shard X but its bytes were not provided"*, blaming the + manifest for a store failure. ``raise ... from None`` would have discarded the + traceback that said otherwise. + + The shard here IS pinned, so "not provided" would be a false diagnosis. + """ + manifest = json.loads((_FIX / _MANIFEST_NAME).read_text()) + + class KeyErroringStore: + def download(self, file_id): + raise KeyError("data") + + lease = sel.TargetLease( + target=manifest["target"], + manifest=manifest, + shard_file_ids={entry["name"]: "pinned-id" for entry in manifest["shards"]}, + store=KeyErroringStore(), + expected_ensemble="fixture_ensemble", + ) + with pytest.raises(sel.SourceSelectionError) as excinfo: + lease.load() + message = str(excinfo.value) + assert "store fault" in message, "the refusal must say where the fault is" + assert "not provided" not in message, ( + "a store KeyError must not be reported as a missing manifest entry" + ) + assert excinfo.value.__cause__ is not None, ( + "the store's own KeyError must be chained, not discarded" + ) diff --git a/views_postprocessing/contract/store_metadata.py b/views_postprocessing/contract/store_metadata.py index ffe4708..1672b37 100644 --- a/views_postprocessing/contract/store_metadata.py +++ b/views_postprocessing/contract/store_metadata.py @@ -15,9 +15,9 @@ frame readers became unreachable and this function was the sole survivor of a module named for a seam it was never part of. -The one caller is ``_ContractStorePort.file_metadata`` in the manager, which adapts -``DatastoreModule`` to the wire's ports (DIP) — so the store's document shape is -known here, and nowhere above. +The one caller is ``_ContractStorePort.file_metadata`` in each partner's +``store_port.py``, which adapts the store client to the wire's ports (DIP) — so the +store's document shape is known here, and nowhere above. """ from __future__ import annotations diff --git a/views_postprocessing/contract/track_a_source.py b/views_postprocessing/contract/track_a_source.py index 2aeeb80..416f0a8 100644 --- a/views_postprocessing/contract/track_a_source.py +++ b/views_postprocessing/contract/track_a_source.py @@ -106,47 +106,105 @@ def read_shard(shard_bytes: bytes, *, expected_sha256: str) -> tuple[PredictionF def frames_for_target( - manifest: dict, shard_bytes_by_name: dict + manifest: dict, fetch_shard_bytes ) -> tuple[PredictionFrame, list[dict]]: """A (run, target)'s verified shards → ``(PredictionFrame, headers)``. - ``shard_bytes_by_name`` maps shard ``name`` → downloaded bytes; the manifest is the - only source of which shards exist (§3.3: names are locators, manifest content is - identity). Verifies run completeness against the manifest's own declarations — - months covered exactly, cell count per month — then stacks months into one frame. - The returned ``headers`` (manifest shard order) carry the producer-minted - provenance the sink passes through untouched (§10.2 — nothing is minted - downstream). + ``fetch_shard_bytes(name) -> bytes`` returns one shard's bytes on demand; the + manifest is the only source of which shards exist (§3.3: names are locators, + manifest content is identity). Verifies run completeness against the manifest's own + declarations — months covered exactly, cell count per month — then stacks months + into one frame. The returned ``headers`` (manifest shard order) carry the + producer-minted provenance the sink passes through untouched (§10.2 — nothing is + minted downstream). + + **A callback rather than a pre-built dict, and the output filled in place rather + than concatenated — both for memory (register C-101).** Measured on 2026-08-14 at + 36 shards: taking a dict meant every shard's bytes were resident before the first + was decoded, and stacking with ``np.concatenate`` meant the per-shard frames and + the finished array were resident together. Peak was **3.06x the delivered frame**, + in three roughly equal thirds. Fetching per shard and writing into a + manifest-sized buffer keeps one shard's bytes and one shard's frame alive at a + time, so peak is the frame plus a shard. + + The buffer is sized from ``expected_cell_count`` x shard count, which is a + declaration this function already enforces per shard — a shard whose row count + disagrees is refused *before* anything is written, so the slot arithmetic can + never straddle two months. """ - frames, months_seen, headers = [], [], [] - for entry in manifest["shards"]: + entries = manifest["shards"] + if not entries: + raise TrackASourceError( + "run: the manifest lists no shards — an empty run must not be assembled." + ) + expected_cells = manifest["expected_cell_count"] + # It sizes an array now, where it used to sit on one side of a `!=`. `6.0` compares + # equal to `6` and passed the old check happily; here it reaches `np.empty` and + # raises a bare `TypeError: 'float' object cannot be interpreted as an integer`, + # naming neither the manifest nor the field. `read_manifest` checks that the key is + # present, never what it holds, and the manifest crosses a repository boundary. + if type(expected_cells) is not int or expected_cells < 1: + raise TrackASourceError( + f"run: manifest declares expected_cell_count={expected_cells!r} " + f"({type(expected_cells).__name__}) — it must be a positive integer, " + f"because it sizes the assembled frame." + ) + values = time = unit = None + months_seen, headers = [], [] + + for position, entry in enumerate(entries): name = entry["name"] - if name not in shard_bytes_by_name: + try: + shard_bytes = fetch_shard_bytes(name) + except KeyError: raise TrackASourceError( f"run: manifest lists shard {name!r} but its bytes were not provided — " f"an unmanifested or missing shard must not be silently skipped." - ) - frame, header = read_shard(shard_bytes_by_name[name], expected_sha256=entry["sha256"]) + ) from None + frame, header = read_shard(shard_bytes, expected_sha256=entry["sha256"]) + # The bytes are dead the moment they are decoded. Dropping the reference here + # is the whole of the first third: without it every shard's bytes outlive the + # loop. + del shard_bytes if header.get("target") != manifest["target"]: raise TrackASourceError( f"run: shard header target {header.get('target')!r} != manifest target " f"{manifest['target']!r}." ) - if frame.n_rows != manifest["expected_cell_count"]: + if frame.n_rows != expected_cells: raise TrackASourceError( f"run: shard {name!r} carries {frame.n_rows} cells, manifest expects " - f"{manifest['expected_cell_count']}." + f"{expected_cells}." + ) + if values is None: + total = expected_cells * len(entries) + frame_time, frame_unit = np.asarray(frame.index.time), np.asarray(frame.index.unit) + values = np.empty((total, frame.values.shape[1]), dtype=frame.values.dtype) + time = np.empty(total, dtype=frame_time.dtype) + unit = np.empty(total, dtype=frame_unit.dtype) + elif frame.values.shape[1] != values.shape[1]: + # The buffer's width is fixed by the first shard, so a draw-count + # disagreement is now this function's constraint rather than numpy's. + # Left to the assignment it reads "could not broadcast input array from + # shape (a,b) into shape (a,c)" — no shard named, no mention of draws. + # Stacking said the same in more words; neither is a refusal (C-99). + raise TrackASourceError( + f"run: shard {name!r} carries {frame.values.shape[1]} draws per cell, " + f"the run's first shard carried {values.shape[1]} — a target assembled " + f"from shards with different sample counts is not one forecast." ) - frames.append(frame) + start = position * expected_cells + stop = start + expected_cells + values[start:stop] = frame.values + time[start:stop] = np.asarray(frame.index.time) + unit[start:stop] = np.asarray(frame.index.unit) headers.append(header) months_seen.append(int(header.get("time_id"))) + del frame if sorted(months_seen) != sorted(int(m) for m in manifest["expected_months"]): raise TrackASourceError( f"run: months covered {sorted(months_seen)} != manifest expected " f"{sorted(manifest['expected_months'])} — a torn run must not be assembled." ) - values = np.concatenate([f.values for f in frames], axis=0) - time = np.concatenate([np.asarray(f.index.time) for f in frames]) - unit = np.concatenate([np.asarray(f.index.unit) for f in frames]) return build_prediction_frame(values, time, unit), headers diff --git a/views_postprocessing/contract/wire/source_selection.py b/views_postprocessing/contract/wire/source_selection.py index 92d4266..25a79bc 100644 --- a/views_postprocessing/contract/wire/source_selection.py +++ b/views_postprocessing/contract/wire/source_selection.py @@ -64,12 +64,34 @@ def run_id(self) -> str: return self.manifest["run_id"] def load(self): - """Fetch (by pinned id), verify, curate — return the PRODUCT ``(frame, headers)``.""" - shard_bytes = { - name: self.store.download(file_id) - for name, file_id in self.shard_file_ids.items() - } - frame, headers = track_a_source.frames_for_target(self.manifest, shard_bytes) + """Fetch (by pinned id), verify, curate — return the PRODUCT ``(frame, headers)``. + + Shards are fetched **one at a time**, by handing ``frames_for_target`` a lookup + rather than a filled dict. The dict comprehension that stood here downloaded + every shard of the target before the first was decoded; with the stacking fix + beside it that made peak 3.06x the delivered frame (register C-101, measured). + Fetch-by-pinned-id is unchanged — the ids were pinned by ``resolve_run`` and a + newer run still cannot be mixed in. + """ + def fetch(name): + # `frames_for_target` reads a KeyError as "this shard was never pinned". + # Only the lookup is allowed to say that: a KeyError thrown from inside the + # store — a response shape indexed with [] somewhere in the client — would + # otherwise be relabelled "bytes were not provided", blaming the manifest + # for a store failure and discarding the traceback that says otherwise. + # C-99 was that exact substitution one layer down. + file_id = self.shard_file_ids[name] + try: + return self.store.download(file_id) + except KeyError as exc: + raise SourceSelectionError( + f"run {self.run_id!r}, target {self.target!r}: the store raised " + f"KeyError({exc}) downloading shard {name!r} (file_id {file_id!r}). " + f"The shard was pinned and requested — this is a store fault, not a " + f"missing manifest entry." + ) from exc + + frame, headers = track_a_source.frames_for_target(self.manifest, fetch) for header in headers: found = header.get("provenance", {}).get("ensemble") if found != self.expected_ensemble: diff --git a/views_postprocessing/crafd/managers/crafd.py b/views_postprocessing/crafd/managers/crafd.py index 903e47b..efb6152 100644 --- a/views_postprocessing/crafd/managers/crafd.py +++ b/views_postprocessing/crafd/managers/crafd.py @@ -12,8 +12,9 @@ from datetime import datetime import os from views_pipeline_core.modules.dataloaders.datafactory_contract import declared_data_format -from views_postprocessing.contract import frame_extraction, gaul_lookup, historical, launch_config, source_metadata, store_metadata +from views_postprocessing.contract import frame_extraction, gaul_lookup, historical, launch_config, source_metadata from views_postprocessing.crafd import appwrite_env, product +from views_postprocessing.crafd.store_port import _ContractStorePort from views_postprocessing.contract.wire import sink as wire_sink from views_postprocessing.contract.wire import source_selection from views_postprocessing.delivery import coverage, observed_range, provenance @@ -22,60 +23,6 @@ logger = logging.getLogger(__name__) -class _ContractStorePort: - """Adapts ``DatastoreModule`` to the wire ports (ADR-013 epic #105; DIP — - ``wire/source_selection`` and ``wire/sink`` never see Appwrite types).""" - - def __init__(self, datastore: DatastoreModule) -> None: - self._dsm = datastore - - def latest_file_id(self, filters: dict): - return self._dsm.get_latest_file_id(filters=filters) - - def file_metadata(self, file_id: str) -> dict: - return store_metadata.file_metadata(self._dsm.get_file_metadata(file_id)) - - def download(self, file_id: str) -> bytes: - return ( - self._dsm.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None) - ) - - def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, description=None) -> None: - result = self._dsm.upload_data( - file=file_path, - filename=filename, - name=name, - type=doc_type, - category=category, - loa=loa, - targets=targets, - description=description, - ) - # On a metadata failure the store logs, then RETURNS success=False with the - # file already uploaded (pipeline-core modules/appwrite/file.py — the file is - # the claim; its line number moves between releases). It never raises, so a - # caller that discards the result ships an invisible orphan: run-0's historical - # artifact, 2026-07-27. This check is the whole mechanism. - # - # **Refuse unless success is explicitly True** (register C-79). The earlier - # `if success is False` failed OPEN: a result that was None, or lacked the - # attribute, or carried a non-bool, sailed through as though the upload had - # worked. Today `upload_data` has a single return path and `success` is a - # `bool` dataclass field, so the two polarities agree — but the moment that - # stops being true is exactly this entry's trigger, and fail-open is the wrong - # side to be on when the subject is "did the delivery actually land". - # - # The old `to_dict()` fallback is gone with it: dead on the real path, and an - # unrecognised result should be refused and named, not adapted to silently. - success = getattr(result, "success", None) - if success is not True: - error = getattr(result, "error", None) or "unknown store error" - raise RuntimeError( - f"upload of {filename!r} did not fully succeed (file may be an orphan " - f"without a metadata document): {error}. The store reported " - f"success={success!r} (result type {type(result).__name__})." - ) - def _build_prod_forecasts_store(ensemble_name: str | None) -> DatastoreModule: """The shared internal store (ADR-013's "shared shelf"), built from the diff --git a/views_postprocessing/crafd/store_port.py b/views_postprocessing/crafd/store_port.py new file mode 100644 index 0000000..cda03ff --- /dev/null +++ b/views_postprocessing/crafd/store_port.py @@ -0,0 +1,100 @@ +"""The prediction store behind a four-method port — the DIP seam of ADR-013 epic #105. + +``wire/source_selection`` and ``wire/sink`` drive the store through this object and +never see the client's types. That is the whole point of the seam, so the constructor +takes **any** object carrying the four methods below rather than naming a concrete +client class — the contract is the methods, not the type. + +Both refusals here are the same rule applied twice: *an unrecognised result should be +refused and named, not adapted to silently.* ``upload`` learned it as register C-79 +(2026-08-05), ``download`` as C-99 (2026-08-14) after the shape it did not check cost +views-crafdapi an evening. Tests: ``tests/test_store_port.py``. +""" + +from views_postprocessing.contract import store_metadata + + +class _ContractStorePort: + """Adapts a prediction-store client to the wire ports. + + ``datastore`` is any object exposing ``get_latest_file_id``, ``get_file_metadata``, + ``download_prediction`` and ``upload_data``. + """ + + def __init__(self, datastore) -> 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: + """Fetch a pinned artifact's bytes, refusing any result that is not bytes. + + Fail CLOSED, for the reason C-79 recorded of ``upload`` below: *an unrecognised + result should be refused and named, not adapted to silently.* Register C-99. + """ + result = self._dsm.download_prediction(file_id) + # `.get("data", {})` was the defect: when the key is PRESENT and null the default + # never applies, so `.get("file_bytes")` raised AttributeError three frames away + # in a dict comprehension, naming neither the file_id nor the fact that a + # download had failed (views-crafdapi#44, 2026-08-13 — it cost an evening). + to_dict = getattr(result, "to_dict", None) + payload = to_dict() if callable(to_dict) else None + data = payload.get("data") if isinstance(payload, dict) else None + file_bytes = data.get("file_bytes") if isinstance(data, dict) else None + + if isinstance(file_bytes, (bytes, bytearray)): + if file_bytes: + return bytes(file_bytes) + # Zero bytes is refused too: no shard, sidecar or manifest is ever empty, and + # returning b"" only moves the same failure to the parser. + observed = "'file_bytes' was present but empty (0 bytes)" + else: + observed = ( + f"the store result was {type(result).__name__}, its 'data' was " + f"{type(data).__name__}, its 'file_bytes' was {type(file_bytes).__name__}" + ) + raise RuntimeError( + f"download of file_id {file_id!r} did not return usable bytes: {observed}. " + "Refused here, where the file_id is still in hand — the caller assembles " + "these by name and cannot tell a failed download from an empty artifact." + ) + + def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, description=None) -> None: + result = self._dsm.upload_data( + file=file_path, + filename=filename, + name=name, + type=doc_type, + category=category, + loa=loa, + targets=targets, + description=description, + ) + # On a metadata failure the store logs, then RETURNS success=False with the + # file already uploaded (pipeline-core modules/appwrite/file.py — the file is + # the claim; its line number moves between releases). It never raises, so a + # caller that discards the result ships an invisible orphan: run-0's historical + # artifact, 2026-07-27. This check is the whole mechanism. + # + # **Refuse unless success is explicitly True** (register C-79). The earlier + # `if success is False` failed OPEN: a result that was None, or lacked the + # attribute, or carried a non-bool, sailed through as though the upload had + # worked. Today `upload_data` has a single return path and `success` is a + # `bool` dataclass field, so the two polarities agree — but the moment that + # stops being true is exactly this entry's trigger, and fail-open is the wrong + # side to be on when the subject is "did the delivery actually land". + # + # The old `to_dict()` fallback is gone with it: dead on the real path, and an + # unrecognised result should be refused and named, not adapted to silently. + success = getattr(result, "success", None) + if success is not True: + error = getattr(result, "error", None) or "unknown store error" + raise RuntimeError( + f"upload of {filename!r} did not fully succeed (file may be an orphan " + f"without a metadata document): {error}. The store reported " + f"success={success!r} (result type {type(result).__name__})." + ) diff --git a/views_postprocessing/unfao/managers/unfao.py b/views_postprocessing/unfao/managers/unfao.py index e787bcd..3b727f8 100644 --- a/views_postprocessing/unfao/managers/unfao.py +++ b/views_postprocessing/unfao/managers/unfao.py @@ -12,8 +12,9 @@ from datetime import datetime import os from views_pipeline_core.modules.dataloaders.datafactory_contract import declared_data_format -from views_postprocessing.contract import frame_extraction, gaul_lookup, historical, launch_config, source_metadata, store_metadata +from views_postprocessing.contract import frame_extraction, gaul_lookup, historical, launch_config, source_metadata from views_postprocessing.unfao import appwrite_env, product +from views_postprocessing.unfao.store_port import _ContractStorePort from views_postprocessing.contract.wire import sink as wire_sink from views_postprocessing.contract.wire import source_selection from views_postprocessing.delivery import coverage, observed_range, provenance @@ -22,60 +23,6 @@ logger = logging.getLogger(__name__) -class _ContractStorePort: - """Adapts ``DatastoreModule`` to the wire ports (ADR-013 epic #105; DIP — - ``wire/source_selection`` and ``wire/sink`` never see Appwrite types).""" - - def __init__(self, datastore: DatastoreModule) -> None: - self._dsm = datastore - - def latest_file_id(self, filters: dict): - return self._dsm.get_latest_file_id(filters=filters) - - def file_metadata(self, file_id: str) -> dict: - return store_metadata.file_metadata(self._dsm.get_file_metadata(file_id)) - - def download(self, file_id: str) -> bytes: - return ( - self._dsm.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None) - ) - - def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, description=None) -> None: - result = self._dsm.upload_data( - file=file_path, - filename=filename, - name=name, - type=doc_type, - category=category, - loa=loa, - targets=targets, - description=description, - ) - # On a metadata failure the store logs, then RETURNS success=False with the - # file already uploaded (pipeline-core modules/appwrite/file.py — the file is - # the claim; its line number moves between releases). It never raises, so a - # caller that discards the result ships an invisible orphan: run-0's historical - # artifact, 2026-07-27. This check is the whole mechanism. - # - # **Refuse unless success is explicitly True** (register C-79). The earlier - # `if success is False` failed OPEN: a result that was None, or lacked the - # attribute, or carried a non-bool, sailed through as though the upload had - # worked. Today `upload_data` has a single return path and `success` is a - # `bool` dataclass field, so the two polarities agree — but the moment that - # stops being true is exactly this entry's trigger, and fail-open is the wrong - # side to be on when the subject is "did the delivery actually land". - # - # The old `to_dict()` fallback is gone with it: dead on the real path, and an - # unrecognised result should be refused and named, not adapted to silently. - success = getattr(result, "success", None) - if success is not True: - error = getattr(result, "error", None) or "unknown store error" - raise RuntimeError( - f"upload of {filename!r} did not fully succeed (file may be an orphan " - f"without a metadata document): {error}. The store reported " - f"success={success!r} (result type {type(result).__name__})." - ) - def _build_prod_forecasts_store(ensemble_name: str | None) -> DatastoreModule: """The shared internal store (ADR-013's "shared shelf"), built from the diff --git a/views_postprocessing/unfao/store_port.py b/views_postprocessing/unfao/store_port.py new file mode 100644 index 0000000..cda03ff --- /dev/null +++ b/views_postprocessing/unfao/store_port.py @@ -0,0 +1,100 @@ +"""The prediction store behind a four-method port — the DIP seam of ADR-013 epic #105. + +``wire/source_selection`` and ``wire/sink`` drive the store through this object and +never see the client's types. That is the whole point of the seam, so the constructor +takes **any** object carrying the four methods below rather than naming a concrete +client class — the contract is the methods, not the type. + +Both refusals here are the same rule applied twice: *an unrecognised result should be +refused and named, not adapted to silently.* ``upload`` learned it as register C-79 +(2026-08-05), ``download`` as C-99 (2026-08-14) after the shape it did not check cost +views-crafdapi an evening. Tests: ``tests/test_store_port.py``. +""" + +from views_postprocessing.contract import store_metadata + + +class _ContractStorePort: + """Adapts a prediction-store client to the wire ports. + + ``datastore`` is any object exposing ``get_latest_file_id``, ``get_file_metadata``, + ``download_prediction`` and ``upload_data``. + """ + + def __init__(self, datastore) -> 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: + """Fetch a pinned artifact's bytes, refusing any result that is not bytes. + + Fail CLOSED, for the reason C-79 recorded of ``upload`` below: *an unrecognised + result should be refused and named, not adapted to silently.* Register C-99. + """ + result = self._dsm.download_prediction(file_id) + # `.get("data", {})` was the defect: when the key is PRESENT and null the default + # never applies, so `.get("file_bytes")` raised AttributeError three frames away + # in a dict comprehension, naming neither the file_id nor the fact that a + # download had failed (views-crafdapi#44, 2026-08-13 — it cost an evening). + to_dict = getattr(result, "to_dict", None) + payload = to_dict() if callable(to_dict) else None + data = payload.get("data") if isinstance(payload, dict) else None + file_bytes = data.get("file_bytes") if isinstance(data, dict) else None + + if isinstance(file_bytes, (bytes, bytearray)): + if file_bytes: + return bytes(file_bytes) + # Zero bytes is refused too: no shard, sidecar or manifest is ever empty, and + # returning b"" only moves the same failure to the parser. + observed = "'file_bytes' was present but empty (0 bytes)" + else: + observed = ( + f"the store result was {type(result).__name__}, its 'data' was " + f"{type(data).__name__}, its 'file_bytes' was {type(file_bytes).__name__}" + ) + raise RuntimeError( + f"download of file_id {file_id!r} did not return usable bytes: {observed}. " + "Refused here, where the file_id is still in hand — the caller assembles " + "these by name and cannot tell a failed download from an empty artifact." + ) + + def upload(self, file_path, *, filename, name, doc_type, category, loa, targets, description=None) -> None: + result = self._dsm.upload_data( + file=file_path, + filename=filename, + name=name, + type=doc_type, + category=category, + loa=loa, + targets=targets, + description=description, + ) + # On a metadata failure the store logs, then RETURNS success=False with the + # file already uploaded (pipeline-core modules/appwrite/file.py — the file is + # the claim; its line number moves between releases). It never raises, so a + # caller that discards the result ships an invisible orphan: run-0's historical + # artifact, 2026-07-27. This check is the whole mechanism. + # + # **Refuse unless success is explicitly True** (register C-79). The earlier + # `if success is False` failed OPEN: a result that was None, or lacked the + # attribute, or carried a non-bool, sailed through as though the upload had + # worked. Today `upload_data` has a single return path and `success` is a + # `bool` dataclass field, so the two polarities agree — but the moment that + # stops being true is exactly this entry's trigger, and fail-open is the wrong + # side to be on when the subject is "did the delivery actually land". + # + # The old `to_dict()` fallback is gone with it: dead on the real path, and an + # unrecognised result should be refused and named, not adapted to silently. + success = getattr(result, "success", None) + if success is not True: + error = getattr(result, "error", None) or "unknown store error" + raise RuntimeError( + f"upload of {filename!r} did not fully succeed (file may be an orphan " + f"without a metadata document): {error}. The store reported " + f"success={success!r} (result type {type(result).__name__})." + )