diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 54459bb..bef03a1 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 | 19 | -| Resolved Concerns | 55 | +| Open Concerns | 18 | +| Resolved Concerns | 56 | --- @@ -63,7 +63,7 @@ covered a single open entry (see Historical clusters below). **Entries:** C-59, C-61 (build-time guarantees unenforced), C-60 (provenance stamp can silently degrade), C-46 (the one cross-repo check is CI-skipped on a hardcoded path), C-43 (the value-correctness debt this cluster's fix discharges) **Highest tier:** 3 (C-59, C-60, C-61 — C-59 recalibrated 2→3 on 2026-07-31 mutation evidence) **Fix strategy:** one test file — `tests/test_gaul_lookup_fidelity.py` — split into an always-on half (gid uniqueness, region-set equality, coordinate formula against a committed ground-truth sample, no nulls, no `-1` codes) and a `skipif`-gated half comparing all 7 GAUL columns against the datafactory sibling. Plus two one-line hardenings in `scripts/build_gaul_lookup.py`: assert index uniqueness, and convert the bare `assert`s to explicit raises. -**Resolution scope:** Full for C-59/C-61/C-43's residual guard; partial for C-60 (needs the flat declared `lookup_version` key) and C-46 (needs the hardcoded path removed). +**Resolution scope:** Full for all five. C-59/C-61/C-43 discharged by the one test file this strategy predicted; C-60 by the flat declared `lookup_version` key (S5 / #186); C-46 by one declared way of resolving the producer's checkout (S7 / #188). **✅ MOSTLY CLOSED 2026-08-02 (S2 / #183).** The prediction held: **one test file discharged three entries.** `tests/test_gaul_lookup_fidelity.py` (18 tests) closed **C-43**, **C-59** and **C-61** together, exactly as the fix strategy above said it would — the always-on half against the committed artifact, the `skipif` half against views-datafactory, plus the builder's bare `assert`s converted to `LookupBuildError` raises. @@ -71,7 +71,11 @@ 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.** -**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. +**✅ CLOSED 2026-08-02 (epic #181).** All five entries resolved: C-59 and C-61 (build-time guarantees, S2), C-43 (the value-correctness debt, S2), C-60 (the declared `lookup_version`, S5) and C-46 (the machine-specific path, S7). + +**The fix strategy predicted the shape correctly** — *"one test file, split into an always-on half and a `skipif`-gated half, plus two one-line hardenings in the builder"* — and that is what discharged three of the five. What it did not anticipate is that the cluster's own tooling was part of the problem: the gated half it prescribed could not run for anyone but the maintainer (C-46), and the provenance stamp it relied on could silently become the string `"unknown"` (C-60). A cluster about *"the artifact is trusted but unverified"* had a verification apparatus that was itself partly unverifiable. + +**What remains is not this repo's:** the artifact is now checked value-for-value against views-datafactory's parquets, but whether the **producer's** area-majority join is correct at high latitudes is views-datafactory#387. C-43's scope split holds — transcription fidelity is proven, assignment correctness is not ours to prove. ### 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. @@ -386,24 +390,6 @@ This entry's trigger holds the bump on **two** conditions. Their status has dive --- -### C-46: `test_datafactory_deploy_readiness` is hardcoded to a local path — CI-skipped, and currently failing on the one machine that runs it `[backlog]` - -| Field | Value | -|-------|-------| -| ID | C-46 | -| Tier | 4 | -| Source | `repo-assimilation` (2026-06-27) | -| Trigger | When treating `test_datafactory_deploy_readiness` as a release gate (it never runs in CI), or when a contributor's local `pytest` fails on it — re-promote / re-pin the strict-xfail now that views-datafactory has advanced to `1.5.0`-dev past its `v1.4.0` tag | -| Location | `tests/test_datafactory_deploy_readiness.py` (`_DF = Path("/home/simon/.../views-datafactory")`, `skipif(not _DF.exists())`) | - -The cross-repo deploy-readiness gates introduced under C-36 are guarded by `skipif` on a **hardcoded local datafactory checkout path**, so they are **skipped in CI** and only ever execute on one developer's machine. There, `test_version_bumped_past_latest_tag` is currently **failing**: it is an `xfail(strict)` that flipped to XPASS because datafactory moved to `1.5.0`-dev past its `v1.4.0` tag — exactly the auto-flip C-36's resolution anticipated, but because of the hardcoded path the flip surfaces as a **local red** rather than a CI signal, and breaks local `pytest` runs (the suite is run with this test deselected). No correctness/reliability impact on the delivery → **Tier 4** (test hygiene). C-36 (resolved) converted these gates to strict-xfail but did not capture the local-path / CI-skip dimension. - -See also C-36 (the resolved strict-xfail conversion this extends), C-44 (the datafactory version-state coupling). - -**Tagged `[backlog]` during review-rr (2026-07-31):** Tier 4, single-machine scope, mechanical fix. Kept in the register for completeness rather than active risk management — see the Register Conventions note on the `[backlog]` tag. - ---- - ### C-58: A wrong Appwrite coordinate auto-provisions a new empty target instead of raising — both client lineages, on every write | Field | Value | @@ -650,6 +636,43 @@ See also C-40 (the inheritance/representation coupling this migration unwinds), ## Resolved Concerns +### C-46: `test_datafactory_deploy_readiness` is hardcoded to a local path — CI-skipped, and currently failing on the one machine that runs it — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-46 | +| Resolved | 2026-08-02 | +| Resolution | **Closed by S7 (#188).** Four sites resolved views-datafactory four different ways; one of them was `Path("/home/simon/Documents/scripts/views_platform/views-datafactory")`, so the repo's only cross-repo release gate had **never run anywhere but one laptop**. All four now go through one declared resolution — `$VIEWS_DATAFACTORY`, else the conventional directory beside this repo, never an absolute path to a particular machine. + +| site | was | +|---|---| +| `tests/test_datafactory_deploy_readiness.py` | an absolute path to one developer's home directory | +| `tests/test_gaul_lookup_fidelity.py` | its own `_REPO.parent / "views-datafactory"` | +| `tests/test_delivery_coverage.py` | its own `parents[2] / …` (a fourth site, found while doing the work — the issue listed three) | +| `scripts/build_gaul_lookup.py` | the shape that was already right, kept | + +Verified 2026-08-02: `grep -rn "/home/" tests/ scripts/ views_postprocessing/ --include=*.py` → **0**. With the sibling present the deploy gate runs (1 passed, 3 xfailed — its deliberately-tuned `xfail`s untouched); without it, 13 clean skips and no errors. Every skip names `VIEWS_DATAFACTORY` **and** the conventional path, so a contributor can run the test rather than watch it skip. + +**The builder keeps its own resolver, deliberately.** A script must not import from `tests/` — that is the dependency direction backwards — and the contracts genuinely differ: the script returns a `Path` even when the checkout is absent so `main` can raise naming both the flag and the variable, while the test helper returns `None` because a missing sibling is a normal skip. WET before DRY: two copies that are understood beat one abstraction that is guessed. What is guarded instead is the property that actually matters — that they **agree** — because a builder writing from one checkout while the tests verify against another would report success on a lookup compared to a producer it was not built from. + +**Residual, and the decision it needs.** These checks still do not run in CI. There are three such gated groups now — this deploy gate, the fidelity suite's producer-comparison half, and **C-57**'s registry-drift half — and the question should be answered once for all three rather than three times. + +**Recommendation, for whoever takes it:** do **not** add sibling checkouts to the per-PR workflow. It couples this repo's CI to another repo's default branch, so an unrelated upstream commit turns this repo red — precisely the flapping `TestReleaseGate` already documents and was re-pinned for once. The always-on halves already guard the committed artifact; the gated halves answer *"is the producer's current state still consistent with ours?"*, which is a **scheduled** cross-repo question, not a merge gate. If it is wanted, the vehicle is a weekly workflow that opens an issue on divergence. **Named trigger:** the next time an upstream change reaches FAO through this repo without anyone noticing first. | +| Tier | 4 | +| Source | `repo-assimilation` (2026-06-27) | +| Trigger | When treating `test_datafactory_deploy_readiness` as a release gate (it never runs in CI), or when a contributor's local `pytest` fails on it — re-promote / re-pin the strict-xfail now that views-datafactory has advanced to `1.5.0`-dev past its `v1.4.0` tag | +| Location | `tests/test_datafactory_deploy_readiness.py` (`_DF = Path("/home/simon/.../views-datafactory")`, `skipif(not _DF.exists())`) | + +The cross-repo deploy-readiness gates introduced under C-36 are guarded by `skipif` on a **hardcoded local datafactory checkout path**, so they are **skipped in CI** and only ever execute on one developer's machine. There, `test_version_bumped_past_latest_tag` is currently **failing**: it is an `xfail(strict)` that flipped to XPASS because datafactory moved to `1.5.0`-dev past its `v1.4.0` tag — exactly the auto-flip C-36's resolution anticipated, but because of the hardcoded path the flip surfaces as a **local red** rather than a CI signal, and breaks local `pytest` runs (the suite is run with this test deselected). No correctness/reliability impact on the delivery → **Tier 4** (test hygiene). C-36 (resolved) converted these gates to strict-xfail but did not capture the local-path / CI-skip dimension. + +See also C-36 (the resolved strict-xfail conversion this extends), C-44 (the datafactory version-state coupling). + +**Was tagged `[backlog]` at review-rr (2026-07-31)** — Tier 4, single-machine scope, mechanical fix — and kept for completeness rather than active risk management. The tag was dropped when S7 resolved it. Worth noting for the convention itself: a `[backlog]` entry is deprioritised, not dormant, and this one turned out to be blocking three gated cross-repo checks from running anywhere but one machine. + +--- + +--- + ### C-57: PLATFORM-001 coordinate registry is referenced by URL, so nothing detects drift between it and this repo's declared environment — RESOLVED | Field | Value | diff --git a/scripts/build_gaul_lookup.py b/scripts/build_gaul_lookup.py index b488cdb..46eb21d 100644 --- a/scripts/build_gaul_lookup.py +++ b/scripts/build_gaul_lookup.py @@ -52,6 +52,22 @@ def _resolve_datafactory() -> Path: Order: $VIEWS_DATAFACTORY, then the sibling repo next to this one (views_platform/views-datafactory). Overridable with --datafactory. + + **Deliberately duplicated with** ``tests/conftest.sibling_repo`` (S7 / #188, + register C-46). Not an oversight and not laziness: + + - a script must not import from ``tests/`` — that is the dependency direction + backwards, and it would make the build depend on the test tree; + - the contracts differ. This returns a ``Path`` **even when the checkout is + absent**, so ``main`` can raise its own message naming both the flag and the + variable. The test helper returns ``None``, because a missing sibling is a + normal skip, not an error. + + Two copies that are understood beat one abstraction that is guessed. What is + guarded instead is the thing that actually matters — that they **agree** — + pinned by ``tests/test_gaul_lookup_fidelity.py``. If they ever resolve to + different checkouts, a rebuilt artifact would be verified against a producer it + was not built from. """ env = os.environ.get("VIEWS_DATAFACTORY") if env: diff --git a/tests/test_datafactory_deploy_readiness.py b/tests/test_datafactory_deploy_readiness.py index b1792b4..f8ffa8d 100644 --- a/tests/test_datafactory_deploy_readiness.py +++ b/tests/test_datafactory_deploy_readiness.py @@ -7,18 +7,29 @@ until the datafactory deploy candidate is actually releasable and the served artifact matches the branch. -Point _DF at the local datafactory checkout to run. +Resolution is the repo's one declared way of finding a sibling checkout +(``tests/conftest.sibling_repo``): ``$VIEWS_DATAFACTORY``, else the conventional +directory beside this repo. Never an absolute path to a particular machine — that +was C-46, and it kept this gate from ever running anywhere but one laptop. """ import json import subprocess -from pathlib import Path import pytest -_DF = Path("/home/simon/Documents/scripts/views_platform/views-datafactory") -_pytestmark = pytest.mark.skipif(not _DF.exists(), reason="datafactory checkout not present") -pytestmark = _pytestmark +from tests.conftest import sibling_repo + +_DF = sibling_repo("views-datafactory") +pytestmark = pytest.mark.skipif( + _DF is None, + reason=( + "views-datafactory checkout not found — set VIEWS_DATAFACTORY=/path/to/" + "views-datafactory, or place it alongside this repo. Until S7 (#188) this " + "module hardcoded an absolute path to one developer's machine, so the only " + "cross-repo release gate in the repo could not run anywhere else (C-46)." + ), +) def _git(*args: str) -> str: diff --git a/tests/test_delivery_coverage.py b/tests/test_delivery_coverage.py index cebab92..716e194 100644 --- a/tests/test_delivery_coverage.py +++ b/tests/test_delivery_coverage.py @@ -4,10 +4,11 @@ """ import json -from pathlib import Path import pytest +from tests.conftest import sibling_repo + from views_postprocessing.delivery.coverage import ( EXPECTED_CELLS, CoverageError, @@ -93,12 +94,16 @@ def test_no_excluded_cells_is_noop_when_region_unpinned(): # Cross-check the frozen manifest against the live producer when its checkout is present # (CI has no sibling → skip). This is the drift tripwire C-30 asks for. -_DF = Path(__file__).resolve().parents[2] / "views-datafactory" / "src" / "datafactory_query" +_DATAFACTORY = sibling_repo("views-datafactory") +_DF = None if _DATAFACTORY is None else _DATAFACTORY / "src" / "datafactory_query" @pytest.mark.skipif( - not (_DF / "land_pgids.json").exists(), - reason="views-datafactory sibling checkout not present", + _DF is None or not (_DF / "land_pgids.json").exists(), + reason=( + "views-datafactory checkout not found — set VIEWS_DATAFACTORY=/path/to/" + "views-datafactory, or place it alongside this repo" + ), ) def test_manifest_matches_datafactory_land_minus_land_gaul(): land = {int(x) for x in json.loads((_DF / "land_pgids.json").read_text())} diff --git a/tests/test_gaul_lookup_fidelity.py b/tests/test_gaul_lookup_fidelity.py index a451188..870672d 100644 --- a/tests/test_gaul_lookup_fidelity.py +++ b/tests/test_gaul_lookup_fidelity.py @@ -33,12 +33,14 @@ from __future__ import annotations import json +import os from pathlib import Path import numpy as np import pyarrow.parquet as pq import pytest +from tests.conftest import sibling_repo from views_postprocessing.delivery import coverage from views_postprocessing.contract.gaul_schema import ( CODE_COLS, @@ -57,13 +59,17 @@ # The lookup is built for this region; its count is pinned in delivery/coverage.py. _REGION = "land_gaul" -_DATAFACTORY = _REPO.parent / "views-datafactory" -_HAS_DATAFACTORY = (_DATAFACTORY / "data" / "raw" / "gaul_admin").is_dir() +_DATAFACTORY = sibling_repo("views-datafactory") +_HAS_DATAFACTORY = _DATAFACTORY is not None and ( + _DATAFACTORY / "data" / "raw" / "gaul_admin" +).is_dir() _needs_datafactory = pytest.mark.skipif( not _HAS_DATAFACTORY, reason=( - "views-datafactory sibling checkout not present — the producer-comparison half " - "cannot run here. The always-on tests in this module still guard the artifact." + "views-datafactory checkout not found — set VIEWS_DATAFACTORY=/path/to/" + "views-datafactory, or place it alongside this repo. Only the " + "producer-comparison half is skipped; the always-on tests still guard the " + "committed artifact." ), ) @@ -471,3 +477,37 @@ def test_a_short_digest_is_refused_rather_than_truncated_silently(): with pytest.raises(builder.LookupBuildError, match="at least 8 characters"): builder._lookup_version("land_gaul", {"land_gaul_region": {"content_digest": "abcd"}}) + + +def test_the_builder_and_the_tests_resolve_the_same_datafactory(): + """The one thing worth guarding about the deliberate duplication (S7 / #188). + + ``scripts/build_gaul_lookup._resolve_datafactory`` and + ``tests/conftest.sibling_repo`` are separate on purpose — a script must not import + from ``tests/``, and their contracts differ (a Path even when absent, versus None). + Register **C-46** was never about there being two functions; it was about one of + them being an absolute path to a single machine. + + What must not drift is the **answer**. If the builder wrote an artifact from one + checkout while these tests verified it against another, the fidelity suite would be + comparing a lookup to a producer it was not built from — and reporting success. + """ + import scripts.build_gaul_lookup as builder + + # The FALLBACK is the only place they can diverge — both read $VIEWS_DATAFACTORY + # first — and it is exercised precisely when no checkout exists. So compare the + # computed paths unconditionally: gating this on a checkout being present would + # skip the one case the test is for, and skip it in CI, where it matters most. + if "VIEWS_DATAFACTORY" not in os.environ: + assert builder._resolve_datafactory() == _REPO.parent / "views-datafactory", ( + "the builder's fallback and the tests' fallback resolve different " + "directories; with no environment override they would disagree silently" + ) + + resolved = sibling_repo("views-datafactory") + if resolved is None: + return # nothing further to compare; the fallback agreement is asserted above + assert builder._resolve_datafactory().resolve() == resolved.resolve(), ( + "the builder and the tests resolve different views-datafactory checkouts; a " + "rebuilt artifact would be verified against a producer it was not built from" + )