From 9de7fe9b325a7210c90cd390a7b8b46f1deb40a8 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Fri, 30 Oct 2026 07:34:09 -0400 Subject: [PATCH 1/2] fix: an unmodeled diff path degrades to every journey; verify's UNKNOWN NONE is not a pass (#117, #113) #117: the diff parser dropped every path outside PRODUCT_EXT before impact analysis, so a migration-, Dockerfile-, CI- or CSS-only diff answered "0 changed files, NONE" with no warning. Every path is now accounted for (diff_files) and classified: product (seeded as before), exempt with a reason (test, docs, type declaration), or unmodeled, which joins the #29 unmapped fallback: warning, recall_degraded, every journey listed verify_manually. select names each unmodeled path; the hook gives them a capped NOTE line of their own. #113: verify's NONE headline read "nothing to verify" with the UNKNOWN reasons indented below it. It now uses select's two-sentence headline, and an UNKNOWN empty selection exits 3 instead of 0. --- README.md | 7 +- TECHNICAL.md | 43 +++++++++- skills/testgraph-verify/SKILL.md | 1 + testgraph/hook.py | 9 +++ testgraph/select.py | 87 +++++++++++++++++++-- testgraph/verify.py | 20 ++++- tests/test_core.py | 130 +++++++++++++++++++++++++++++++ tests/test_verify.py | 27 +++++++ 8 files changed, 311 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index a4a3d25..2c5f81a 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ python3 -m testgraph.verify --repo . --base origin/main -- tests/ -q # exit 0 every selected journey ran and passed # exit 1 a journey's tests failed # exit 2 refused: the index can't be trusted -# exit 3 a selected journey has no journey-level test — named, not hidden +# exit 3 a selected journey has no journey-level test, or the selection is UNKNOWN — named, not hidden ``` That last case is not hypothetical. Run on itself, testgraph found that the 20 @@ -176,7 +176,10 @@ It runs `codegraph sync` first. Seeds come from *line ranges*, so an index built before the code moved resolves the diff against stale spans, and nothing else refreshes these indexes on commit. If a changed file's bytes still disagree with the indexed copy afterwards, the answer degrades to `RECALL DEGRADED` and names the file -instead of quietly trusting the wrong line numbers. +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"). 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 3365c84..ce2f9e6 100644 --- a/TECHNICAL.md +++ b/TECHNICAL.md @@ -69,7 +69,7 @@ leaving edges wrong) — only a full `codegraph index` did. Three checks: - `testgraph/registry.py` — loads the journey JSON, resolves entry `name`+`file` to node ids at run time (ids are stable, names drift), maps every matching node to its journey. -- `testgraph/select.py` — diff parsing (test/e2e files excluded as seeds), +- `testgraph/select.py` — diff parsing (test/e2e/docs files exempt, unmodeled paths degrade — #117), orchestration, ranking, human + `--json` output, the CLI. - `testgraph/propose.py` — drafts a registry for a repo that has none: ast scan for route-decorated handlers, index resolution, spot-check derivation, declared @@ -649,6 +649,47 @@ 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. +## Unmodeled Paths Degrade Too (issue #117) + +The parser used to drop every path outside `PRODUCT_EXT` before anything looked +at it, so an external trial's migration-only diff reported **0 changed files, 0 +journeys and no warning**. A SQL migration, a `Dockerfile`, CI YAML or a +stylesheet can change runtime behaviour without ever becoming a CodeGraph node; +"not indexed" is not "no impact". + +Every path in the diff is now accounted for before filtering +(`_parse_unified_diff(diff, paths)`: both sides of a rename, the `---` side of a +deletion, binary and mode-only blocks) and lands in one of three buckets on the +result: + +| bucket | field | effect | +|---|---|---| +| product (`_is_product`) | `changed_files` | seeded as before | +| exempt | `exempt_files` `{path: reason}` | none; reason is `test` (`_is_test`), `type declaration` (`.d.ts` family) or `docs` (`.md`/`.rst`/`.adoc`, a `docs/` or `doc/` segment, `LICENSE*`/`COPYING*`/`CHANGELOG*`/`NOTICE*`/`AUTHORS*`) | +| unmodeled (anything else) | `unmodeled_files` | joins `unmapped`: warns, `recall_degraded: true`, every journey listed `verify_manually` | + +`diff_files` lists every raw path, and the text renderer prints +`changed files: N product of M in diff (E exempt, U unmodeled)` plus one +`unmodeled:` line per path. The hook gives unmodeled paths their own capped NOTE +line, like `closure_confined`, so the capped warnings cannot hide them. + +All-journeys rather than UNKNOWN-only was chosen deliberately. UNKNOWN exists +only for an EMPTY list, so a mixed product + unmodeled diff would have needed a +new "partial" signal in select, the hook, MCP and verify; the #29 fallback +already covers all four, and verify then runs every journey and exits 3 unless +each one passes. The exemption list is short on purpose: `requirements.txt`, +`package.json`, `.dockerignore` and the like all change what runs. + +The cost, measured on the last 40 first-parent commits of each dogfooded repo: +22/40 (testgraph), 16/36 (honeyslate) and 21/36 (signedintake) touch an +unmodeled path, mostly `.json`; 12, 4 and 7 of them touch nothing else, and those +used to answer NONE. A per-registry exemption list is the lever if that noise +bites; it is not built. + +verify follows the same rule from the other side: an empty selection whose +`unknown_because` is non-empty prints `journeys selected: NONE — but UNKNOWN, not +verified-safe:` as its headline (#113) and exits 3, not 0. UNKNOWN is not a pass. + ## Re-verification After the Extractor Fix (2026-09-09) Both harnesses were re-run after `codegraph` was rebuilt at extraction version diff --git a/skills/testgraph-verify/SKILL.md b/skills/testgraph-verify/SKILL.md index c067093..a33a5e5 100644 --- a/skills/testgraph-verify/SKILL.md +++ b/skills/testgraph-verify/SKILL.md @@ -56,6 +56,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. | | 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/hook.py b/testgraph/hook.py index f6225c4..574f38b 100644 --- a/testgraph/hook.py +++ b/testgraph/hook.py @@ -109,6 +109,15 @@ def render(result, repo, more_cmd=None): f" NOTE: impact for {shown} did not leave the file it started " f"in — that file's own contribution is UNKNOWN, not verified-safe" ) + # 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", []) + if unmodeled: + shown = ", ".join(unmodeled[:MAX_CONFINED]) + if len(unmodeled) > MAX_CONFINED: + shown += f", … {len(unmodeled) - MAX_CONFINED} more" + lines.append(f" NOTE: not modeled by the index: {shown} — impact UNKNOWN") # Warnings are the channel that carries an unapproved registry and entry # drift — both mean "this answer may be understated", so they are worth the # lines. Capped: an unbounded warning block is the noise problem again. diff --git a/testgraph/select.py b/testgraph/select.py index 70a17f7..3baa84f 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -1,8 +1,9 @@ """testgraph select — git diff -> ranked affected journeys. Recall-first: the closure over-selects on shared-symbol changes rather than -risk dropping a truly-affected journey. Test/e2e file changes are ignored as -seeds (they don't change product behavior). +risk dropping a truly-affected journey. Test, docs and type-declaration changes +are exempt (they don't change product behavior); any other path the index does +not model degrades the answer to every journey (issue #117). Usage: python -m testgraph.select --repo [--base SHA] [--head SHA] @@ -46,6 +47,15 @@ # directory entirely, so root-level test files were seeded as product code. TEST_DIRS = frozenset({"tests", "e2e", "__tests__"}) +# Paths exempt from the unmodeled-path fallback (issue #117) because they cannot +# change runtime behavior: prose, by extension, by directory segment, or by the +# conventional top-level file names. Deliberately short. Everything else outside +# PRODUCT_EXT -- SQL, Dockerfile, CI YAML, CSS, JSON, toml, requirements.txt, +# anything unrecognized -- is UNKNOWN impact, not "safe". +DOC_EXT = (".md", ".rst", ".adoc") +DOC_DIRS = frozenset({"docs", "doc"}) +DOC_BASENAMES = ("LICENSE", "COPYING", "CHANGELOG", "NOTICE", "AUTHORS") + def _is_product(path): return ( @@ -55,6 +65,25 @@ def _is_product(path): ) +def _exemption(path): + """Why a non-product path cannot change behavior, or None when it might.""" + if _is_test(path): + return "test" + if path.endswith(DECLARATION_EXT): + return "type declaration" + parts = path.split("/") + if (path.endswith(DOC_EXT) or DOC_DIRS.intersection(parts[:-1]) + or parts[-1].upper().startswith(DOC_BASENAMES)): + return "docs" + return None + + +def _unmodeled_kind(path): + base = path.rsplit("/", 1)[-1] + ext = os.path.splitext(base)[1] + return f"{ext} file" if ext else base + + def _is_test(path): parts = path.split("/") base = parts[-1] @@ -126,7 +155,7 @@ def _diff_git_path(rest): return None -def _parse_unified_diff(diff): +def _parse_unified_diff(diff, paths=None): """Parse a `git diff --unified=0` into `(ranges, whole_files)` where @@ -152,11 +181,21 @@ def _parse_unified_diff(diff): unknown, not absent. The one exception is a block git marks as a new or deleted file without a header, which is an empty file — no content to seed, and a mode-only change (`old mode`/`new mode`, no '+++'), which is a chmod. + + `paths`, when given, is a set that collects EVERY path in the diff, product + or not, before any filtering (issue #117): the caller accounts for each one + instead of inferring "nothing changed" from what survived the filter. """ ranges, whole_files, cur, prev = {}, {}, None, None blk = None # state of the current `diff --git` block + def note(path): + if paths is not None and path: + paths.add(path) + def close_block(): + if blk: + note(blk["path"]) if (blk and not blk["header"] and not blk["rename"] and not blk["empty"] and not blk["mode"]): path = blk["path"] @@ -183,12 +222,14 @@ def close_block(): blk["empty"] = True elif line.startswith("rename from "): p = _unquote_git_path(line[len("rename from "):]) + note(p) if blk: blk["rename"] = True if _is_product(p): whole_files[p] = "renamed from" elif line.startswith("rename to "): p = _unquote_git_path(line[len("rename to "):]) + note(p) if blk: blk["rename"] = True if _is_product(p): @@ -202,12 +243,14 @@ def close_block(): blk["header"] = True if path == "/dev/null": # whole-file deletion: the surviving path is on the '---' line + note(prev) if prev and _is_product(prev): whole_files[prev] = "deleted" cur = None continue if path.startswith("b/"): path = path[2:] + note(path) cur = path if _is_product(path) else None if cur is not None: ranges.setdefault(cur, []) @@ -228,7 +271,7 @@ def close_block(): return {f: r for f, r in ranges.items() if r}, whole_files -def changed_ranges(repo, base, head): +def changed_ranges(repo, base, head, paths=None): diff = subprocess.run( # Every flag pins something a user's git config could otherwise change # under us (audit C1): diff.noprefix / mnemonicPrefix drop the `b/` the @@ -248,7 +291,7 @@ def changed_ranges(repo, base, head): errors="surrogateescape", check=True, ).stdout - return _parse_unified_diff(diff) + return _parse_unified_diff(diff, paths) def select(repo, base, head, db_path, registry_path, strict_registry=True): @@ -335,7 +378,8 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): result["blocking"] = blocking return result - ranges, whole_files = changed_ranges(repo, base, head) + diff_paths = set() + ranges, whole_files = changed_ranges(repo, base, head, diff_paths) # A changed file whose hunks resolve to NO node is the same failure as an # unmappable whole-file change, and it used to pass silently: seeds stayed @@ -407,6 +451,23 @@ def _mark_unmapped(path, detail): # # The `pre-push` hook runs `codegraph sync` first precisely so this stays the # exceptional path rather than every push. + # A changed path the index does not model at all — SQL, a Dockerfile, CI + # YAML, CSS — never becomes a seed, and used to vanish in the parser: a + # migration-only diff answered "0 changed files, NONE" with no warning + # (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 = {}, [] + for path in sorted(diff_paths): + if _is_product(path): + continue + why = _exemption(path) + if why: + exempt_files[path] = why + else: + unmodeled_files.append(path) + _mark_unmapped(path, f"not modeled by the index: {_unmodeled_kind(path)}") + drifted = integrity.content_drift(conn, repo, set(ranges) | set(whole_files)) for path in sorted(drifted): _mark_unmapped(path, "bytes differ from the indexed copy — line spans are stale") @@ -541,6 +602,9 @@ def _mark_unmapped(path, detail): result.update( status="OK", changed_files=sorted(ranges), + diff_files=sorted(diff_paths), + exempt_files=exempt_files, + unmodeled_files=unmodeled_files, whole_file_changes=whole_files, recall_degraded=bool(unmapped), closure_confined=confined_files, @@ -623,12 +687,21 @@ def _render(result): for b in result["blocking"]: lines.append(f" x {b}") return "\n".join(lines) + changed = f"changed files: {len(result['changed_files'])}" + if "diff_files" in result: + changed += ( + f" product of {len(result['diff_files'])} in diff " + f"({len(result.get('exempt_files', {}))} exempt, " + f"{len(result.get('unmodeled_files', []))} unmodeled)" + ) lines.append( - f"changed files: {len(result['changed_files'])} | " + f"{changed} | " f"seeds: {result['seed_symbols']} | impacted symbols: {result['impacted_symbols']}" ) for path, reason in sorted(result.get("whole_file_changes", {}).items()): lines.append(f" whole-file: {path} ({reason})") + for path in result.get("unmodeled_files", []): + 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", []): diff --git a/testgraph/verify.py b/testgraph/verify.py index b305944..71936c4 100644 --- a/testgraph/verify.py +++ b/testgraph/verify.py @@ -264,6 +264,10 @@ def run(repo, base, head="HEAD", registry_path=None, db_path=None, conn.close() +def _unknown_because(selection): + return selection.get("unknown_because") or sel.none_is_unknown(selection) + + def exit_code(summary): """OK / tests-failed / refused / incomplete, as distinct answers. @@ -290,6 +294,10 @@ def exit_code(summary): # is not a pass, and the exit code is what a CI job actually reads. if summary.get("uncovered") or summary.get("incomplete"): return EXIT_INCOMPLETE + # An empty selection the selector itself cannot vouch for verified nothing + # (#117): exit 0 would turn UNKNOWN into a pass. + if not summary.get("journeys") and _unknown_because(summary.get("selection") or {}): + return EXIT_INCOMPLETE if any(credited.get(j) != "pass" for j in summary.get("journeys", [])): return EXIT_INCOMPLETE if summary.get("pytest_exit") not in (None, 0, 5): @@ -318,9 +326,15 @@ def render(summary, registry=None): lines.append(f" WARN: {w}") if not summary["journeys"]: - lines.append(" journeys selected: NONE — nothing to verify") - for r in sel.none_is_unknown(sel_): - lines.append(f" UNKNOWN, not verified-safe: {r}") + # Two sentences for two claims, as in select._render and hook.render + # (#75): the UNKNOWN belongs in the headline, not indented below a + # line that reads as a clean bill (#113). + unknown = _unknown_because(sel_) + if unknown: + lines.append(" journeys selected: NONE — but UNKNOWN, not verified-safe:") + lines.extend(f" {r}" for r in unknown) + else: + lines.append(" journeys selected: NONE — nothing to verify") return "\n".join(lines) lines.append(f" journeys selected: {', '.join(summary['journeys'])}") diff --git a/tests/test_core.py b/tests/test_core.py index 81770c7..e2ef398 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -25,6 +25,7 @@ from testgraph import registry as reg # noqa: E402 from testgraph import export as exp # noqa: E402 from testgraph import select as sel # noqa: E402 +from testgraph import hook # noqa: E402 def build_fixture(): @@ -2141,6 +2142,135 @@ def test_one_mapped_file_does_not_mask_an_unmapped_one(self): self.assertGreater(res["seed_symbols"], 0) +class UnmodeledDiffPathTests(unittest.TestCase): + """Issue #117: an external trial's migration-only diff reported 0 changed + files, 0 journeys and no warning. The parser dropped every path outside + PRODUCT_EXT before impact analysis, so a SQL, Dockerfile, CI or CSS change + read as a confident NONE. A path the index does not model is UNKNOWN impact, + and recall-first means every journey (the #29 fallback), not silence.""" + + REG = ZeroSeedDegradesTests.REG + + 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.registry = _registry_file(self.tmp, self.REG) + self.repo, self.run = _git_repo(self.tmp, {"app/svc.py": 22}) + + def _commit(self, files): + for rel, text in files.items(): + full = os.path.join(self.repo, rel) + os.makedirs(os.path.dirname(full) or self.repo, 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) + + UNMODELED = ( + "packages/db/migrations/001_add_rls.sql", + "services/api/Dockerfile", + ".github/workflows/ci.yml", + "apps/web/src/app.css", + ) + + def test_each_unmodeled_path_alone_degrades_to_every_journey(self): + for rel in self.UNMODELED: + with self.subTest(rel=rel): + self._commit({rel: f"-- {rel}\n"}) + res = self._select() + self.assertEqual(res["status"], "OK") + # 1. the raw path is accounted for, not lost + self.assertIn(rel, res["diff_files"]) + self.assertIn(rel, res["unmodeled_files"]) + # 2. named, with why its impact is unknown + self.assertTrue(any(rel in w and "not modeled" in w + for w in res["warnings"]), res["warnings"]) + # 3. not a clean NONE in JSON or text + self.assertTrue(res["recall_degraded"]) + self.assertEqual({"J1", "J2"}, {j["id"] for j in res["journeys"]}) + text = sel._render(res) + self.assertNotIn("no product-behavior change detected", text) + self.assertIn(f"unmodeled: {rel}", text) + pushed = hook.render(res, self.repo) + self.assertNotIn("no product-behavior change detected", pushed) + self.assertIn(f"not modeled by the index: {rel}", pushed) + + def test_counts_tell_raw_changes_from_eligible_ones(self): + self._commit({"migrations/001.sql": "x\n", "README.md": "x\n"}) + res = self._select() + self.assertEqual(res["changed_files"], []) + self.assertEqual(sorted(res["diff_files"]), + ["README.md", "migrations/001.sql"]) + self.assertIn("0 product of 2 in diff (1 exempt, 1 unmodeled)", + sel._render(res)) + + def test_docs_and_tests_only_diffs_keep_their_exemption(self): + self._commit({"README.md": "x\n", "docs/guide.txt": "x\n", + "tests/fixtures/seed.sql": "x\n", "LICENSE": "x\n", + "app/types.d.ts": "x\n"}) + res = self._select() + self.assertFalse(res["recall_degraded"], res["warnings"]) + self.assertEqual(res["journeys"], []) + self.assertEqual(res["unmodeled_files"], []) + self.assertEqual(set(res["exempt_files"]), + {"README.md", "docs/guide.txt", "tests/fixtures/seed.sql", + "LICENSE", "app/types.d.ts"}) + self.assertEqual(res["exempt_files"]["tests/fixtures/seed.sql"], "test") + self.assertEqual(res["exempt_files"]["README.md"], "docs") + + def test_an_unmodeled_path_is_not_hidden_behind_a_real_selection(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 (10-20) -> J1 + with open(full, "w") as fh: + fh.writelines(lines) + self._commit({"Dockerfile": "FROM x\n"}) + res = self._select() + self.assertTrue(res["recall_degraded"]) + self.assertEqual({"J1", "J2"}, {j["id"] for j in res["journeys"]}) + self.assertIn("Dockerfile", res["unmodeled_files"]) + j1 = next(j for j in res["journeys"] if j["id"] == "J1") + self.assertNotIn("reason", j1) + + def test_paths_survive_an_unreadable_diff_git_line(self): + """The `diff --git` line is only a fallback; the headers must still + account for the path when it cannot be split.""" + diff = ("diff --git ??? ???\n" + "deleted file mode 100644\n" + "--- a/db/old.sql\n" + "+++ /dev/null\n" + "@@ -1 +0,0 @@\n" + "-x\n" + "diff --git ??? ???\n" + "--- a/db/new.sql\n" + "+++ b/db/new.sql\n" + "@@ -1 +1 @@\n" + "-x\n" + "+y\n") + paths = set() + sel._parse_unified_diff(diff, paths) + self.assertEqual(paths, {"db/old.sql", "db/new.sql"}) + + def test_a_deleted_or_renamed_unmodeled_path_counts(self): + self._commit({"db/schema.sql": "x\n"}) + self.run("git", "mv", "db/schema.sql", "db/schema2.sql") + self.run("git", "commit", "-qm", "rename") + res = self._select() + self.assertTrue(res["recall_degraded"]) + self.assertIn("db/schema2.sql", res["unmodeled_files"]) + os.remove(os.path.join(self.repo, "db", "schema2.sql")) + self.run("git", "add", "-A") + self.run("git", "commit", "-qm", "delete") + res = self._select() + self.assertIn("db/schema2.sql", res["unmodeled_files"]) + self.assertTrue(res["recall_degraded"]) + + 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, diff --git a/tests/test_verify.py b/tests/test_verify.py index 96ade40..6f76ae5 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -389,6 +389,33 @@ def test_the_repo_name_reaches_a_none_answer(self): class RenderTests(unittest.TestCase): + def test_a_none_with_unknown_reasons_says_so_in_the_headline(self): + """#113: the headline used to read `NONE — nothing to verify` with the + UNKNOWN reasons only indented below it; select and the hook put + UNKNOWN in the headline itself (#75).""" + sel_ = {"base": "a", "head": "b", "warnings": ["stale index"], + "journeys": [], "unknown_because": ["1 trust warning(s) above"]} + out = verify.render(verify._summary("/tmp/demo", "HEAD", sel_), REGISTRY) + headline = next(l for l in out.splitlines() if "journeys selected" in l) + self.assertIn("UNKNOWN, not verified-safe", headline) + self.assertNotIn("nothing to verify", headline) + + def test_a_clean_none_keeps_its_wording(self): + sel_ = {"base": "a", "head": "b", "warnings": [], "journeys": [], + "unknown_because": []} + out = verify.render(verify._summary("/tmp/demo", "HEAD", sel_), REGISTRY) + self.assertIn("journeys selected: NONE — nothing to verify", out) + + def test_an_unknown_none_is_not_an_exit_0_pass(self): + """#117 criterion 4: verify cannot turn UNKNOWN into exit-0 PASS.""" + sel_ = {"base": "a", "head": "b", "warnings": ["stale"], "journeys": [], + "unknown_because": ["1 trust warning(s) above"]} + self.assertEqual(verify.exit_code(verify._summary("/x", "HEAD", sel_)), + verify.EXIT_INCOMPLETE) + clean = dict(sel_, warnings=[], unknown_because=[]) + self.assertEqual(verify.exit_code(verify._summary("/x", "HEAD", clean)), + verify.EXIT_OK) + def test_uncovered_journeys_are_named_loudly_and_denied_a_pass(self): out = verify.render({ "repo": "demo", From 935bc0c6e716970af9f5f7c263c28b413b4e806e Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Fri, 30 Oct 2026 07:46:13 -0400 Subject: [PATCH 2/2] fix: narrow the #117 exemptions and pin submodule diffs (review round 1) - H1: --submodule=short --ignore-submodules=none, so diff.ignoreSubmodules, a .gitmodules ignore or diff.submodule=log cannot hide a submodule bump. - H2: doc basenames match exactly (optionally with a prose extension); the prefix match exempted db/authors.sql and config/licenses.json. - H3: only a test directory exempts a non-code file; _is_test's name rules exempted test_and_deploy.yml and openapi.spec.yaml. - H4: verify's exit decision ignores entries_unchecked, which never clears; the headline still shows it. - H5: docs/ and doc/ exempt only at the top level. - verify --help epilog and a misplaced comment updated. --- TECHNICAL.md | 9 ++++++- testgraph/select.py | 63 ++++++++++++++++++++++++++------------------ testgraph/verify.py | 11 +++++--- tests/test_core.py | 41 ++++++++++++++++++++++++++++ tests/test_verify.py | 16 +++++++++++ 5 files changed, 111 insertions(+), 29 deletions(-) diff --git a/TECHNICAL.md b/TECHNICAL.md index ce2f9e6..58dc733 100644 --- a/TECHNICAL.md +++ b/TECHNICAL.md @@ -665,7 +665,7 @@ result: | bucket | field | effect | |---|---|---| | product (`_is_product`) | `changed_files` | seeded as before | -| exempt | `exempt_files` `{path: reason}` | none; reason is `test` (`_is_test`), `type declaration` (`.d.ts` family) or `docs` (`.md`/`.rst`/`.adoc`, a `docs/` or `doc/` segment, `LICENSE*`/`COPYING*`/`CHANGELOG*`/`NOTICE*`/`AUTHORS*`) | +| exempt | `exempt_files` `{path: reason}` | none; reason is `test` (under a `tests/`, `e2e/` or `__tests__/` directory — not `_is_test`'s name rules, which exempted `test_and_deploy.yml`), `type declaration` (`.d.ts` family) or `docs` (`.md`/`.rst`/`.adoc`, a top-level `docs/` or `doc/`, or exactly `LICENSE`/`COPYING`/`CHANGELOG`/`NOTICE`/`AUTHORS` with no extension or a prose one — a prefix match exempted `db/authors.sql`) | | unmodeled (anything else) | `unmodeled_files` | joins `unmapped`: warns, `recall_degraded: true`, every journey listed `verify_manually` | `diff_files` lists every raw path, and the text renderer prints @@ -689,6 +689,13 @@ bites; it is not built. verify follows the same rule from the other side: an empty selection whose `unknown_because` is non-empty prints `journeys selected: NONE — but UNKNOWN, not verified-safe:` as its headline (#113) and exits 3, not 0. UNKNOWN is not a pass. +One reason is left out of the exit decision: an entry no parser covers +(`entries_unchecked`) never clears, so counting it would hold every NONE in such a +repo at exit 3 for good. The headline still lists it. + +`git diff` also runs with `--submodule=short --ignore-submodules=none`: with +`diff.ignoreSubmodules`, a `.gitmodules` `ignore`, or `diff.submodule=log`, a +submodule bump produced no `diff --git` block and answered a clean NONE. ## Re-verification After the Extractor Fix (2026-09-09) diff --git a/testgraph/select.py b/testgraph/select.py index 3baa84f..bb33e78 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -48,13 +48,16 @@ TEST_DIRS = frozenset({"tests", "e2e", "__tests__"}) # Paths exempt from the unmodeled-path fallback (issue #117) because they cannot -# change runtime behavior: prose, by extension, by directory segment, or by the -# conventional top-level file names. Deliberately short. Everything else outside +# change runtime behavior: prose, by extension, by a top-level docs directory, +# or by the conventional file names — matched EXACTLY (optionally with a prose +# extension), because a prefix match exempted `db/authors.sql` and +# `config/licenses.json`. Deliberately short. Everything else outside # PRODUCT_EXT -- SQL, Dockerfile, CI YAML, CSS, JSON, toml, requirements.txt, # anything unrecognized -- is UNKNOWN impact, not "safe". DOC_EXT = (".md", ".rst", ".adoc") DOC_DIRS = frozenset({"docs", "doc"}) -DOC_BASENAMES = ("LICENSE", "COPYING", "CHANGELOG", "NOTICE", "AUTHORS") +DOC_BASENAMES = frozenset({"LICENSE", "COPYING", "CHANGELOG", "NOTICE", "AUTHORS"}) +DOC_BASENAME_EXT = ("", ".txt", ".md", ".rst", ".adoc") def _is_product(path): @@ -66,14 +69,19 @@ def _is_product(path): def _exemption(path): - """Why a non-product path cannot change behavior, or None when it might.""" - if _is_test(path): + """Why a non-product path cannot change behavior, or None when it might. + + Only a test DIRECTORY exempts a non-code file. `_is_test`'s name rules + (`test_*`, `*.test.*`, `*.spec.*`) are conventions for code; on other files + they exempted `test_and_deploy.yml` and `openapi.spec.yaml`.""" + parts = path.split("/") + if TEST_DIRS.intersection(parts[:-1]): return "test" if path.endswith(DECLARATION_EXT): return "type declaration" - parts = path.split("/") - if (path.endswith(DOC_EXT) or DOC_DIRS.intersection(parts[:-1]) - or parts[-1].upper().startswith(DOC_BASENAMES)): + stem, ext = os.path.splitext(parts[-1]) + if (path.endswith(DOC_EXT) or (len(parts) > 1 and parts[0] in DOC_DIRS) + or (stem.upper() in DOC_BASENAMES and ext.lower() in DOC_BASENAME_EXT)): return "docs" return None @@ -284,7 +292,12 @@ def changed_ranges(repo, base, head, paths=None): # --end-of-options: `base`/`head` are never parsed as git options (M2). ["git", "-C", repo, "-c", "core.quotePath=false", "diff", "--no-color", "--no-ext-diff", "--no-textconv", "--src-prefix=a/", "--dst-prefix=b/", - "--unified=0", "-M", "--end-of-options", f"{base}..{head}"], + "--unified=0", "-M", + # diff.ignoreSubmodules, a .gitmodules `ignore`, or diff.submodule=log + # drop or reshape a submodule bump so no `diff --git` block names it + # (#117 review): pin both so the path is always accounted for. + "--submodule=short", "--ignore-submodules=none", + "--end-of-options", f"{base}..{head}"], capture_output=True, text=True, encoding="utf-8", @@ -435,22 +448,6 @@ def _mark_unmapped(path, detail): else: _mark_unmapped(path, reason) - # 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 - # obvious; this one resolves to the WRONG node. Seeds come from line ranges - # (`nodes_for_lines`), so a file that gained twenty lines above a function - # since it was indexed hands the diff's line numbers to whatever symbol used - # to live there — a neighbouring function, or nothing. The answer stays - # confident and can be narrower than the truth, which is the one failure - # this selector is built to refuse. - # - # The seeds computed above are KEPT, not discarded: whatever they resolved to - # is still evidence, and recall-first means adding doubt, not removing rows. - # Joining `unmapped` degrades the answer to "every journey, verify manually" - # — the same treatment issue #29 established for a file with no symbols. - # - # The `pre-push` hook runs `codegraph sync` first precisely so this stays the - # exceptional path rather than every push. # A changed path the index does not model at all — SQL, a Dockerfile, CI # YAML, CSS — never becomes a seed, and used to vanish in the parser: a # migration-only diff answered "0 changed files, NONE" with no warning @@ -468,6 +465,22 @@ def _mark_unmapped(path, detail): unmodeled_files.append(path) _mark_unmapped(path, f"not modeled by the index: {_unmodeled_kind(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 + # obvious; this one resolves to the WRONG node. Seeds come from line ranges + # (`nodes_for_lines`), so a file that gained twenty lines above a function + # since it was indexed hands the diff's line numbers to whatever symbol used + # to live there — a neighbouring function, or nothing. The answer stays + # confident and can be narrower than the truth, which is the one failure + # this selector is built to refuse. + # + # The seeds computed above are KEPT, not discarded: whatever they resolved to + # is still evidence, and recall-first means adding doubt, not removing rows. + # Joining `unmapped` degrades the answer to "every journey, verify manually" + # — the same treatment issue #29 established for a file with no symbols. + # + # The `pre-push` hook runs `codegraph sync` first precisely so this stays the + # exceptional path rather than every push. drifted = integrity.content_drift(conn, repo, set(ranges) | set(whole_files)) for path in sorted(drifted): _mark_unmapped(path, "bytes differ from the indexed copy — line spans are stale") diff --git a/testgraph/verify.py b/testgraph/verify.py index 71936c4..52e2887 100644 --- a/testgraph/verify.py +++ b/testgraph/verify.py @@ -295,8 +295,12 @@ def exit_code(summary): if summary.get("uncovered") or summary.get("incomplete"): return EXIT_INCOMPLETE # An empty selection the selector itself cannot vouch for verified nothing - # (#117): exit 0 would turn UNKNOWN into a pass. - if not summary.get("journeys") and _unknown_because(summary.get("selection") or {}): + # (#117): exit 0 would turn UNKNOWN into a pass. Entries no parser covers + # are left out of this decision: they never clear (registry.unchecked_entries), + # so counting them would hold every NONE in such a repo at exit 3 for good. + # The headline still shows them. + if not summary.get("journeys") and sel.none_is_unknown( + dict(summary.get("selection") or {}, entries_unchecked=[])): return EXIT_INCOMPLETE if any(credited.get(j) != "pass" for j in summary.get("journeys", [])): return EXIT_INCOMPLETE @@ -380,7 +384,8 @@ def main(argv=None): description="run the declared tests for the journeys a diff endangers", epilog="Exit: 0 every selected journey ran and passed, 1 a journey " "failed, 2 refused (untrustworthy index), 3 incomplete — at " - "least one selected journey had no declared test.\n" + "least one selected journey had no passing declared test, or " + "the selection was NONE but UNKNOWN.\n" "Put pytest's own flags after `--`.", formatter_class=argparse.RawDescriptionHelpFormatter, ) diff --git a/tests/test_core.py b/tests/test_core.py index e2ef398..2753125 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2256,6 +2256,47 @@ 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_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, + which answered a clean NONE — the #117 failure again.""" + exempt = { + "README.md": "docs", "LICENSE": "docs", "LICENSE.txt": "docs", + "COPYING": "docs", "NOTICE": "docs", "AUTHORS.rst": "docs", + "docs/x.sql": "docs", "doc/guide.txt": "docs", + "tests/fixtures/seed.sql": "test", "e2e/data.json": "test", + "app/__tests__/snap.json": "test", "app/types.d.ts": "type declaration", + } + for path, why in exempt.items(): + self.assertEqual(sel._exemption(path), why, path) + for path in ("db/authors.sql", "db/changelog_entries.sql", + "db/notice_board.sql", "config/licenses.json", + "config/copying.yaml", "COPYING.sql", + ".github/workflows/license-check.yml", + ".github/workflows/test_and_deploy.yml", + "scripts/test_release.sh", "docker-compose.test.yml", + "db/test_users.sql", "api/openapi.spec.yaml", + "src/app.spec.json", "app/docs/schema.json"): + self.assertIsNone(sel._exemption(path), path) + + def test_a_submodule_bump_is_seen_whatever_the_git_config(self): + """Review round 1 (H1): diff.ignoreSubmodules / diff.submodule=log made + a submodule bump vanish from the diff, so it answered a clean NONE.""" + def bump(sha): + self.run("git", "update-index", "--add", "--cacheinfo", + f"160000,{sha},vendor/sub") + self.run("git", "commit", "-qm", "bump") + bump("1" * 40) + bump("2" * 40) + for key, value in (("diff.ignoreSubmodules", "all"), + ("diff.submodule", "log")): + with self.subTest(config=key): + self.run("git", "config", key, value) + res = self._select() + self.assertIn("vendor/sub", res["diff_files"]) + self.assertTrue(res["recall_degraded"]) + self.run("git", "config", "--unset", key) + def test_a_deleted_or_renamed_unmodeled_path_counts(self): self._commit({"db/schema.sql": "x\n"}) self.run("git", "mv", "db/schema.sql", "db/schema2.sql") diff --git a/tests/test_verify.py b/tests/test_verify.py index 6f76ae5..226faee 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -400,6 +400,22 @@ def test_a_none_with_unknown_reasons_says_so_in_the_headline(self): self.assertIn("UNKNOWN, not verified-safe", headline) self.assertNotIn("nothing to verify", headline) + def test_a_permanent_unchecked_entry_does_not_hold_ci_red(self): + """Review round 1 (H4): an entry no parser covers never clears, so a + NONE whose only UNKNOWN reason is that would exit 3 on every run. The + headline still says UNKNOWN; the exit code does not decide on it.""" + sel_ = {"base": "a", "head": "b", "warnings": [], "journeys": [], + "closure_confined": [], + "entries_unchecked": [{"journey": "J1", "entry": "x", "file": "a.ts"}]} + sel_["unknown_because"] = verify.sel.none_is_unknown(sel_) + summary = verify._summary("/x", "HEAD", sel_) + self.assertIn("UNKNOWN", verify.render(summary, REGISTRY)) + self.assertEqual(verify.exit_code(summary), verify.EXIT_OK) + stale = dict(sel_, warnings=["stale"]) + stale["unknown_because"] = verify.sel.none_is_unknown(stale) + self.assertEqual(verify.exit_code(verify._summary("/x", "HEAD", stale)), + verify.EXIT_INCOMPLETE) + def test_a_clean_none_keeps_its_wording(self): sel_ = {"base": "a", "head": "b", "warnings": [], "journeys": [], "unknown_because": []}