From cad45f9f83f0d1cd7a3cb6393197065c6cf083bc Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Tue, 18 Aug 2026 22:36:46 -0400 Subject: [PATCH 1/6] fix: flag a closure that resolves but never leaves its own file (#63) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A changed file's seeds can map to real symbols whose closure still stays confined to that one file — indistinguishable today from a confident, correct NONE. select() now checks this per changed file (distinct from recall_degraded, which covers unmapped seeds) and reports it as closure_confined plus a warning/NOTE. --- testgraph/db.py | 17 +++++++++++++ testgraph/select.py | 39 +++++++++++++++++++++++++++++ tests/test_core.py | 61 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+) diff --git a/testgraph/db.py b/testgraph/db.py index 1d7d358..0967fbd 100644 --- a/testgraph/db.py +++ b/testgraph/db.py @@ -84,6 +84,23 @@ def file_node_id(conn, file_path): return row[0] if row else None +def closure_files(conn, node_ids): + """Distinct `file_path` values for a set of node ids (file-kind nodes + counted by their own path). Used to detect a closure that never leaves + the file its seeds started in — an edge-resolution blind spot distinct + from an unmapped seed (issue #63): the seeds resolved fine, they just + have no outbound reach on record.""" + node_ids = list(node_ids) + if not node_ids: + return set() + placeholders = ",".join("?" for _ in node_ids) + rows = conn.execute( + f"SELECT DISTINCT file_path FROM nodes WHERE id IN ({placeholders})", + node_ids, + ) + return {r[0] for r in rows} + + def impacted_closure(conn, seed_ids): """Transitive reverse-reachability closure of `seed_ids`, as `{node_id: confidence}`. diff --git a/testgraph/select.py b/testgraph/select.py index 157bd95..4317d28 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -226,15 +226,19 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): # PRODUCT_EXT but not in this repo's graph. Per-file node sets, not one # running total, so one mapped file cannot mask an unmapped one. unmapped = [] + unmapped_files = set() seeds = set() + seeds_by_file = {} for f, rs in sorted(ranges.items()): in_file = set() for lo, hi in rs: in_file.update(dbmod.nodes_for_lines(conn, f, lo, hi)) if in_file: seeds.update(in_file) + seeds_by_file[f] = in_file else: unmapped.append(f"{f} (changed lines map to no indexed symbol)") + unmapped_files.add(f) # Whole-file changes (deletions, renames) have no line ranges to map: seed # every symbol the file contains. A file deleted in `head` is usually absent @@ -246,8 +250,10 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): nodes = dbmod.nodes_in_file(conn, path) if nodes: seeds.update(nodes) + seeds_by_file[path] = set(nodes) else: unmapped.append(f"{path} ({reason})") + unmapped_files.add(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 @@ -268,8 +274,26 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): drifted = integrity.content_drift(conn, repo, set(ranges) | set(whole_files)) for path in sorted(drifted): unmapped.append(f"{path} (bytes differ from the indexed copy — line spans are stale)") + unmapped_files.add(path) impacted = dbmod.impacted_closure(conn, seeds) + + # A closure that resolves fine but never leaves the file(s) its seeds + # started in is a second, silent way to be blind (issue #63) — distinct + # from `unmapped` above (no node at all). The seeds have symbols; those + # symbols just have no recorded outbound reach. Checked per file, not + # against the union of every seeded file in this diff, so a genuinely + # cross-file change cannot mask a same-diff file that stayed confined. + # Files already in `unmapped` are skipped: their seeds are untrusted, not + # evidence of confinement. + confined_files = [] + for f, file_seeds in sorted(seeds_by_file.items()): + if f in unmapped_files: + continue + file_impacted = dbmod.impacted_closure(conn, file_seeds) + reached_files = dbmod.closure_files(conn, file_impacted.keys()) + if reached_files and reached_files <= {f}: + confined_files.append(f) entry_map = reg.resolve_entries(conn, registry) touched = {} @@ -318,11 +342,21 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): journeys.sort(key=lambda j: (-j["rank"], reg.journey_sort_key(j["id"]))) + if confined_files: + warnings.append( + f"impact for {', '.join(confined_files)} did not leave the file it " + f"started in ({sum(len(seeds_by_file[f]) for f in confined_files)} " + f"node(s), all local) — either the module is genuinely leaf-only, or " + f"its callers are not linked in the index; a NONE here means UNKNOWN, " + f"not verified-safe" + ) + result.update( status="OK", changed_files=sorted(ranges), whole_file_changes=whole_files, recall_degraded=bool(unmapped), + closure_confined=confined_files, seed_symbols=len(seeds), impacted_symbols=len(impacted), journeys=journeys, @@ -354,6 +388,11 @@ def _render(result): lines.append(f" whole-file: {path} ({reason})") if result.get("recall_degraded"): lines.append(" RECALL DEGRADED — unbounded impact, all journeys listed") + for f in result.get("closure_confined", []): + lines.append( + f" NOTE: impact for {f} did not leave the file it started in — " + f"a NONE here means UNKNOWN, not verified-safe" + ) if not result["journeys"]: lines.append("journeys to test: NONE (no product-behavior change detected)") else: diff --git a/tests/test_core.py b/tests/test_core.py index bfda04e..1157b4b 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1434,6 +1434,67 @@ def test_one_mapped_file_does_not_mask_an_unmapped_one(self): self.assertGreater(res["seed_symbols"], 0) +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, + correct `NONE` — no signal at all. `function:leaf` (app/leaf.py) has zero + outbound edges in the fixture (see ClosureTests.test_leaf_stays_tight), + so editing it is exactly this blind spot.""" + + REG = {"J1": {"name": "one", "entries": [{"name": "handler_a", + "file": "app/svc.py"}]}} + + 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, "app/leaf.py": 5, "app/config.py": 5} + ) + + def _edit_leaf(self): + full = os.path.join(self.repo, "app", "leaf.py") + with open(full) as fh: + lines = fh.readlines() + lines[1] = "changed = 1\n" # inside function:leaf (1-5) + with open(full, "w") as fh: + fh.writelines(lines) + self.run("git", "add", "-A") + self.run("git", "commit", "-qm", "edit leaf") + + def test_confined_closure_is_flagged(self): + self._edit_leaf() + res = sel.select(self.repo, "HEAD~1", "HEAD", self.db, self.registry) + self.assertEqual(res["status"], "OK") + self.assertFalse(res["recall_degraded"], "not the no-node case") + self.assertEqual(res["closure_confined"], ["app/leaf.py"]) + self.assertTrue( + any("app/leaf.py" in w and "did not leave the file" in w + for w in res["warnings"]), + res["warnings"], + ) + # leaf has no journey entry, so the answer is still NONE -- the point + # is that NONE now carries a NOTE saying it may mean UNKNOWN. + self.assertEqual(res["journeys"], []) + + def test_change_reaching_outside_its_file_is_not_flagged(self): + # get_settings' closure crosses into app/svc.py via the imports edge + + # file expansion (ClosureTests.test_imports_and_file_expansion_reach_ + # handler) -- confirm a real cross-file reach produces no false + # positive. + full = os.path.join(self.repo, "app", "config.py") + with open(full) as fh: + lines = fh.readlines() + lines[1] = "changed = 1\n" # inside get_settings (1-5) + with open(full, "w") as fh: + fh.writelines(lines) + self.run("git", "add", "-A") + self.run("git", "commit", "-qm", "edit get_settings") + res = sel.select(self.repo, "HEAD~1", "HEAD", self.db, self.registry) + self.assertEqual(res["closure_confined"], []) + + class ChangedFileContentDriftTests(unittest.TestCase): """A changed file whose bytes no longer match the indexed copy is the quietest way to be unmappable. The other two resolve to no node and are From 3f3a72c357408fa53dc7cb099fd195f57d4b22ac Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Tue, 18 Aug 2026 22:44:13 -0400 Subject: [PATCH 2/6] fix: review findings on #63's closure-confined check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - closure_files() routes ids through a TEMP TABLE like impacted_closure does, instead of one bound placeholder per id — a wide fan-out could exceed SQLite's variable ceiling and raise. - Reuse the already-computed `impacted` closure when a diff seeds exactly one file, instead of re-running the same recursive traversal. - hook.py's ledger record and selectivity.py's per-commit sweep now also track closure_confined, so it isn't invisible to the project's own measurement tooling the way recall_degraded already is. --- harness/selectivity.py | 10 +++++++--- testgraph/db.py | 17 +++++++++++++---- testgraph/hook.py | 1 + testgraph/select.py | 6 +++++- 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/harness/selectivity.py b/harness/selectivity.py index 8da07d7..f75b6fc 100644 --- a/harness/selectivity.py +++ b/harness/selectivity.py @@ -106,20 +106,22 @@ def main(): sha]).stdout.strip() rows.append((sha, desc[:50], len(selected), total, result["recall_degraded"], - len(result["unresolved_journeys"]), selected)) + len(result["unresolved_journeys"]), selected, + len(result.get("closure_confined", [])))) wtlib.remove(args.bare, wt) finally: shutil.rmtree(tmp, ignore_errors=True) print(f"\n=== testgraph selectivity — {os.path.basename(args.registry)} " f"({total} journeys), {len(shas)}-commit sweep, per-commit index ===") - scored = [r for r in rows if len(r) == 7] + scored = [r for r in rows if len(r) == 8] for r in rows: if len(r) == 3: print(f" {r[0][:10]} {r[1]}: {r[2]}") continue - sha, desc, n_sel, tot, degraded, unresolved, selected = r + sha, desc, n_sel, tot, degraded, unresolved, selected, confined = r flag = " RECALL_DEGRADED" if degraded else "" + flag += f" CLOSURE_CONFINED({confined})" if confined else "" unres = f" ({unresolved} journey(s) not yet in this commit's index)" if unresolved else "" print(f"\n {sha[:10]} {desc}") print(f" selected : {n_sel}/{tot} {selected}{flag}{unres}") @@ -130,6 +132,7 @@ def main(): return 1 counts = [r[2] for r in scored] degrades = sum(1 for r in scored if r[4]) + confines = sum(1 for r in scored if r[7]) mean_sel = sum(counts) / len(counts) avoided_pct = 100 * (1 - mean_sel / total) le2 = sum(1 for c in counts if c <= 2) @@ -143,6 +146,7 @@ def main(): print(f" all/most (>= {total - 1}) : {most} of {len(scored)}" f" (all {total}: {all_n})") print(f" recall_degraded fired : {degrades} of {len(scored)}") + print(f" closure_confined fired : {confines} of {len(scored)}") # Full histogram — nothing bucketed away, so a reader can re-derive any # threshold the two headline buckets above don't happen to answer. hist = {} diff --git a/testgraph/db.py b/testgraph/db.py index 0967fbd..96ee578 100644 --- a/testgraph/db.py +++ b/testgraph/db.py @@ -89,14 +89,23 @@ def closure_files(conn, node_ids): counted by their own path). Used to detect a closure that never leaves the file its seeds started in — an edge-resolution blind spot distinct from an unmapped seed (issue #63): the seeds resolved fine, they just - have no outbound reach on record.""" + have no outbound reach on record. + + Routed through a TEMP TABLE rather than one `IN (?,?,...)` placeholder + per id, same as `impacted_closure` — a widely-imported module's closure + can fan out past SQLite's bound-parameter ceiling (999 on some packaged + builds), which a raw IN-list would hit and raise on. + """ node_ids = list(node_ids) if not node_ids: return set() - placeholders = ",".join("?" for _ in node_ids) + conn.execute("CREATE TEMP TABLE IF NOT EXISTS _closure_ids(id TEXT PRIMARY KEY)") + conn.execute("DELETE FROM _closure_ids") + conn.executemany( + "INSERT OR IGNORE INTO _closure_ids(id) VALUES (?)", [(i,) for i in node_ids] + ) rows = conn.execute( - f"SELECT DISTINCT file_path FROM nodes WHERE id IN ({placeholders})", - node_ids, + "SELECT DISTINCT file_path FROM nodes WHERE id IN (SELECT id FROM _closure_ids)" ) return {r[0] for r in rows} diff --git a/testgraph/hook.py b/testgraph/hook.py index 2899196..0c4a7a4 100644 --- a/testgraph/hook.py +++ b/testgraph/hook.py @@ -141,6 +141,7 @@ def run(repo, base, head, registry_path=None, caller="pre-push"): record["n_journeys"] = len(result.get("journeys", [])) record["journey_ids"] = [j["id"] for j in result.get("journeys", [])] record["recall_degraded"] = bool(result.get("recall_degraded")) + record["closure_confined"] = result.get("closure_confined", []) record["duration_ms"] = int((time.time() - started) * 1000) more = ( f"python3 -m testgraph.select --repo {repo} --base {base} --head {head}" diff --git a/testgraph/select.py b/testgraph/select.py index 4317d28..0dab95c 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -287,10 +287,14 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): # Files already in `unmapped` are skipped: their seeds are untrusted, not # evidence of confinement. confined_files = [] + single_file = len(seeds_by_file) == 1 for f, file_seeds in sorted(seeds_by_file.items()): if f in unmapped_files: continue - file_impacted = dbmod.impacted_closure(conn, file_seeds) + # When exactly one file contributed seeds, its seeds ARE `seeds` and + # its closure IS `impacted` — reuse it instead of re-running the same + # recursive traversal a second time. + file_impacted = impacted if single_file else dbmod.impacted_closure(conn, file_seeds) reached_files = dbmod.closure_files(conn, file_impacted.keys()) if reached_files and reached_files <= {f}: confined_files.append(f) From 160a250c8f9873d9446ac9e9493658da34261cf6 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Tue, 18 Aug 2026 22:52:46 -0400 Subject: [PATCH 3/6] fix: second round of review findings on #63 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reword the confinement NOTE/warning: it no longer implies the whole answer is NONE when other changed files did select journeys — it's scoped to that one file's own contribution. - Verify the single-file reuse shortcut by value (file_seeds == seeds) instead of inferring it from len(seeds_by_file) == 1, so a future seed source can't silently desync the two. - Extract the TEMP TABLE load duplicated between impacted_closure and closure_files into one _load_id_temp_table helper. - Document the accepted per-file traversal cost on wide diffs instead of silently leaving it unexplained. --- testgraph/db.py | 32 +++++++++++++++++--------------- testgraph/select.py | 29 +++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 23 deletions(-) diff --git a/testgraph/db.py b/testgraph/db.py index 96ee578..5d40c3d 100644 --- a/testgraph/db.py +++ b/testgraph/db.py @@ -84,26 +84,32 @@ def file_node_id(conn, file_path): return row[0] if row else None +def _load_id_temp_table(conn, table_name, ids): + """(Re)fill a single-column TEMP TABLE with `ids`, for callers that need + to join or filter against a large id set. A raw `IN (?,?,...)` with one + bound placeholder per id hits SQLite's bound-parameter ceiling (999 on + some packaged builds) once a closure fans out wide enough; a temp table + has no such limit. Shared by `impacted_closure` and `closure_files` so + that fix applies once, not per copy. + """ + conn.execute(f"CREATE TEMP TABLE IF NOT EXISTS {table_name}(id TEXT PRIMARY KEY)") + conn.execute(f"DELETE FROM {table_name}") + conn.executemany( + f"INSERT OR IGNORE INTO {table_name}(id) VALUES (?)", [(i,) for i in ids] + ) + + def closure_files(conn, node_ids): """Distinct `file_path` values for a set of node ids (file-kind nodes counted by their own path). Used to detect a closure that never leaves the file its seeds started in — an edge-resolution blind spot distinct from an unmapped seed (issue #63): the seeds resolved fine, they just have no outbound reach on record. - - Routed through a TEMP TABLE rather than one `IN (?,?,...)` placeholder - per id, same as `impacted_closure` — a widely-imported module's closure - can fan out past SQLite's bound-parameter ceiling (999 on some packaged - builds), which a raw IN-list would hit and raise on. """ node_ids = list(node_ids) if not node_ids: return set() - conn.execute("CREATE TEMP TABLE IF NOT EXISTS _closure_ids(id TEXT PRIMARY KEY)") - conn.execute("DELETE FROM _closure_ids") - conn.executemany( - "INSERT OR IGNORE INTO _closure_ids(id) VALUES (?)", [(i,) for i in node_ids] - ) + _load_id_temp_table(conn, "_closure_ids", node_ids) rows = conn.execute( "SELECT DISTINCT file_path FROM nodes WHERE id IN (SELECT id FROM _closure_ids)" ) @@ -130,11 +136,7 @@ def impacted_closure(conn, seed_ids): """ if not seed_ids: return {} - conn.execute("CREATE TEMP TABLE IF NOT EXISTS _seeds(id TEXT PRIMARY KEY)") - conn.execute("DELETE FROM _seeds") - conn.executemany( - "INSERT OR IGNORE INTO _seeds(id) VALUES (?)", [(s,) for s in seed_ids] - ) + _load_id_temp_table(conn, "_seeds", seed_ids) kinds = ",".join("'%s'" % k for k in REACH_KINDS) # constants, safe to inline edge_conf = ( f"MIN(COALESCE(json_extract(e.metadata, '$.confidence'), " diff --git a/testgraph/select.py b/testgraph/select.py index 0dab95c..2645688 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -286,15 +286,27 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): # cross-file change cannot mask a same-diff file that stayed confined. # Files already in `unmapped` are skipped: their seeds are untrusted, not # evidence of confinement. + # + # One recursive traversal per seeded file (beyond the single-file reuse + # below): a mechanical rename/refactor touching many files pays for many + # extra closures. Accepted for the same reason `journeys` already loops + # `caller_edge_count` per entry above — this selector is recall-first and + # already spends per-item DB round trips elsewhere; a diff wide enough to + # feel this is also wide enough to be a whole-file/`unmapped` case on the + # commonest paths. Revisit if this ever shows up in profiling. confined_files = [] - single_file = len(seeds_by_file) == 1 for f, file_seeds in sorted(seeds_by_file.items()): if f in unmapped_files: continue - # When exactly one file contributed seeds, its seeds ARE `seeds` and - # its closure IS `impacted` — reuse it instead of re-running the same - # recursive traversal a second time. - file_impacted = impacted if single_file else dbmod.impacted_closure(conn, file_seeds) + # `impacted` is already this file's closure whenever its seeds equal + # the full seed set — reuse it instead of re-running the same + # recursive traversal. Checked by value, not by `len(seeds_by_file) + # == 1`: inferring it from the file count silently breaks if a future + # seed source populates `seeds` without also updating + # `seeds_by_file`. + file_impacted = ( + impacted if file_seeds == seeds else dbmod.impacted_closure(conn, file_seeds) + ) reached_files = dbmod.closure_files(conn, file_impacted.keys()) if reached_files and reached_files <= {f}: confined_files.append(f) @@ -351,8 +363,9 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): f"impact for {', '.join(confined_files)} did not leave the file it " f"started in ({sum(len(seeds_by_file[f]) for f in confined_files)} " f"node(s), all local) — either the module is genuinely leaf-only, or " - f"its callers are not linked in the index; a NONE here means UNKNOWN, " - f"not verified-safe" + f"its callers are not linked in the index; that file's own " + f"contribution to this answer is UNKNOWN, not verified-safe, even " + f"though other changed files may still be selecting journeys above" ) result.update( @@ -395,7 +408,7 @@ def _render(result): for f in result.get("closure_confined", []): lines.append( f" NOTE: impact for {f} did not leave the file it started in — " - f"a NONE here means UNKNOWN, not verified-safe" + f"that file's own contribution is UNKNOWN, not verified-safe" ) if not result["journeys"]: lines.append("journeys to test: NONE (no product-behavior change detected)") From 0e0630e5b4c92798373177d3058cb501a20ea71b Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Tue, 18 Aug 2026 23:03:08 -0400 Subject: [PATCH 4/6] fix: third round of review findings on #63 - hook.py's pre-push render() gave closure_confined its own uncapped line, like recall_degraded already has -- riding the MAX_WARNINGS=3 channel could push the one signal this issue exists to surface off the actual push output. - A rename that also carries edited hunks landed the new path in both ranges and whole_files; the whole-file loop was unconditionally overwriting seeds_by_file with the broader whole-file node set, which could mask a real confinement in the lines that actually changed behind an untouched sibling symbol that reaches elsewhere. Keep the precise range-based entry when one already exists. - The confinement warning is now one line per file instead of one combined message with a summed node count, matching what _render already does per file. - Regression tests for both: a rename+edit fixture (new leaf_sibling node in the shared fixture) and hook.py's render() cap behavior. --- testgraph/hook.py | 11 +++++++++++ testgraph/select.py | 30 ++++++++++++++++++++-------- tests/test_core.py | 48 +++++++++++++++++++++++++++++++++++++++++++++ tests/test_hook.py | 21 ++++++++++++++++++++ 4 files changed, 102 insertions(+), 8 deletions(-) diff --git a/testgraph/hook.py b/testgraph/hook.py index 0c4a7a4..28162c2 100644 --- a/testgraph/hook.py +++ b/testgraph/hook.py @@ -78,6 +78,17 @@ def render(result, repo, more_cmd=None): if result.get("recall_degraded"): lines.append(" RECALL DEGRADED — unbounded impact, all journeys listed") + # Like recall_degraded above, this gets its own uncapped line rather than + # riding the warnings channel below: capped at MAX_WARNINGS, a push with + # an unapproved-registry warning plus entry drift already queued ahead of + # it would silently swallow the one signal issue #63 exists to surface. + confined = result.get("closure_confined", []) + if confined: + lines.append( + f" NOTE: impact for {', '.join(confined)} did not leave the file " + f"it started in — that file's own contribution is UNKNOWN, not " + f"verified-safe" + ) # 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 2645688..3f5199c 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -250,7 +250,17 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): nodes = dbmod.nodes_in_file(conn, path) if nodes: seeds.update(nodes) - seeds_by_file[path] = set(nodes) + # A rename that ALSO carries edited hunks lands in both `ranges` + # and `whole_files` for the new path. `seeds` above always gets + # the full file (recall-first: a rename changes the module path + # for every importer, so the whole file is in play regardless of + # which lines moved) — but for the confinement check specifically, + # letting the broader whole-file set clobber a precise range-based + # entry can mask a real issue-#63 confinement in the lines that + # actually changed behind unrelated untouched symbols that happen + # to reach elsewhere. Keep the narrower, already-set entry. + if path not in seeds_by_file: + seeds_by_file[path] = set(nodes) else: unmapped.append(f"{path} ({reason})") unmapped_files.add(path) @@ -358,14 +368,18 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): journeys.sort(key=lambda j: (-j["rank"], reg.journey_sort_key(j["id"]))) - if confined_files: + # One warning per file, not one combined message: a summed node count + # across several confined files tells a reader nothing about which file + # contributed how much, and the per-file NOTE lines in `_render` below + # already report them separately -- the warnings channel should agree. + for f in confined_files: warnings.append( - f"impact for {', '.join(confined_files)} did not leave the file it " - f"started in ({sum(len(seeds_by_file[f]) for f in confined_files)} " - f"node(s), all local) — either the module is genuinely leaf-only, or " - f"its callers are not linked in the index; that file's own " - f"contribution to this answer is UNKNOWN, not verified-safe, even " - f"though other changed files may still be selecting journeys above" + f"impact for {f} did not leave the file it started in " + f"({len(seeds_by_file[f])} node(s), all local) — either the module " + f"is genuinely leaf-only, or its callers are not linked in the " + f"index; that file's own contribution to this answer is UNKNOWN, " + f"not verified-safe, even though other changed files may still be " + f"selecting journeys above" ) result.update( diff --git a/tests/test_core.py b/tests/test_core.py index 1157b4b..b776373 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -48,6 +48,13 @@ def build_fixture(): ("function:handler_a", "function", "handler_a", "handler_a", "app/svc.py", 10, 20), ("function:leaf", "function", "leaf", "leaf", "app/leaf.py", 1, 5), + # a second, untouched symbol in the same file that IS reached from + # elsewhere -- lets a test tell a whole-file seed set (which would + # pull this in) apart from a range-precise one (which would not). + ("function:leaf_sibling", "function", "leaf_sibling", "leaf_sibling", + "app/leaf.py", 7, 10), + ("function:external_caller", "function", "external_caller", + "external_caller", "app/othercaller.py", 1, 5), # confidence fixture: base <- {mid_a weak, mid_b strong} <- top, # plus a synthesized (heuristic) caller. ("function:base", "function", "base", "base", "app/conf.py", 1, 5), @@ -69,6 +76,8 @@ def build_fixture(): ("function:top", "function:mid_b", "calls", '{"confidence":0.9}', None), # synthesized edge: capped regardless of the confidence it claims ("function:hcaller", "function:base", "calls", '{"confidence":0.9}', "heuristic"), + # leaf_sibling IS reached from another file -- leaf itself is not. + ("function:external_caller", "function:leaf_sibling", "calls", None, None), ] conn.executemany( "INSERT INTO edges(source,target,kind,metadata,provenance) VALUES (?,?,?,?,?)", @@ -1495,6 +1504,45 @@ def test_change_reaching_outside_its_file_is_not_flagged(self): self.assertEqual(res["closure_confined"], []) +class RenameWithEditConfinementTests(unittest.TestCase): + """A rename that also carries edited hunks lands the new path in BOTH + `ranges` (precise, from the hunk) and `whole_files` (the full file, from + the rename). `function:leaf_sibling` (app/leaf.py:7-10) is untouched by + the edit below but IS reached from app/othercaller.py -- so the + whole-file seed set escapes app/leaf.py while the precise, edited-lines + seed set (just `function:leaf`, 1-5) does not. Confinement must be + judged on the precise set, or a real issue-#63 blind spot in the lines + that actually changed gets masked by an unrelated symbol in the same + file.""" + + REG = {"J1": {"name": "one", "entries": [{"name": "handler_a", + "file": "app/svc.py"}]}} + + 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/oldname.py": 10}) + + def test_confinement_uses_the_precise_range_not_the_whole_file(self): + self.run("git", "mv", "app/oldname.py", "app/leaf.py") + with open(os.path.join(self.repo, "app", "leaf.py")) as fh: + lines = fh.readlines() + lines[1] = "changed = 1\n" # inside function:leaf (1-5), not the sibling (7-10) + with open(os.path.join(self.repo, "app", "leaf.py"), "w") as fh: + fh.writelines(lines) + self.run("git", "add", "-A") + self.run("git", "commit", "-qm", "rename + edit leaf") + res = sel.select(self.repo, "HEAD~1", "HEAD", self.db, self.registry) + self.assertEqual(res["status"], "OK") + self.assertEqual( + res["closure_confined"], ["app/leaf.py"], + "whole-file seeding pulled in leaf_sibling and masked the " + "confined edit — confinement should track the edited lines", + ) + + class ChangedFileContentDriftTests(unittest.TestCase): """A changed file whose bytes no longer match the indexed copy is the quietest way to be unmappable. The other two resolve to no node and are diff --git a/tests/test_hook.py b/tests/test_hook.py index 37efebf..b5d451f 100644 --- a/tests/test_hook.py +++ b/tests/test_hook.py @@ -136,6 +136,27 @@ def test_keeps_the_signals_that_mean_the_answer_may_be_understated(self): self.assertIn("RECALL DEGRADED", text) self.assertIn("registry not approved", text) + def test_closure_confined_gets_its_own_line_like_recall_degraded(self): + text = hook.render(_result(1, closure_confined=["app/leaf.py"]), SIGNEDINTAKE) + self.assertIn("app/leaf.py", text) + self.assertIn("did not leave the file", text) + + def test_closure_confined_survives_the_warning_cap(self): + # issue #63's whole point is a signal that must not go silent. Riding + # the capped `warnings` channel like other detail does would let + # enough queued-ahead warnings push it past MAX_WARNINGS and off the + # rendered push output entirely. + text = hook.render( + _result( + 1, + closure_confined=["app/leaf.py"], + warnings=[f"w{i}" for i in range(hook.MAX_WARNINGS + 5)], + ), + SIGNEDINTAKE, + ) + self.assertIn("app/leaf.py", text) + self.assertIn("did not leave the file", text) + def test_caps_warnings_without_hiding_the_count(self): text = hook.render( _result(1, warnings=[f"w{i}" for i in range(9)]), SIGNEDINTAKE From 850fae90b289031b74e05b503e28ba8b1ffb1ce6 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Tue, 18 Aug 2026 23:15:13 -0400 Subject: [PATCH 5/6] =?UTF-8?q?fix:=20fourth=20round=20of=20review=20findi?= =?UTF-8?q?ngs=20on=20#63=20=E2=80=94=20the=20false-positive=20was=20real?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The confinement check flagged a file even when its own seed IS a registered journey entry with no callers on record -- a correctly confident selection, not an unknown. Reproduced against this repo's own index: 7 of 11 "confined" files had already selected a journey at confidence 1.0. Now skipped when the file's closure intersects entry_map. - Dropped the confinement text from `warnings` entirely -- it was printed twice on a push (once via hook.py's dedicated line, once via the capped warnings loop). `closure_confined` on the result is now the sole source; both select._render and hook.render read from it independently. - Removed the dead `reached_files and` guard (closure_files can never return empty here) and consolidated the three unmapped/unmapped_files append sites behind one _mark_unmapped helper so the pairing can't drift. - Regression tests for the false-positive and the no-longer-possible double-print. --- testgraph/select.py | 72 +++++++++++++++++++++++++++------------------ tests/test_core.py | 32 +++++++++++++++++--- tests/test_hook.py | 15 ++++++++++ 3 files changed, 87 insertions(+), 32 deletions(-) diff --git a/testgraph/select.py b/testgraph/select.py index 3f5199c..5a8c714 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -227,6 +227,15 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): # running total, so one mapped file cannot mask an unmapped one. unmapped = [] unmapped_files = set() + + def _mark_unmapped(path, detail): + # `unmapped_files` gates the confinement check below (an untrusted + # seed must not read as evidence of confinement) — one helper instead + # of three independent append/add pairs means that gate can't drift + # out of sync with `unmapped` itself. + unmapped.append(f"{path} ({detail})") + unmapped_files.add(path) + seeds = set() seeds_by_file = {} for f, rs in sorted(ranges.items()): @@ -237,8 +246,7 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): seeds.update(in_file) seeds_by_file[f] = in_file else: - unmapped.append(f"{f} (changed lines map to no indexed symbol)") - unmapped_files.add(f) + _mark_unmapped(f, "changed lines map to no indexed symbol") # Whole-file changes (deletions, renames) have no line ranges to map: seed # every symbol the file contains. A file deleted in `head` is usually absent @@ -262,8 +270,7 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): if path not in seeds_by_file: seeds_by_file[path] = set(nodes) else: - unmapped.append(f"{path} ({reason})") - unmapped_files.add(path) + _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 @@ -283,10 +290,10 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): # exceptional path rather than every push. drifted = integrity.content_drift(conn, repo, set(ranges) | set(whole_files)) for path in sorted(drifted): - unmapped.append(f"{path} (bytes differ from the indexed copy — line spans are stale)") - unmapped_files.add(path) + _mark_unmapped(path, "bytes differ from the indexed copy — line spans are stale") impacted = dbmod.impacted_closure(conn, seeds) + entry_map = reg.resolve_entries(conn, registry) # A closure that resolves fine but never leaves the file(s) its seeds # started in is a second, silent way to be blind (issue #63) — distinct @@ -297,13 +304,23 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): # Files already in `unmapped` are skipped: their seeds are untrusted, not # evidence of confinement. # + # NOT flagged when the file's own closure already lands on a registered + # entry point (`entry_map`): a route handler with no callers on record is + # not a blind spot, it's a correctly-confident answer — the seed IS the + # thing the registry names, so there is nothing "unknown" left to say. + # Reproduced against this repo's own index before this guard existed: 7 + # of 11 "confined" files had already selected a journey at confidence + # 1.0, and the warning called that UNKNOWN anyway. + # # One recursive traversal per seeded file (beyond the single-file reuse - # below): a mechanical rename/refactor touching many files pays for many - # extra closures. Accepted for the same reason `journeys` already loops - # `caller_edge_count` per entry above — this selector is recall-first and - # already spends per-item DB round trips elsewhere; a diff wide enough to - # feel this is also wide enough to be a whole-file/`unmapped` case on the - # commonest paths. Revisit if this ever shows up in profiling. + # below), and `closure_files` scans every resulting node rather than + # short-circuiting on the first one outside `f`: a mechanical + # rename/refactor touching many files pays for it on every one. Accepted + # for the same reason `journeys` already loops `caller_edge_count` per + # entry above — this selector is recall-first and already spends + # per-item DB round trips elsewhere; a diff wide enough to feel this is + # also wide enough to be a whole-file/`unmapped` case on the commonest + # paths. Revisit if this ever shows up in profiling. confined_files = [] for f, file_seeds in sorted(seeds_by_file.items()): if f in unmapped_files: @@ -317,10 +334,15 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): file_impacted = ( impacted if file_seeds == seeds else dbmod.impacted_closure(conn, file_seeds) ) + if file_impacted.keys() & entry_map.keys(): + continue + # `file_impacted` always contains at least the seeds themselves + # (`impacted_closure` seeds every id at 1.0), and every seed in + # `file_seeds` came from a `nodes` row whose own `file_path == f` — + # so `closure_files` here can never come back empty. reached_files = dbmod.closure_files(conn, file_impacted.keys()) - if reached_files and reached_files <= {f}: + if reached_files <= {f}: confined_files.append(f) - entry_map = reg.resolve_entries(conn, registry) touched = {} for nid in impacted.keys() & set(entry_map): @@ -368,19 +390,11 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): journeys.sort(key=lambda j: (-j["rank"], reg.journey_sort_key(j["id"]))) - # One warning per file, not one combined message: a summed node count - # across several confined files tells a reader nothing about which file - # contributed how much, and the per-file NOTE lines in `_render` below - # already report them separately -- the warnings channel should agree. - for f in confined_files: - warnings.append( - f"impact for {f} did not leave the file it started in " - f"({len(seeds_by_file[f])} node(s), all local) — either the module " - f"is genuinely leaf-only, or its callers are not linked in the " - f"index; that file's own contribution to this answer is UNKNOWN, " - f"not verified-safe, even though other changed files may still be " - f"selecting journeys above" - ) + # Not appended to `warnings`: this is its own signal, deliberately kept + # off the capped, generic channel. `_render` below and `hook.render` each + # give it a dedicated, uncapped line (`closure_confined` on the result is + # the data both read from) — riding `warnings` would let it silently drop + # off a push whose other warnings already filled hook.py's MAX_WARNINGS. result.update( status="OK", @@ -422,7 +436,9 @@ def _render(result): for f in result.get("closure_confined", []): lines.append( f" NOTE: impact for {f} did not leave the file it started in — " - f"that file's own contribution is UNKNOWN, not verified-safe" + f"either the module is genuinely leaf-only, or its callers are " + f"not linked in the index; that file's own contribution is " + f"UNKNOWN, not verified-safe" ) if not result["journeys"]: lines.append("journeys to test: NONE (no product-behavior change detected)") diff --git a/tests/test_core.py b/tests/test_core.py index b776373..a33fcea 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1478,11 +1478,15 @@ def test_confined_closure_is_flagged(self): self.assertEqual(res["status"], "OK") self.assertFalse(res["recall_degraded"], "not the no-node case") self.assertEqual(res["closure_confined"], ["app/leaf.py"]) - self.assertTrue( - any("app/leaf.py" in w and "did not leave the file" in w - for w in res["warnings"]), - res["warnings"], + # The signal is NOT on the capped `warnings` channel (see hook.py's + # dedicated line) -- it's carried structurally on the result, and + # `_render` turns it into its own NOTE line. + self.assertNotIn( + "did not leave the file", " ".join(res["warnings"]), res["warnings"] ) + rendered = sel._render(res) + self.assertIn("app/leaf.py", rendered) + self.assertIn("did not leave the file", rendered) # leaf has no journey entry, so the answer is still NONE -- the point # is that NONE now carries a NOTE saying it may mean UNKNOWN. self.assertEqual(res["journeys"], []) @@ -1503,6 +1507,26 @@ def test_change_reaching_outside_its_file_is_not_flagged(self): res = sel.select(self.repo, "HEAD~1", "HEAD", self.db, self.registry) self.assertEqual(res["closure_confined"], []) + def test_an_edited_entry_point_with_no_callers_is_not_a_false_positive(self): + # handler_a is ITSELF the registered J1 entry and has no callers on + # record in this fixture, so its own closure never leaves app/svc.py + # -- but that is not unknown, it's a confidently-selected journey. + # Flagging it anyway was reproduced against this repo's own index + # (issue #63 PR review): 7 of 11 "confined" files had already + # selected a journey at confidence 1.0. + 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) + with open(full, "w") as fh: + fh.writelines(lines) + self.run("git", "add", "-A") + self.run("git", "commit", "-qm", "edit handler_a") + res = sel.select(self.repo, "HEAD~1", "HEAD", self.db, self.registry) + self.assertEqual(res["closure_confined"], []) + j1 = next(j for j in res["journeys"] if j["id"] == "J1") + self.assertEqual(j1["confidence"], 1.0) + class RenameWithEditConfinementTests(unittest.TestCase): """A rename that also carries edited hunks lands the new path in BOTH diff --git a/tests/test_hook.py b/tests/test_hook.py index b5d451f..884507b 100644 --- a/tests/test_hook.py +++ b/tests/test_hook.py @@ -141,6 +141,21 @@ def test_closure_confined_gets_its_own_line_like_recall_degraded(self): self.assertIn("app/leaf.py", text) self.assertIn("did not leave the file", text) + def test_closure_confined_is_not_also_printed_via_warnings(self): + # select() no longer puts this text on `warnings` at all (it's + # structural data on `closure_confined` only) -- confirm the render + # path doesn't print it twice even if a caller's `warnings` happens + # to mention the same file for an unrelated reason. + text = hook.render( + _result( + 1, + closure_confined=["app/leaf.py"], + warnings=["app/leaf.py: registry not approved"], + ), + SIGNEDINTAKE, + ) + self.assertEqual(text.count("app/leaf.py"), 2) # NOTE line + WARN line, not 3 + def test_closure_confined_survives_the_warning_cap(self): # issue #63's whole point is a signal that must not go silent. Riding # the capped `warnings` channel like other detail does would let From edffad1cc5881951634e1558c586bc634a5879b3 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Tue, 18 Aug 2026 23:23:14 -0400 Subject: [PATCH 6/6] fix: fifth round of review findings on #63 - Judge confinement per SEED, not per FILE: a file with both a registered entry (fine on its own) and an unrelated non-entry seed was having the entry silently clear the whole file, swallowing the NOTE for the seed that actually needed it. - closure_files() returns None, not a smaller set, when the closure reaches an id with no matching nodes row -- a dangling edge read identically to "resolves to no other file" and could manufacture a false confinement out of an inconsistent index. - hook.py's NOTE line is now capped (MAX_CONFINED=3) like journeys and warnings already are, so a wide rename touching many files can't print one unbroken multi-hundred-character line. - Regression tests for all three. --- testgraph/db.py | 17 ++++++++++++---- testgraph/hook.py | 21 +++++++++++++------- testgraph/select.py | 32 +++++++++++++++++++----------- tests/test_core.py | 47 +++++++++++++++++++++++++++++++++++++++++++++ tests/test_hook.py | 7 +++++++ 5 files changed, 102 insertions(+), 22 deletions(-) diff --git a/testgraph/db.py b/testgraph/db.py index 5d40c3d..9a81b8f 100644 --- a/testgraph/db.py +++ b/testgraph/db.py @@ -105,15 +105,24 @@ def closure_files(conn, node_ids): the file its seeds started in — an edge-resolution blind spot distinct from an unmapped seed (issue #63): the seeds resolved fine, they just have no outbound reach on record. + + Returns `None`, not a smaller set, if any id has no matching `nodes` row. + `edges` can name an id `nodes` has no row for (a dangling edge — the same + kind of drift `integrity.content_drift` exists elsewhere to catch); a + plain `WHERE id IN (...)` join silently drops such an id, which would + otherwise read identically to "this id resolves to no other file" and + manufacture a false confinement signal out of an untrustworthy index. """ node_ids = list(node_ids) if not node_ids: return set() _load_id_temp_table(conn, "_closure_ids", node_ids) - rows = conn.execute( - "SELECT DISTINCT file_path FROM nodes WHERE id IN (SELECT id FROM _closure_ids)" - ) - return {r[0] for r in rows} + rows = list(conn.execute( + "SELECT id, file_path FROM nodes WHERE id IN (SELECT id FROM _closure_ids)" + )) + if len({r[0] for r in rows}) != len(set(node_ids)): + return None + return {r[1] for r in rows} def impacted_closure(conn, seed_ids): diff --git a/testgraph/hook.py b/testgraph/hook.py index 28162c2..35b34e5 100644 --- a/testgraph/hook.py +++ b/testgraph/hook.py @@ -26,6 +26,7 @@ MAX_JOURNEYS = 8 MAX_WARNINGS = 3 +MAX_CONFINED = 3 # Kept as an alias: `ledger` now owns where state lives, but this name is the # one the tests and the docs already point at. @@ -78,16 +79,22 @@ def render(result, repo, more_cmd=None): if result.get("recall_degraded"): lines.append(" RECALL DEGRADED — unbounded impact, all journeys listed") - # Like recall_degraded above, this gets its own uncapped line rather than - # riding the warnings channel below: capped at MAX_WARNINGS, a push with - # an unapproved-registry warning plus entry drift already queued ahead of - # it would silently swallow the one signal issue #63 exists to surface. + # Like recall_degraded above, this gets its own line rather than riding + # the warnings channel below: capped at MAX_WARNINGS, a push with an + # unapproved-registry warning plus entry drift already queued ahead of it + # would silently swallow the one signal issue #63 exists to surface. But + # NEVER just swallowed, not the same as UNCAPPED IN LENGTH: a wide + # rename/refactor confining many files would otherwise print one + # unbroken multi-hundred-character line, the exact noise rule 2 in this + # module's docstring exists to prevent. confined = result.get("closure_confined", []) if confined: + shown = ", ".join(confined[:MAX_CONFINED]) + if len(confined) > MAX_CONFINED: + shown += f", … {len(confined) - MAX_CONFINED} more" lines.append( - f" NOTE: impact for {', '.join(confined)} did not leave the file " - f"it started in — that file's own contribution is UNKNOWN, not " - f"verified-safe" + f" NOTE: impact for {shown} did not leave the file it started " + f"in — that file's own contribution is UNKNOWN, not verified-safe" ) # Warnings are the channel that carries an unapproved registry and entry # drift — both mean "this answer may be understated", so they are worth the diff --git a/testgraph/select.py b/testgraph/select.py index 5a8c714..7918069 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -325,23 +325,33 @@ def _mark_unmapped(path, detail): for f, file_seeds in sorted(seeds_by_file.items()): if f in unmapped_files: continue - # `impacted` is already this file's closure whenever its seeds equal + # Judged per SEED, not per file: a file can carry both a registered + # entry (correctly confident on its own) and an unrelated edited + # symbol that is the actual blind spot. Clearing the whole file + # because ANY of its seeds happens to be an entry point silently + # swallowed the NOTE for the seed that needed it — reproduced with + # two unrelated edited symbols in one file, one an entry, one not. + uncovered = file_seeds - entry_map.keys() + if not uncovered: + continue + # `impacted` is already this closure whenever the uncovered seeds are # the full seed set — reuse it instead of re-running the same - # recursive traversal. Checked by value, not by `len(seeds_by_file) - # == 1`: inferring it from the file count silently breaks if a future - # seed source populates `seeds` without also updating - # `seeds_by_file`. + # recursive traversal. Checked by value, not by file/seed counts: + # inferring it silently breaks if a future seed source desyncs from + # `seeds`. file_impacted = ( - impacted if file_seeds == seeds else dbmod.impacted_closure(conn, file_seeds) + impacted if uncovered == seeds else dbmod.impacted_closure(conn, uncovered) ) - if file_impacted.keys() & entry_map.keys(): - continue # `file_impacted` always contains at least the seeds themselves # (`impacted_closure` seeds every id at 1.0), and every seed in - # `file_seeds` came from a `nodes` row whose own `file_path == f` — - # so `closure_files` here can never come back empty. + # `uncovered` came from a `nodes` row whose own `file_path == f` — so + # `closure_files` here can never come back empty, UNLESS the closure + # reached an id with no matching `nodes` row (a dangling edge / stale + # index), which `closure_files` reports as `None` rather than + # silently reading as "resolves to no other file". An index + # inconsistent enough to produce that is not evidence of anything. reached_files = dbmod.closure_files(conn, file_impacted.keys()) - if reached_files <= {f}: + if reached_files is not None and reached_files <= {f}: confined_files.append(f) touched = {} diff --git a/tests/test_core.py b/tests/test_core.py index a33fcea..a26962b 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -47,6 +47,10 @@ def build_fixture(): ("file:app/svc.py", "file", "svc.py", "svc.py", "app/svc.py", 1, 999), ("function:handler_a", "function", "handler_a", "handler_a", "app/svc.py", 10, 20), + # unrelated to handler_a, no edges, not a registered entry -- the + # actual issue-#63 blind spot when both are edited in one diff. + ("function:other_in_svc", "function", "other_in_svc", "other_in_svc", + "app/svc.py", 1, 3), ("function:leaf", "function", "leaf", "leaf", "app/leaf.py", 1, 5), # a second, untouched symbol in the same file that IS reached from # elsewhere -- lets a test tell a whole-file seed set (which would @@ -91,6 +95,28 @@ def build_fixture(): return conn +class ClosureFilesTests(unittest.TestCase): + """Issue #63 PR review: `closure_files` must not silently treat a + dangling edge (an id `edges` names that `nodes` has no row for -- the + same kind of drift `integrity.content_drift` exists elsewhere to catch) + as "resolves to no other file". That reads identically to a legitimate + confinement and would manufacture a false signal out of an untrustworthy + index.""" + + def setUp(self): + self.conn = build_fixture() + + def test_all_ids_resolved_returns_the_file_set(self): + self.assertEqual( + dbmod.closure_files(self.conn, {"function:leaf"}), {"app/leaf.py"} + ) + + def test_a_dangling_id_returns_none_not_a_partial_set(self): + self.assertIsNone( + dbmod.closure_files(self.conn, {"function:leaf", "function:ghost"}) + ) + + class ClosureTests(unittest.TestCase): def setUp(self): self.conn = build_fixture() @@ -1527,6 +1553,27 @@ def test_an_edited_entry_point_with_no_callers_is_not_a_false_positive(self): j1 = next(j for j in res["journeys"] if j["id"] == "J1") self.assertEqual(j1["confidence"], 1.0) + def test_an_entry_seed_does_not_mask_an_unrelated_confined_seed(self): + # Both handler_a (the entry -- fine on its own) and other_in_svc (no + # edges, no entry, the actual blind spot) are edited in ONE diff to + # app/svc.py. Judging confinement per FILE instead of per SEED let + # handler_a's entry-hit clear the whole file, silently swallowing the + # NOTE for other_in_svc -- reproduced directly against this fixture + # before the per-seed fix. + full = os.path.join(self.repo, "app", "svc.py") + with open(full) as fh: + lines = fh.readlines() + lines[0] = "changed = 1\n" # inside other_in_svc (1-3) + lines[11] = "changed = 1\n" # inside handler_a (10-20) + with open(full, "w") as fh: + fh.writelines(lines) + self.run("git", "add", "-A") + self.run("git", "commit", "-qm", "edit both svc.py symbols") + res = sel.select(self.repo, "HEAD~1", "HEAD", self.db, self.registry) + self.assertEqual(res["closure_confined"], ["app/svc.py"]) + j1 = next(j for j in res["journeys"] if j["id"] == "J1") + self.assertEqual(j1["confidence"], 1.0) + class RenameWithEditConfinementTests(unittest.TestCase): """A rename that also carries edited hunks lands the new path in BOTH diff --git a/tests/test_hook.py b/tests/test_hook.py index 884507b..32bd0d8 100644 --- a/tests/test_hook.py +++ b/tests/test_hook.py @@ -141,6 +141,13 @@ def test_closure_confined_gets_its_own_line_like_recall_degraded(self): self.assertIn("app/leaf.py", text) self.assertIn("did not leave the file", text) + def test_closure_confined_line_is_capped_like_journeys_and_warnings(self): + confined = [f"app/f{i}.py" for i in range(40)] + text = hook.render(_result(1, closure_confined=confined), SIGNEDINTAKE) + note = next(ln for ln in text.splitlines() if "did not leave the file" in ln) + self.assertLess(len(note), 300, note) + self.assertIn(f"… {40 - hook.MAX_CONFINED} more", note) + def test_closure_confined_is_not_also_printed_via_warnings(self): # select() no longer puts this text on `warnings` at all (it's # structural data on `closure_confined` only) -- confirm the render