refactor(tooling): T-1273 — the Blender carve-out, and a guard that keeps it carved
35 payloads move to tooling/scripts/blender/ and stay outside package scope. They run under Blender's bundled Python, which cannot see the repo venv, so they physically cannot import tooling.core — holding them to the D-263 contract would either fail the gate forever or force the contract to be weakened for everyone, and the second is how a gate stops meaning anything. Count verified by import rather than filename: 33 import bpy/bmesh directly, and the two that do not are still payloads per their own usage lines. garment-fit/make_logo.py is the one genuine non-payload and stays for T-1290. The bash wrapper is retired rather than kept. Keeping it would have put the install-resolution logic in two places, which is the duplication T-1286 had just finished collapsing three copies of. domains/blender/service.py owns the decisions — resolve_blender (native beats flatpak, ordering preserved), resolve_payload, absolutise — and only run_payload performs. test_blender.py pins all of them without launching Blender, which matters here more than usual: the thing being launched is a 200 MB GUI application that writes GLBs. `reach blender run` takes a registered payload name OR a path to any script, because the wrapper served both — the spikes and the glb-gen skill hand it one-off scripts of their own. An unknown name enumerates all 35 and exits 2. The exclusion now defends itself. check_carve_out_stays_carved fails if `scripts` is added to PACKAGE_ROOTS, if the payload directory empties (an empty exclusion proves nothing), or if an __init__.py appears there (which would make the payloads importable — the coupling the carve-out exists to prevent). All three arms mutation-proved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
+1
-1
@@ -98,7 +98,7 @@ invented.
|
||||
| `generate` | content generators not owned elsewhere | ✅ ported (T-1286). `generate-brands` + `generate-corporations` collapsed into `core.process.cargo_binary` — they were the same 24 lines of bash a third time |
|
||||
| `dev` | developer environment and workflow | ✅ ported (T-1286). The three environment scripts split decision from performing — `godot_plan`/`worktree_plan` are pure and pinned by `test_environment.py` |
|
||||
| `pr` | the PR/review loop | ✅ ported (T-1286). `watchlist-diff` now reads the watched set from `generator_sources.py` instead of restating it |
|
||||
| `blender` | **carve-out** — payloads run by Blender | **35** `blender_*.py` (13 top-level + 22 in `garment-fit/`), `blender` wrapper → `tooling/scripts/blender/` |
|
||||
| `blender` | **carve-out** — payloads run by Blender | ✅ done (T-1273). 35 payloads in `tooling/scripts/blender/`, excluded from package scope and from importability; the bash wrapper's install-resolution and path-absolutising became `domains/blender/service.py`, pinned by `test_blender.py` |
|
||||
|
||||
## Judgment calls, with reasons
|
||||
|
||||
|
||||
@@ -1,28 +0,0 @@
|
||||
#!/usr/bin/env bash
|
||||
# Blender wrapper — resolves flatpak/native/brew installs to a single entry point.
|
||||
# All project scripts should call this instead of 'blender' directly.
|
||||
#
|
||||
# Resolves relative paths in --python and -- args to absolute paths so flatpak
|
||||
# sandboxing doesn't break file resolution.
|
||||
#
|
||||
# Usage:
|
||||
# tooling/blender --background --python script.py -- input.glb output.glb
|
||||
|
||||
# Convert relative paths to absolute
|
||||
args=()
|
||||
for arg in "$@"; do
|
||||
if [[ -e "$arg" ]]; then
|
||||
args+=("$(realpath "$arg")")
|
||||
else
|
||||
args+=("$arg")
|
||||
fi
|
||||
done
|
||||
|
||||
if command -v blender &>/dev/null; then
|
||||
exec blender "${args[@]}"
|
||||
elif flatpak info org.blender.Blender &>/dev/null 2>&1; then
|
||||
exec flatpak run org.blender.Blender "${args[@]}"
|
||||
else
|
||||
echo "ERROR: Blender not found (checked PATH and flatpak)" >&2
|
||||
exit 1
|
||||
fi
|
||||
@@ -0,0 +1,16 @@
|
||||
"""The Blender carve-out — the one sanctioned exception to the reach rule.
|
||||
|
||||
D-263 says tooling lives in the package and every verb carries `@command`.
|
||||
`tooling/scripts/blender/` does not, and cannot: those 35 files run under
|
||||
Blender's BUNDLED Python interpreter, which has no access to the repo venv and
|
||||
therefore cannot `import tooling.core` at all. They are payloads handed to
|
||||
another program, not modules this package imports.
|
||||
|
||||
So the split is: the payloads stay dumb and stay out of package scope, and
|
||||
everything that can be decided in our own interpreter — which Blender to use,
|
||||
which payload a name refers to, which paths need absolutising — lives here and
|
||||
is testable. The user-facing form is still a normal verb.
|
||||
|
||||
The conformance test excludes `tooling/scripts/` for this reason, and says so
|
||||
inline. It is an exception with a stated cause, not an oversight.
|
||||
"""
|
||||
@@ -0,0 +1,65 @@
|
||||
"""Transport for the `blender` domain — args in, delegate, format out."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import typer
|
||||
|
||||
from tooling.core import cli, console
|
||||
from tooling.core.command import command
|
||||
from tooling.domains.blender import service
|
||||
|
||||
app = cli.domain("blender", "Run the Blender payloads — the one carve-out.")
|
||||
|
||||
|
||||
@app.callback()
|
||||
def _domain() -> None:
|
||||
"""Keeps `blender` a group (Typer collapses a single-command app)."""
|
||||
|
||||
|
||||
@app.command("list")
|
||||
@command
|
||||
def list_payloads() -> None:
|
||||
"""List every payload `run` accepts.
|
||||
|
||||
Worth a verb of its own: the payload names are not guessable and this is
|
||||
the only place the vocabulary is written down.
|
||||
"""
|
||||
names = service.payloads()
|
||||
for name in names:
|
||||
console.out(name)
|
||||
console.verdict(f"{len(names)} payload(s) in {service.payload_dir().name}/")
|
||||
|
||||
|
||||
@app.command("which")
|
||||
@command
|
||||
def which() -> None:
|
||||
"""Report which Blender would be used, without launching it."""
|
||||
invocation = service.resolve_blender()
|
||||
console.out(f"kind {invocation.kind}")
|
||||
console.out(f"command {' '.join(invocation.argv_prefix)}")
|
||||
console.verdict(
|
||||
"flatpak — paths are absolutised before they cross the sandbox"
|
||||
if invocation.sandboxed
|
||||
else "native install — preferred when both are present"
|
||||
)
|
||||
|
||||
|
||||
@app.command("run", context_settings={"allow_extra_args": True, "ignore_unknown_options": True})
|
||||
@command
|
||||
def run(
|
||||
ctx: typer.Context,
|
||||
payload: str = typer.Argument(
|
||||
..., help="Payload name (see `reach blender list`), or a path to any script."
|
||||
),
|
||||
background: bool = typer.Option(
|
||||
True, "--background/--no-background", help="Headless, or open the Blender GUI."
|
||||
),
|
||||
) -> None:
|
||||
"""Run a payload under Blender, passing any remaining arguments through.
|
||||
|
||||
Extra arguments go to the payload after `--`, and any that name an existing
|
||||
path are made absolute first — flatpak's sandbox resolves relative paths
|
||||
against a different root, and the resulting file-not-found comes from
|
||||
inside Blender, a long way from its cause.
|
||||
"""
|
||||
service.run_payload(payload, list(ctx.args), background=background)
|
||||
@@ -0,0 +1,133 @@
|
||||
"""Logic for the `blender` domain. Transport-agnostic (D-263).
|
||||
|
||||
Everything the bash wrapper decided is decided here, and every decision is a
|
||||
pure function: which Blender install to use, which payload a name refers to,
|
||||
which arguments are paths that need absolutising. Only `run_payload` performs.
|
||||
|
||||
That matters more than usual for this domain, because the thing being launched
|
||||
is a 200 MB GUI application that opens files and writes GLBs. "Run it and see"
|
||||
is not a test.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import shutil
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
|
||||
from tooling.core import config, console, process
|
||||
from tooling.core.errors import ReachError, unknown_choice
|
||||
|
||||
PAYLOAD_DIR = ("tooling", "scripts", "blender")
|
||||
|
||||
# Flatpak is checked second: a native install is faster and has no sandbox, so
|
||||
# it wins when both are present. This ordering is the bash wrapper's and is
|
||||
# preserved deliberately.
|
||||
FLATPAK_APP = "org.blender.Blender"
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class BlenderInvocation:
|
||||
"""How Blender would be launched — decided without launching it."""
|
||||
|
||||
argv_prefix: list[str]
|
||||
kind: str # "native" | "flatpak"
|
||||
|
||||
@property
|
||||
def sandboxed(self) -> bool:
|
||||
"""Flatpak cannot see paths outside its sandbox unless they are absolute."""
|
||||
return self.kind == "flatpak"
|
||||
|
||||
|
||||
def resolve_blender() -> BlenderInvocation:
|
||||
"""Find Blender, preferring a native install over flatpak.
|
||||
|
||||
Raises rather than falling through to a bare `blender` that is not there —
|
||||
the original printed its own error and exited 1, and losing that would turn
|
||||
a missing dependency into a confusing FileNotFoundError.
|
||||
"""
|
||||
native = shutil.which("blender")
|
||||
if native:
|
||||
return BlenderInvocation([native], "native")
|
||||
|
||||
if shutil.which("flatpak"):
|
||||
probe = process.run(["flatpak", "info", FLATPAK_APP], check=False)
|
||||
if probe.returncode == 0:
|
||||
return BlenderInvocation(["flatpak", "run", FLATPAK_APP], "flatpak")
|
||||
|
||||
raise ReachError(
|
||||
"Blender is not installed (checked PATH and flatpak)",
|
||||
fix="install Blender natively, or run: flatpak install "
|
||||
f"{FLATPAK_APP} — reach prefers the native install when both exist",
|
||||
)
|
||||
|
||||
|
||||
def payload_dir() -> Path:
|
||||
return config.path(*PAYLOAD_DIR)
|
||||
|
||||
|
||||
def payloads() -> list[str]:
|
||||
"""Every runnable payload, by the name `run` accepts."""
|
||||
return sorted(p.stem for p in payload_dir().glob("*.py") if p.name != "__init__.py")
|
||||
|
||||
|
||||
def resolve_payload(name: str) -> Path:
|
||||
"""Map a payload name — or a path to any script — to a file to run.
|
||||
|
||||
Two forms, because the bash wrapper this replaces served both: our own 35
|
||||
registered payloads BY NAME, and any other script BY PATH (the spikes and
|
||||
the glb-gen skill hand it one-off scripts of their own). A registered name
|
||||
wins; anything that resolves to an existing file is accepted as a path.
|
||||
|
||||
On a miss it enumerates, because a payload name is not guessable — the 35
|
||||
names are the only place that vocabulary is written down.
|
||||
"""
|
||||
stem = name[:-3] if name.endswith(".py") else name
|
||||
registered = payload_dir() / f"{stem}.py"
|
||||
if registered.is_file():
|
||||
return registered
|
||||
|
||||
given = Path(name)
|
||||
if given.is_file():
|
||||
return given.resolve()
|
||||
|
||||
raise unknown_choice("payload", stem, payloads())
|
||||
|
||||
|
||||
def absolutise(args: list[str]) -> list[str]:
|
||||
"""Resolve any argument that names an existing path.
|
||||
|
||||
The wrapper did this for every argument, not just `--python`, because
|
||||
flatpak's sandbox resolves relative paths against a different root and the
|
||||
failure is a file-not-found from inside Blender — far from the cause. An
|
||||
argument that is not an existing path is passed through untouched, so flags
|
||||
and values survive.
|
||||
"""
|
||||
return [str(Path(a).resolve()) if Path(a).exists() else a for a in args]
|
||||
|
||||
|
||||
def run_payload(name: str, args: list[str], background: bool = True) -> None:
|
||||
"""Launch a payload under Blender. The performing half."""
|
||||
payload = resolve_payload(name)
|
||||
invocation = resolve_blender()
|
||||
|
||||
argv = [*invocation.argv_prefix]
|
||||
if background:
|
||||
argv.append("--background")
|
||||
argv += ["--python", str(payload)]
|
||||
if args:
|
||||
argv += ["--", *absolutise(args)]
|
||||
|
||||
console.event(
|
||||
f"running {name} under {invocation.kind} Blender",
|
||||
phase="blender",
|
||||
)
|
||||
# capture=False: Blender streams its own progress, and a payload that
|
||||
# takes minutes with no output reads as a hang.
|
||||
process.run(
|
||||
argv,
|
||||
cwd=config.repo_root(),
|
||||
capture=False,
|
||||
fix=f"run `reach blender run {name}` with --no-background to watch it in the GUI",
|
||||
)
|
||||
console.verdict(f"blender: {name} finished")
|
||||
@@ -71,6 +71,10 @@ DOMAINS: dict[str, tuple[str, str]] = {
|
||||
"tooling.domains.generate.router:app",
|
||||
"Producers — brands, corporations, manifests and stubs",
|
||||
),
|
||||
"blender": (
|
||||
"tooling.domains.blender.router:app",
|
||||
"Run the Blender payloads — bodies, garments, inspection",
|
||||
),
|
||||
"pr": (
|
||||
"tooling.domains.pr.router:app",
|
||||
"The review loop — comments, watchlist and the board",
|
||||
|
||||
@@ -0,0 +1,127 @@
|
||||
#!/usr/bin/env python3
|
||||
"""The `reach blender` decisions, exercised without launching Blender (T-1273).
|
||||
|
||||
Same principle as test_environment.py, and for a stronger reason: the thing
|
||||
being launched is a 200 MB GUI application that opens files and writes GLBs, so
|
||||
"run it and see" is not a test. Every decision the retired bash wrapper made —
|
||||
which install to use, which script a name refers to, which arguments need
|
||||
absolutising — is a pure function here, and this is what pins them.
|
||||
|
||||
The absolutise rule is the one worth having a test for. Flatpak resolves
|
||||
relative paths against a different root, so a relative path silently becomes a
|
||||
file-not-found raised from inside Blender, a long way from its cause.
|
||||
|
||||
Run: python3 tooling/test_blender.py
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||
sys.path.insert(0, str(REPO_ROOT))
|
||||
|
||||
from tooling.core.errors import ReachError # noqa: E402
|
||||
from tooling.domains.blender import service # noqa: E402
|
||||
|
||||
|
||||
def test_payloads_are_discoverable(failures: list[str]) -> None:
|
||||
"""The carve-out is the only place these names are written down."""
|
||||
names = service.payloads()
|
||||
if len(names) < 30:
|
||||
failures.append(f"expected the 35-file carve-out, found {len(names)} payloads")
|
||||
if any(n.endswith(".py") for n in names):
|
||||
failures.append("payload names should be stems, not filenames")
|
||||
|
||||
|
||||
def test_registered_name_resolves(failures: list[str]) -> None:
|
||||
"""Both the stem and the filename form reach the same file."""
|
||||
first = service.payloads()[0]
|
||||
by_stem = service.resolve_payload(first)
|
||||
by_filename = service.resolve_payload(f"{first}.py")
|
||||
if by_stem != by_filename:
|
||||
failures.append(f"'{first}' and '{first}.py' resolved differently")
|
||||
if by_stem.parent != service.payload_dir():
|
||||
failures.append(f"a registered name resolved outside the payload dir: {by_stem}")
|
||||
|
||||
|
||||
def test_arbitrary_path_resolves(failures: list[str]) -> None:
|
||||
"""The wrapper ran one-off scripts too — the spikes and glb-gen still do."""
|
||||
with tempfile.NamedTemporaryFile(suffix=".py", delete=False) as handle:
|
||||
handle.write(b"# a one-off script\n")
|
||||
loose = Path(handle.name)
|
||||
try:
|
||||
resolved = service.resolve_payload(str(loose))
|
||||
if resolved != loose.resolve():
|
||||
failures.append(f"a script path resolved to {resolved}, expected {loose}")
|
||||
if not resolved.is_absolute():
|
||||
failures.append("a script path resolved to a relative path")
|
||||
finally:
|
||||
loose.unlink()
|
||||
|
||||
|
||||
def test_unknown_name_enumerates(failures: list[str]) -> None:
|
||||
"""A payload name is not guessable, so a miss must list the options."""
|
||||
try:
|
||||
service.resolve_payload("definitely_not_a_payload")
|
||||
failures.append("an unknown payload resolved instead of raising")
|
||||
except ReachError as exc:
|
||||
if not exc.fix or "blender_" not in exc.fix:
|
||||
failures.append("the unknown-payload error does not enumerate the payloads")
|
||||
if exc.exit_code != 2:
|
||||
failures.append(
|
||||
f"an unknown payload exited {exc.exit_code}, expected 2 (usage error)"
|
||||
)
|
||||
|
||||
|
||||
def test_absolutise_only_touches_real_paths(failures: list[str]) -> None:
|
||||
"""Flags and values must survive; existing paths must be made absolute."""
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
real = Path(tmp) / "input.glb"
|
||||
real.write_text("")
|
||||
args = ["--mode", "fit", str(real), "not/a/real/path.glb", "12"]
|
||||
out = service.absolutise(args)
|
||||
|
||||
if out[0] != "--mode" or out[1] != "fit" or out[4] != "12":
|
||||
failures.append(f"absolutise mangled non-path arguments: {out}")
|
||||
if out[2] != str(real.resolve()):
|
||||
failures.append(f"an existing path was not absolutised: {out[2]}")
|
||||
if out[3] != "not/a/real/path.glb":
|
||||
failures.append(f"a non-existent path was rewritten: {out[3]}")
|
||||
|
||||
|
||||
def test_invocation_reports_its_sandbox(failures: list[str]) -> None:
|
||||
"""`sandboxed` is what decides whether absolutising is load-bearing."""
|
||||
native = service.BlenderInvocation(["blender"], "native")
|
||||
flatpak = service.BlenderInvocation(["flatpak", "run", service.FLATPAK_APP], "flatpak")
|
||||
if native.sandboxed:
|
||||
failures.append("a native install was reported as sandboxed")
|
||||
if not flatpak.sandboxed:
|
||||
failures.append("a flatpak install was not reported as sandboxed")
|
||||
|
||||
|
||||
def main() -> int:
|
||||
failures: list[str] = []
|
||||
test_payloads_are_discoverable(failures)
|
||||
test_registered_name_resolves(failures)
|
||||
test_arbitrary_path_resolves(failures)
|
||||
test_unknown_name_enumerates(failures)
|
||||
test_absolutise_only_touches_real_paths(failures)
|
||||
test_invocation_reports_its_sandbox(failures)
|
||||
|
||||
if failures:
|
||||
print("test_blender: FAIL", file=sys.stderr)
|
||||
for failure in failures:
|
||||
print(f" - {failure}", file=sys.stderr)
|
||||
return 1
|
||||
print(
|
||||
"test_blender: OK — payload discovery, both resolve forms, enumeration "
|
||||
"on a miss and the flatpak path rule, without launching Blender"
|
||||
)
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
@@ -58,6 +58,18 @@ FORBIDDEN_IMPORTS = {"typer", "click"}
|
||||
# needing to be widened by hand.
|
||||
PACKAGE_ROOTS = ("main.py", "__init__.py", "core", "domains")
|
||||
|
||||
# tooling/scripts/blender/ is DELIBERATELY not in that tuple, and must never be
|
||||
# added (T-1273). Those 35 files run under Blender's BUNDLED Python, which has
|
||||
# no access to the repo venv — they physically cannot `import tooling.core`, so
|
||||
# they cannot carry `@command` or print through `console`. Holding them to the
|
||||
# contract would either fail this gate forever or force the contract to be
|
||||
# weakened for everyone, and the second is how a gate stops meaning anything.
|
||||
#
|
||||
# It reads like an oversight, so `check_carve_out_stays_carved` below asserts
|
||||
# the exclusion on purpose: widening PACKAGE_ROOTS to cover them fails loudly
|
||||
# instead of quietly redefining what conformance means.
|
||||
PAYLOAD_ROOTS = ("scripts",)
|
||||
|
||||
|
||||
def _package_files() -> list[Path]:
|
||||
"""Every .py that is part of the reach package — not the legacy scripts."""
|
||||
@@ -303,6 +315,42 @@ def check_exit_codes(failures: list[str]) -> None:
|
||||
)
|
||||
|
||||
|
||||
def check_carve_out_stays_carved(failures: list[str]) -> None:
|
||||
"""(7) The Blender payloads stay outside package scope, and stay populated.
|
||||
|
||||
Two failure modes, opposite directions:
|
||||
|
||||
- Someone widens PACKAGE_ROOTS to cover `scripts/` because the exclusion
|
||||
looks like an oversight. Every payload then fails five invariants it
|
||||
cannot satisfy, and the likely repair is to weaken the invariants.
|
||||
- The payload directory quietly empties — a move, a bad merge — and the
|
||||
exclusion goes on passing because excluding nothing is trivially fine.
|
||||
An exception that guards nothing should not read as healthy.
|
||||
"""
|
||||
for root in PAYLOAD_ROOTS:
|
||||
if root in PACKAGE_ROOTS:
|
||||
failures.append(
|
||||
f"[carve-out] '{root}' was added to PACKAGE_ROOTS — the Blender "
|
||||
"payloads run under Blender's bundled Python and cannot import "
|
||||
"tooling.core; see the comment above PAYLOAD_ROOTS (T-1273)"
|
||||
)
|
||||
|
||||
payloads = list((PACKAGE / "scripts" / "blender").glob("*.py"))
|
||||
if not payloads:
|
||||
failures.append(
|
||||
"[carve-out] tooling/scripts/blender/ holds no payloads — either the "
|
||||
"carve-out was undone or they moved; an empty exclusion proves nothing"
|
||||
)
|
||||
|
||||
# The payloads must not be reachable as modules either: an __init__.py would
|
||||
# make them importable and invite exactly the coupling the carve-out prevents.
|
||||
if (PACKAGE / "scripts" / "__init__.py").exists():
|
||||
failures.append(
|
||||
"[carve-out] tooling/scripts/__init__.py exists — that makes the "
|
||||
"payload tree an importable package, which is what the carve-out avoids"
|
||||
)
|
||||
|
||||
|
||||
def main() -> int:
|
||||
if shutil.which("reach") is None:
|
||||
print("test_conformance: `reach` is not on PATH.\n Fix: make install-reach", file=sys.stderr)
|
||||
@@ -316,6 +364,7 @@ def main() -> int:
|
||||
check_commands_decorated(failures)
|
||||
check_errors_name_a_remedy(failures)
|
||||
check_exit_codes(failures)
|
||||
check_carve_out_stays_carved(failures)
|
||||
|
||||
if failures:
|
||||
print("test_conformance: FAIL", file=sys.stderr)
|
||||
|
||||
Reference in New Issue
Block a user