Skip to content

S5 — The lookup declares its own version instead of inferring it (C-60) #186

Closed

Description

@Polichinel

Part of epic #181. Closes register C-60 (Tier 3).

Background

gaul_lookup.version() produces the stamp that ties a delivered artifact to the exact lookup build that made it — C-15's traceability provenance, the field you would need after a suspect delivery to answer "which lookup produced this?".

It gets that stamp by reaching three levels into views-datafactory's ingestion-ledger shape:

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}"

Today it returns land_gaul@f74d3b2b. If views-datafactory renames the land_gaul_region ledger entry, moves content_digest, or restructures source_provenance, this returns the string "unknown" — silently, through a bare except … pass, and nothing in the delivery notices. The provenance record still gets written; it just stops meaning anything.

That is a declare-don't-infer violation at the consumer: the reader knows the producer's private schema, and degradation is indistinguishable from success. gaul_lookup.version's own docstring already flags it as a known limitation and names this issue's scope as the fix.

De-risking already done — read this before scoping the change

The obvious fear is that changing the lookup artifact changes the wire. It does not, and this was verified empirically rather than assumed:

>>> sc = build_sidecar(gaul_lookup.load(), gids)
>>> sorted(k.decode() for k in (sc.schema.metadata or {}))
[]

sidecar.build_sidecar constructs a fresh pa.table(columns) from extracted arrays (wire/sidecar.py:58-73). It does not replace_schema_metadata, and pa.table() starts with none — so the lookup's parquet key-value metadata cannot reach the sidecar. lookup_version flows to delivery/provenance.py:46 → the store document's delivery description, not to any wire artifact.

Consequences for this story:

  • ADR-013 contract_version does not move. The tests/fixtures/wire_contract/ bytes are untouched.
  • The change is safe to make in one PR.
  • Verify this claim again in the PR rather than trusting this paragraph — git diff --stat origin/main -- tests/fixtures/ must be empty and tests/test_wire_fixture.py must be green.

Work

Producer — scripts/build_gaul_lookup.py:

Write a flat, declared lookup_version key into the parquet metadata alongside the existing adr / region / n_cells keys, holding the composed value (<region>@<short digest>). Keep source_provenance — it is richer than the stamp and is worth carrying — but stop making the stamp derivable only from it.

The builder is the right place to compose it: it already has region and _provenance(datafactory) in hand, and it is the only thing that knows both.

Decide and record: what should the builder do when _provenance finds no ledger entry? _provenance is documented "best-effort" and returns {} if the ledger file is absent. A build with no resolvable digest produces an artifact that cannot be traced. Per this repo's fail-loud idiom, that should be a LookupBuildError at build time, not an "unknown" at delivery time — the build is the moment a human is present to fix it. State the choice in the docstring.

Consumer — views_postprocessing/contract/gaul_lookup.py:

version() reads the single lookup_version key and raises if it is absent or empty. Delete the three-level traversal, the bare except, and the "unknown" branch. Log before raising (ADR-008) — the same shape S1 gives appwrite_env. Update the docstring: the C-60 "known limitation" paragraph is replaced by a statement of the declared key.

The committed artifact — views_postprocessing/data/gaul_lookup.parquet:

Must be rebuilt, because the existing one has no lookup_version key and the new reader raises without it. views-datafactory is available locally (../views-datafactory, 7 GAUL parquets present):

python scripts/build_gaul_lookup.py --datafactory ../views-datafactory --region land_gaul

The rebuild must be metadata-only in effect: same 64,742 rows, same values, same column order. Prove it, do not assume it — tests/test_gaul_lookup_fidelity.py (18 tests) is exactly the instrument, and it must stay green against the rebuilt artifact.

Consider also stamping lookup_version into the sidecar's own parquet metadata so a delivered artifact is self-describing. Not in this story — it would change wire bytes and therefore contract_version, and that is an ADR-013 amendment with three repos to notify. Note it in the register as C-60's deliberate residual if it still seems worth doing.

