From 1c15a4d047ba371f89b2cef50bbc1cfcb18f7427 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Sun, 2 Aug 2026 04:33:51 +0200 Subject: [PATCH 1/2] =?UTF-8?q?feat(lookup):=20S5=20#186=20=E2=80=94=20the?= =?UTF-8?q?=20lookup=20declares=20its=20own=20version=20instead=20of=20the?= =?UTF-8?q?=20consumer=20inferring=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gaul_lookup.version() reconstructed the delivery's traceability stamp by traversing three levels of views-datafactory's ingestion-ledger shape (source_provenance -> land_gaul_region -> content_digest) inside a bare `except (ValueError, AttributeError): pass`, returning the string "unknown" when any level was absent or reshaped. A rename upstream would therefore have made every delivery untraceable, silently, in the one field C-15 exists to answer AFTER a suspect delivery. Nothing would have noticed: a provenance record was still written; it just stopped meaning anything. That is the declare-don't-infer violation ADR-003 forbids, at the consumer. The builder now composes the stamp — it is the only thing holding both the region and the producer's ledger — and writes it as one flat `lookup_version` key. The consumer reads that key and RAISES when it is absent, logging first (ADR-008). The json import and every exception handler are gone from the module. An untraceable build fails at BUILD, not at delivery. _provenance is best-effort by design; a missing ledger used to surface as a placeholder in production, and now surfaces where a human is present to fix it. Review caught the fix reintroducing its own defect class one level up. The first draft read `land_gaul_region or gaul_admin_area_majority`, and since _provenance does not take a region, land_gaul_region is present for EVERY build — so `--region all` would have been stamped `all@f74d3b2b`, the land_gaul region definition's digest, on a global artifact. Authoritative-looking and wrong, silently. Replaced by a declared `stamp_dataset(region)`: area-majority for `all`, `_region` otherwise, and a region whose entry is absent is refused rather than substituted. Verified: all -> all@272cdb01, land_gaul -> land_gaul@f74d3b2b, africa_me_legacy -> refused. The committed artifact is rebuilt. Proven metadata-only: 64,742 rows, column order and dtypes unchanged, a content hash over every column's values IDENTICAL, and the stamp resolves to land_gaul@f74d3b2b — the same string the old traversal produced, so no delivered provenance changes meaning. tests/test_gaul_lookup_fidelity.py (now 26) still compares it value-for-value against the seven datafactory parquets. ADR-013 is untouched: contract_version 1.5, fixtures byte-identical, and the sidecar cannot inherit lookup metadata because build_sidecar constructs a fresh table. Also from review: the ledger is append-only and the LAST entry per dataset wins, which was harmless while provenance was decorative and is load-bearing now that the stamp raises — stated in _provenance rather than left in the loop. Digest length is declared so producer and consumer agree on what a valid stamp is. And the GaulLookupEnricher CIC still documented the "unknown" degrade this removes. Co-Authored-By: Claude Opus 5 (1M context) --- docs/CICs/GaulLookupEnricher.md | 13 ++- scripts/build_gaul_lookup.py | 85 ++++++++++++++-- tests/test_gaul_lookup_access.py | 83 +++++++++++++++- tests/test_gaul_lookup_fidelity.py | 93 +++++++++++++++++- views_postprocessing/contract/gaul_lookup.py | 68 ++++++++----- views_postprocessing/data/gaul_lookup.parquet | Bin 902066 -> 902187 bytes 6 files changed, 302 insertions(+), 40 deletions(-) diff --git a/docs/CICs/GaulLookupEnricher.md b/docs/CICs/GaulLookupEnricher.md index a9d7ef0..4d0938e 100644 --- a/docs/CICs/GaulLookupEnricher.md +++ b/docs/CICs/GaulLookupEnricher.md @@ -62,9 +62,16 @@ views-datafactory area-majority join), so this class does only a table join. - Output: the input frame (or, with `only_metadata=True`, just `pg_id_col` + `time_id_col`) left-merged with the 9 metadata columns. - Public attribute: `lookup_version` — a short, stampable id read from the - lookup's embedded provenance at construction (`@`, or - `"unknown"` if the lookup carries none). The manager stamps it on each - delivery so a delivery is traceable to the exact lookup build. + lookup's **declared** `lookup_version` metadata key at construction + (`@<8-char source digest>`). Delegates to + `contract.gaul_lookup.version`. The manager stamps it on each delivery so a + delivery is traceable to the exact lookup build. + **It does not degrade.** An artifact carrying no declared key raises + `gaul_lookup.LookupVersionError` (logged at ERROR first, per ADR-008) rather + than returning a placeholder. Until S5 (#186) this returned the string + `"unknown"` whenever views-datafactory's ingestion-ledger shape moved under + it — silently, in the one field register C-15 exists to answer *after* a + suspect delivery. See register **C-60**. - Side effects: logs the lookup size + version at construction (INFO); logs a WARNING with the count and sample of unmatched cell ids when any occur; logs ignored mapper-only kwargs at DEBUG. No file writes, no network. diff --git a/scripts/build_gaul_lookup.py b/scripts/build_gaul_lookup.py index 7b60b44..b488cdb 100644 --- a/scripts/build_gaul_lookup.py +++ b/scripts/build_gaul_lookup.py @@ -90,8 +90,79 @@ def _region_gids(datafactory: Path, region: str) -> set[int] | None: return set(json.loads(path.read_text())) -def _provenance(datafactory: Path) -> dict: - """Pull the land_gaul ledger entry for traceability (best-effort).""" +#: The ledger dataset naming the whole GAUL area-majority join, used to stamp an +#: unregionalised (``--region all``) build. +AREA_MAJORITY_DATASET = "gaul_admin_area_majority" + +#: Length of the digest carried in a stamp. Declared so producer and consumer agree. +DIGEST_CHARS = 8 + + +def stamp_dataset(region: str) -> str: + """DECLARED: which ledger dataset's digest stamps this region's build. + + One rule, no fallback. ``--region all`` is stamped by the area-majority join that + produced every value; a regional build is stamped by that region's own definition + entry, because the region determines which rows exist and a change to it changes + the artifact. + + **Why not "whichever entry happens to be present" (register C-60, review of #186).** + The first draft read ``land_gaul_region or gaul_admin_area_majority``. Since + ``_provenance`` does not take a region, ``land_gaul_region`` is present for *every* + build — so ``--region all`` would have been stamped ``all@f74d3b2b``, the land_gaul + region definition's digest, on a global artifact. Authoritative-looking and wrong, + silently: the exact defect class C-60 exists to close, reintroduced one level up. + A region with no ledger entry is now refused, not substituted. + """ + return AREA_MAJORITY_DATASET if region == "all" else f"{region}_region" + + +def _lookup_version(region: str, provenance: dict) -> str: + """Compose the DECLARED build stamp: ``@``. + + This is the value written as a flat ``lookup_version`` key and read back verbatim + by ``contract.gaul_lookup.version`` — register **C-60**. Composing it here is the + point: the builder is the only thing holding both the region and the producer's + ledger, so the consumer stops needing to know views-datafactory's schema in order + to answer "which lookup produced this delivery?". + + **Refuses an untraceable build.** ``_provenance`` is best-effort by design — a + missing or reshaped ledger yields ``{}``. Previously that produced the string + ``"unknown"`` at *delivery* time, silently, in the one field C-15 exists to answer + after a suspect delivery. Failing here moves the error to the moment a human is + present to fix it: the build. + """ + dataset = stamp_dataset(region) + digest = str((provenance.get(dataset) or {}).get("content_digest", "")) + if len(digest) < DIGEST_CHARS: + raise LookupBuildError( + f"cannot compose a lookup_version for region {region!r}: the datafactory " + f"ingestion ledger has no usable content_digest for {dataset!r} " + f"(provenance/gaul_admin/ingestion_ledger.jsonl; need at least " + f"{DIGEST_CHARS} characters, got {len(digest)}). An artifact without a " + "declared version cannot be traced back to the build that made it, which " + "is what register C-15 needs after a suspect delivery. Rebuild against a " + "datafactory checkout whose ledger declares that dataset — this build is " + "NOT stamped from some other region's entry." + ) + return f"{region}@{digest[:DIGEST_CHARS]}" + + +def _provenance(datafactory: Path, *, datasets: tuple[str, ...]) -> dict: + """Pull the named ledger entries for traceability (best-effort). + + Kept richer than the stamp — it carries timestamps and the upstream GAUL digest, + which are worth having. What changed in C-60 is that the STAMP is no longer + derived from it by the consumer; see ``_lookup_version``. + + **The ledger is append-only and the LAST entry per dataset wins.** The real file + carries 14 ``gaul_admin_area_majority`` entries and 2 ``land_gaul_region`` ones; + this loop overwrites, so the most recent ingestion is what stamps the build. That + was harmless while provenance was decorative. Since C-60 the stamp *raises* on its + absence, so the selection rule is load-bearing and is stated here rather than left + to be inferred from the loop. (Recency-over-a-broad-match is the shape register + **C-73** was opened for on the forecast path; here it is intended, and now said.) + """ ledger = (datafactory / "provenance" / "gaul_admin" / "ingestion_ledger.jsonl") out = {} @@ -101,8 +172,7 @@ def _provenance(datafactory: Path) -> dict: e = json.loads(line) except json.JSONDecodeError: continue - if e.get("dataset") in ("land_gaul_region", - "gaul_admin_area_majority"): + if e.get("dataset") in datasets: out[e["dataset"]] = { k: e[k] for k in ("content_digest", "source_gaul_digest", "timestamp") @@ -188,8 +258,11 @@ def build(datafactory: Path, region: str, out: Path) -> pd.DataFrame: meta[b"region"] = region.encode() meta[b"n_cells"] = str(len(df)).encode() meta[b"n_dropped_incomplete"] = str(dropped).encode() - meta[b"source_provenance"] = json.dumps( - _provenance(datafactory)).encode() + prov = _provenance(datafactory, datasets=(AREA_MAJORITY_DATASET, stamp_dataset(region))) + # The DECLARED stamp: one flat key, composed here, read verbatim by the consumer + # (C-60). Key order in parquet metadata carries no meaning; this is a dict. + meta[b"lookup_version"] = _lookup_version(region, prov).encode() + meta[b"source_provenance"] = json.dumps(prov).encode() table = table.replace_schema_metadata(meta) pq.write_table(table, out) diff --git a/tests/test_gaul_lookup_access.py b/tests/test_gaul_lookup_access.py index e6be468..d0de6c6 100644 --- a/tests/test_gaul_lookup_access.py +++ b/tests/test_gaul_lookup_access.py @@ -11,9 +11,15 @@ read per run, and the read *injected* into both consumers rather than fetched by each. """ +import ast +import importlib +import inspect +import logging from pathlib import Path import pyarrow as pa +import pyarrow.parquet as pq +import pytest from views_postprocessing.contract import gaul_lookup @@ -34,13 +40,80 @@ def test_load_returns_the_arrow_table(): assert "priogrid_gid" in table.column_names -def test_version_reports_the_build_stamp(): +def test_version_reports_the_declared_build_stamp(): + """C-60: the stamp is read from one declared key, not reconstructed. + + This used to assert ``stamp != "unknown"`` — a real check while ``version()`` + could *return* that placeholder. It no longer can: the branch is gone and the + function raises instead. What is worth pinning now is the shape a delivery + traces by. + """ stamp = gaul_lookup.version() - assert stamp != "unknown", ( - "the lookup's provenance stamp did not resolve — a delivery would ship " - "untraceable provenance (register C-60)" + assert "@" in stamp, f"expected `@`, got {stamp!r}" + region, digest = stamp.split("@", 1) + assert region and len(digest) == 8, stamp + + +def test_version_raises_rather_than_degrading_when_the_key_is_absent(tmp_path): + """The load-bearing half of C-60's fix, without which it is unproven. + + An artifact built before the declared key existed — or by a future builder that + forgets it — must stop the delivery, not hand it a placeholder that reads like a + version. ``"unknown"`` in a provenance record is worse than a crash: it is + written, stored and shipped, and nothing downstream can tell it from a real stamp. + """ + unstamped = tmp_path / "no_version.parquet" + pq.write_table(pa.table({"priogrid_gid": pa.array([1, 2, 3])}), unstamped) + with pytest.raises(gaul_lookup.LookupVersionError, match="lookup_version"): + gaul_lookup.version(unstamped) + + +def test_version_logs_before_it_raises(tmp_path, caplog): + """ADR-008:48/51, matching the shape S1 (#182) gave the entry validators.""" + unstamped = tmp_path / "no_version.parquet" + pq.write_table(pa.table({"priogrid_gid": pa.array([1])}), unstamped) + with caplog.at_level(logging.ERROR): + with pytest.raises(gaul_lookup.LookupVersionError): + gaul_lookup.version(unstamped) + errors = [r for r in caplog.records if r.levelno >= logging.ERROR] + assert len(errors) == 1 and "lookup_version" in errors[0].getMessage() + + +def test_version_no_longer_knows_the_producers_ledger_schema(): + """The declare-don't-infer half: the consumer stopped traversing upstream shape. + + ``version()`` reached three levels into views-datafactory's ingestion ledger — + ``source_provenance`` → ``land_gaul_region`` → ``content_digest`` — inside a bare + ``except … pass``. A rename upstream made every delivery untraceable with no + signal. The builder composes the stamp now; this reads one key. + + **Tested by mechanism, not by word.** An earlier draft scanned the file for those + key names and failed on the docstring above, which names them in order to explain + the fix. Prose that records why a thing changed is not the thing. So this asserts + the two capabilities the traversal required and the degrade depended on: JSON + decoding of the producer's blob, and an exception handler that swallows. Neither + can be present without the defect being reachable, and neither is triggered by a + sentence. + """ + module = importlib.import_module("views_postprocessing.contract.gaul_lookup") + tree = ast.parse(inspect.getsource(module)) + + imports = { + alias.name + for node in ast.walk(tree) + if isinstance(node, ast.Import) + for alias in node.names + } + assert "json" not in imports, ( + "gaul_lookup imports json again — decoding the producer's provenance blob is " + "how the consumer came to know views-datafactory's schema (register C-60)." + ) + + handlers = [n for n in ast.walk(tree) if isinstance(n, ast.ExceptHandler)] + assert not handlers, ( + f"gaul_lookup has {len(handlers)} exception handler(s). The stamp degraded to " + '"unknown" through a bare `except … pass`; this module now declares or raises.' ) - assert "@" in stamp # `@` def test_the_delivery_reads_the_lookup_exactly_once(monkeypatch): diff --git a/tests/test_gaul_lookup_fidelity.py b/tests/test_gaul_lookup_fidelity.py index 78fe669..a451188 100644 --- a/tests/test_gaul_lookup_fidelity.py +++ b/tests/test_gaul_lookup_fidelity.py @@ -315,15 +315,28 @@ def _synthetic_source(n: int = 6): ) -def _build_with(monkeypatch, source, tmp_path): +#: A resolvable ledger entry, so a synthetic build can reach the invariant block. +#: Keyed on the area-majority dataset because ``_build_with`` builds ``region="all"``, +#: whose DECLARED stamp source is that entry — see ``builder.stamp_dataset``. +_SYNTHETIC_PROVENANCE = {"gaul_admin_area_majority": {"content_digest": "0123456789abcdef"}} + + +def _build_with(monkeypatch, source, tmp_path, provenance=None): """Run the builder against ``source``, bypassing the datafactory entirely. - ``region="all"`` short-circuits ``_region_gids`` (returns None), and - ``_provenance`` is best-effort, so no checkout is required. + ``region="all"`` short-circuits ``_region_gids`` (returns None). Provenance is + stubbed for the same reason the source is: since C-60 the builder **refuses** a + build whose ledger yields no digest, so a synthetic run needs a synthetic ledger + to reach the invariants these tests are about. That refusal is not bypassed — + it is proven directly by ``test_builder_refuses_a_build_it_cannot_stamp``. """ import scripts.build_gaul_lookup as builder monkeypatch.setattr(builder, "_load_source", lambda _df: source) + monkeypatch.setattr( + builder, "_provenance", + lambda _df, **_kw: _SYNTHETIC_PROVENANCE if provenance is None else provenance, + ) return builder.build(Path("/nonexistent"), "all", tmp_path / "lookup.parquet") @@ -384,3 +397,77 @@ def test_coord_dtypes_are_wire_stable(lookup): ) for col in COORD_COLS: assert lookup.schema.field(col).type == "double", f"{col} must be float64" + + +# ── the declared build stamp (S5 / #186, register C-60) ───────────────────── + +def test_builder_refuses_a_build_it_cannot_stamp(monkeypatch, tmp_path): + """C-60: an untraceable artifact must fail at BUILD, not degrade at delivery. + + Before this, an unresolvable ledger produced the string ``"unknown"`` in the + delivery's provenance — silently, in the one field C-15 exists to answer after a + suspect delivery. The record was still written; it just stopped meaning anything. + Moving the failure to the build puts it where a human is present to fix it. + """ + import scripts.build_gaul_lookup as builder + + with pytest.raises(builder.LookupBuildError, match="lookup_version"): + _build_with(monkeypatch, _synthetic_source(), tmp_path, provenance={}) + + +def test_the_builder_writes_the_declared_version_key(monkeypatch, tmp_path): + """The producer composes the stamp; nothing downstream reconstructs it.""" + import pyarrow.parquet as pq + + out = tmp_path / "lookup.parquet" + _build_with(monkeypatch, _synthetic_source(), tmp_path) + meta = {k.decode(): v.decode() for k, v in (pq.read_metadata(out).metadata or {}).items()} + assert meta["lookup_version"] == "all@01234567", ( + "the builder must write a flat, declared lookup_version — @" + ) + + +def test_the_committed_artifact_declares_its_version(): + """The artifact this repo actually ships, not a synthetic one.""" + from views_postprocessing.contract import gaul_lookup + + stamp = gaul_lookup.version() + assert stamp.startswith("land_gaul@"), stamp + assert "@" in stamp and len(stamp.split("@")[1]) == 8, ( + f"expected @<8-char digest>, got {stamp!r}" + ) + + +def test_the_stamp_source_is_declared_per_region_never_borrowed(): + """C-60, review of #186: the first draft borrowed whatever entry was present. + + ``_provenance`` does not take a region, so ``land_gaul_region`` is in the blob for + *every* build. A ``land_gaul_region or gaul_admin_area_majority`` fallback would + therefore have stamped ``--region all`` as ``all@`` — an + authoritative-looking claim about a global artifact, sourced from one region's + definition. That is the defect class C-60 exists to close, one level up. + """ + import scripts.build_gaul_lookup as builder + + assert builder.stamp_dataset("all") == builder.AREA_MAJORITY_DATASET + assert builder.stamp_dataset("land_gaul") == "land_gaul_region" + assert builder.stamp_dataset("africa_me_legacy") == "africa_me_legacy_region" + + # A region whose declared entry is absent is REFUSED, not silently substituted — + # even though another dataset's digest is sitting right there in the blob. + blob = {"land_gaul_region": {"content_digest": "f74d3b2bdeadbeef"}} + assert builder._lookup_version("land_gaul", blob) == "land_gaul@f74d3b2b" + with pytest.raises(builder.LookupBuildError, match="africa_me_legacy_region"): + builder._lookup_version("africa_me_legacy", blob) + + +def test_a_short_digest_is_refused_rather_than_truncated_silently(): + """Producer and consumer must agree on what a valid stamp is. + + ``[:8]`` alone would emit ``land_gaul@abcd`` from a 4-char digest — accepted by + the builder, rejected by ``test_the_committed_artifact_declares_its_version``. + """ + import scripts.build_gaul_lookup as builder + + with pytest.raises(builder.LookupBuildError, match="at least 8 characters"): + builder._lookup_version("land_gaul", {"land_gaul_region": {"content_digest": "abcd"}}) diff --git a/views_postprocessing/contract/gaul_lookup.py b/views_postprocessing/contract/gaul_lookup.py index 098d21f..6907f31 100644 --- a/views_postprocessing/contract/gaul_lookup.py +++ b/views_postprocessing/contract/gaul_lookup.py @@ -20,41 +20,63 @@ from __future__ import annotations -import json +import logging from pathlib import Path import pyarrow as pa import pyarrow.parquet as pq +logger = logging.getLogger(__name__) + +#: The single declared metadata key carrying the artifact's build stamp. +#: Written by ``scripts/build_gaul_lookup.py``; read verbatim here (C-60). +VERSION_KEY = "lookup_version" + + +class LookupVersionError(RuntimeError): + """The lookup artifact does not declare which build produced it.""" + #: The committed ADR-011 lookup. Declared public — three modules depend on it, and a #: leading underscore said the opposite (C-68). LOOKUP_PATH: Path = Path(__file__).resolve().parent.parent / "data" / "gaul_lookup.parquet" def version(path: Path | None = None) -> str: - """A short, stampable build id for the lookup, for delivery provenance (C-15). - - Format: ``@`` (e.g. ``land_gaul@f74d3b2b``) so a - delivery can be traced to the exact lookup build. - - **Known limitation, tracked as register C-60:** this reaches three levels into - views-datafactory's ingestion-ledger shape (``source_provenance`` → - ``land_gaul_region`` → ``content_digest``) and returns ``"unknown"`` if any level - is absent or reshaped — so the traceability stamp can silently degrade. Moved - verbatim from ``GaulLookupEnricher._read_version`` in #152 **without behaviour - change**; fixing it means having the builder write a flat declared key and - raising here when it is missing, which is C-60's own scope. + """The lookup's DECLARED build stamp, for delivery provenance (C-15). + + Reads one flat key — ``lookup_version`` — written by the builder, which is the + only thing holding both the region and the producer's ledger. Format: + ``@`` (e.g. ``land_gaul@f74d3b2b``), so a delivery + can be traced to the exact lookup build that made it. + + Raises: + LookupVersionError: if the artifact declares no version. It does not + degrade to a placeholder — see below. + + **Why it raises (register C-60).** This function used to reconstruct the stamp + by traversing three levels of views-datafactory's ingestion-ledger shape + (``source_provenance`` → ``land_gaul_region`` → ``content_digest``) inside a + bare ``except … pass``, returning the string ``"unknown"`` when any level was + absent or reshaped. A rename upstream would therefore have made every delivery + untraceable, silently, in the one field C-15 exists to answer *after* a suspect + delivery — and no gate would have noticed, because a provenance record was + still written; it just stopped meaning anything. + + The consumer no longer knows the producer's schema. The builder declares; this + reads. ADR-003. """ - meta = pq.read_metadata(path or LOOKUP_PATH).metadata or {} - meta = {k.decode(): v.decode() for k, v in meta.items()} - region = meta.get("region", "?") - digest = "?" - try: - prov = json.loads(meta.get("source_provenance", "{}")) - digest = (prov.get("land_gaul_region", {}).get("content_digest", "?"))[:8] - except (ValueError, AttributeError): - pass - return "unknown" if region == "?" and digest == "?" else f"{region}@{digest}" + raw = pq.read_metadata(path or LOOKUP_PATH).metadata or {} + declared = raw.get(VERSION_KEY.encode()) + if not declared: + err_msg = ( + f"the GAUL lookup at {path or LOOKUP_PATH} declares no {VERSION_KEY!r} — " + "a delivery built from it could not be traced back to the build that " + "produced it (register C-15). Rebuild the artifact with " + "scripts/build_gaul_lookup.py, which writes the key (C-60)." + ) + logger.error(err_msg) # ADR-008: logged persistently AND raised + raise LookupVersionError(err_msg) + return declared.decode() def load(path: Path | None = None) -> pa.Table: diff --git a/views_postprocessing/data/gaul_lookup.parquet b/views_postprocessing/data/gaul_lookup.parquet index 9c28ccbad39524532515327ecf1dabddfdc03c3b..307ac497a30e790223fd22d0301d07ce1f509a7c 100644 GIT binary patch delta 2612 zcma)8&5zsE7N^jmg>v}@jYH*37)vNV~^9ZXOdtaXP|%pOLp$L_k5qC8ee>xYK82o6DP4#6=#hGKl~S9E zb*fRimG0N;w3>0iIm%Fx9-a53I;UaR!=sq?kOpz12W(PI;k=6)swf8EUHDekin-h* zPB(y-kzsl0JAzVhmCB~07Pj56vgt+UrX>_M+fgNHTaB_7Z1pyhTHp%hywfV@EupdP z2# zs{t*p2KC8G;7#;tePZ~-SeLhAU8+@9BXG$^w^PUK~J=yCe@~H(M)W?>}J9B+pSLM73==0PyJQ75!sQQw_A480V<`eS5?&{@I(Fl5i5zs5TOkaf8?^=0C7o-ugN|%js$AW;(O6)}y0TQz5Mn;I| zD{D~U1Xq|@_&`;tI)U>>>PJly&i%y z&M=Hhw`_5(d{vKI8S@GME8FVD}^&xp&g)&2i0nd@gxlOa7t~7f9jlyldcnRDIWe0)* zShP~jp4#bds1Q@J#$LHePM`XX?lb38a{3-;=n6_6s>7}wlEsC{RV$n9jRTTfh@C>E z%#1y9_E?!+*(0MzBETP9LT?au+m%E8WX6-e&gA0XfZ{`jVVTrMvs&%oBf=7r@rsvop-TL0(|Ld`j*=F|h=U zko|X`Ja^oUlpS_;pFF|7y+K|(H#M62zp;O20dpE2Iq(1@2Za24FCIERci{s23?$jL zeex)}@E%JJ$tm{W0XfIs|BO7wv;)$|)xGTI0Xfeu9RlA^2V~oC)|1udKPKx4PJf0X zmyOyja|m7PZ}`|p`^4JKz+yG>0A{~qL3t1`%R9W++0fr?t1<`v@^yM+8lM_6Ydp$Y!6cfl^`9J?pH)U`SX)^+_6R*g7%g5N-;gY0FcQs+7{I6)L4dw8CEhB;5l@&tCuRCcB9p$Ln3eA-j%$HoH#1 zcGhc6@@c!;{Zq%KRtptaRV{))=tw(thDxHm)5E8RM}{-Qqr+pvsiqb#mdztt&T(Fg5t> z`3OF}x=JZ6z>CiUr3sp<(=4TRny&M`RO%7FkDHl&{!&2>X^MK3E*C&tlJUeAisg|G zPk>)a)BVel$W@yPAiy`Mj{@_uS8Y`FUd_;poi#mKGb)N}l*NvrZ*AM28(m*vpWGzp zPpOU_7_nkgm9m~DnT{IM8tMw0{gS*gP*)OhwWLPXlJS-*(K&dVwH3cEd%cF`6>DZJ zD_B`EF!kz|6-zXinrK(swpv9O-kOhC+UWP=JYj$4Qu6pIJp%0Rd ziG|FSpsrVWNy7}I0_Amsx&l90GeODDr(82DZdsUtN~&#EiY~IL!u3~i8S3+8Oq1-W z${}GRr(20>u9fTc>7s-73_93X*uYiW(32Zsin#3@Y*O1Rx7KvA3mB;*{J29Q{PM^z ztQJ=mOSGpJsDw1YAwaAF5n>KEwE~{Sqw4tvV65E)$ZV9%N~31P#CBB|x^^JX$=P-j z(gU@w25UOj-`$YiXj@Jzjuq<(7T^pOLC+24JAj8|!_rp~8(qL-n{Dop*{5nA6dPFa^+0hk;Q*Pg zi7uvmZj=92ReN#9rCRlxkyfBquAMTKDq?Kw_;^+|I zIxS;t-kM%+LuCOHg3DocK!)AFOU}-ER!(eGC1dPk7z6uwhn!{3F1f_6+$PVm_6|9D z(gB=xt9d0^iMf4C5F2X1ez{AYV}I_F*V(UoB*DJ8LuT0dT|(I95jn%&*dsGjS*DGM z&)4i*Wc`S&YalsQX8(-H+tZFJz)=tOh=2#o?&}_7OCz$4nzQ>#$JoCk(m&_W0B?~M zSD<8y3*pYmIkREqHMdq_pO44|)IE-B7s2sKiCQVV{S8}S?~cf6Cfp^b*iU!JQTFo= zd6E4wf}lUTL(V)CjKK>390TmN+oUty(!lM75#erUi zyI*WnbD4dBjjP_M=8FJQMX2V=yz=Yu5Djo+Kwb>S=xKvY0L?KuE^HFkffcvI{^FGb zJLYa%O+f@=uaDP|AL7Ve5bVpXXxPH@<9xxbJ!Q<)@-5RhQ=@k#Jh^Qx=epltjkv zk(XI!L{417{k((&2^B(P>f^?I_V+#VQqdoy0wraP1Cqj9640ShjF3N)9NFfvQ(D9g zi6ZSx{3*!XAB9ElkxNmIQUb}V@cN)goxaNMe?z31_s$ Date: Sun, 2 Aug 2026 04:34:34 +0200 Subject: [PATCH 2/2] =?UTF-8?q?chore(register):=20S5=20#186=20=E2=80=94=20?= =?UTF-8?q?resolve=20C-60,=20and=20record=20that=20the=20fix=20reproduced?= =?UTF-8?q?=20its=20own=20defect?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The entry closes citing the tests, with the sidecar-stamping residual deferred behind a named trigger (the next ADR-013 version bump) rather than a vague later. Kept visible in the resolution: the first draft of the fix reintroduced C-60's own defect class — a silent fallback that would have stamped `--region all` with the land_gaul region's digest. The reflex that produced the concern reappeared while fixing the concern, and that is worth more to a future reader than a clean narrative. Cluster K now has one entry left: C-46. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 58 ++++++++++++++++++------------ 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index b0c1c7c..457a3cc 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -6,8 +6,8 @@ | Owner | Dylan Pinheiro / PRIO MD&D Team | | Last Updated | 2026-08-02 | | Total Concerns | 74 | -| Open Concerns | 21 | -| Resolved Concerns | 53 | +| Open Concerns | 20 | +| Resolved Concerns | 54 | --- @@ -71,7 +71,7 @@ covered a single open entry (see Historical clusters below). **The lesson, and it generalises past this cluster.** C-43's residual said the forward-check *"was a one-off session result, not a standing guarantee"* — and the fix was to attach it to something the interpreter runs. The entries then reproduced the identical error one level up: they stated their closing conditions in prose and attached them to nothing. S2 therefore added a closing-condition check to `test_register_integrity.py`. **A guarantee needs a check, and that applies to the register's own guarantees too.** -**Remaining in this cluster:** **C-60** (the flat declared `lookup_version` key — S5 / #186) and **C-46** (the hardcoded datafactory path — S7 / #188). Both unblocked, both in epic #181. +**C-60 closed 2026-08-02 (S5 / #186)** — the flat declared key landed and the committed artifact was rebuilt, metadata-only. **Remaining in this cluster: C-46 alone** (the hardcoded datafactory path — S7 / #188), which is the last thing keeping the cluster open. ### Cluster L: The won migration was never cleaned up **Root cause:** the frame-native contract path replaced the pandas path and **won** — run-0 delivered global-land on 2026-07-27 and FAO has been served from it since. The replaced path was deliberately kept behind a config fork "until run 0 proves the contract path live" (C-40) and was then never removed. Everything below is residue of that one omission, not independent defects. @@ -454,26 +454,6 @@ Cross-refs: C-57 (registry drift — the most likely way a coordinate goes wrong --- -### C-60: The lookup provenance stamp reaches into the producer's ledger schema and degrades to `"unknown"` on a bare except - -| Field | Value | -|-------|-------| -| ID | C-60 | -| Tier | 3 | -| Source | `expert-code-review` (2026-07-31) — Ousterhout lens (information leakage / silent degradation) | -| Trigger | When views-datafactory renames or restructures its ingestion-ledger entries (`dataset` key, `content_digest` field, or the `land_gaul_region` entry name) — verify `lookup_version` still resolves to a real value rather than the string `"unknown"`; nothing fails if it does not | -| Location | `views_postprocessing/unfao/enrichment.py:61-79` (`_read_version`); written at `scripts/build_gaul_lookup.py:89-107` (`_provenance`) and `:163-164`; consumed as the C-15 provenance field via the manager's delivery description | - -`_read_version` reconstructs the stamp by traversing three levels of the producer's schema — parquet metadata → `source_provenance` JSON → `land_gaul_region` → `content_digest` — and wraps the traversal in `except (ValueError, AttributeError): pass`, returning `"unknown"` when anything along the path is absent. This is a **declare-don't-infer violation at the consumer**: the value that ties a delivered artifact to the exact lookup build (C-15's traceability provenance) can silently become a placeholder, and no gate notices. - -No wrong data results — this is a traceability failure, not a correctness one → **Tier 3**. But it defeats the specific question C-15 exists to answer *after* a suspect delivery ("which lookup produced this?"), and C-22 has no correction procedure that could compensate. - -**Mitigation:** have `build_gaul_lookup.py` write a **flat, declared `lookup_version` key** into the parquet metadata, and have `_read_version` read that one key and **raise** if absent. The consumer stops knowing the producer's nested ledger schema, and the stamp stops being able to vanish quietly. Separately worth stamping `lookup_version` into the sidecar's own parquet metadata so a delivered artifact is self-describing without the store document. - -Cross-refs: C-15 (the provenance this field serves), C-22 (the recall process that would need it), C-57 (the same class — a cross-repo fact this repo reads without a way to detect drift), **Cluster K**. - ---- - ### C-62: The pinned pipeline-core release still installs geopandas and torch into a repo that architecturally excised them | Field | Value | @@ -694,6 +674,38 @@ See also C-40 (the inheritance/representation coupling this migration unwinds), ## Resolved Concerns +### C-60: The lookup provenance stamp reaches into the producer's ledger schema and degrades to `"unknown"` on a bare except — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-60 | +| Resolved | 2026-08-02 | +| Resolution | **Closed by S5 (#186).** The builder composes the stamp and writes one flat `lookup_version` key; `contract/gaul_lookup.version` reads that key and **raises** `LookupVersionError` (logged first, ADR-008) when it is absent. The three-level traversal of views-datafactory's ledger shape, the bare `except … pass` and the `"unknown"` branch are all gone — pinned by `tests/test_gaul_lookup_access.py`, which asserts by **AST** rather than by word-matching that the module imports no `json` and holds zero exception handlers. An untraceable *build* now fails at build time, where a human is present, instead of surfacing as a placeholder in production. + +**The committed artifact was rebuilt, metadata-only.** 64,742 rows, column order and dtypes unchanged, a content hash over every column's values identical, and the stamp resolves to `land_gaul@f74d3b2b` — the same string the old traversal produced, so no delivered provenance changes meaning. ADR-013 untouched: `contract_version` 1.5, `tests/fixtures/` byte-identical (`build_sidecar` constructs a fresh table, so lookup metadata cannot reach the wire — verified empirically, sidecar schema metadata is `[]`). + +**⚠ The fix reintroduced this entry's own defect class, caught in review before merge.** The first draft read `provenance.get("land_gaul_region") or provenance.get("gaul_admin_area_majority")`. Since `_provenance` takes no region, `land_gaul_region` is present for *every* build — so `--region all` would have been stamped `all@f74d3b2b`, the land_gaul region definition's digest, on a global artifact: authoritative-looking, wrong, silent. Replaced by a declared `stamp_dataset(region)` with no fallback; a region whose ledger entry is absent is **refused**, not substituted. Verified: `all@272cdb01`, `land_gaul@f74d3b2b`, `africa_me_legacy` → raises. Worth keeping visible — the reflex that produced C-60 reappeared while fixing C-60. + +**Residual, deliberately deferred:** stamping `lookup_version` into the **sidecar's own** parquet metadata, so a delivered artifact is self-describing without the store document. Not done here because it changes wire bytes and therefore `contract_version` — an ADR-013 amendment with three repos to notify. Named trigger: **the next ADR-013 version bump**, whatever prompts it. + +**Also recorded from review:** the ingestion ledger is append-only and the **last entry per dataset wins** (14 `gaul_admin_area_majority` entries, 2 `land_gaul_region`). Harmless while provenance was decorative; load-bearing now that the stamp raises, so it is stated in `_provenance` rather than left implicit — the same shape **C-73** was opened for on the forecast path, intended here and now said. `docs/CICs/GaulLookupEnricher.md` still documented the `"unknown"` degrade and was corrected. | +| Tier | 3 | +| Source | `expert-code-review` (2026-07-31) — Ousterhout lens (information leakage / silent degradation) | +| Trigger | When views-datafactory renames or restructures its ingestion-ledger entries (`dataset` key, `content_digest` field, or the `land_gaul_region` entry name) — verify `lookup_version` still resolves to a real value rather than the string `"unknown"`; nothing fails if it does not | +| Location | `views_postprocessing/unfao/enrichment.py:61-79` (`_read_version`); written at `scripts/build_gaul_lookup.py:89-107` (`_provenance`) and `:163-164`; consumed as the C-15 provenance field via the manager's delivery description | + +`_read_version` reconstructs the stamp by traversing three levels of the producer's schema — parquet metadata → `source_provenance` JSON → `land_gaul_region` → `content_digest` — and wraps the traversal in `except (ValueError, AttributeError): pass`, returning `"unknown"` when anything along the path is absent. This is a **declare-don't-infer violation at the consumer**: the value that ties a delivered artifact to the exact lookup build (C-15's traceability provenance) can silently become a placeholder, and no gate notices. + +No wrong data results — this is a traceability failure, not a correctness one → **Tier 3**. But it defeats the specific question C-15 exists to answer *after* a suspect delivery ("which lookup produced this?"), and C-22 has no correction procedure that could compensate. + +**Mitigation:** have `build_gaul_lookup.py` write a **flat, declared `lookup_version` key** into the parquet metadata, and have `_read_version` read that one key and **raise** if absent. The consumer stops knowing the producer's nested ledger schema, and the stamp stops being able to vanish quietly. Separately worth stamping `lookup_version` into the sidecar's own parquet metadata so a delivered artifact is self-describing without the store document. + +Cross-refs: C-15 (the provenance this field serves), C-22 (the recall process that would need it), C-57 (the same class — a cross-repo fact this repo reads without a way to detect drift), **Cluster K**. + +--- + +--- + ### C-03: Test coverage gaps in manager validation and the enrich→validate path — RESOLVED | Field | Value |