diff --git a/.pql/changelog/ticket_history/2026-08.sql b/.pql/changelog/ticket_history/2026-08.sql index 985d266d2..9ed5b6592 100644 --- a/.pql/changelog/ticket_history/2026-08.sql +++ b/.pql/changelog/ticket_history/2026-08.sql @@ -2085,3 +2085,6 @@ SEQUENCING SET 2026-08-31 by decision. (1) T-1264 (streaming, as decorators) lan WAVE 1 FILED 2026-08-31: T-1281 check (four remaining gates), T-1282 validate, T-1283 godot and visual together. Deliberately NOT filing all fourteen domains at once — the standard port acceptance is defined once on T-1281 and referenced, and the remaining tickets (atlas, starmap, planet, db, wiki, assets, character, generate, dev, pr) get written after wave 1 lands, when the real friction of a port is known rather than guessed. Four descriptions written from experience beat ten written from a map. T-1272 was also RESCOPED today: the hyphenated directories are emptied by the domain moves rather than renamed, so it becomes a closing verification instead of an opening step. SCOPE CORRECTION 2026-08-31, found on starting the first port and materially larger than the epic assumed. SEVENTEEN of the 33 extensionless executables are BASH, not Python — about 900 lines. This description and the domain map both assumed a Python tree, so those are REWRITES, not moves. Decided with Jeroen: rewrite all of them in Python rather than wrap any. Wrapping would achieve one door while leaving half the CLI surface outside the contract — no @command, no remedy on failure, no streaming, no testable service — so reach --help would list verbs that behave differently from the ones beside them, which is worse than two doors because the inconsistency is invisible until something fails. D-263 amended. The bash inventory, by shape: THIN WRAPPERS that mostly invoke something else (atlas-systems-done 9, atlas-names 17, generate-brands 23, atlas 24, generate-corporations 23, blender 28) — these become routers calling the same thing, nearly free. LOGIC that computes a verdict (check-fact-ids 89, validate-ron 137, godot-parse-sweep 64, godot-cold-parse 96, pr-watchlist-diff 37, atlas-update-field 59, atlas-commit-and-sync 60, tea-comment 41) — these gain the most from becoming services. ENVIRONMENT scripts (install-godot 108, install-rust 33, worktree-setup 49) — these gain least and carry the most regression risk, because downloading and unzipping a Godot build or driving rustup is awkward to exercise in a gate; port their DECISION logic into a testable service and keep the irreducible external calls behind core/process. Per-domain port tickets must state which of their sources are bash, since a bash source changes a port from mechanical to a rewrite needing its own parity evidence.', NULL, '2026-08-31 15:33:16', '2026-08-31 15:33:16.713', '2026-08-31 15:33:16.713', NULL, '158c68d98b9221d0ebe6d876be60918e', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5GWCTS89VSST07DFVBVG064', 'description', 'Finish what T-1262 started. domains/check/ currently has one verb (client-version); the other four gates join it: check-canvas-version, check-systems-db-stamp, check-fact-ids and check-dataflow-graph.py. Per the domain map, canvas_sources.py and generator_sources.py move alongside as module-level REGISTRIES, not verbs — nothing types them, and putting them in reach --help would answer a question nobody has. schema_version.py goes to the db domain where it is defined; check imports it, which is what a service layer is for. STANDARD PORT ACCEPTANCE, applying to every per-domain ticket under this epic: (a) service.py holds the logic and is transport-agnostic — no printing, no sys.exit, no typer; (b) router.py holds no logic and every command wears @command; (c) a parity test per gate, copying the pattern in tooling/test_check_parity.py, which builds a throwaway fixture repo and runs BOTH implementations — exit codes must match exactly and no fact the old message carried may be lost; (d) the old scripts are RETIRED once parity passes, not left alongside; (e) that domain''s tooling make targets are retired too rather than left as wrappers, per the D-263 make/reach split — build orchestration stays with make. Note check-dataflow-graph.py and check-canvas-version both parse git output, so their fixtures need a git repo rather than a bare directory; that is the one thing here harder than client-version was.', 'Finish what T-1262 started. domains/check/ currently has one verb (client-version); the other four gates join it: check-canvas-version, check-systems-db-stamp, check-fact-ids and check-dataflow-graph.py. Per the domain map, canvas_sources.py and generator_sources.py move alongside as module-level REGISTRIES, not verbs — nothing types them, and putting them in reach --help would answer a question nobody has. schema_version.py goes to the db domain where it is defined; check imports it, which is what a service layer is for. STANDARD PORT ACCEPTANCE, applying to every per-domain ticket under this epic: (a) service.py holds the logic and is transport-agnostic — no printing, no sys.exit, no typer; (b) router.py holds no logic and every command wears @command; (c) a parity test per gate, copying the pattern in tooling/test_check_parity.py, which builds a throwaway fixture repo and runs BOTH implementations — exit codes must match exactly and no fact the old message carried may be lost; (d) the old scripts are RETIRED once parity passes, not left alongside; (e) that domain''s tooling make targets are retired too rather than left as wrappers, per the D-263 make/reach split — build orchestration stays with make. Note check-dataflow-graph.py and check-canvas-version both parse git output, so their fixtures need a git repo rather than a bare directory; that is the one thing here harder than client-version was. + +PROGRESS 2026-08-31 — fact-ids ported, the first bash-to-Python rewrite under the D-263 decision. It was 89 lines of grep/sed pipeline; it is now service.fact_ids() returning a FactIdCheck, with a router that renders it. PARITY IS EXACT ON THE LIVE TREE: both implementations print ''check-fact-ids: OK — 6 references validated against 61 canonical facts'' and exit 0. The matching COUNTS are the real evidence — if the line-matching regex differed from the grep chain even slightly, 6 and 61 would diverge. Kept line-matched rather than YAML-parsed on purpose: parsing properly would change which lines count (anchors, merge keys, multi-document files would start contributing ids the old check never saw), which is a different check wearing the same name. Three parity cases added to test_check_parity.py — ok, unknown fact_id, and the advisory mode where catalogs are unpopulated. Proven to fail: removing the entity-attributes.yaml exclusion was caught, and interestingly by the ADVISORY case rather than the exclusion assertion — including that file made the catalog non-empty, so the new implementation went to enforcing mode while the old stayed advisory. A real behavioural divergence, caught by exit code. RETIREMENT DEFERRED to the end of this ticket, and there is a reason worth recording. The rule is retire per DOMAIN, and three gates remain. It also resolves a tension: the parity test COPIES the old script into its fixture, so deleting the script early would delete the test''s own subject. A parity test is scaffolding with a defined lifetime — when the old script goes, its parity case goes with it and is replaced by a unit test of the new behaviour. Same will apply to every port under T-1250. NOTE the pre-commit hook calls tooling/check-fact-ids directly, so retirement means editing that hook — and a mistake there is felt on every commit rather than every push. Do it together with validate-content (T-1282), which is in the same hook, so it is edited once.', NULL, '2026-08-31 15:38:42', '2026-08-31 15:38:42.109', '2026-08-31 15:38:42.109', NULL, '0ea9d87f8863bc38b0020df38923641c', 2) ON CONFLICT(hash) DO NOTHING; diff --git a/.pql/changelog/tickets/2026-08.sql b/.pql/changelog/tickets/2026-08.sql index d1b98c545..f98d65b2c 100644 --- a/.pql/changelog/tickets/2026-08.sql +++ b/.pql/changelog/tickets/2026-08.sql @@ -2435,3 +2435,6 @@ SEQUENCING SET 2026-08-31 by decision. (1) T-1264 (streaming, as decorators) lan WAVE 1 FILED 2026-08-31: T-1281 check (four remaining gates), T-1282 validate, T-1283 godot and visual together. Deliberately NOT filing all fourteen domains at once — the standard port acceptance is defined once on T-1281 and referenced, and the remaining tickets (atlas, starmap, planet, db, wiki, assets, character, generate, dev, pr) get written after wave 1 lands, when the real friction of a port is known rather than guessed. Four descriptions written from experience beat ten written from a map. T-1272 was also RESCOPED today: the hyphenated directories are emptied by the domain moves rather than renamed, so it becomes a closing verification instead of an opening step. SCOPE CORRECTION 2026-08-31, found on starting the first port and materially larger than the epic assumed. SEVENTEEN of the 33 extensionless executables are BASH, not Python — about 900 lines. This description and the domain map both assumed a Python tree, so those are REWRITES, not moves. Decided with Jeroen: rewrite all of them in Python rather than wrap any. Wrapping would achieve one door while leaving half the CLI surface outside the contract — no @command, no remedy on failure, no streaming, no testable service — so reach --help would list verbs that behave differently from the ones beside them, which is worse than two doors because the inconsistency is invisible until something fails. D-263 amended. The bash inventory, by shape: THIN WRAPPERS that mostly invoke something else (atlas-systems-done 9, atlas-names 17, generate-brands 23, atlas 24, generate-corporations 23, blender 28) — these become routers calling the same thing, nearly free. LOGIC that computes a verdict (check-fact-ids 89, validate-ron 137, godot-parse-sweep 64, godot-cold-parse 96, pr-watchlist-diff 37, atlas-update-field 59, atlas-commit-and-sync 60, tea-comment 41) — these gain the most from becoming services. ENVIRONMENT scripts (install-godot 108, install-rust 33, worktree-setup 49) — these gain least and carry the most regression risk, because downloading and unzipping a Godot build or driving rustup is awkward to exercise in a gate; port their DECISION logic into a testable service and keep the irreducible external calls behind core/process. Per-domain port tickets must state which of their sources are bash, since a bash source changes a port from mechanical to a rewrite needing its own parity evidence.', 'in_progress', 'medium', NULL, NULL, 'D-263', '2026-08-20 00:23:58.880', '2026-08-31 15:33:16.713', NULL, '42606b68b3a882a1ba18a2c9dad2e848', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at; +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5GWCTS89VSST07DFVBVG064', 'task', '06G1S3D0M1TQW0GMFBBPQZG3ZM', 'Port the check domain — the four remaining gates', 'Finish what T-1262 started. domains/check/ currently has one verb (client-version); the other four gates join it: check-canvas-version, check-systems-db-stamp, check-fact-ids and check-dataflow-graph.py. Per the domain map, canvas_sources.py and generator_sources.py move alongside as module-level REGISTRIES, not verbs — nothing types them, and putting them in reach --help would answer a question nobody has. schema_version.py goes to the db domain where it is defined; check imports it, which is what a service layer is for. STANDARD PORT ACCEPTANCE, applying to every per-domain ticket under this epic: (a) service.py holds the logic and is transport-agnostic — no printing, no sys.exit, no typer; (b) router.py holds no logic and every command wears @command; (c) a parity test per gate, copying the pattern in tooling/test_check_parity.py, which builds a throwaway fixture repo and runs BOTH implementations — exit codes must match exactly and no fact the old message carried may be lost; (d) the old scripts are RETIRED once parity passes, not left alongside; (e) that domain''s tooling make targets are retired too rather than left as wrappers, per the D-263 make/reach split — build orchestration stays with make. Note check-dataflow-graph.py and check-canvas-version both parse git output, so their fixtures need a git repo rather than a bare directory; that is the one thing here harder than client-version was. + +PROGRESS 2026-08-31 — fact-ids ported, the first bash-to-Python rewrite under the D-263 decision. It was 89 lines of grep/sed pipeline; it is now service.fact_ids() returning a FactIdCheck, with a router that renders it. PARITY IS EXACT ON THE LIVE TREE: both implementations print ''check-fact-ids: OK — 6 references validated against 61 canonical facts'' and exit 0. The matching COUNTS are the real evidence — if the line-matching regex differed from the grep chain even slightly, 6 and 61 would diverge. Kept line-matched rather than YAML-parsed on purpose: parsing properly would change which lines count (anchors, merge keys, multi-document files would start contributing ids the old check never saw), which is a different check wearing the same name. Three parity cases added to test_check_parity.py — ok, unknown fact_id, and the advisory mode where catalogs are unpopulated. Proven to fail: removing the entity-attributes.yaml exclusion was caught, and interestingly by the ADVISORY case rather than the exclusion assertion — including that file made the catalog non-empty, so the new implementation went to enforcing mode while the old stayed advisory. A real behavioural divergence, caught by exit code. RETIREMENT DEFERRED to the end of this ticket, and there is a reason worth recording. The rule is retire per DOMAIN, and three gates remain. It also resolves a tension: the parity test COPIES the old script into its fixture, so deleting the script early would delete the test''s own subject. A parity test is scaffolding with a defined lifetime — when the old script goes, its parity case goes with it and is replaced by a unit test of the new behaviour. Same will apply to every port under T-1250. NOTE the pre-commit hook calls tooling/check-fact-ids directly, so retirement means editing that hook — and a mistake there is felt on every commit rather than every push. Do it together with validate-content (T-1282), which is in the same hook, so it is edited once.', 'in_progress', 'high', NULL, NULL, 'D-263', '2026-08-31 15:30:35.338', '2026-08-31 15:38:42.109', NULL, '9e8af2a6085ac6f63a902ce84ce8619d', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at; diff --git a/tooling/domains/check/router.py b/tooling/domains/check/router.py index c6e6fe529..364593521 100644 --- a/tooling/domains/check/router.py +++ b/tooling/domains/check/router.py @@ -64,3 +64,44 @@ def client_version() -> None: ) console.verdict(f"check-client-version: OK — {result.yaml_version}") + + +@app.command("fact-ids") +@command +def fact_ids() -> None: + """Fail if content references a fact_id no knowledge catalog defines.""" + result = service.fact_ids() + + if result.advisory: + # Advisory rather than failing: until the catalogs hold definitions, + # failing every commit would teach people to bypass the hook, and a gate + # people route around protects nothing. + console.event( + "check-fact-ids: WARNING — no canonical fact_ids in knowledge catalogs", + level="warn", + ) + for fact in result.referenced: + console.event(f" {fact}", level="warn") + console.verdict( + f"check-fact-ids: advisory — catalogs unpopulated, " + f"{len(result.referenced)} reference(s) unchecked" + ) + return + + if result.unknown: + detail = "\n".join( + f" {fact.fact_id}\n" + "".join(f" {where}\n" for where in fact.locations) + for fact in result.unknown + ) + raise ReachError( + f"check-fact-ids: {len(result.unknown)} unknown fact_id(s)\n{detail}", + fix=( + "define them in server/content/global/knowledge/*.yaml, or correct " + "the references above" + ), + ) + + console.verdict( + f"check-fact-ids: OK — {len(result.referenced)} references validated " + f"against {result.canonical_count} canonical facts" + ) diff --git a/tooling/domains/check/schemas.py b/tooling/domains/check/schemas.py index 7bf05177c..4abd3ee6d 100644 --- a/tooling/domains/check/schemas.py +++ b/tooling/domains/check/schemas.py @@ -15,6 +15,36 @@ from __future__ import annotations from pydantic import BaseModel, ConfigDict +class UnknownFact(BaseModel): + """A fact_id referenced by content that no knowledge catalog defines.""" + + model_config = ConfigDict(frozen=True) + + fact_id: str + locations: list[str] = [] + + +class FactIdCheck(BaseModel): + """The outcome of validating content fact_id references against the catalogs. + + `advisory` is the mode where the catalogs hold no definitions at all. The + check then reports what it found and exits 0 — deliberately, because failing + every commit until the catalogs are populated would train people to bypass + the hook, and a gate people route around protects nothing. + """ + + model_config = ConfigDict(frozen=True) + + advisory: bool + canonical_count: int + referenced: list[str] = [] + unknown: list[UnknownFact] = [] + + @property + def ok(self) -> bool: + return not self.unknown + + class VersionCheck(BaseModel): """The outcome of comparing project.yaml against client/project.godot. diff --git a/tooling/domains/check/service.py b/tooling/domains/check/service.py index 8a3ebe7c0..8530435e4 100644 --- a/tooling/domains/check/service.py +++ b/tooling/domains/check/service.py @@ -12,7 +12,7 @@ import re from pathlib import Path from tooling.core import config -from tooling.domains.check.schemas import VersionCheck +from tooling.domains.check.schemas import FactIdCheck, UnknownFact, VersionCheck # Anchored to line start so the commentary above `version:` (which mentions # earlier versions by number) can never be mistaken for the field itself. @@ -52,6 +52,86 @@ def client_version() -> VersionCheck: ) +# Ported from the bash `check-fact-ids` (D-263: the shell scripts are rewritten, +# not wrapped). Deliberately still LINE-MATCHED rather than YAML-parsed: the +# original was grep-based, and parsing YAML properly would change which lines +# count — anchors, merge keys and multi-document files would all start +# contributing ids the old check never saw. That is a different check wearing +# the same name, and this port is not the place to make it. +_FACT_ID = re.compile(r"^\s*(?:-\s*)?fact_id:\s*(.+?)\s*$") + +# Its schema is attribute keys, not fact_ids — excluded by the original too. +_NOT_A_CATALOG = "entity-attributes.yaml" + + +def fact_ids() -> FactIdCheck: + """Validate fact_id references in campaign content against the catalogs.""" + root = config.repo_root() + canonical = _collect(root / "server" / "content" / "global" / "knowledge", catalogs=True) + referenced = _collect(root / "server" / "content" / "campaigns", catalogs=False) + + if not canonical: + return FactIdCheck( + advisory=True, + canonical_count=0, + referenced=sorted(referenced), + ) + + unknown = [ + UnknownFact(fact_id=ref, locations=_locate(root, ref)) + for ref in sorted(referenced) + if ref not in canonical + ] + return FactIdCheck( + advisory=False, + canonical_count=len(canonical), + referenced=sorted(referenced), + unknown=unknown, + ) + + +def _collect(directory: Path, *, catalogs: bool) -> set[str]: + found: set[str] = set() + if not directory.is_dir(): + return found + for path in directory.rglob("*.yaml"): + if catalogs and path.name == _NOT_A_CATALOG: + continue + for line in path.read_text(encoding="utf-8", errors="replace").splitlines(): + if line.lstrip().startswith("#"): + continue + match = _FACT_ID.match(line) + if match: + value = _clean(match.group(1)) + if value: + found.add(value) + return found + + +def _clean(value: str) -> str: + """Strip a trailing comment and surrounding quotes, as the sed chain did.""" + value = re.sub(r"\s+#.*$", "", value).strip() + if len(value) >= 2 and value[0] == value[-1] and value[0] in "\"'": + value = value[1:-1] + return value.strip() + + +def _locate(root: Path, fact_id: str) -> list[str]: + """Repo-relative file:line for every reference, so a failure is actionable.""" + content = root / "server" / "content" / "campaigns" + hits: list[str] = [] + if not content.is_dir(): + return hits + for path in sorted(content.rglob("*.yaml")): + for number, line in enumerate( + path.read_text(encoding="utf-8", errors="replace").splitlines(), start=1 + ): + match = _FACT_ID.match(line) + if match and _clean(match.group(1)) == fact_id: + hits.append(f"{path.relative_to(root)}:{number}") + return hits + + 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(): diff --git a/tooling/test_check_parity.py b/tooling/test_check_parity.py index d4a0d5568..42afc121a 100644 --- a/tooling/test_check_parity.py +++ b/tooling/test_check_parity.py @@ -109,6 +109,92 @@ def _run_new(root: Path) -> tuple[int, str, str]: return result.returncode, result.stdout + result.stderr, as_json.stderr +FACT_CASES = [ + # (label, catalog fact_ids, referenced fact_ids, expect non-zero) + ("fact-ok", ["alpha", "beta"], ["alpha"], False), + ("fact-unknown", ["alpha"], ["alpha", "ghost"], True), + ("fact-advisory", [], ["alpha"], False), # no catalogs: advisory, exit 0 +] + + +def _build_fact_fixture(tmp: Path, catalog: list[str], referenced: list[str]) -> Path: + """A miniature repo with knowledge catalogs and campaign content.""" + root = tmp / "fixture" + knowledge = root / "server" / "content" / "global" / "knowledge" + campaigns = root / "server" / "content" / "campaigns" + knowledge.mkdir(parents=True) + campaigns.mkdir(parents=True) + (root / "project.yaml").write_text("version: 0.0.0\n", encoding="utf-8") + (root / "tooling").mkdir() + + if catalog: + (knowledge / "facts.yaml").write_text( + "# a comment mentioning fact_id: decoy_should_not_count\n" + + "".join(f" - fact_id: {name}\n" for name in catalog), + encoding="utf-8", + ) + # Excluded by name in both implementations — if either starts counting it, + # the canonical totals diverge and this case catches it. + (knowledge / "entity-attributes.yaml").write_text( + " - fact_id: attribute_not_a_fact\n", encoding="utf-8" + ) + (campaigns / "story.yaml").write_text( + "".join(f' - fact_id: "{name}"\n' for name in referenced), encoding="utf-8" + ) + + shutil.copy2(REPO_ROOT / "tooling" / "check-fact-ids", root / "tooling" / "check-fact-ids") + (root / "tooling" / "check-fact-ids").chmod(0o755) + return root + + +def check_fact_ids(failures: list[str]) -> None: + """Parity for the first bash-to-Python rewrite (D-263). + + Worth more scrutiny than a Python move: nothing here was relocated, it was + reimplemented, so the two could agree on the happy path and diverge on + exactly the inputs the grep chain handled by accident — quoted values, + trailing comments, the excluded catalog. + """ + for label, catalog, referenced, expect_failure in FACT_CASES: + with tempfile.TemporaryDirectory() as tmpdir: + root = _build_fact_fixture(Path(tmpdir), catalog, referenced) + + old = subprocess.run( + [str(root / "tooling" / "check-fact-ids")], + capture_output=True, + text=True, + ) + env = {**os.environ, "SR_REPO_ROOT": str(root), "SR_OUTPUT_FORMAT": "text"} + new = subprocess.run( + ["reach", "check", "fact-ids"], + capture_output=True, + text=True, + env=env, + ) + + if (old.returncode != 0) != expect_failure: + failures.append( + f"[{label}] the OLD script exited {old.returncode}; the fixture " + "does not set up the case it claims to" + ) + if old.returncode != new.returncode: + failures.append( + f"[{label}] exit differs: old={old.returncode} new={new.returncode}" + ) + + combined = new.stdout + new.stderr + if expect_failure and "ghost" not in combined: + failures.append( + f"[{label}] the new output does not name the unknown fact_id, so a " + "failure says something is wrong without saying what" + ) + if "attribute_not_a_fact" in combined: + failures.append( + f"[{label}] entity-attributes.yaml was counted as a catalog — its " + "schema is attribute keys, and including it inflates the canonical set" + ) + + def main() -> int: if shutil.which("reach") is None: print( @@ -182,13 +268,18 @@ def main() -> int: f" new: {new_text.strip()!r}" ) + check_fact_ids(failures) + if failures: print("test_check_parity: FAIL", file=sys.stderr) for failure in failures: print(f" - {failure}", file=sys.stderr) return 1 - print(f"test_check_parity: OK — {len(CASES)} paths agree on exit code and content") + print( + f"test_check_parity: OK — client-version {len(CASES)} paths, " + f"fact-ids {len(FACT_CASES)} paths, all agreeing on exit code and content" + ) return 0