feat(config): T-1281 — check fact-ids, the first bash rewrite
89 lines of grep/sed pipeline become a service returning a FactIdCheck and a router that renders it. Parity on the live tree is exact: both implementations print "check-fact-ids: OK — 6 references validated against 61 canonical facts" and exit 0. The matching counts are the real evidence — a line-matching regex that differed from the grep chain even slightly would move 6 or 61. Kept line-matched rather than YAML-parsed on purpose. Parsing properly would change which lines count: anchors, merge keys and multi-document files would start contributing ids the old check never saw. That is a different check wearing the same name, and a port is not the place to make it. Three parity cases: ok, unknown fact_id, and the advisory mode where the catalogs hold no definitions and the gate deliberately exits 0 — failing every commit until they are populated would teach people to bypass the hook, and a gate people route around protects nothing. Proven to fail by removing the entity-attributes.yaml exclusion, and caught in a way worth noting: not by the assertion aimed at it, but by the advisory case, where including that file made the catalog non-empty so the new implementation enforced while the old stayed advisory. A real behavioural divergence, surfaced by exit code. Retirement waits for the whole domain, per the per-domain rule — 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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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"
|
||||
)
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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():
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user