diff --git a/.github/workflows/loop-tools-gate.yml b/.github/workflows/loop-tools-gate.yml index e61f59861c..230d9fa2b0 100644 --- a/.github/workflows/loop-tools-gate.yml +++ b/.github/workflows/loop-tools-gate.yml @@ -17,6 +17,7 @@ on: - "scripts/tri" - "scripts/tri_loop/**" - "scripts/ci/loop-tools-tracked.sh" + - "scripts/ci/test_damage_repair_snapshot_required.py" - ".github/workflows/loop-tools-gate.yml" push: branches: [master] @@ -24,6 +25,7 @@ on: - "scripts/tri" - "scripts/tri_loop/**" - "scripts/ci/loop-tools-tracked.sh" + - "scripts/ci/test_damage_repair_snapshot_required.py" permissions: contents: read @@ -51,6 +53,12 @@ jobs: || { echo "BROKEN $f"; exit 1; } done + - name: damage-repair requires --snapshot rather than defaulting to one + # A reader that defaults to a snapshot path is guessing which freeze the + # caller meant, and the path it guessed pointed at a file no default + # workflow produced (#2327). Pure Python, no compiler, no corpus. + run: python3 scripts/ci/test_damage_repair_snapshot_required.py + - name: The dispatcher runs its helpers without a built compiler # tri triage and tri damage read the tracker and the spec text and have no # use for t27c. The dispatcher used to look for the binary first and diff --git a/docs/now/2026-08-21-damage-repair-snapshot-required.md b/docs/now/2026-08-21-damage-repair-snapshot-required.md new file mode 100644 index 0000000000..f1e0a2d822 --- /dev/null +++ b/docs/now/2026-08-21-damage-repair-snapshot-required.md @@ -0,0 +1,11 @@ +# NOW -- damage-repair names its snapshot or refuses (2026-08-21) + +## tri damage-repair: --snapshot is required, not defaulted to a path nothing creates (Closes #2327) + +- `damage_repair.py` carried `DEFAULT_SNAPSHOT = "docs/corpus/damage_snapshot_2026-08-15.json"`. `docs/corpus/` has **zero paths on master**, and the companion writer `damage_freeze.py` defaults its `--out` to `docs/corpus/damage_snapshot.json` -- a *different* name. So the pair never connected on defaults even once the directory existed. Reproduced on a clean checkout of `origin/master`: `tri damage-freeze specs` writes `docs/corpus/damage_snapshot.json`, and `tri damage-repair` immediately after reports `no snapshot at docs/corpus/damage_snapshot_2026-08-15.json` while that fresh freeze sits beside it in the same directory +- Fixed by removing the default rather than by making the two agree. Agreeing would mean committing a snapshot, and a snapshot pins per-file digests that go stale the instant any spec changes -- the tool's own staleness check would then refuse on every run. The rule adopted instead, which `corpus_status.py:132` already followed for this same artifact: a **reader** must not guess at an input path, because it cannot know which freeze the caller meant; a **writer** may default its output path, because it creates the file rather than hoping one is there. `damage-freeze --out` keeps its default; `damage-repair --snapshot` has none +- Blast radius checked before choosing: `damage_snapshot` appears in exactly two places repo-wide, both the defaults themselves. No script, workflow or doc invokes `damage-repair` at all -- `scripts/ci/loop-tools-tracked.sh` only asserts the file is tracked and the subcommand routes. Making the argument required breaks no existing caller +- The second error path no longer names a hardcoded literal either: `no snapshot at ` now echoes back the path the caller actually passed, so the remedy it prints cannot drift from the request the way the two defaults did +- `scripts/ci/test_damage_repair_snapshot_required.py` builds a one-spec corpus, freezes it at the **writer's** default name, then asserts four independent properties: refuses with no `--snapshot` (G1), says `--snapshot` and `required` in those words (G2), names no concrete snapshot path -- no `docs/corpus`, no `.json` (G3), and still runs to completion when given one (G4). Against master's code it fails 3/4, quoting `no snapshot at docs/corpus/damage_snapshot_2026-08-15.json` +- **G1 passes on master**: the defective tool already exited 2. A guard that only asked "did it exit non-zero" would have been green on this defect, which is why G2 and G3 exist. G4 is the anti-vacuity guard -- without it, `if True: return 2` would satisfy G1-G3. Each of the four was proven by its own mutant, and M2/M3/M4 each fired their guard *alone* with the others still passing +- Wired into `loop-tools-gate`, which already covers these tools and needs no compiler or corpus. `Corpus Ratchet`, `Seal Coverage` and `FPGA E2E Build` are red on master already and are unrelated to this change diff --git a/scripts/ci/test_damage_repair_snapshot_required.py b/scripts/ci/test_damage_repair_snapshot_required.py new file mode 100644 index 0000000000..d45ef34379 --- /dev/null +++ b/scripts/ci/test_damage_repair_snapshot_required.py @@ -0,0 +1,175 @@ +#!/usr/bin/env python3 +"""`tri damage-repair` has no default snapshot, and refuses instead of guessing (#2327). + +The defect this pins down. + +`damage_repair.py` used to carry `DEFAULT_SNAPSHOT = +"docs/corpus/damage_snapshot_2026-08-15.json"`. No default workflow produces that +file. The companion writer `tri damage-freeze` defaults its `--out` to +`docs/corpus/damage_snapshot.json` -- a DIFFERENT name -- so running the pair back +to back with no arguments never connected even once. The reader printed "no +snapshot" while a freeze it could have used sat beside it in the same directory. + +That is the scenario this test reconstructs, and it is why the fixture runs +`damage-freeze` with no `--out` first: the snapshot at the writer's own default +name is present on disk for every case below. A reader that quietly adopts it is +guessing which freeze the caller meant, and a reader that reports the old dated +path is back to naming a file nothing creates. Both are failures here. + +Four independent properties, each checked separately and each reported by name, +because "it exited non-zero" is satisfied by a tool that is simply broken: + + G1 refuses no `--snapshot` exits non-zero, even with a default-named + snapshot sitting in docs/corpus/ + G2 for the right the refusal names `--snapshot` and says it is required, so + reason it is distinguishable from an import error or a missing + corpus, which also exit non-zero + G3 resolves no the refusal names no concrete snapshot path -- no + path `docs/corpus`, no `.json`. Reintroducing a default in any + form, including as a "helpful" suggested path, trips this + G4 still works given an explicit `--snapshot`, the tool runs to completion + and reports that snapshot. Without this, "always exit 2" + would satisfy G1-G3 + +Failures are collected rather than raised, so one broken property does not hide +the state of the other three. + +No compiler: `--binary` is pointed at a path that does not exist, so validation +is skipped and the run is pure Python. Nothing outside the temporary directory is +read or written. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +import tempfile +from pathlib import Path + +TRI_LOOP = Path(__file__).resolve().parent.parent / "tri_loop" +FREEZE = TRI_LOOP / "damage_freeze.py" +REPAIR = TRI_LOOP / "damage_repair.py" + +# One damaged field, of the restorable shape the repair tool exists to handle: +# the opening quote of the type string was replaced by `[`, which trips both of +# damage.py's signals (doubled-bracket, odd-quote). +DAMAGED_SPEC = '''Struct QuadNode { + name : "TypeText", + children : [[]QuadNode", +} +''' + +# The writer's own default output path. Named here so the fixture can assert the +# trap condition really is set up; the reader under test must not know it. +FREEZE_DEFAULT_OUT = Path("docs") / "corpus" / "damage_snapshot.json" + + +def run(args, cwd): + return subprocess.run([sys.executable, *args], cwd=cwd, + capture_output=True, text=True) + + +def build_fixture(tmp: Path) -> None: + """A corpus with one damaged line, frozen at the WRITER's default path.""" + specs = tmp / "specs" + specs.mkdir(parents=True) + (specs / "quadnode.t27").write_text(DAMAGED_SPEC) + + r = run([str(FREEZE), "specs"], cwd=tmp) + if r.returncode != 0: + raise SystemExit(f"fixture: damage-freeze failed ({r.returncode})\n" + f"{r.stdout}\n{r.stderr}") + made = tmp / FREEZE_DEFAULT_OUT + if not made.is_file(): + raise SystemExit( + "fixture: damage-freeze with no --out did not write " + f"{FREEZE_DEFAULT_OUT}. This test's premise is that the writer's " + "default output exists on disk while the reader still refuses; " + "without it G1 would pass for the wrong reason.") + + +def main() -> int: + failures = [] + + with tempfile.TemporaryDirectory() as td: + tmp = Path(td) + build_fixture(tmp) + print(f"fixture: 1 damaged spec, frozen at {FREEZE_DEFAULT_OUT} " + "(the writer's default)\n") + + # ---- no --snapshot ------------------------------------------------- + bare = run([str(REPAIR)], cwd=tmp) + err = bare.stderr + print("--- tri damage-repair (no --snapshot) ---") + print(f"exit={bare.returncode}") + for line in err.splitlines(): + print(f" | {line}") + print() + + # G1 + if bare.returncode == 0: + failures.append( + "G1 refuses: exited 0 with no --snapshot. A snapshot named " + f"{FREEZE_DEFAULT_OUT} was present, so the tool adopted a " + "freeze the caller never named.") + + # G2 -- the refusal is about the argument, not about something else + low = err.lower() + if "--snapshot" not in err or "required" not in low: + failures.append( + "G2 right reason: the refusal does not both name `--snapshot` " + "and say it is required. Exiting non-zero for an unrelated " + f"reason would look identical. stderr was: {err!r}") + + # G3 -- no default path may be resolved or suggested, in any form + for needle in ("docs/corpus", ".json"): + if needle in err: + failures.append( + f"G3 resolves no path: the refusal names {needle!r}. The " + "reader must not point at a concrete snapshot file it was " + "not given -- a hardcoded path here is exactly the defect " + f"#2327 reported. stderr was: {err!r}") + + # ---- explicit --snapshot (anti-vacuity) ---------------------------- + snap = str(FREEZE_DEFAULT_OUT) + good = run([str(REPAIR), "--snapshot", snap, + "--binary", str(tmp / "no-such-binary")], cwd=tmp) + print("--- tri damage-repair --snapshot ---") + print(f"exit={good.returncode}") + for line in good.stdout.splitlines()[:4]: + print(f" | {line}") + print() + + # G4 + if good.returncode != 0: + failures.append( + "G4 still works: an explicit --snapshot to a real freeze " + f"exited {good.returncode}. The refusal in G1 is then not a " + "required-argument check, it is a tool that never runs.\n" + f" stdout: {good.stdout[-400:]!r}\n" + f" stderr: {good.stderr[-400:]!r}") + elif snap not in good.stdout: + failures.append( + "G4 still works: the run did not report the snapshot it was " + f"given ({snap!r}), so there is no evidence it read that file " + "rather than some other one.") + + if failures: + print(f"FAIL ({len(failures)}):") + for f in failures: + print(" - " + f) + return 1 + + print("OK: 4/4 -- damage-repair refuses without --snapshot (G1), says so in " + "those\nwords (G2), names no snapshot path of its own (G3), and still " + "runs when given\none (G4). The writer's default output was present " + "throughout and was not adopted.") + print("Scope: this covers damage-repair's snapshot argument only. It says " + "nothing\nabout whether the repairs it proposes are correct -- that is " + "the double\nvalidation inside the tool.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/tri_loop/damage_repair.py b/scripts/tri_loop/damage_repair.py index 7197623a75..39161f2473 100644 --- a/scripts/tri_loop/damage_repair.py +++ b/scripts/tri_loop/damage_repair.py @@ -60,10 +60,29 @@ needs-human-language-decision no patch was attempted, information destroyed Usage: - tri damage-repair [--snapshot PATH] [--binary PATH] [--class DC-xxxxxxxx] + tri damage-repair --snapshot PATH [--binary PATH] [--class DC-xxxxxxxx] [--diff] [--apply-to DIR] [--json PATH] `--apply-to` writes repaired copies into a scratch tree; specs/ is never touched. + +## Why --snapshot has no default (#2327) + +It used to default to `docs/corpus/damage_snapshot_2026-08-15.json`, a path that +no default workflow produces: the companion writer `tri damage-freeze` defaults +its `--out` to `docs/corpus/damage_snapshot.json`, a different name, so running +the two tools back to back with no arguments never connected. The reader reported +"no snapshot" while a perfectly good freeze sat beside it in the same directory. + +The rule that replaced it, and that `corpus_status.py` already followed for this +same artifact: a READER must not guess at an input path, because it cannot know +which freeze the caller meant; a WRITER may default its output path, because it +creates the file rather than hoping one is there. So `damage-freeze --out` keeps +its default and `damage-repair --snapshot` has none. + +This is the same instinct as the staleness check further down, which refuses to +repair against a snapshot whose digests no longer match the corpus. A tool that +refuses to repair against the wrong snapshot should equally refuse to invent +which snapshot you meant. """ import difflib @@ -79,7 +98,6 @@ from diffbin import parse_fields # noqa: E402 (same directory, deliberate) CLOSED_STRING = re.compile(r'^"[^"]*",?$') -DEFAULT_SNAPSHOT = "docs/corpus/damage_snapshot_2026-08-15.json" TIMEOUT = 25 EFFECTS = ( @@ -259,7 +277,7 @@ def combined(snap, rows, binary, have_binary, tmpdir, apply_to, json_out): def main(argv): - snapshot = DEFAULT_SNAPSHOT + snapshot = None binary = "/tmp/t27c.fixed" only = None apply_to = None @@ -277,9 +295,25 @@ def main(argv): elif a == "--json" and i + 1 < len(argv): json_out = argv[i + 1] + # No default. Which freeze to repair against is a statement the caller has to + # make: the snapshot fixes both the corpus digest every later citation refers + # to and the file digests the staleness check below enforces. A guessed path + # either finds nothing, or silently picks up a freeze of a corpus and a date + # nobody named. `corpus_status.py` requires this same artifact for the same + # reason. + if snapshot is None: + print("REFUSING: --snapshot is required; there is no default.", file=sys.stderr) + print("", file=sys.stderr) + print("A repair is only meaningful against a named frozen snapshot, whose", file=sys.stderr) + print("digests are what let this tool refuse when the corpus has moved.", file=sys.stderr) + print("", file=sys.stderr) + print(" tri damage-freeze specs --out PATH", file=sys.stderr) + print(" tri damage-repair --snapshot PATH", file=sys.stderr) + return 2 + if not os.path.exists(snapshot): print(f"no snapshot at {snapshot}", file=sys.stderr) - print("run: tri damage-freeze specs --out " + DEFAULT_SNAPSHOT, file=sys.stderr) + print("freeze one first: tri damage-freeze specs --out " + snapshot, file=sys.stderr) return 2 snap = json.load(open(snapshot))