Acceptance criteria

  • gaul_lookup.version() has no "unknown" branch and no bare except.
  • It raises, with a log record, when the declared key is absent.
  • The committed artifact carries a flat lookup_version key.
  • The rebuilt artifact is value-identical to the old one: 64,742 rows, 9 columns, same order, same values.
  • tests/test_gaul_lookup_fidelity.py — 18 passed, unchanged.
  • git diff --stat origin/main -- tests/fixtures/ is empty; contract_version is still "1.5".
  • The builder's behaviour on an unresolvable ledger is decided, implemented, and documented.

Testing

New tests (home: tests/test_gaul_lookup_access.py, which already owns version()):

  • test_version_reads_the_declared_key — round-trip a tiny synthetic parquet with the key set.
  • test_version_raises_when_the_declared_key_is_absent — write one without it, assert the raise. This is the test that makes the whole story load-bearing; without it the fix is unproven.
  • test_version_logs_before_raising — ADR-008, matching S1's shape.
  • Builder-side: assert the written artifact carries the key, and (if you chose fail-loud) that an unresolvable ledger raises.

Existing test to watch: tests/test_gaul_lookup_access.py:37 already asserts version() != "unknown" — it should now be impossible rather than merely true. Re-point or keep as a belt-and-braces check, your call, but say which in the PR.

Validation:

ruff check .
pytest -q tests/test_gaul_lookup_fidelity.py tests/test_gaul_lookup_access.py tests/test_wire_sidecar.py tests/test_wire_fixture.py
pytest -q
git diff --stat origin/main -- tests/fixtures/      # must be empty

Dependencies

None. Blocks S7 and S8.

Files

  • scripts/build_gaul_lookup.py:93-108 (_provenance), :184-196 (metadata write)
  • views_postprocessing/contract/gaul_lookup.py:36-62 (version)
  • views_postprocessing/data/gaul_lookup.parquet — rebuilt
  • views_postprocessing/delivery/provenance.py:20,46 — the consumer of the stamp
  • views_postprocessing/unfao/managers/unfao.py:387 — where it enters a delivery
  • views_postprocessing/contract/wire/sidecar.py:58-73 — why the wire is unaffected

Activity

  1. added
    storyA single reviewable unit of an epic
    testingTest/parity/validation work
    on Aug 1, 2026
  2. added 2 commits that reference this issue on Aug 2, 2026
  3. Polichinel commented on Aug 2, 2026

    @Polichinel
    CollaboratorAuthor

    Landed in #201. C-60 closed.

    The builder composes the stamp and writes one flat lookup_version key; version() reads it and raises when absent, logging first. The three-level ledger traversal, the bare except … pass and the "unknown" branch are gone — pinned by AST, not word-matching, so a docstring explaining the fix cannot satisfy the guard.

    The rebuilt artifact is metadata-only, proven: 64,742 rows, column order and dtypes unchanged, a content hash over every column's values identical, and the stamp still resolves to land_gaul@f74d3b2b — the same string the old traversal produced, so no delivered provenance changes meaning. contract_version stays 1.5 and the fixtures are byte-identical.

    Review caught the fix reintroducing C-60's own defect. The first draft read land_gaul_region or gaul_admin_area_majority — and since _provenance takes no region, --region all would have been stamped all@f74d3b2b, the land_gaul region definition's digest, on a global artifact. Replaced by a declared stamp_dataset(region) with no fallback; an undeclared region is refused, not substituted.

    Residual deferred behind a named trigger rather than a vague later: stamping lookup_version into the sidecar's own metadata would change wire bytes and therefore contract_version, so it waits for the next ADR-013 bump.

    Register: 74 concerns, 20 open. Cluster K is down to C-46 alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    implementationCode implementation workstoryA single reviewable unit of an epictestingTest/parity/validation work

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions