From 42acf0b03b010d713f71739982533bca558f1c3b Mon Sep 17 00:00:00 2001 From: Polichinl Date: Sun, 2 Aug 2026 06:18:52 +0200 Subject: [PATCH] =?UTF-8?q?test(siblings):=20S7=20#188=20=E2=80=94=20one?= =?UTF-8?q?=20declared=20way=20to=20find=20views-datafactory;=20Cluster=20?= =?UTF-8?q?K=20closes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four sites resolved the producer's checkout four different ways, and one of them was an absolute path to a single developer's home directory. The repo's only cross-repo release gate had therefore never run anywhere but that laptop — which is what register C-46 was open for, and why "it is skipped in CI" understated it. All four now resolve the same way: $VIEWS_DATAFACTORY, else the conventional directory beside this repo, never an absolute path to a particular machine. Every skip names the variable AND the path, so a contributor can run the test rather than watch it skip. A fourth site turned up while doing the work — tests/test_delivery_coverage.py's drift tripwire had its own parents[2] walk. The issue listed three. Leaving it would have made a closure titled "one declared way" false. The builder keeps its own resolver, deliberately and now argued in writing. 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. What is guarded is not the duplication but the property that matters: that the two AGREE. 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. Review sharpened that guard. It first skipped when no checkout was present — which is exactly when the fallback path is exercised, so the one case it existed for was the case it declined to check, and it declined in CI where it matters most. The fallback paths are now compared unconditionally; mutation-proven by pointing the builder at a different directory. Review also caught this change contradicting itself in the register: Cluster K's resolution-scope line still called C-60 and C-46 "partial" eight lines above the note closing them, and C-46's body still asserted a [backlog] state that ended on resolution. Both fixed. Cluster K closes: five entries, all full. Its own note records what the strategy did not anticipate — that a cluster about "the artifact is trusted but unverified" had a verification apparatus that was itself partly unverifiable. The CI question is recorded as a residual with a recommendation and a named trigger rather than implemented, because the issue put it out of scope: do not couple this repo's per-PR CI to another repo's default branch; if wanted, a weekly scheduled check that opens an issue on divergence. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 67 +++++++++++++++------- scripts/build_gaul_lookup.py | 16 ++++++ tests/test_datafactory_deploy_readiness.py | 21 +++++-- tests/test_delivery_coverage.py | 13 +++-- tests/test_gaul_lookup_fidelity.py | 48 ++++++++++++++-- 5 files changed, 130 insertions(+), 35 deletions(-) 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" + )