From a5f5273c327f4b0c9ee1ff9766e4aa30325f2c01 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Fri, 30 Oct 2026 08:34:33 -0400 Subject: [PATCH 1/2] feat: registry file_triggers map changed paths straight to journeys (#118) `"file_triggers": {"": ["J1", ...]}` declares dependencies the graph cannot model (SQL migrations -> RLS). Globs are repo-relative, whole-path, `**` / `*` / `?`, matched against every raw diff path before the product filter. Additive: a graph-selected journey gains `triggered_by`; a trigger-only journey gets a confident `file-trigger` row. A matched unmodeled path is declared covered and no longer forces the #117 all-journeys fallback; an unmatched one still does. Invalid triggers (bad glob, empty list, unknown journey id) BLOCK. In passing: #136 caps the unmodeled-path list in its warning; #138 gives unmodeled fallback rows their own reason and warning. --- README.md | 3 +- TECHNICAL.md | 36 +++++++ skills/testgraph-propose/SKILL.md | 5 + testgraph/hook.py | 5 +- testgraph/registry.py | 68 ++++++++++++ testgraph/select.py | 80 +++++++++++++- tests/test_core.py | 173 ++++++++++++++++++++++++++++++ 7 files changed, 363 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 2c5f81a..dce00da 100644 --- a/README.md +++ b/README.md @@ -179,7 +179,8 @@ indexed copy afterwards, the answer degrades to `RECALL DEGRADED` and names the instead of quietly trusting the wrong line numbers. So does a change to a file the index does not model at all — SQL, a `Dockerfile`, CI YAML, CSS, config — unless it is a test, docs or a type declaration: not indexed is not "no impact" (TECHNICAL.md -"Unmodeled Paths Degrade Too"). +"Unmodeled Paths Degrade Too"). A registry's `file_triggers` maps such paths to the +journeys they affect instead (TECHNICAL.md "File Triggers"). Each run appends one `selection` row to `~/.local/share/testgraph/ledger.jsonl` (`{kind, ts, repo, base, head, commit, status, n_journeys, journey_ids, duration_ms, diff --git a/TECHNICAL.md b/TECHNICAL.md index 58dc733..42a1840 100644 --- a/TECHNICAL.md +++ b/TECHNICAL.md @@ -118,6 +118,8 @@ leaving edges wrong) — only a full `codegraph index` did. Three checks: - `TESTGRAPH_STATE_DIR` relocates the invocation log (default `${XDG_DATA_HOME:-~/.local/share}/testgraph`). - Registry `spot_checks`: `{symbol: {min_caller_edges, file}}` — the guard floors. +- Registry `file_triggers`: `{glob: [journey ids]}` — paths mapped straight to + journeys (issue #118; "File Triggers"). ## Deployment @@ -649,6 +651,40 @@ byte-identical with the degrade reverted, because no labeled commit exercises th zero-seed path (`0b4135f` already degraded via a whole-file change). The guard is justified by the reproduced failure in #29, not by a metric move. +## File Triggers (issue #118) + +Some dependencies the graph cannot model are still known: a SQL migration under +`packages/db/migrations/` can change RLS for every journey. A registry declares them: + +```json +"file_triggers": { + "packages/db/migrations/**": ["J1", "J2", "J3"], + "apps/web/src/**/*.css": ["J8"] +} +``` + +- **Globs** are repo-relative and match the whole path, case-sensitively: `**` is + zero or more whole segments, `*` any run inside one segment, `?` one character + inside a segment. No `[...]`, `{...}`, `!`, `\`, leading `/` or `..`. +- **Matched against every raw diff path** (`diff_files`: added, deleted, both + sides of a rename, binary, submodule), before the product filter. +- **Additive.** A journey the graph also selected keeps its graph row and gains + `triggered_by: [{pattern, path}]`, with no `reason`. A trigger-only journey gets + a row with `reason: "file-trigger"`, `confidence: 1.0`, `verify_manually: false`: + a human declared that dependency, which is stronger than any edge. The result + carries `file_triggers: [{path, pattern, journeys}]`; select prints + ``trigger: matches `` → J…``, and both select and the hook label the row. +- **A trigger is a complete claim** for the paths it matches. A matched + unmodeled path does not force the #117 all-journeys fallback, and is not + reported as unmodeled in the text or the hook. There is no partial coverage: to + be conservative, list more journeys. A matched product path still goes + through the graph (and #29) as before. +- **An unmatched unmodeled path still degrades.** A typo'd glob cannot be + detected, but it fails safe, into the #117 fallback. +- **Invalid triggers BLOCK**: a malformed glob, a value that isn't a non-empty + list, or an unknown journey id. A typo'd id would otherwise drop a journey + the registry declares affected. + ## Unmodeled Paths Degrade Too (issue #117) The parser used to drop every path outside `PRODUCT_EXT` before anything looked diff --git a/skills/testgraph-propose/SKILL.md b/skills/testgraph-propose/SKILL.md index a66147c..3fcad98 100644 --- a/skills/testgraph-propose/SKILL.md +++ b/skills/testgraph-propose/SKILL.md @@ -108,6 +108,11 @@ python3 -m testgraph.select --repo --registry /.testgraph/journeys/ ## 6. Approve +If the app has behaviour the graph cannot see — SQL migrations, RLS policies, +stylesheets a journey depends on — declare it before approving, so those diffs +select the right journeys instead of every one (TECHNICAL.md "File Triggers"): +`"file_triggers": {"packages/db/migrations/**": ["J1", "J2"]}`. + Set `"approved": true`, drop the `blind_spots` you resolved (keep the ones you consciously accepted), rewrite `note` to say who reviewed it and what is knowingly uncovered, and rename the file to `.testgraph/journeys/.json` — in the same diff --git a/testgraph/hook.py b/testgraph/hook.py index 574f38b..bd665db 100644 --- a/testgraph/hook.py +++ b/testgraph/hook.py @@ -83,6 +83,8 @@ def render(result, repo, more_cmd=None): ] for j in journeys[:MAX_JOURNEYS]: flag = " ! VERIFY MANUALLY" if j.get("verify_manually") else "" + if j.get("reason") == "file-trigger": + flag = " (file-trigger)" lines.append( f" [{j['rank']:>3}] {j['id']} {j['name']} " f"(conf {j['confidence']}){flag}" @@ -112,7 +114,8 @@ def render(result, repo, more_cmd=None): # Same reasoning as the confined line: a path the index does not model # forced the all-journeys fallback (issue #117), and the reader should see # which one without depending on the capped warnings below. - unmodeled = result.get("unmodeled_files", []) + covered = {t["path"] for t in result.get("file_triggers", [])} + unmodeled = [p for p in result.get("unmodeled_files", []) if p not in covered] if unmodeled: shown = ", ".join(unmodeled[:MAX_CONFINED]) if len(unmodeled) > MAX_CONFINED: diff --git a/testgraph/registry.py b/testgraph/registry.py index fab9a0e..a5e4f40 100644 --- a/testgraph/registry.py +++ b/testgraph/registry.py @@ -3,8 +3,10 @@ node ids at run time. """ import ast +import functools import json import os +import re from . import db as dbmod @@ -461,6 +463,72 @@ def _binds(node, name): return any(isinstance(t, ast.Name) and t.id == name for t in targets if t) +# `file_triggers` (issue #118): {"": ["J1", ...]} maps changed paths straight +# to journeys, for dependencies the graph cannot model (a SQL migration that +# changes RLS for every journey). Globs are repo-relative and match the whole +# path: `**` is zero or more whole segments, `*` any run inside one segment, `?` +# one character inside a segment. fnmatch's `*` crosses `/`, and +# PurePath.full_match / glob.translate are 3.13+, so the translation is ours. +_GLOB_UNSUPPORTED = re.compile(r"[\[\]!{}\\]") + + +@functools.lru_cache(maxsize=None) +def _trigger_regex(pattern): + out = [] + parts = pattern.split("/") + for i, part in enumerate(parts): + last = i == len(parts) - 1 + if part == "**": + out.append(".*" if last else "(?:[^/]+/)*") + continue + seg = "".join("[^/]*" if c == "*" else "[^/]" if c == "?" else re.escape(c) + for c in part) + out.append(seg if last else seg + "/") + return re.compile("".join(out) + r"\Z") + + +def trigger_matches(pattern, path): + """Whether a repo-relative `path` matches a `file_triggers` glob.""" + return _trigger_regex(pattern).match(path) is not None + + +def trigger_problems(registry): + """Why `file_triggers` is unusable, as blocking messages; [] when fine. + + Blocking, not a warning: an unknown journey id would silently drop a journey + a human declared affected. A typo'd GLOB cannot be caught here (it matches + nothing), but it fails safe: the path falls through to the #117 fallback.""" + triggers = registry.get("file_triggers") + if triggers is None: + return [] + if not isinstance(triggers, dict): + return ["file_triggers must be an object of {glob: [journey ids]}"] + known = registry.get("journeys", {}) + problems = [] + for pattern, jids in triggers.items(): + if (not isinstance(pattern, str) or not pattern or pattern.startswith("/") + or ".." in pattern.split("/") or _GLOB_UNSUPPORTED.search(pattern)): + problems.append( + f"file_triggers glob {pattern!r} is not a repo-relative glob " + f"(no leading /, no .., only * ** ?)" + ) + continue + if (not isinstance(jids, list) or not jids + or not all(isinstance(j, str) for j in jids)): + problems.append( + f"file_triggers[{pattern!r}] must be a non-empty list of journey ids" + ) + continue + unknown = [j for j in jids if j not in known] + if unknown: + problems.append( + f"file_triggers[{pattern!r}] names unknown journey(s) " + f"{', '.join(unknown)} — a typo here would silently drop a " + f"journey the registry declares affected" + ) + return problems + + def journey_name(registry, jid): return registry["journeys"][jid]["name"] diff --git a/testgraph/select.py b/testgraph/select.py index bb33e78..f930248 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -42,6 +42,9 @@ # whole answer to "test everything", so admitting them would be actively worse. DECLARATION_EXT = (".d.ts", ".d.mts", ".d.cts") +# How many paths one warning names before "… N more" (#136). +MAX_LISTED = 5 + # Directory names that mean "this is test code", matched as whole path segments. # Substring matching on "/tests/" missed a repo-root `tests/` or `__tests__/` # directory entirely, so root-level test files were seeded as product code. @@ -357,6 +360,8 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): f"journeys absent from this index (not scored): {detail}" ) + blocking.extend(reg.trigger_problems(registry)) + # Drift the index cannot see: the registry and the index can agree while both # are stale against the source. A live parse is the only check in this pipeline # that reads the working tree, so it is the only one that catches a handler @@ -454,7 +459,26 @@ def _mark_unmapped(path, detail): # (issue #117). Not indexed is not "no impact". Unless the path is # exempt (tests, docs, type declarations), it degrades the answer to # every journey, exactly like a code file with no symbols (#29). - exempt_files, unmodeled_files = {}, [] + # + # A registry `file_triggers` glob (issue #118) is the exception: it maps + # matched paths straight to journeys, additively, and is the registry's + # claim of complete coverage for them, so a matched unmodeled path does + # not force the fallback. Every raw path is matched, product and exempt + # included, so a trigger is never hidden by the filter. + triggers = registry.get("file_triggers") or {} + trigger_hits = [] + triggered = {} # jid -> [{"pattern", "path"}] + for path in sorted(diff_paths): + for pattern, jids in triggers.items(): + if reg.trigger_matches(pattern, path): + trigger_hits.append( + {"path": path, "pattern": pattern, "journeys": list(jids)}) + for jid in jids: + triggered.setdefault(jid, []).append( + {"pattern": pattern, "path": path}) + trigger_covered = {t["path"] for t in trigger_hits} + + exempt_files, unmodeled_files, untriggered = {}, [], [] for path in sorted(diff_paths): if _is_product(path): continue @@ -463,7 +487,8 @@ def _mark_unmapped(path, detail): exempt_files[path] = why else: unmodeled_files.append(path) - _mark_unmapped(path, f"not modeled by the index: {_unmodeled_kind(path)}") + if path not in trigger_covered: + untriggered.append(path) # A changed file that is NEWER than its index row is a third way to be # unmappable, and the quietest. The other two resolve to no node and are @@ -581,6 +606,27 @@ def _mark_unmapped(path, detail): weak_reason = dbmod.weak_edge_reason(impacted_caps.get(best, "")) if weak_reason and journeys[-1]["verify_manually"]: journeys[-1]["weak_reason"] = weak_reason + # Declared by a file trigger: a graph row keeps its graph meaning (no + # `reason`) and gains `triggered_by`; a trigger-only journey gets its own + # confident row — a human declared the dependency — labelled + # `file-trigger` so it never reads as a graph edge. + by_id = {j["id"]: j for j in journeys} + for jid in sorted(triggered, key=reg.journey_sort_key): + if jid in by_id: + by_id[jid]["triggered_by"] = triggered[jid] + else: + journeys.append( + { + "id": jid, + "name": reg.journey_name(registry, jid), + "entries_hit": 0, + "rank": 0, + "confidence": 1.0, + "verify_manually": False, + "reason": "file-trigger", + "triggered_by": triggered[jid], + } + ) # Unmappable whole-file change -> unbounded impact. Add every journey the # closure did not already select, flagged for manual verification, so the # answer degrades toward "test everything" instead of toward silence. @@ -589,6 +635,22 @@ def _mark_unmapped(path, detail): f"{len(unmapped)} changed file(s) the index cannot be trusted for " f"({', '.join(unmapped)}) — impact is unbounded; all journeys listed" ) + # Its own warning and its own row reason (#138): re-indexing can never + # change the answer for a file the index does not model, so the #29 + # wording would send the reader the wrong way. The path list is capped + # (#136); `unmodeled_files` carries all of them. + if untriggered: + shown = ", ".join(untriggered[:MAX_LISTED]) + if len(untriggered) > MAX_LISTED: + shown += f", … {len(untriggered) - MAX_LISTED} more" + warnings.append( + f"{len(untriggered)} changed path(s) not modeled by the index and " + f"matched by no file trigger ({shown}) — impact unknown; all " + f"journeys listed" + ) + if unmapped or untriggered: + fallback_reason = ("change with no resolvable symbols" if unmapped + else "path the index does not model") selected = {j["id"] for j in journeys} for jid in sorted(registry.get("journeys", {}), key=reg.journey_sort_key): if jid not in selected: @@ -600,7 +662,7 @@ def _mark_unmapped(path, detail): "rank": 0, "confidence": 0.0, "verify_manually": True, - "reason": "change with no resolvable symbols", + "reason": fallback_reason, } ) @@ -618,8 +680,9 @@ def _mark_unmapped(path, detail): diff_files=sorted(diff_paths), exempt_files=exempt_files, unmodeled_files=unmodeled_files, + file_triggers=trigger_hits, whole_file_changes=whole_files, - recall_degraded=bool(unmapped), + recall_degraded=bool(unmapped or untriggered), closure_confined=confined_files, seed_symbols=len(seeds), impacted_symbols=len(impacted), @@ -713,8 +776,13 @@ def _render(result): ) for path, reason in sorted(result.get("whole_file_changes", {}).items()): lines.append(f" whole-file: {path} ({reason})") + for t in result.get("file_triggers", []): + lines.append(f" trigger: {t['path']} matches `{t['pattern']}` → " + f"{', '.join(t['journeys'])}") + covered = {t["path"] for t in result.get("file_triggers", [])} for path in result.get("unmodeled_files", []): - lines.append(f" unmodeled: {path} — not in the index, impact UNKNOWN") + if path not in covered: + lines.append(f" unmodeled: {path} — not in the index, impact UNKNOWN") if result.get("recall_degraded"): lines.append(" RECALL DEGRADED — unbounded impact, all journeys listed") for f in result.get("closure_confined", []): @@ -743,6 +811,8 @@ def _render(result): for j in result["journeys"]: why = j.get("weak_reason") or j.get("reason") or "weak edge path" flag = " ! VERIFY MANUALLY — %s" % why if j["verify_manually"] else "" + if j.get("reason") == "file-trigger": + flag = " — file-trigger" lines.append( f" [{j['rank']:>3}] {j['id']} {j['name']} " f"({j['entries_hit']} entry, conf {j['confidence']}){flag}" diff --git a/tests/test_core.py b/tests/test_core.py index 2753125..bb6d952 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2256,6 +2256,18 @@ def test_paths_survive_an_unreadable_diff_git_line(self): sel._parse_unified_diff(diff, paths) self.assertEqual(paths, {"db/old.sql", "db/new.sql"}) + def test_the_unmodeled_warning_is_capped_and_worded_for_its_cause(self): + """#136: the warning listed every path uncapped. #138: the fallback rows + reused #29's "no resolvable symbols", which points at re-indexing.""" + self._commit({f"data/f{i}.json": "x\n" for i in range(7)}) + res = self._select() + w = next(w for w in res["warnings"] if "not modeled" in w) + self.assertIn("… 2 more", w) + self.assertNotIn("data/f6.json", w) + self.assertEqual(len(res["unmodeled_files"]), 7) + self.assertTrue(all(j["reason"] == "path the index does not model" + for j in res["journeys"]), res["journeys"]) + def test_the_exemption_names_only_what_cannot_run(self): """Review round 1 (H2, H3, H5): a prefix match on doc basenames and the code-file test-name rules exempted migrations, CI and runtime config, @@ -2312,6 +2324,167 @@ def test_a_deleted_or_renamed_unmodeled_path_counts(self): self.assertTrue(res["recall_degraded"]) +class FileTriggerTests(unittest.TestCase): + """Issue #118: a registry can map repo-relative globs straight to journeys, + for dependencies the graph cannot model (SQL migrations → RLS). Additive to + the graph; a matched unmodeled path is declared covered and does not force + the #117 all-journeys fallback.""" + + REG = { + "J1": {"name": "one", "entries": [{"name": "handler_a", "file": "app/svc.py"}]}, + "J2": {"name": "two", "entries": [{"name": "leaf", "file": "app/leaf.py"}]}, + "J3": {"name": "three", "entries": [{"name": "leaf", "file": "app/leaf.py"}]}, + } + TRIGGERS = {"packages/db/migrations/**": ["J2", "J3"], + "packages/db/migrations/*.sql": ["J3"], + "styles/*.css": ["J1"]} + + def setUp(self): + self.tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.tmp, ignore_errors=True) + self.db = _db_on_disk(self.tmp, build_fixture()) + self.repo, self.run = _git_repo(self.tmp, {"app/svc.py": 22}) + self.registry = self._registry(self.TRIGGERS) + + def _registry(self, triggers): + path = os.path.join(self.tmp, "reg.json") + with open(path, "w") as fh: + json.dump({"codegraph_schema_version": 8, "journeys": self.REG, + "spot_checks": {}, "file_triggers": triggers}, fh) + return path + + def _commit(self, files): + for rel, text in files.items(): + full = os.path.join(self.repo, rel) + os.makedirs(os.path.dirname(full), exist_ok=True) + with open(full, "w") as fh: + fh.write(text) + self.run("git", "add", "-A") + self.run("git", "commit", "-qm", "change") + + def _select(self): + return sel.select(self.repo, "HEAD~1", "HEAD", self.db, self.registry) + + def test_a_migration_only_diff_selects_exactly_the_declared_journeys(self): + self._commit({"packages/db/migrations/001_add_rls.sql": "x\n"}) + res = self._select() + self.assertEqual(res["status"], "OK") + self.assertFalse(res["recall_degraded"], res["warnings"]) + self.assertEqual(["J2", "J3"], sorted(j["id"] for j in res["journeys"])) + for j in res["journeys"]: + self.assertEqual(j["reason"], "file-trigger") + self.assertFalse(j["verify_manually"]) + j3 = next(j for j in res["journeys"] if j["id"] == "J3") + self.assertEqual( + sorted(t["pattern"] for t in j3["triggered_by"]), + ["packages/db/migrations/**", "packages/db/migrations/*.sql"]) + text = sel._render(res) + self.assertIn("trigger: packages/db/migrations/001_add_rls.sql matches " + "`packages/db/migrations/**`", text) + self.assertIn("file-trigger", text) + pushed = hook.render(res, self.repo) + self.assertIn("file-trigger", pushed) + # declared covered: not also reported as an unmodeled blind spot + self.assertNotIn("not modeled by the index", pushed) + self.assertNotIn("unmodeled:", text) + self.assertEqual( + {(t["path"], t["pattern"]) for t in res["file_triggers"]}, + {("packages/db/migrations/001_add_rls.sql", "packages/db/migrations/**"), + ("packages/db/migrations/001_add_rls.sql", "packages/db/migrations/*.sql")}) + + def test_a_trigger_on_a_graph_selected_journey_adds_no_duplicate(self): + full = os.path.join(self.repo, "app", "svc.py") + with open(full) as fh: + lines = fh.readlines() + lines[11] = "changed = 1\n" # inside handler_a -> J1 via the graph + with open(full, "w") as fh: + fh.writelines(lines) + self._commit({"styles/site.css": "x\n"}) + res = self._select() + self.assertFalse(res["recall_degraded"], res["warnings"]) + ids = [j["id"] for j in res["journeys"]] + self.assertEqual(ids.count("J1"), 1, ids) + j1 = next(j for j in res["journeys"] if j["id"] == "J1") + self.assertNotIn("reason", j1) # still a graph selection + self.assertEqual(j1["triggered_by"], + [{"pattern": "styles/*.css", "path": "styles/site.css"}]) + + def test_an_unmatched_unmodeled_path_still_degrades(self): + self._commit({"packages/db/migrations/002.sql": "x\n", "Dockerfile": "x\n"}) + res = self._select() + self.assertTrue(res["recall_degraded"]) + self.assertEqual({"J1", "J2", "J3"}, {j["id"] for j in res["journeys"]}) + self.assertIn("Dockerfile", res["unmodeled_files"]) + self.assertTrue(any("Dockerfile" in w for w in res["warnings"])) + self.assertFalse(any("002.sql" in w for w in res["warnings"]), res["warnings"]) + + def test_no_match_is_the_117_fallback(self): + self._commit({"packages/web/migrations/001.sql": "x\n"}) + res = self._select() + self.assertTrue(res["recall_degraded"]) + self.assertEqual(res["file_triggers"], []) + + def test_star_does_not_cross_a_directory(self): + self._commit({"styles/nested/site.css": "x\n"}) + res = self._select() + self.assertEqual(res["file_triggers"], []) + self.assertTrue(res["recall_degraded"]) + + def test_renames_match_on_either_side_and_deletions_match(self): + self._commit({"packages/db/migrations/003.sql": "x\n"}) + os.makedirs(os.path.join(self.repo, "archive")) + self.run("git", "mv", "packages/db/migrations/003.sql", "archive/003.sql") + self.run("git", "commit", "-qm", "move out") + res = self._select() + self.assertIn("packages/db/migrations/003.sql", + {t["path"] for t in res["file_triggers"]}) + # the new side, archive/003.sql, matches nothing -> #117 still applies + self.assertTrue(res["recall_degraded"]) + os.remove(os.path.join(self.repo, "archive", "003.sql")) + self._commit({"packages/db/migrations/004.sql": "x\n"}) + self.run("git", "rm", "-q", "packages/db/migrations/004.sql") + self.run("git", "commit", "-qm", "delete") + res = self._select() + self.assertFalse(res["recall_degraded"], res["warnings"]) + self.assertEqual(["J2", "J3"], sorted(j["id"] for j in res["journeys"])) + + def test_glob_semantics(self): + cases = [ + ("db/**", "db/a.sql", True), ("db/**", "db/x/y/a.sql", True), + ("db/**", "db", False), ("**/schema.sql", "schema.sql", True), + ("**/schema.sql", "a/b/schema.sql", True), + ("a/**/b.sql", "a/b.sql", True), ("a/**/b.sql", "a/x/y/b.sql", True), + ("a/**/b.sql", "a/xb.sql", False), ("**/b.sql", "xb.sql", False), + ("a/*.sql", "a/b.sql", True), ("a/*.sql", "a/x/b.sql", False), + ("a/?.sql", "a/b.sql", True), ("a/?.sql", "a/bc.sql", False), + ("a/b.sql", "a/b.sql", True), ("a/b.sql", "x/a/b.sql", False), + ("a.b/*", "aXb/c", False), + ] + for pattern, path, want in cases: + self.assertEqual(reg.trigger_matches(pattern, path), want, (pattern, path)) + + def test_an_invalid_trigger_blocks(self): + bad = { + "unknown journey": {"db/**": ["J1", "J9"]}, + "empty list": {"db/**": []}, + "not a list": {"db/**": "J1"}, + "absolute": {"/db/**": ["J1"]}, + "dotdot": {"../db/**": ["J1"]}, + "brackets": {"db/[ab].sql": ["J1"]}, + "empty glob": {"": ["J1"]}, + } + self._commit({"README.md": "x\n"}) + for label, triggers in bad.items(): + with self.subTest(label): + self.registry = self._registry(triggers) + res = self._select() + self.assertEqual(res["status"], "BLOCKED", res.get("warnings")) + self.assertTrue(any("file_triggers" in b for b in res["blocking"]), + res["blocking"]) + self.registry = self._registry(["db/**"]) + self.assertEqual(self._select()["status"], "BLOCKED") + + class ClosureConfinedTests(unittest.TestCase): """Issue #63: seeds that resolve fine but whose closure never leaves the file they started in used to be indistinguishable from a confident, From e7f754fc69a6b456be3d80de961ac8467a90a3ec Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Fri, 30 Oct 2026 08:48:58 -0400 Subject: [PATCH 2/2] fix: refuse never-matching trigger globs and carry triggers into the export map (#118 review) - H1: export lists file_triggers in the map (## File triggers) and the --json sidecar, and refuses an invalid trigger like select does; the verify skill unions matching triggers into the set. - H2: a glob with an empty, `.` or `..` segment, or `**` inside a segment, can never match a file and now BLOCKs instead of dropping the declared journeys behind a clean NONE. - H3: a trailing `**` matches a path containing a newline. - H4: at most two `**` segments per glob; more backtracked for >20s. --- TECHNICAL.md | 14 +++++-- skills/testgraph-verify/SKILL.md | 6 ++- testgraph/export.py | 28 ++++++++++++- testgraph/registry.py | 23 ++++++++--- tests/test_core.py | 68 ++++++++++++++++++++++++++++++++ 5 files changed, 128 insertions(+), 11 deletions(-) diff --git a/TECHNICAL.md b/TECHNICAL.md index 42a1840..13db91b 100644 --- a/TECHNICAL.md +++ b/TECHNICAL.md @@ -665,7 +665,9 @@ Some dependencies the graph cannot model are still known: a SQL migration under - **Globs** are repo-relative and match the whole path, case-sensitively: `**` is zero or more whole segments, `*` any run inside one segment, `?` one character - inside a segment. No `[...]`, `{...}`, `!`, `\`, leading `/` or `..`. + inside a segment. No `[...]`, `{...}`, `!` or `\`; no empty, `.` or `..` + segment (so no leading `./` or `/`, trailing `/`, or `//`); `**` only as a whole + segment, at most two per glob (more backtrack for seconds on deep paths). - **Matched against every raw diff path** (`diff_files`: added, deleted, both sides of a rename, binary, submodule), before the product filter. - **Additive.** A journey the graph also selected keeps its graph row and gains @@ -679,8 +681,14 @@ Some dependencies the graph cannot model are still known: a SQL migration under reported as unmodeled in the text or the hook. There is no partial coverage: to be conservative, list more journeys. A matched product path still goes through the graph (and #29) as before. -- **An unmatched unmodeled path still degrades.** A typo'd glob cannot be - detected, but it fails safe, into the #117 fallback. +- **An unmatched unmodeled path still degrades.** The globs that can never + match a file (the shapes above) are refused. A plausible but wrong glob can't + be detected. It fails safe for an unmodeled path, which falls back to every + journey, but NOT for an exempt or product path, which just loses the declared + journeys, so check new triggers against a real diff. +- **The export map lists the triggers** under `## File triggers` (and in the + `--json` sidecar's `meta.file_triggers`), and refuses an invalid trigger as + `select` does. Otherwise the map would disagree with the selector. - **Invalid triggers BLOCK**: a malformed glob, a value that isn't a non-empty list, or an unknown journey id. A typo'd id would otherwise drop a journey the registry declares affected. diff --git a/skills/testgraph-verify/SKILL.md b/skills/testgraph-verify/SKILL.md index a33a5e5..9793ee8 100644 --- a/skills/testgraph-verify/SKILL.md +++ b/skills/testgraph-verify/SKILL.md @@ -41,7 +41,9 @@ out of scope. use the line range as the fallback key and read every row it plausibly covers. An edit you cannot attribute to any row is *unknown* — the absence rule below does not apply to it. -4. Union the journey IDs. That is your verification set. +4. Union the journey IDs, plus the journeys of every `## File triggers` glob a + changed path matches (declared in the registry, not derived from the graph). + That is your verification set. 5. A journey marked `!` was reached only through weak or synthesized graph edges. Do not treat it as probably-fine — verify by hand or say you did not. 6. Report the journey IDs and names you verified, and name any you did not. @@ -56,7 +58,7 @@ below means *unknown*, never *none*: | You **added** a `.py` function or file | Run `select` (below). A new symbol has no row by definition. | | You **deleted** or **renamed** a `.py` file | Run `select`. If the file is still in the index it seeds that file's symbols precisely and returns a *narrow* list — correct, not a malfunction. Only when the file has already left the index does it report unbounded impact and list every journey. | | You changed **any non-`.py` file** (`.js`, `.jsx`, `.ts`, `.tsx`, `.svelte`, `.vue`, …) | Run `select` — it seeds these paths (issue #21; it used to answer `NONE` for all of them). If your extension is not in the index it prints `RECALL DEGRADED` and lists every journey: that is *unknown*, not none. A journey reached only through frontend code often arrives on a weak edge — expect `!` and verify by hand. | -| You changed a file the index does not model — SQL, `Dockerfile`, CI YAML, CSS, JSON/toml config, anything that is not code, a test, docs or a `.d.ts` | Run `select`. It names the path as `unmodeled:`, prints `RECALL DEGRADED` and lists every journey (issue #117): its impact is *unknown*, not none. The map can never answer for it. | +| You changed a file the index does not model — SQL, `Dockerfile`, CI YAML, CSS, JSON/toml config, anything that is not code, a test, docs or a `.d.ts` | If a `## File triggers` glob in the map matches it, its journeys are the declared answer (issue #118). Otherwise run `select`: it names the path as `unmodeled:`, prints `RECALL DEGRADED` and lists every journey (issue #117), because its impact is *unknown*, not none. | | The file has no `###` section at all | Unknown. The indexer may not cover it. Escalate and say so. | | `generated from commit` is far behind HEAD | The map under-reports. Regenerate (below) or escalate. | | The header says the **registry was not human-approved** | A different failure from the one below, with a different fix. The index may be fine; what is unverified is whether the registry lists every journey this app has. Absence of a symbol from every journey is *unknown*, not *unaffected*. **Do not re-index** — it cannot clear this. Escalate, or review and approve the registry (`skills/testgraph-propose`). | diff --git a/testgraph/export.py b/testgraph/export.py index 7ac301a..4d9add4 100644 --- a/testgraph/export.py +++ b/testgraph/export.py @@ -263,6 +263,22 @@ def render_markdown(rows_by_file, registry, meta): entries = ", ".join(f"`{e['name']}`" for e in spec["entries"]) out.append(f"- **{jid}** {spec['name']} — entry: {entries}") out.append("") + # The selector adds these journeys for any changed path matching the glob, + # whatever the graph says (#118). Without them here the map disagrees with + # `select`, and an agent reading it would skip a declared journey. + triggers = registry.get("file_triggers") or {} + if triggers: + out += [ + "## File triggers", + "", + "Declared, not graph-derived: if a path you changed matches one of " + "these globs (`**` = any number of directories, `*` = within one), " + "add its journeys to your set, whatever the rows below say.", + "", + ] + for pattern, jids in triggers.items(): + out.append(f"- `{pattern}` → {', '.join(jids)}") + out.append("") out.append("## Symbols by file") for path in sorted(rows_by_file): out.append("") @@ -397,6 +413,14 @@ def main(argv=None): f"index; it would silently vanish from every row" ) + # Same refusal as `select`: an unknown journey id in a trigger would drop a + # declared journey from the map's trigger list too (#118). + trigger_problems = reg.trigger_problems(registry) + if trigger_problems: + print("BLOCKED — registry file_triggers invalid; map NOT written", + file=sys.stderr) + for t in trigger_problems: + print(f" x {t}", file=sys.stderr) if provenance: print("BLOCKED — provenance unverifiable; map NOT written", file=sys.stderr) for p in provenance: @@ -405,7 +429,7 @@ def main(argv=None): print("BLOCKED — index not trustworthy; map NOT written", file=sys.stderr) for b in blocking: print(f" x {b}", file=sys.stderr) - if provenance or blocking: + if provenance or blocking or trigger_problems: return 2 rows_by_file = build_map(conn, registry) @@ -423,6 +447,8 @@ def main(argv=None): # same rule, different cause: whether a human ever reviewed the registry # is a provenance fact about the map's INPUT, not about the index "registry_approval": approval, + # the --json consumer needs the declared triggers too (#118) + "file_triggers": registry.get("file_triggers") or {}, } md = render_markdown(rows_by_file, registry, meta) diff --git a/testgraph/registry.py b/testgraph/registry.py index a5e4f40..93210bd 100644 --- a/testgraph/registry.py +++ b/testgraph/registry.py @@ -470,6 +470,9 @@ def _binds(node, name): # one character inside a segment. fnmatch's `*` crosses `/`, and # PurePath.full_match / glob.translate are 3.13+, so the translation is ours. _GLOB_UNSUPPORTED = re.compile(r"[\[\]!{}\\]") +# More `**` segments than this backtrack badly on deep paths (8 chained took +# >20s, enough to hang the pre-push hook), and no real trigger needs them. +MAX_DOUBLE_STAR = 2 @functools.lru_cache(maxsize=None) @@ -479,7 +482,9 @@ def _trigger_regex(pattern): for i, part in enumerate(parts): last = i == len(parts) - 1 if part == "**": - out.append(".*" if last else "(?:[^/]+/)*") + # [\s\S], not `.`: a path may contain a newline (git quotes it, the + # diff parser unquotes it), and `.` would silently miss it. + out.append(r"[\s\S]*" if last else "(?:[^/]+/)*") continue seg = "".join("[^/]*" if c == "*" else "[^/]" if c == "?" else re.escape(c) for c in part) @@ -506,11 +511,19 @@ def trigger_problems(registry): known = registry.get("journeys", {}) problems = [] for pattern, jids in triggers.items(): - if (not isinstance(pattern, str) or not pattern or pattern.startswith("/") - or ".." in pattern.split("/") or _GLOB_UNSUPPORTED.search(pattern)): + segs = pattern.split("/") if isinstance(pattern, str) else [] + # Every one of these can never match a file path, so it would drop the + # declared journeys behind a clean NONE on an exempt or graph-mapped + # path: an empty segment (`docs/`, `a//b`, a leading `/`), `.` or `..`, + # `**` inside a segment (`tests/**.json` is a one-segment `*`). + if (not segs or _GLOB_UNSUPPORTED.search(pattern) + or any(s in ("", ".", "..") or ("**" in s and s != "**") + for s in segs) + or segs.count("**") > MAX_DOUBLE_STAR): problems.append( - f"file_triggers glob {pattern!r} is not a repo-relative glob " - f"(no leading /, no .., only * ** ?)" + f"file_triggers glob {pattern!r} can never match as written — " + f"repo-relative, no empty, `.` or `..` segments, `**` only as a " + f"whole segment (at most {MAX_DOUBLE_STAR}), only * ** ?" ) continue if (not isinstance(jids, list) or not jids diff --git a/tests/test_core.py b/tests/test_core.py index bb6d952..d61ea81 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2463,6 +2463,33 @@ def test_glob_semantics(self): for pattern, path, want in cases: self.assertEqual(reg.trigger_matches(pattern, path), want, (pattern, path)) + def test_a_glob_that_can_never_match_blocks(self): + """Review round 1 (H2): these validated but matched nothing, so on an + exempt or graph-mapped path the declared journey vanished behind a + clean NONE.""" + for pattern in ("tests/**.json", "a**b/c", "docs/", "docs/**/", "db//x.sql", + "./db/**", "db/./x.sql"): + with self.subTest(pattern): + self.assertTrue( + reg.trigger_problems({"journeys": self.REG, + "file_triggers": {pattern: ["J1"]}}), + pattern) + + def test_a_trailing_double_star_matches_any_name(self): + """H3: `.*` without DOTALL missed a path containing a newline.""" + self.assertTrue(reg.trigger_matches("docs/**", "docs/gui\nde.md")) + self.assertTrue(reg.trigger_matches("**", "a\nb")) + + def test_pathological_globs_are_refused_or_fast(self): + """H4: chained stars backtracked for seconds, enough to hang the hook.""" + many = "/".join(["**", "a"] * 4) + "/x" + self.assertTrue(reg.trigger_problems( + {"journeys": self.REG, "file_triggers": {many: ["J1"]}})) + start = time.monotonic() + reg.trigger_matches("**/**/a/x", "/".join(["a"] * 60) + "/y") + reg.trigger_matches("a*a*a*a*b", "a" * 200) + self.assertLess(time.monotonic() - start, 0.5) + def test_an_invalid_trigger_blocks(self): bad = { "unknown journey": {"db/**": ["J1", "J9"]}, @@ -2485,6 +2512,47 @@ def test_an_invalid_trigger_blocks(self): self.assertEqual(self._select()["status"], "BLOCKED") +class ExportFileTriggerTests(unittest.TestCase): + """Review round 1 (H1): the static map must carry the triggers the selector + applies, or an agent reading it skips a declared journey; and an invalid + trigger must stop the map as it stops `select`.""" + + def _registry(self, tmp, triggers): + path = os.path.join(tmp, "reg.json") + with open(path, "w") as fh: + json.dump({"codegraph_schema_version": 8, "approved": True, + "journeys": FileTriggerTests.REG, "spot_checks": {}, + "file_triggers": triggers}, fh) + return path + + def setUp(self): + self.tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.tmp, ignore_errors=True) + self.db = _db_on_disk(self.tmp, build_fixture()) + self.repo, _ = _git_repo(self.tmp, {"app/svc.py": 22}) + + def _export(self, triggers): + out = os.path.join(self.tmp, "map.md") + err = io.StringIO() + with contextlib.redirect_stderr(err), contextlib.redirect_stdout(io.StringIO()): + rc = exp.main(["--repo", self.repo, "--db", self.db, "--registry", + self._registry(self.tmp, triggers), "--out", out]) + text = open(out).read() if os.path.exists(out) else "" + return rc, text, err.getvalue() + + def test_the_map_lists_the_triggers(self): + rc, text, err = self._export({"packages/db/migrations/**": ["J2", "J3"]}) + self.assertEqual(rc, 0, err) + self.assertIn("## File triggers", text) + self.assertIn("`packages/db/migrations/**` → J2, J3", text) + + def test_an_invalid_trigger_blocks_the_map(self): + rc, text, err = self._export({"db/**": ["J9"]}) + self.assertEqual(rc, 2) + self.assertIn("file_triggers", err) + self.assertEqual(text, "") + + class ClosureConfinedTests(unittest.TestCase): """Issue #63: seeds that resolve fine but whose closure never leaves the file they started in used to be indistinguishable from a confident,