feat(config): make the canvas-generation/version pairing a gate, not a habit (T-1242)
project.yaml's version is the Atlas disk cache's only invalidation signal, and nothing enforced that changing canvas GENERATION also moved it. It broke five times -- 0.4.2 lake_margin_q, 0.4.3 coast_warp_px, 0.4.4 the extent inversion, 0.4.5 the Global sentinel, 0.4.6 one-course-per-river -- each bumped only after someone noticed a wrong map. The failure is invisible to its author: it needs a warm cache to reproduce, so a cold checkout looks fine. T-1239 is the last one, and it took eight days. tooling/canvas_sources.py is the path registry; tooling/check-canvas-version rejects a push that touches those paths without moving project.yaml's version line. Wired into the pre-push hook, `make check-canvas-version`, and, for the parsing units, `make test-tooling`. Verified against real history rather than a synthetic branch: run over4e503c356-- the commit that actually caused T-1239 -- the gate rejects and names the three files. Run over the commits that DID bump (bdea71953,39f0fd8c5, and T-1239's own fix), it passes. The registry is globbed, not hand-listed. step_canvas.rs imports ten sibling modules and those import more, so a traced closure would be stale within a month, and stale here is silent. It over-includes on purpose: a false positive costs one bump and one round of cache misses, a false negative costs another week of a wrong map -- the ticket's own ruling. Two deliberate calls worth naming. The registry includes ITSELF, which closes the narrowing hole: remove a path and change that same path in one push, and the gate still fires because the registry file is in the set. And there is no override flag -- it would be reached for exactly when someone is certain their change is harmless, which is the reasoning behind all five regressions. Version bumped 0.4.6 -> 0.4.7 with NO canvas-generation change: self-inclusion means adding the registry trips its own rule. Spent rather than special-cased, because the first exception is how a rule like this dies. The units cover the property no branch run can show -- that editing project.yaml's comment block, which quotes old version NUMBERS directly above the field, is not a bump -- plus a registry-coverage test naming the files each of the five known regressions touched, so a future narrowing past them fails loudly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -0,0 +1,158 @@
|
||||
#!/usr/bin/env python3
|
||||
"""
|
||||
canvas_sources — single source of truth for the Atlas canvas-generation path set.
|
||||
|
||||
`project.yaml`'s `version:` is the Atlas disk cache's ONLY invalidation signal
|
||||
(client/scripts/build_version.gd, D-255). A change to how a canvas is GENERATED
|
||||
is therefore only half a change; the other half is bumping that version, or every
|
||||
warm cache keeps serving canvases built by code that no longer exists.
|
||||
|
||||
Nothing enforced that pairing, and it broke five times: project.yaml's own
|
||||
comments record 0.4.2 (lake_margin_q semantics), 0.4.3 (coast_warp_px at orbital
|
||||
sampling), 0.4.4 (the D-255 extent inversion), 0.4.5 (the Global sentinel), and
|
||||
0.4.6 (T-1237 one-course-per-river) — every one of them bumped *after the fact*,
|
||||
the last only after T-1239 spent eight days diagnosing a map drawn from a canvas
|
||||
whose generating code had been replaced. The failure is invisible to its author:
|
||||
it reproduces only where a warm cache exists, so a cold checkout looks fine.
|
||||
|
||||
This module is the registry `tooling/check-canvas-version` intersects against,
|
||||
kept here rather than inline in the hook for the same reason
|
||||
`tooling/generator_sources.py` exists (T-1067): one list, one place, imported by
|
||||
everything that needs it.
|
||||
|
||||
WHY THIS DELIBERATELY OVER-INCLUDES
|
||||
-----------------------------------
|
||||
The set is collected by GLOB, not hand-listed, and covers the whole atlas module
|
||||
rather than a traced dependency closure.
|
||||
|
||||
`step_canvas.rs` directly imports ten sibling modules and those pull in more
|
||||
(district_profile -> domain_warp/detail_scatter/coast_invention/..., layer1 ->
|
||||
drainage/hydrology_equilibrium/features). A hand-maintained closure of that would
|
||||
be wrong within a month, and being wrong here is silent — exactly the failure
|
||||
this registry exists to stop. Globbing is self-maintaining: a module added in a
|
||||
future split is covered the moment it exists.
|
||||
|
||||
The cost asymmetry is the whole argument, and it is the ticket's own ruling
|
||||
(T-1242): a false positive costs one version bump and one round of cache misses;
|
||||
a false negative costs another week of a wrong map. So when the choice is
|
||||
"include a file that might not change canvas bytes" versus "risk missing one that
|
||||
does", this includes it.
|
||||
|
||||
Fail closed on an empty glob, per generator_sources.py's precedent: an empty set
|
||||
would silently pass every push.
|
||||
|
||||
Usage:
|
||||
python3 tooling/canvas_sources.py --list
|
||||
"""
|
||||
|
||||
import argparse
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Server: the code that PRODUCES canvas bytes
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
# The whole atlas module. Canvas bytes are produced by step_canvas.rs out of
|
||||
# layer1/TerrainAnalysis/district_profile/river_course/hydrology and a long tail
|
||||
# of invention modules; see this file's header for why the boundary is the
|
||||
# module rather than a traced import closure.
|
||||
ATLAS_DIR: Path = REPO_ROOT / "server" / "src" / "atlas"
|
||||
|
||||
# The seed chain feeds every deterministic decision the cascade makes
|
||||
# (step_canvas.rs: `use crate::seed::SeedChain`), so a change to seed derivation
|
||||
# changes canvas bytes without touching atlas/ at all.
|
||||
SEED_RS: Path = REPO_ROOT / "server" / "src" / "seed.rs"
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Client: the code that KEYS, STORES and INTERPRETS those bytes
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
# The step_canvas cluster: the disk cache and its index format, the in-memory
|
||||
# cache, the request/transport layer that derives extents and spacing (and
|
||||
# therefore cache KEYS), and the layers that decode the payload.
|
||||
#
|
||||
# The render-only members (terrain/annotation layers) are included on purpose.
|
||||
# A pure draw change cannot make cached bytes wrong, so including them can force
|
||||
# an unnecessary re-derive — but drawing the line *inside* this directory means
|
||||
# hand-judging which file is "really" wire-shaped, and T-1237 changed the server
|
||||
# course shape and the client annotation layer in the same commit. That judgement
|
||||
# is precisely where a false negative would come from.
|
||||
CLIENT_STEP_CANVAS_DIR: Path = (
|
||||
REPO_ROOT / "client" / "ui" / "implant" / "apps" / "atlas" / "step_canvas"
|
||||
)
|
||||
|
||||
# The accessor the cache reads its invalidation tag through.
|
||||
BUILD_VERSION_GD: Path = REPO_ROOT / "client" / "scripts" / "build_version.gd"
|
||||
|
||||
|
||||
def _rust_sources(directory: Path, label: str) -> tuple[Path, ...]:
|
||||
"""Every .rs file in `directory`, collected by glob and fail-closed."""
|
||||
sources = tuple(sorted(directory.rglob("*.rs")))
|
||||
if not sources:
|
||||
raise RuntimeError(
|
||||
f"{label} sources not found at {directory} — the canvas-generation "
|
||||
"path set would be incomplete, and this check would pass every push"
|
||||
)
|
||||
return sources
|
||||
|
||||
|
||||
def _gdscript_sources(directory: Path, label: str) -> tuple[Path, ...]:
|
||||
"""Every .gd file in `directory`, collected by glob and fail-closed.
|
||||
|
||||
`.uid` sidecars are Godot bookkeeping and carry no behaviour, so they are
|
||||
excluded — a uid churn should not demand a version bump.
|
||||
"""
|
||||
sources = tuple(sorted(directory.rglob("*.gd")))
|
||||
if not sources:
|
||||
raise RuntimeError(
|
||||
f"{label} sources not found at {directory} — the canvas-generation "
|
||||
"path set would be incomplete, and this check would pass every push"
|
||||
)
|
||||
return sources
|
||||
|
||||
|
||||
def canvas_sources() -> tuple[Path, ...]:
|
||||
"""Every file whose change may alter canvas bytes or their interpretation.
|
||||
|
||||
This registry itself is a member: loosening the set must be as visible as
|
||||
any other canvas-generation change (generator_sources.py makes the same
|
||||
call for the same reason).
|
||||
"""
|
||||
return (
|
||||
Path(__file__).resolve(),
|
||||
*_rust_sources(ATLAS_DIR, "server atlas"),
|
||||
SEED_RS,
|
||||
*_gdscript_sources(CLIENT_STEP_CANVAS_DIR, "client step_canvas"),
|
||||
BUILD_VERSION_GD,
|
||||
)
|
||||
|
||||
|
||||
def relative_paths() -> tuple[str, ...]:
|
||||
"""The registry as repo-relative POSIX paths, for matching git output."""
|
||||
return tuple(p.relative_to(REPO_ROOT).as_posix() for p in canvas_sources())
|
||||
|
||||
|
||||
def main() -> None:
|
||||
parser = argparse.ArgumentParser(
|
||||
description="Single source of truth for the Atlas canvas-generation path set"
|
||||
)
|
||||
parser.add_argument(
|
||||
"--list",
|
||||
action="store_true",
|
||||
help="Print the canvas-generation paths, one repo-relative path per line",
|
||||
)
|
||||
args = parser.parse_args()
|
||||
|
||||
if not args.list:
|
||||
parser.print_help()
|
||||
sys.exit(2)
|
||||
|
||||
for path in relative_paths():
|
||||
print(path)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
Executable
+164
@@ -0,0 +1,164 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Fail if a push changes canvas generation without moving project.yaml's version.
|
||||
|
||||
`project.yaml`'s `version:` is the Atlas disk cache's only invalidation signal.
|
||||
Change how a canvas is generated without moving it and every warm cache keeps
|
||||
serving canvases built by code that no longer exists — silently, and only on
|
||||
machines that have a warm cache, so the author never sees it. That has happened
|
||||
five times (see tooling/canvas_sources.py for the roll-call); T-1239 is what the
|
||||
last one cost.
|
||||
|
||||
The rule: if the push touches anything in the canvas-generation registry, the
|
||||
`version:` line in project.yaml must change in the SAME range.
|
||||
|
||||
Deliberately no override flag. The ticket's ruling (T-1242) is that a false
|
||||
positive is cheap — one version bump, one round of cache misses — and a false
|
||||
negative is another week of a wrong map. An escape hatch would be reached for
|
||||
exactly when someone is sure their change is harmless, which is the state of mind
|
||||
that produced all five regressions.
|
||||
|
||||
Exit: 0 = fine (or nothing relevant in range), 1 = version bump required.
|
||||
"""
|
||||
|
||||
import argparse
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parent))
|
||||
from canvas_sources import relative_paths # noqa: E402
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||
DEFAULT_BASE = "origin/main"
|
||||
|
||||
|
||||
def git(*args: str) -> str | None:
|
||||
"""Run a git command, returning stdout, or None if it failed."""
|
||||
result = subprocess.run(
|
||||
["git", "-C", str(REPO_ROOT), *args],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
)
|
||||
if result.returncode != 0:
|
||||
return None
|
||||
return result.stdout
|
||||
|
||||
|
||||
def changed_files(commit_range: str) -> list[str] | None:
|
||||
out = git("diff", "--name-only", commit_range)
|
||||
if out is None:
|
||||
return None
|
||||
return [line for line in out.splitlines() if line]
|
||||
|
||||
|
||||
def diff_has_version_bump(diff_text: str) -> bool:
|
||||
"""Does this project.yaml diff actually move the `version:` field?
|
||||
|
||||
Pure, so the property is testable without constructing git history
|
||||
(tooling/test_canvas_version_check.py).
|
||||
|
||||
Matched on the diff body rather than on the file appearing in --name-only:
|
||||
project.yaml carries a long comment block documenting past bumps — including
|
||||
lines that quote old version NUMBERS — so editing that commentary, or any
|
||||
other field in the file, must NOT count as bumping the version.
|
||||
|
||||
Requires the ADDED side: a lone deletion means the field was removed, not
|
||||
moved. Diff context/metadata lines such as `+++ b/project.yaml` must not
|
||||
match either, which is why this anchors on `+version:` exactly.
|
||||
"""
|
||||
for line in diff_text.splitlines():
|
||||
if line.startswith("+++"):
|
||||
continue # diff header, not content
|
||||
if line.startswith("+version:"):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def version_line_changed(commit_range: str) -> bool:
|
||||
"""Did project.yaml's `version:` line itself change in this range?"""
|
||||
out = git("diff", "-U0", commit_range, "--", "project.yaml")
|
||||
if out is None:
|
||||
return False
|
||||
return diff_has_version_bump(out)
|
||||
|
||||
|
||||
def main() -> int:
|
||||
parser = argparse.ArgumentParser(
|
||||
description="Require a project.yaml version bump alongside canvas-generation changes"
|
||||
)
|
||||
parser.add_argument(
|
||||
"--base",
|
||||
default=DEFAULT_BASE,
|
||||
help=f"Base ref to compare against (default: {DEFAULT_BASE})",
|
||||
)
|
||||
parser.add_argument(
|
||||
"--head",
|
||||
default="HEAD",
|
||||
help="Head ref to compare (default: HEAD)",
|
||||
)
|
||||
args = parser.parse_args()
|
||||
|
||||
# Three-dot: what HEAD added since the merge base, matching the systems.db
|
||||
# stamp check's own convention in .config/hooks/pre-push.
|
||||
commit_range = f"{args.base}...{args.head}"
|
||||
|
||||
if git("rev-parse", "--verify", args.base) is None:
|
||||
# No base to compare against (fresh clone, no remote yet). Skipping is
|
||||
# correct rather than failing: there is no "range" to judge.
|
||||
print(
|
||||
f"check-canvas-version: {args.base} not found — skipping (nothing to compare)"
|
||||
)
|
||||
return 0
|
||||
|
||||
changed = changed_files(commit_range)
|
||||
if changed is None:
|
||||
print(
|
||||
f"check-canvas-version: could not diff {commit_range} — skipping",
|
||||
file=sys.stderr,
|
||||
)
|
||||
return 0
|
||||
|
||||
registry = set(relative_paths())
|
||||
touched = sorted(set(changed) & registry)
|
||||
|
||||
if not touched:
|
||||
print("check-canvas-version: no canvas-generation changes in range — OK")
|
||||
return 0
|
||||
|
||||
if version_line_changed(commit_range):
|
||||
print(
|
||||
f"check-canvas-version: OK — {len(touched)} canvas-generation file(s) "
|
||||
"changed and project.yaml's version moved with them"
|
||||
)
|
||||
return 0
|
||||
|
||||
shown = touched[:10]
|
||||
remainder = len(touched) - len(shown)
|
||||
print(
|
||||
"check-canvas-version: canvas generation changed without a version bump\n"
|
||||
"\n"
|
||||
f" Range: {commit_range}\n"
|
||||
" Changed canvas-generation files:\n"
|
||||
+ "".join(f" {p}\n" for p in shown)
|
||||
+ (f" ... and {remainder} more\n" if remainder else "")
|
||||
+ "\n"
|
||||
"project.yaml's `version:` is the Atlas disk cache's ONLY invalidation\n"
|
||||
"signal. Without a bump, every warm cache keeps serving canvases built by\n"
|
||||
"the code you just changed — silently, and only on machines that have a\n"
|
||||
"warm cache, so you will not see it on a cold checkout.\n"
|
||||
"\n"
|
||||
"Fix: bump `version:` in project.yaml (scheme 0.{phase}.{n}), add a line to\n"
|
||||
"the comment block above it saying what the old entries carried, and mirror\n"
|
||||
"the new value into client/project.godot's config/version.\n"
|
||||
"\n"
|
||||
"If you are certain this change cannot alter canvas bytes, bump it anyway:\n"
|
||||
"the cost is one round of cache misses. That trade is the point — this has\n"
|
||||
"shipped broken five times, most recently T-1239, which took eight days to\n"
|
||||
"find.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
return 1
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
@@ -0,0 +1,171 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Units for the canvas-generation/version pairing gate (T-1242).
|
||||
|
||||
The gate's whole value is that it fires on the cases that shipped broken and
|
||||
stays quiet otherwise. Two properties carry that, and neither is observable from
|
||||
"the check passed on this branch":
|
||||
|
||||
1. Editing project.yaml WITHOUT moving `version:` is not a bump. The file
|
||||
carries a comment block that quotes old version numbers (0.4.2 … 0.4.6), so
|
||||
a naive "did project.yaml change" or "does the diff mention a version"
|
||||
test would count commentary edits as a bump and wave through exactly the
|
||||
regressions this exists to catch.
|
||||
2. The registry is non-empty and covers the files the five known regressions
|
||||
actually touched. An empty or narrowed registry passes every push silently.
|
||||
|
||||
Run: python3 tooling/test_canvas_version_check.py
|
||||
"""
|
||||
|
||||
import importlib.util
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||
TOOLING = REPO_ROOT / "tooling"
|
||||
|
||||
sys.path.insert(0, str(TOOLING))
|
||||
from canvas_sources import relative_paths # noqa: E402
|
||||
|
||||
|
||||
def _load_check_module():
|
||||
"""Import the extensionless check script as a module."""
|
||||
path = TOOLING / "check-canvas-version"
|
||||
spec = importlib.util.spec_from_loader(
|
||||
"check_canvas_version",
|
||||
importlib.machinery.SourceFileLoader("check_canvas_version", str(path)),
|
||||
)
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(module)
|
||||
return module
|
||||
|
||||
|
||||
check = _load_check_module()
|
||||
|
||||
FAILURES: list[str] = []
|
||||
|
||||
|
||||
def expect(condition: bool, label: str) -> None:
|
||||
if condition:
|
||||
print(f" ok {label}")
|
||||
else:
|
||||
print(f" FAIL {label}")
|
||||
FAILURES.append(label)
|
||||
|
||||
|
||||
def test_real_bump_counts() -> None:
|
||||
diff = (
|
||||
"--- a/project.yaml\n"
|
||||
"+++ b/project.yaml\n"
|
||||
"@@ -22 +22 @@\n"
|
||||
"-version: 0.4.5\n"
|
||||
"+version: 0.4.6\n"
|
||||
)
|
||||
expect(check.diff_has_version_bump(diff), "a real -version/+version pair is a bump")
|
||||
|
||||
|
||||
def test_comment_edit_is_not_a_bump() -> None:
|
||||
# The exact shape that must NOT count: adding a line to the comment block
|
||||
# that documents what earlier versions carried. It mentions version numbers
|
||||
# and sits directly above the field.
|
||||
diff = (
|
||||
"--- a/project.yaml\n"
|
||||
"+++ b/project.yaml\n"
|
||||
"@@ -20,0 +21,2 @@\n"
|
||||
"+# 0.4.5-tagged canvases carry pre-T-1237 river courses: one course per\n"
|
||||
"+# D8 hop rather than one per river. 0.4.6 forces them to miss.\n"
|
||||
)
|
||||
expect(
|
||||
not check.diff_has_version_bump(diff),
|
||||
"a comment-block edit quoting version numbers is NOT a bump",
|
||||
)
|
||||
|
||||
|
||||
def test_other_field_edit_is_not_a_bump() -> None:
|
||||
diff = (
|
||||
"--- a/project.yaml\n"
|
||||
"+++ b/project.yaml\n"
|
||||
"@@ -23 +23 @@\n"
|
||||
"-repository: settled-reach\n"
|
||||
"+repository: settled-reach-renamed\n"
|
||||
)
|
||||
expect(
|
||||
not check.diff_has_version_bump(diff),
|
||||
"editing another field is NOT a bump",
|
||||
)
|
||||
|
||||
|
||||
def test_diff_header_is_not_a_bump() -> None:
|
||||
# `+++ b/project.yaml` starts with '+' and must not be mistaken for content.
|
||||
diff = "--- a/project.yaml\n+++ b/project.yaml\n@@ -1 +1 @@\n-name: x\n+name: y\n"
|
||||
expect(
|
||||
not check.diff_has_version_bump(diff),
|
||||
"the +++ diff header is NOT a bump",
|
||||
)
|
||||
|
||||
|
||||
def test_deletion_alone_is_not_a_bump() -> None:
|
||||
diff = "--- a/project.yaml\n+++ b/project.yaml\n@@ -22 +21,0 @@\n-version: 0.4.6\n"
|
||||
expect(
|
||||
not check.diff_has_version_bump(diff),
|
||||
"removing the version field is NOT a bump",
|
||||
)
|
||||
|
||||
|
||||
def test_empty_diff_is_not_a_bump() -> None:
|
||||
expect(not check.diff_has_version_bump(""), "an empty diff is NOT a bump")
|
||||
|
||||
|
||||
def test_registry_is_populated() -> None:
|
||||
paths = relative_paths()
|
||||
expect(len(paths) > 10, f"registry is populated ({len(paths)} paths)")
|
||||
|
||||
|
||||
def test_registry_covers_the_known_regressions() -> None:
|
||||
"""The files the five documented stale-cache regressions actually touched.
|
||||
|
||||
If a future edit narrows the registry past any of these, the gate stops
|
||||
catching the exact class of bug it was built for — silently. This is the
|
||||
test that makes such a narrowing loud.
|
||||
"""
|
||||
paths = set(relative_paths())
|
||||
required = [
|
||||
# T-1237 / T-1239: one course per river (0.4.6)
|
||||
"server/src/atlas/step_canvas.rs",
|
||||
"server/src/atlas/river_course.rs",
|
||||
"client/ui/implant/apps/atlas/step_canvas/step_canvas_annotation_layer.gd",
|
||||
# D-255 extent inversion (0.4.4) + Global sentinel (0.4.5)
|
||||
"client/ui/implant/apps/atlas/step_canvas/step_canvas_transport.gd",
|
||||
# coast_warp_px at orbital sampling (0.4.3)
|
||||
"server/src/atlas/coast_invention.rs",
|
||||
# lake_margin_q semantics (0.4.2)
|
||||
"server/src/atlas/hydrology_equilibrium.rs",
|
||||
# the layer every rung samples
|
||||
"server/src/atlas/layer1.rs",
|
||||
"server/src/atlas/district_profile.rs",
|
||||
# the cache that the version tag actually governs
|
||||
"client/ui/implant/apps/atlas/step_canvas/step_canvas_disk_cache.gd",
|
||||
]
|
||||
for path in required:
|
||||
expect(path in paths, f"registry covers {path}")
|
||||
|
||||
|
||||
def main() -> int:
|
||||
print("test_canvas_version_check:")
|
||||
test_real_bump_counts()
|
||||
test_comment_edit_is_not_a_bump()
|
||||
test_other_field_edit_is_not_a_bump()
|
||||
test_diff_header_is_not_a_bump()
|
||||
test_deletion_alone_is_not_a_bump()
|
||||
test_empty_diff_is_not_a_bump()
|
||||
test_registry_is_populated()
|
||||
test_registry_covers_the_known_regressions()
|
||||
|
||||
if FAILURES:
|
||||
print(f"\nFAILED ({len(FAILURES)}): " + "; ".join(FAILURES), file=sys.stderr)
|
||||
return 1
|
||||
print("test_canvas_version_check: PASS")
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
Reference in New Issue
Block a user