feat(config): T-1281 — canvas-version, and typer's other rich path
All five gates now live in the check domain. canvas-version produces byte-identical output to the original on the live tree. It is the first real consumer of core/process.run. The git calls pass check=False deliberately: a git failure here is not an error to report but a signal that there is nothing to compare, since a fresh clone with no remote is a legitimate state rather than a broken one. The argv-list and missing-binary guards still apply. Its two skips are kept distinct from its pass. NO_BASE and DIFF_FAILED exit 0, as does CLEAN — but only CLEAN means the gate actually looked at something. Collapsing them would hide a gate that had silently stopped running, which for this check in particular is the exact failure it exists to prevent. Found a second rich path while a NameError was rendering as a full-width box-drawn traceback: typer's pretty-exception handler is a different mechanism from rich_markup_mode, and setting one does nothing about the other. Same log pollution T-1259 thought it had closed, arriving through another door and landing in the worst place — a hook log at the moment something has already gone wrong. pretty_exceptions_enable=False now on the root and on every domain built by cli.domain(). test_canvas_version_check.py moves with the code it guards. It had been loading the extensionless script through a SourceFileLoader and reaching canvas_sources by sys.path insert, both only because tooling/ was not importable. Second instance of that debt evaporating on contact. What it asserts is unchanged, which is the point: diff_has_version_bump was kept pure in the port so its six properties still hold without constructing git history. Also restores an import the check router dropped in T-1267 when it moved to cli.domain() — caught by running the command rather than by reading it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -84,4 +84,11 @@ def domain(name: str, help: str) -> typer.Typer:
|
||||
no_args_is_help=True,
|
||||
add_completion=False,
|
||||
rich_markup_mode=None,
|
||||
# A SECOND rich path, separate from rich_markup_mode and easy to miss:
|
||||
# typer's pretty-exception handler renders unhandled errors as box-art
|
||||
# with syntax highlighting. That is the same log pollution
|
||||
# rich_markup_mode=None prevents for help text, arriving through a
|
||||
# different door — and it lands in the worst place, a hook log at the
|
||||
# moment something has already gone wrong. Plain tracebacks instead.
|
||||
pretty_exceptions_enable=False,
|
||||
)
|
||||
|
||||
@@ -17,11 +17,13 @@ nothing left to defer. Laziness lives one level up, in `main.py`.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import typer
|
||||
|
||||
from tooling.core import cli, console
|
||||
from tooling.core.command import command
|
||||
from tooling.core.errors import ReachError
|
||||
from tooling.domains.check import service
|
||||
from tooling.domains.check.schemas import StampState
|
||||
from tooling.domains.check.schemas import CanvasState, StampState
|
||||
|
||||
app = cli.domain("check", "Consistency gates — the checks the push hook runs.")
|
||||
|
||||
@@ -67,6 +69,63 @@ def client_version() -> None:
|
||||
console.verdict(f"check-client-version: OK — {result.yaml_version}")
|
||||
|
||||
|
||||
@app.command("canvas-version")
|
||||
@command
|
||||
def canvas_version(
|
||||
base: str = typer.Option(
|
||||
service.DEFAULT_BASE, "--base", help="Base ref to compare against."
|
||||
),
|
||||
head: str = typer.Option("HEAD", "--head", help="Head ref to compare."),
|
||||
) -> None:
|
||||
"""Fail if canvas generation changed without project.yaml's version moving."""
|
||||
result = service.canvas_version(base, head)
|
||||
|
||||
if result.state is CanvasState.NO_BASE:
|
||||
console.verdict(
|
||||
f"check-canvas-version: {result.base} not found — skipping (nothing to compare)"
|
||||
)
|
||||
return
|
||||
if result.state is CanvasState.DIFF_FAILED:
|
||||
console.verdict(
|
||||
f"check-canvas-version: could not diff {result.commit_range} — skipping"
|
||||
)
|
||||
return
|
||||
if result.state is CanvasState.CLEAN:
|
||||
console.verdict("check-canvas-version: no canvas-generation changes in range — OK")
|
||||
return
|
||||
if result.state is CanvasState.BUMPED:
|
||||
console.verdict(
|
||||
f"check-canvas-version: OK — {len(result.touched)} canvas-generation "
|
||||
"file(s) changed and project.yaml's version moved with them"
|
||||
)
|
||||
return
|
||||
|
||||
shown = result.touched[:10]
|
||||
remainder = len(result.touched) - len(shown)
|
||||
listing = "".join(f" {path}\n" for path in shown)
|
||||
if remainder:
|
||||
listing += f" ... and {remainder} more\n"
|
||||
|
||||
raise ReachError(
|
||||
"check-canvas-version: canvas generation changed without a version bump\n"
|
||||
"\n"
|
||||
f" Range: {result.commit_range}\n"
|
||||
" Changed canvas-generation files:\n" + listing + "\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.",
|
||||
fix=(
|
||||
"bump `version:` in project.yaml (scheme 0.{phase}.{n}), note it in the "
|
||||
"comment block above, and mirror the value into client/project.godot's "
|
||||
"config/version. If you are certain the change cannot alter canvas bytes, "
|
||||
"bump it anyway — the cost is one round of cache misses, and this has "
|
||||
"shipped broken five times, most recently T-1239, which took eight days "
|
||||
"to find."
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
_STAMP_REMEDY = {
|
||||
StampState.UNSTAMPED: "make regen-db — systems.db carries no stamp to verify",
|
||||
StampState.BAD_VERSION: "make regen-db — the recorded schema_version predates semver",
|
||||
|
||||
@@ -67,6 +67,37 @@ class DiagramCheck(BaseModel):
|
||||
return not self.unresolved and not self.missing and self.checked > 0
|
||||
|
||||
|
||||
class CanvasState(str, Enum):
|
||||
"""Why the canvas-version gate reached its verdict.
|
||||
|
||||
Two of these are SKIPS rather than passes, and keeping them distinct
|
||||
matters: "no base to compare against" and "nothing relevant changed" both
|
||||
exit 0, but only the second means the gate actually looked at something.
|
||||
Collapsing them would hide a gate that silently stopped running.
|
||||
"""
|
||||
|
||||
NO_BASE = "no_base" # base ref absent (fresh clone) — nothing to compare
|
||||
DIFF_FAILED = "diff_failed" # git could not produce a range
|
||||
CLEAN = "clean" # nothing in the registry was touched
|
||||
BUMPED = "bumped" # canvas files changed AND version moved
|
||||
NEEDS_BUMP = "needs_bump" # canvas files changed, version did not
|
||||
|
||||
|
||||
class CanvasCheck(BaseModel):
|
||||
"""The outcome of pairing canvas-generation changes with a version bump."""
|
||||
|
||||
model_config = ConfigDict(frozen=True)
|
||||
|
||||
state: CanvasState
|
||||
commit_range: str
|
||||
touched: list[str] = []
|
||||
base: str = ""
|
||||
|
||||
@property
|
||||
def ok(self) -> bool:
|
||||
return self.state is not CanvasState.NEEDS_BUMP
|
||||
|
||||
|
||||
class StampState(str, Enum):
|
||||
"""Why the systems.db stamp check reached its verdict.
|
||||
|
||||
|
||||
@@ -12,8 +12,10 @@ import re
|
||||
import sqlite3
|
||||
from pathlib import Path
|
||||
|
||||
from tooling.core import config
|
||||
from tooling.core import config, process
|
||||
from tooling.domains.check.schemas import (
|
||||
CanvasCheck,
|
||||
CanvasState,
|
||||
DiagramCheck,
|
||||
FactIdCheck,
|
||||
StampCheck,
|
||||
@@ -60,6 +62,93 @@ def client_version() -> VersionCheck:
|
||||
)
|
||||
|
||||
|
||||
# --- canvas-generation version pairing ------------------------------------
|
||||
#
|
||||
# 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 with a warm cache, so the author never sees it. That has shipped five
|
||||
# times; T-1239 is what the last one cost (eight days).
|
||||
#
|
||||
# Deliberately no override flag (T-1242): a false positive costs one round of
|
||||
# cache misses, a false negative costs 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.
|
||||
|
||||
DEFAULT_BASE = "origin/main"
|
||||
|
||||
|
||||
def diff_has_version_bump(diff_text: str) -> bool:
|
||||
"""Does this project.yaml diff actually MOVE the `version:` field?
|
||||
|
||||
Kept pure so the property is testable without constructing git history —
|
||||
which is what tooling/test_canvas_version_check.py exercises, and the reason
|
||||
that test could be written at all.
|
||||
|
||||
Matched on the diff BODY rather than on the filename appearing in
|
||||
--name-only: project.yaml carries a comment block documenting past bumps,
|
||||
including lines quoting old version NUMBERS, so editing that commentary must
|
||||
not count. Requires the ADDED side — a lone deletion means the field was
|
||||
removed, not moved — and skips `+++` headers, which would otherwise match.
|
||||
"""
|
||||
for line in diff_text.splitlines():
|
||||
if line.startswith("+++"):
|
||||
continue
|
||||
if line.startswith("+version:"):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def canvas_version(base: str = DEFAULT_BASE, head: str = "HEAD") -> CanvasCheck:
|
||||
"""Require a version bump alongside any canvas-generation change."""
|
||||
from tooling import canvas_sources
|
||||
|
||||
# Three-dot: what HEAD added since the merge base, matching the convention
|
||||
# the pre-push hook already uses for the systems.db stamp check.
|
||||
commit_range = f"{base}...{head}"
|
||||
|
||||
if _git("rev-parse", "--verify", base) is None:
|
||||
# No base to compare against (fresh clone, no remote). Skipping is
|
||||
# correct rather than failing: there is no range to judge.
|
||||
return CanvasCheck(state=CanvasState.NO_BASE, commit_range=commit_range, base=base)
|
||||
|
||||
listing = _git("diff", "--name-only", commit_range)
|
||||
if listing is None:
|
||||
return CanvasCheck(
|
||||
state=CanvasState.DIFF_FAILED, commit_range=commit_range, base=base
|
||||
)
|
||||
|
||||
changed = {line for line in listing.splitlines() if line}
|
||||
touched = sorted(changed & set(canvas_sources.relative_paths()))
|
||||
if not touched:
|
||||
return CanvasCheck(state=CanvasState.CLEAN, commit_range=commit_range, base=base)
|
||||
|
||||
bumped = _git("diff", "-U0", commit_range, "--", "project.yaml")
|
||||
state = (
|
||||
CanvasState.BUMPED
|
||||
if bumped is not None and diff_has_version_bump(bumped)
|
||||
else CanvasState.NEEDS_BUMP
|
||||
)
|
||||
return CanvasCheck(
|
||||
state=state, commit_range=commit_range, touched=touched, base=base
|
||||
)
|
||||
|
||||
|
||||
def _git(*args: str) -> str | None:
|
||||
"""Run git, returning stdout, or None if it failed.
|
||||
|
||||
`check=False` on purpose: a git failure here is not an error to report, it
|
||||
is a signal that there is nothing to compare — a fresh clone with no remote
|
||||
is a legitimate state, not a broken one. Still goes through
|
||||
`core.process.run` so the argv-list and missing-binary guards apply.
|
||||
"""
|
||||
result = process.run(
|
||||
["git", "-C", str(config.repo_root()), *args],
|
||||
check=False,
|
||||
)
|
||||
return result.stdout if result.returncode == 0 else None
|
||||
|
||||
|
||||
# --- systems-db stamp -----------------------------------------------------
|
||||
#
|
||||
# server/data/systems.db is a committed build artefact. Its meta table records
|
||||
|
||||
@@ -126,6 +126,9 @@ cli = typer.Typer(
|
||||
no_args_is_help=True,
|
||||
add_completion=False,
|
||||
rich_markup_mode=None,
|
||||
# See core/cli.py — a second rich path that renders unhandled exceptions as
|
||||
# box-art, arriving through a different door than rich_markup_mode.
|
||||
pretty_exceptions_enable=False,
|
||||
context_settings={"help_option_names": ["-h", "--help"], "max_content_width": 100},
|
||||
)
|
||||
|
||||
|
||||
@@ -16,30 +16,20 @@ stays quiet otherwise. Two properties carry that, and neither is observable from
|
||||
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
|
||||
# Re-pointed at the ported service (T-1281). This test previously loaded the
|
||||
# extensionless tooling/check-canvas-version through a SourceFileLoader and
|
||||
# reached canvas_sources via a sys.path insert — both because tooling/ was not
|
||||
# an importable package. Neither is needed now, and the test moves with the code
|
||||
# it guards rather than being left aimed at a script due for retirement.
|
||||
sys.path.insert(0, str(REPO_ROOT))
|
||||
|
||||
|
||||
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()
|
||||
from tooling.canvas_sources import relative_paths # noqa: E402
|
||||
from tooling.domains.check import service as check # noqa: E402
|
||||
|
||||
FAILURES: list[str] = []
|
||||
|
||||
|
||||
Reference in New Issue
Block a user