feat(config): T-1260 — reach lists its domains without importing them
`reach --help` renders from a declaration table and imports nothing. The cost of help is now flat as the registry grows, which is the property that has to hold going from one domain to a dozen. The trap is real and was confirmed in typer's vendored source rather than assumed from upstream Click: TyperGroup.format_commands loops over list_commands calling get_command on each, purely to read a short help string off the loaded command. With lazy loading underneath, that imports every domain in the registry to render --help — while the output looks entirely correct. Nothing observable changes; only the import graph does. So the test asserts on sys.modules, and it was proven to fail before being trusted. Disabling the format_commands override made it fail and name the cause, listing all five leaked check modules. It also carries a positive control — invoking a domain must import its service — because without one, "nothing was imported" would pass equally for a loader that is simply broken, and it fails on an empty registry, which would otherwise satisfy everything vacuously. The check domain is created here because the test needs a subject: a stub raising NotImplementedError would have been committed dead code. That takes the port out of T-1262, which is rescoped to what it still owns — pydantic schemas, byte-for-byte output parity on the drift path, and the failure tests. The old tooling/check-client-version script stays in place and stays wired to the pre-push hook; the deprecation window is deliberate. One Typer behaviour worth knowing before every future domain: a single-command app collapses into a bare command, so `reach check client-version` failed with "unexpected extra argument" until the router got a callback. Same mechanism as the root callback, different symptom. Help now works at every level, closing item 5 of T-1248. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,6 @@
|
||||
"""The `check` domain — repo consistency gates, run by the pre-push hook.
|
||||
|
||||
These are the highest-frequency commands in the tree and the most
|
||||
timing-sensitive, since they run on every push. Nothing here should acquire a
|
||||
heavy module-level import.
|
||||
"""
|
||||
@@ -0,0 +1,70 @@
|
||||
"""Transport for the `check` domain — args in, delegate, format out.
|
||||
|
||||
**Zero logic lives here.** Every command in this file should read as: parse,
|
||||
call a service, turn the result into output and an exit code. If a command
|
||||
grows a branch that is about the *problem* rather than about *presentation*,
|
||||
that branch belongs in `service.py`.
|
||||
|
||||
The service import is deliberately at module level: by the time this module is
|
||||
imported at all, `reach` has already decided to run a `check` command, so there
|
||||
is nothing left to defer. Laziness lives one level up, in `main.py`.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import typer
|
||||
|
||||
from tooling.core import console
|
||||
from tooling.domains.check import service
|
||||
|
||||
app = typer.Typer(
|
||||
name="check",
|
||||
help="Consistency gates — the checks the push hook runs.",
|
||||
no_args_is_help=True,
|
||||
add_completion=False,
|
||||
rich_markup_mode=None,
|
||||
)
|
||||
|
||||
|
||||
@app.callback()
|
||||
def _domain() -> None:
|
||||
"""Keeps `check` a group.
|
||||
|
||||
Typer collapses a single-command app into a bare command, so without this
|
||||
`reach check client-version` fails with "unexpected extra argument". Every
|
||||
domain router needs this until it has two or more verbs — and keeping it
|
||||
afterwards costs nothing and stops the shape changing under you.
|
||||
"""
|
||||
|
||||
|
||||
@app.command("client-version")
|
||||
def client_version() -> None:
|
||||
"""Fail if the client's baked version has drifted from project.yaml."""
|
||||
result = service.client_version()
|
||||
|
||||
if result.problem:
|
||||
console.verdict(
|
||||
f"check-client-version: {result.problem}",
|
||||
ok=False,
|
||||
fix="check that project.yaml and client/project.godot exist and are readable",
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
if not result.ok:
|
||||
console.verdict(
|
||||
"check-client-version: version drift\n"
|
||||
f" project.yaml {result.yaml_version}\n"
|
||||
f" client/project.godot {result.godot_version}\n"
|
||||
"\n"
|
||||
"This matters beyond cosmetics: the Atlas disk cache keys its\n"
|
||||
"invalidation on this version, so a stale mirror makes a shipped\n"
|
||||
"build serve canvases generated by code it no longer runs (T-1239).",
|
||||
ok=False,
|
||||
fix=(
|
||||
"set config/version in client/project.godot's [application] "
|
||||
f"section to {result.yaml_version} — project.yaml is the source of truth"
|
||||
),
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
console.verdict(f"check-client-version: OK — {result.yaml_version}")
|
||||
@@ -0,0 +1,20 @@
|
||||
"""Data shapes for the `check` domain.
|
||||
|
||||
Stdlib dataclass for now. T-1262 converts this to pydantic as part of making
|
||||
this domain the reference implementation — pydantic is available to every
|
||||
domain since the D-263 budget amendment dropped the gate-path carve-out.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from dataclasses import dataclass
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class VersionCheck:
|
||||
"""The outcome of comparing project.yaml against client/project.godot."""
|
||||
|
||||
ok: bool
|
||||
yaml_version: str | None = None
|
||||
godot_version: str | None = None
|
||||
problem: str | None = None
|
||||
@@ -0,0 +1,62 @@
|
||||
"""Logic for the `check` domain. Transport-agnostic (D-263).
|
||||
|
||||
Nothing here prints, calls `sys.exit`, or imports typer. A service must not know
|
||||
it was called from a CLI — that is what lets a test call it directly, lets one
|
||||
domain's service call another's, and leaves a second front end possible without
|
||||
a rewrite.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import re
|
||||
from pathlib import Path
|
||||
|
||||
from tooling.core import config
|
||||
from tooling.domains.check.schemas import VersionCheck
|
||||
|
||||
# Anchored to line start so the commentary above `version:` (which mentions
|
||||
# earlier versions by number) can never be mistaken for the field itself.
|
||||
_YAML_VERSION = re.compile(r"^version:\s*(\S+)\s*$", re.MULTILINE)
|
||||
_GODOT_VERSION = re.compile(r'^config/version\s*=\s*"([^"]*)"\s*$', re.MULTILINE)
|
||||
|
||||
|
||||
def client_version() -> VersionCheck:
|
||||
"""Compare the version in project.yaml with the one baked into the client.
|
||||
|
||||
project.yaml is the version source of truth (CLAUDE.md). The client cannot
|
||||
read it at runtime — an exported build has no repo root — so the value is
|
||||
mirrored into `application/config/version` in client/project.godot, which
|
||||
Godot bakes into the PCK (T-1241).
|
||||
|
||||
A mirror nobody checks is worse than the bug it replaced: the old code
|
||||
failed LOUDLY in an export ("?.?.?" everywhere), whereas a stale mirror
|
||||
fails SILENTLY — the Atlas disk cache keeps serving canvases under a version
|
||||
that stopped matching the build. That is the T-1239 failure, which cost
|
||||
eight days of a map drawn from a canvas whose generating code no longer
|
||||
existed.
|
||||
"""
|
||||
root = config.repo_root()
|
||||
yaml_version, problem = _read(root / "project.yaml", _YAML_VERSION, "`version:` line")
|
||||
if problem:
|
||||
return VersionCheck(ok=False, problem=problem)
|
||||
|
||||
godot_path = root / "client" / "project.godot"
|
||||
godot_version, problem = _read(godot_path, _GODOT_VERSION, "`config/version=` line")
|
||||
if problem:
|
||||
return VersionCheck(ok=False, yaml_version=yaml_version, problem=problem)
|
||||
|
||||
return VersionCheck(
|
||||
ok=yaml_version == godot_version,
|
||||
yaml_version=yaml_version,
|
||||
godot_version=godot_version,
|
||||
)
|
||||
|
||||
|
||||
def _read(path: Path, pattern: re.Pattern[str], label: str) -> tuple[str | None, str | None]:
|
||||
"""Return (value, problem). Exactly one of the two is ever set."""
|
||||
if not path.exists():
|
||||
return None, f"{path} not found"
|
||||
match = pattern.search(path.read_text(encoding="utf-8"))
|
||||
if not match:
|
||||
return None, f"no {label} in {path}"
|
||||
return match.group(1), None
|
||||
+67
-5
@@ -14,8 +14,8 @@ app" is unsupported. Mixing a real `click.Group` root with typer sub-apps would
|
||||
mean two different Click implementations in one process.
|
||||
|
||||
So the customisation surface is `typer.Typer(cls=...)` with a `TyperGroup`
|
||||
subclass, which is the supported path and is what T-1260 uses to register
|
||||
domains lazily.
|
||||
subclass, which is the supported path and is what `LazyDomainGroup` below uses
|
||||
to register domains lazily.
|
||||
|
||||
`rich_markup_mode=None` is not a style preference — it is worth 94 ms of the
|
||||
168 ms an empty `--help` otherwise costs, and it keeps `rich` and `pygments`
|
||||
@@ -26,17 +26,79 @@ are ever wanted more than the milliseconds.
|
||||
|
||||
The callback below is not decoration. A `typer.Typer` with **no commands and no
|
||||
callback** raises `RuntimeError: Could not get a command for this Typer
|
||||
instance` at build time; with a callback it builds fine and prints help. Since
|
||||
domains are registered lazily and none are eager, the callback is what makes an
|
||||
empty root legal.
|
||||
instance` at build time; with a callback it builds fine. Since every domain is
|
||||
registered lazily and none is eager, the callback is what makes the root legal.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
|
||||
import typer
|
||||
from typer.core import TyperGroup
|
||||
|
||||
# The domain registry: name -> (import target, one-line help).
|
||||
#
|
||||
# This table is the ONLY thing `reach --help` reads. The short help lives here
|
||||
# as a literal string rather than being pulled off the loaded command, because
|
||||
# reading it off the command is precisely what would import the world — see
|
||||
# LazyDomainGroup.format_commands.
|
||||
#
|
||||
# Adding a domain is adding a line here plus a `router.py` that exposes `app`.
|
||||
DOMAINS: dict[str, tuple[str, str]] = {
|
||||
"check": (
|
||||
"tooling.domains.check.router:app",
|
||||
"Consistency gates — the checks the push hook runs",
|
||||
),
|
||||
}
|
||||
|
||||
|
||||
class LazyDomainGroup(TyperGroup):
|
||||
"""Lists domains without importing them; imports exactly the one invoked.
|
||||
|
||||
Three overrides, and the third is the one that matters. `TyperGroup`'s own
|
||||
`format_commands` loops over `list_commands` calling `get_command` on each,
|
||||
just to read a short help string off the loaded command — which, with lazy
|
||||
loading underneath, imports every domain in the registry to render `--help`.
|
||||
That would defeat the whole mechanism silently, while looking correct.
|
||||
|
||||
So `format_commands` is overridden to read help from `DOMAINS` and never
|
||||
touch `get_command`. The cost of `reach --help` is then flat no matter how
|
||||
many domains exist, which is the property that has to hold as this grows
|
||||
from one domain to a dozen.
|
||||
"""
|
||||
|
||||
def list_commands(self, ctx: typer.Context) -> list[str]:
|
||||
return sorted({*super().list_commands(ctx), *DOMAINS})
|
||||
|
||||
def get_command(self, ctx: typer.Context, cmd_name: str):
|
||||
if cmd_name in DOMAINS:
|
||||
return _load_domain(cmd_name)
|
||||
return super().get_command(ctx, cmd_name)
|
||||
|
||||
def format_commands(self, ctx: typer.Context, formatter) -> None:
|
||||
# Deliberately does NOT call get_command. See the class docstring.
|
||||
rows = [(name, short) for name, (_target, short) in sorted(DOMAINS.items())]
|
||||
for name in sorted(super().list_commands(ctx)):
|
||||
command = super().get_command(ctx, name)
|
||||
if command is not None and not command.hidden:
|
||||
rows.append((name, command.get_short_help_str(80)))
|
||||
if rows:
|
||||
with formatter.section("Domains"):
|
||||
formatter.write_dl(sorted(rows))
|
||||
|
||||
|
||||
def _load_domain(name: str):
|
||||
"""Import one domain's router and convert its Typer app to a command."""
|
||||
target, _short = DOMAINS[name]
|
||||
module_name, _, attr = target.partition(":")
|
||||
module = importlib.import_module(module_name)
|
||||
return typer.main.get_command(getattr(module, attr))
|
||||
|
||||
|
||||
cli = typer.Typer(
|
||||
name="reach",
|
||||
cls=LazyDomainGroup,
|
||||
help="Repo tooling for The Settled Reach.\n\nRun `reach <domain> --help` to see what a domain can do.",
|
||||
no_args_is_help=True,
|
||||
add_completion=False,
|
||||
|
||||
@@ -0,0 +1,147 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Units for lazy domain registration (T-1260).
|
||||
|
||||
`reach --help` must list every domain WITHOUT importing any of them. That is not
|
||||
tidiness: an eager entrypoint pays for every domain's imports on every
|
||||
invocation, and the expensive ones are already in this tree — scipy.ndimage
|
||||
alone is 275 ms. The cost of `--help` has to stay flat as the registry grows
|
||||
from one domain to a dozen.
|
||||
|
||||
The trap this guards is specific and quiet. `TyperGroup.format_commands` loops
|
||||
over `list_commands` calling `get_command` on each, purely to read a short help
|
||||
string off the loaded command. With lazy loading underneath, that imports every
|
||||
domain in the registry to render `--help` — while looking entirely correct.
|
||||
Nothing about the output changes; only the import graph does. So the assertion
|
||||
has to be on `sys.modules`, not on what `--help` prints.
|
||||
|
||||
Three properties, and the third is what stops the first two passing vacuously:
|
||||
|
||||
1. After `--help`, nothing under `tooling.domains` is imported.
|
||||
2. After `--help`, no heavy third-party module is imported.
|
||||
3. POSITIVE CONTROL: actually invoking a domain DOES import its service. If
|
||||
this fails, properties 1 and 2 are meaningless — they would also pass for a
|
||||
CLI whose lazy loader is broken and never imports anything at all.
|
||||
|
||||
Run: python3 tooling/test_lazy_domains.py
|
||||
"""
|
||||
|
||||
import json
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||
|
||||
# Modules that must never be dragged in by `--help`. pydantic is on the list
|
||||
# because domain schemas use it: it must load with the domain, not with the CLI.
|
||||
HEAVY = ("rich", "pygments", "numpy", "scipy", "pydantic", "PIL")
|
||||
|
||||
_HELP_PROBE = """
|
||||
import contextlib, io, json, sys
|
||||
from tooling.main import cli, DOMAINS
|
||||
|
||||
buf = io.StringIO()
|
||||
try:
|
||||
with contextlib.redirect_stdout(buf):
|
||||
cli(args=["--help"])
|
||||
except SystemExit:
|
||||
pass
|
||||
|
||||
print(json.dumps({
|
||||
"help": buf.getvalue(),
|
||||
"domain_modules": sorted(
|
||||
m for m in sys.modules
|
||||
if m == "tooling.domains" or m.startswith("tooling.domains.")
|
||||
),
|
||||
"heavy": sorted(m for m in %(heavy)r if m in sys.modules),
|
||||
"declared": sorted(DOMAINS),
|
||||
}))
|
||||
"""
|
||||
|
||||
_INVOKE_PROBE = """
|
||||
import contextlib, io, json, sys
|
||||
from tooling.main import cli
|
||||
|
||||
buf = io.StringIO()
|
||||
try:
|
||||
with contextlib.redirect_stdout(buf):
|
||||
cli(args=["check", "client-version"])
|
||||
except SystemExit:
|
||||
pass
|
||||
|
||||
print(json.dumps({
|
||||
"domain_modules": sorted(
|
||||
m for m in sys.modules
|
||||
if m == "tooling.domains" or m.startswith("tooling.domains.")
|
||||
),
|
||||
}))
|
||||
"""
|
||||
|
||||
|
||||
def _probe(source: str) -> dict:
|
||||
"""Run a snippet in a FRESH interpreter and return its JSON verdict.
|
||||
|
||||
A subprocess, not an in-process import, because this test is entirely about
|
||||
a module graph — running it in the harness's own interpreter would inherit
|
||||
whatever the harness already imported and prove nothing.
|
||||
"""
|
||||
result = subprocess.run(
|
||||
[sys.executable, "-c", source],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
cwd=REPO_ROOT,
|
||||
)
|
||||
if result.returncode != 0:
|
||||
raise SystemExit(f"probe failed (exit {result.returncode}):\n{result.stderr}")
|
||||
return json.loads(result.stdout)
|
||||
|
||||
|
||||
def main() -> int:
|
||||
failures = []
|
||||
|
||||
helped = _probe(_HELP_PROBE % {"heavy": HEAVY})
|
||||
|
||||
# Guard against a vacuous pass: an empty registry would satisfy every
|
||||
# assertion below while proving nothing at all.
|
||||
if not helped["declared"]:
|
||||
failures.append("DOMAINS registry is empty — every assertion here would pass vacuously")
|
||||
|
||||
for name in helped["declared"]:
|
||||
if name not in helped["help"]:
|
||||
failures.append(f"`--help` does not list the declared domain {name!r}")
|
||||
|
||||
if helped["domain_modules"]:
|
||||
failures.append(
|
||||
"`--help` imported domain modules, so registration is not lazy: "
|
||||
+ ", ".join(helped["domain_modules"])
|
||||
+ "\n The usual cause is format_commands falling back to the base "
|
||||
"implementation,\n which calls get_command on every subcommand to read its short help."
|
||||
)
|
||||
|
||||
if helped["heavy"]:
|
||||
failures.append("`--help` imported heavy modules: " + ", ".join(helped["heavy"]))
|
||||
|
||||
# Positive control. Without this, the assertions above would pass for a CLI
|
||||
# that is simply broken and imports nothing ever.
|
||||
invoked = _probe(_INVOKE_PROBE)
|
||||
if "tooling.domains.check.service" not in invoked["domain_modules"]:
|
||||
failures.append(
|
||||
"positive control FAILED: invoking `check client-version` did not import "
|
||||
"tooling.domains.check.service, so the lazy-import assertions above prove nothing"
|
||||
)
|
||||
|
||||
if failures:
|
||||
print("test_lazy_domains: FAIL", file=sys.stderr)
|
||||
for failure in failures:
|
||||
print(f" - {failure}", file=sys.stderr)
|
||||
return 1
|
||||
|
||||
print(
|
||||
f"test_lazy_domains: OK — {len(helped['declared'])} domain(s) listed, "
|
||||
"none imported; positive control confirms the loader works"
|
||||
)
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
Reference in New Issue
Block a user