From e9314620440fb36308bfd55c868aebcb10a5b99a Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Fri, 30 Oct 2026 10:52:47 -0400 Subject: [PATCH 1/2] fix: select diffs from the merge-base, not the base tip (#112) `base..head` compared two trees, so on a branch behind main the changes that landed on main since the fork were seeded as the branch's own: a docs-only branch listed every journey because main had added a migration (and since #117 any unmodeled file on main does that). select now diffs merge-base(base, head)..head. When base is an ancestor of head (HEAD~1, every push) that is base itself, so nothing changes. With no common ancestor it falls back to the tree diff and warns. The result carries `diff_base` when it differs from base, and the text says so. Docs, the MCP description and the verify skill follow. --- TECHNICAL.md | 2 +- skills/testgraph-verify/SKILL.md | 9 ++++-- testgraph/mcp.py | 4 ++- testgraph/select.py | 41 +++++++++++++++++++++++- tests/test_core.py | 54 ++++++++++++++++++++++++++++++++ 5 files changed, 104 insertions(+), 6 deletions(-) diff --git a/TECHNICAL.md b/TECHNICAL.md index e2308ce..3e4fefc 100644 --- a/TECHNICAL.md +++ b/TECHNICAL.md @@ -16,7 +16,7 @@ file-level cross-file bucket, not the transitive symbol closure this needs. Data flow for `testgraph.select`: ``` -git diff --unified=0 base..head +git diff --unified=0 merge-base(base, head)..head [select.diff_base, #112] -> changed (file, line-range) pairs [select.changed_ranges] -> seed nodes: nodes whose [start,end] overlaps a changed range [db.nodes_for_lines] -> impacted set: reverse-transitive closure over the graph [db.impacted_closure] diff --git a/skills/testgraph-verify/SKILL.md b/skills/testgraph-verify/SKILL.md index 9793ee8..4d197db 100644 --- a/skills/testgraph-verify/SKILL.md +++ b/skills/testgraph-verify/SKILL.md @@ -74,10 +74,13 @@ commit or stash first, then: On a feature branch: ```bash -python3 -m testgraph.select --repo \ - --base "$(git -C merge-base main HEAD)" --head HEAD +python3 -m testgraph.select --repo --base main --head HEAD ``` +`select` diffs from the merge-base itself (issue #112), so `--base main` means +"what this branch changed" even when main has moved on. The output names the +merge-base when it differs from `--base`. + Working directly on the default branch, `merge-base main HEAD` **is** `HEAD` and the diff is empty — use the commit count instead: @@ -88,7 +91,7 @@ python3 -m testgraph.select --repo --base HEAD~ --head HEAD **Always confirm the range is non-empty before trusting the answer:** ```bash -git -C diff --name-only ..HEAD # must list your files +git -C diff --name-only ...HEAD # must list your files (three dots: from the merge-base) ``` Do **not** pass `--base HEAD --head ` while checked out on that diff --git a/testgraph/mcp.py b/testgraph/mcp.py index 0ea8c1d..b031be9 100644 --- a/testgraph/mcp.py +++ b/testgraph/mcp.py @@ -75,7 +75,9 @@ "and degrades toward listing everything when the index cannot be " "trusted. Use before committing or reporting work complete. " "`base` defaults to HEAD~1 (the last commit); pass the base branch " - "(e.g. origin/main) to scope it to a whole branch instead." + "(e.g. origin/main) to scope it to a whole branch instead; the diff " + "is taken from the merge-base, so changes that landed on the base " + "since the branch forked are not counted." ), "inputSchema": { "type": "object", diff --git a/testgraph/select.py b/testgraph/select.py index f930248..89b7852 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -310,6 +310,30 @@ def changed_ranges(repo, base, head, paths=None): return _parse_unified_diff(diff, paths) +def diff_base(repo, base, head): + """The revision to diff `head` against: `merge-base(base, head)`, or `None` + when the two share no history. + + `base..head` compared two trees, so on a branch behind `base` the changes + that landed on `base` since the fork were read, reversed, as this branch's + own (issue #112): a docs-only branch listed every journey because main had + added a migration. The merge-base is what `git diff base...head` means. + When `base` is an ancestor of `head` (HEAD~1, every push) it IS `base`, so + nothing changes there.""" + proc = subprocess.run( + ["git", "-C", repo, "merge-base", "--end-of-options", base, head], + capture_output=True, text=True, + ) + return proc.stdout.strip() if proc.returncode == 0 and proc.stdout.strip() else None + + +def _same_rev(repo, sha, rev): + proc = subprocess.run( + ["git", "-C", repo, "rev-parse", "--verify", "-q", "--end-of-options", + f"{rev}^{{commit}}"], capture_output=True, text=True) + return proc.stdout.strip() == sha + + def select(repo, base, head, db_path, registry_path, strict_registry=True): # A revision starting with '-' is an option, not a revision (audit M2): # `--output=` made `git diff` truncate an arbitrary file and return an @@ -397,7 +421,17 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): return result diff_paths = set() - ranges, whole_files = changed_ranges(repo, base, head, diff_paths) + merge_base = diff_base(repo, base, head) + if merge_base is None: + # Unrelated histories or a shallow clone: no fork point to diff from. + # The tree diff over-selects, which is the safe direction. An + # unknown revision still fails loudly in `changed_ranges` below. + warnings.append( + f"{base} and {head} have no common ancestor (unrelated histories " + f"or a shallow clone) — compared the two trees directly, which " + f"may include changes that are not {head}'s" + ) + ranges, whole_files = changed_ranges(repo, merge_base or 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 @@ -674,6 +708,8 @@ def _mark_unmapped(path, detail): # 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. + if merge_base is not None and not _same_rev(repo, merge_base, base): + result["diff_base"] = merge_base result.update( status="OK", changed_files=sorted(ranges), @@ -770,6 +806,9 @@ def _render(result): f"({len(result.get('exempt_files', {}))} exempt, " f"{len(result.get('unmodeled_files', []))} unmodeled)" ) + if result.get("diff_base"): + lines.append(f" diffed from merge-base {result['diff_base'][:12]} " + f"({result['base']} has moved on since {result['head']} forked)") lines.append( f"{changed} | " f"seeds: {result['seed_symbols']} | impacted symbols: {result['impacted_symbols']}" diff --git a/tests/test_core.py b/tests/test_core.py index d61ea81..22b413a 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2553,6 +2553,60 @@ def test_an_invalid_trigger_blocks_the_map(self): self.assertEqual(text, "") +class MergeBaseDiffTests(unittest.TestCase): + """Issue #112: `base..head` compared the two trees, so on a branch behind + main, main's own changes were seeded as the branch's — a docs-only branch + listed every journey because main had added a migration.""" + + 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, ZeroSeedDegradesTests.REG) + self.repo, self.run = _git_repo(self.tmp, {"app/svc.py": 22}) + self.run("git", "branch", "-M", "main") + + def _write(self, rel, text="x\n"): + 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", rel) + + def test_a_branch_behind_main_sees_only_its_own_changes(self): + self.run("git", "switch", "-qc", "feature") + self._write("README.md") + self.run("git", "switch", "-q", "main") + self._write("db/001.sql") + res = sel.select(self.repo, "main", "feature", self.db, self.registry) + self.assertEqual(res["diff_files"], ["README.md"]) + self.assertFalse(res["recall_degraded"], res["warnings"]) + self.assertEqual(res["journeys"], []) + base = subprocess.run(["git", "-C", self.repo, "merge-base", "main", "feature"], + capture_output=True, text=True).stdout.strip() + self.assertEqual(res["diff_base"], base) + self.assertIn(f"diffed from merge-base {base[:12]}", sel._render(res)) + + def test_an_ancestor_base_is_unchanged(self): + self._write("db/001.sql") + res = sel.select(self.repo, "HEAD~1", "HEAD", self.db, self.registry) + self.assertEqual(res["diff_files"], ["db/001.sql"]) + self.assertNotIn("diff_base", res) + + def test_unrelated_histories_fall_back_to_the_tree_diff_and_say_so(self): + self.run("git", "switch", "-q", "--orphan", "other") + self._write("db/002.sql") + res = sel.select(self.repo, "main", "other", self.db, self.registry) + self.assertIn("db/002.sql", res["diff_files"]) + self.assertTrue(any("no common ancestor" in w for w in res["warnings"]), + res["warnings"]) + + def test_an_unknown_revision_still_fails_loudly(self): + with self.assertRaises(subprocess.CalledProcessError): + sel.select(self.repo, "no-such-rev", "HEAD", self.db, self.registry) + + class ClosureConfinedTests(unittest.TestCase): """Issue #63: seeds that resolve fine but whose closure never leaves the file they started in used to be indistinguishable from a confident, From e8337fb0e02501a49b4ad6bea9a136d55bf8debf Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Fri, 30 Oct 2026 11:00:55 -0400 Subject: [PATCH 2/2] fix: the push hook keeps the exact range; head behind base is not an empty diff (#112 review) - The pre-push hook moves the remote from its old tip to the new one; after a rebase or rewind the merge-base hid what the push removes (a dropped migration read as a confident NONE). hook.run now calls select(exact_range=True), and its `full:` command carries --exact-range (new select flag). - head an ancestor of base made the merge-base head itself, an empty diff by construction: fall back to the tree diff with a warning. - Only `git merge-base` exit 1 means "no common ancestor"; anything else raises instead of being mislabelled as unrelated histories. - _same_rev replaced by ledger.resolve_commit. --- TECHNICAL.md | 23 ++++++++++++++++++- testgraph/hook.py | 9 ++++++-- testgraph/select.py | 52 +++++++++++++++++++++++++++++------------- tests/test_core.py | 55 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 19 deletions(-) diff --git a/TECHNICAL.md b/TECHNICAL.md index 3e4fefc..2a19e9f 100644 --- a/TECHNICAL.md +++ b/TECHNICAL.md @@ -16,7 +16,8 @@ file-level cross-file bucket, not the transitive symbol closure this needs. Data flow for `testgraph.select`: ``` -git diff --unified=0 merge-base(base, head)..head [select.diff_base, #112] +git diff --unified=0 merge-base(base, head)..head [select.diff_base, #112; + the pre-push hook passes exact_range: base..head as trees] -> changed (file, line-range) pairs [select.changed_ranges] -> seed nodes: nodes whose [start,end] overlaps a changed range [db.nodes_for_lines] -> impacted set: reverse-transitive closure over the graph [db.impacted_closure] @@ -651,6 +652,26 @@ 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. +## Which range a diff means (issue #112) + +`select --base main --head HEAD` used to compare the two trees, so on a branch +behind main the changes that landed on main since the fork were seeded as the +branch's own: a docs-only branch listed every journey because main had added a +migration. It now diffs from `merge-base(base, head)`, which is what "what did this +branch change" means. The result carries `diff_base` when that differs from `base`. + +A push means something else. It moves the remote from its old tip to the new one, +and after a rebase or a rewind the fork point hides what the push removes: a +dropped migration read as a confident NONE. So the pre-push hook calls +`select(..., exact_range=True)` and its `full:` command carries `--exact-range`. +Two degenerate cases fall back to the tree diff with a warning: +- **head behind base:** the merge-base is head itself, so the diff would be + empty by construction. +- **no common ancestor:** `git merge-base` exits 1, for unrelated histories or + a shallow clone. + +Any other `merge-base` failure raises. + ## File Triggers (issue #118) Some dependencies the graph cannot model are still known: a SQL migration under diff --git a/testgraph/hook.py b/testgraph/hook.py index bd665db..9b345e8 100644 --- a/testgraph/hook.py +++ b/testgraph/hook.py @@ -180,7 +180,11 @@ def run(repo, base, head, registry_path=None, caller="pre-push"): record, ) - result = sel.select(repo, base, head, db_path, registry_path) + # The exact range, not the merge-base: a push moves the remote from `base` + # (its old tip, or the merge-base pre-push already computed for a new + # branch) to `head`, and after a rebase or a rewind the fork point would + # hide what the push removes (#112 review). + result = sel.select(repo, base, head, db_path, registry_path, exact_range=True) record["status"] = result.get("status") record["n_journeys"] = len(result.get("journeys", [])) record["journey_ids"] = [j["id"] for j in result.get("journeys", [])] @@ -188,7 +192,8 @@ def run(repo, base, head, registry_path=None, caller="pre-push"): 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}" + f"python3 -m testgraph.select --repo {repo} --base {base} --head {head} " + f"--exact-range" ) return render(result, repo, more_cmd=more), record diff --git a/testgraph/select.py b/testgraph/select.py index 89b7852..3a7407c 100644 --- a/testgraph/select.py +++ b/testgraph/select.py @@ -18,6 +18,7 @@ from . import db as dbmod from . import integrity +from . import ledger from . import registry as reg HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") @@ -312,29 +313,33 @@ def changed_ranges(repo, base, head, paths=None): def diff_base(repo, base, head): """The revision to diff `head` against: `merge-base(base, head)`, or `None` - when the two share no history. + when the two share no history (git exits 1). Any other git failure (a bad + revision, a missing object) raises, rather than reading as "unrelated". `base..head` compared two trees, so on a branch behind `base` the changes that landed on `base` since the fork were read, reversed, as this branch's own (issue #112): a docs-only branch listed every journey because main had added a migration. The merge-base is what `git diff base...head` means. - When `base` is an ancestor of `head` (HEAD~1, every push) it IS `base`, so - nothing changes there.""" + When `base` is an ancestor of `head` (HEAD~1) it IS `base`, so nothing + changes there. + + Not for a push: a push moves the remote from its old tip to the new one, + and after a rebase or a rewind the fork point hides what the push removed. + The hook asks for the exact range (`select(..., exact_range=True)`).""" proc = subprocess.run( ["git", "-C", repo, "merge-base", "--end-of-options", base, head], capture_output=True, text=True, ) - return proc.stdout.strip() if proc.returncode == 0 and proc.stdout.strip() else None - - -def _same_rev(repo, sha, rev): - proc = subprocess.run( - ["git", "-C", repo, "rev-parse", "--verify", "-q", "--end-of-options", - f"{rev}^{{commit}}"], capture_output=True, text=True) - return proc.stdout.strip() == sha + if proc.returncode == 1: + return None + if proc.returncode != 0: + raise subprocess.CalledProcessError(proc.returncode, proc.args, + proc.stdout, proc.stderr) + return proc.stdout.strip() or None -def select(repo, base, head, db_path, registry_path, strict_registry=True): +def select(repo, base, head, db_path, registry_path, strict_registry=True, + exact_range=False): # A revision starting with '-' is an option, not a revision (audit M2): # `--output=` made `git diff` truncate an arbitrary file and return an # empty diff -- a clean NONE. Reachable from the MCP tool, where the model @@ -421,8 +426,19 @@ def select(repo, base, head, db_path, registry_path, strict_registry=True): return result diff_paths = set() - merge_base = diff_base(repo, base, head) - if merge_base is None: + merge_base = None if exact_range else diff_base(repo, base, head) + head_sha = ledger.resolve_commit(repo, head) + if merge_base is not None and merge_base == head_sha != ledger.resolve_commit(repo, base): + # head is BEHIND base: the fork point is head itself and the diff + # would be empty by construction — a confident NONE for whatever + # base has that head does not (review of #112). The tree diff + # shows it, reversed: over-selection, the safe direction. + warnings.append( + f"{head} is behind {base} (an ancestor of it) — compared the two " + f"trees directly, so this lists {base}'s changes in reverse" + ) + merge_base = None + elif merge_base is None and not exact_range: # Unrelated histories or a shallow clone: no fork point to diff from. # The tree diff over-selects, which is the safe direction. An # unknown revision still fails loudly in `changed_ranges` below. @@ -708,7 +724,7 @@ def _mark_unmapped(path, detail): # 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. - if merge_base is not None and not _same_rev(repo, merge_base, base): + if merge_base is not None and merge_base != ledger.resolve_commit(repo, base): result["diff_base"] = merge_base result.update( status="OK", @@ -871,6 +887,9 @@ def main(argv=None): help="defaults to the journeys/*.json whose `target` matches ", ) ap.add_argument("--json", action="store_true") + ap.add_argument("--exact-range", action="store_true", + help="diff base..head as two trees, not from their merge-base " + "(what a push changes; the pre-push hook uses this)") args = ap.parse_args(argv) db_path = args.db or os.path.join(args.repo, ".codegraph", "codegraph.db") @@ -883,7 +902,8 @@ def main(argv=None): print(reg.not_found_message(args.repo), file=sys.stderr) return 2 try: - result = select(args.repo, args.base, args.head, db_path, registry_path) + result = select(args.repo, args.base, args.head, db_path, registry_path, + exact_range=args.exact_range) except dbmod.IndexMissing as e: print(str(e), file=sys.stderr) return 2 diff --git a/tests/test_core.py b/tests/test_core.py index 22b413a..c3c4a1c 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2602,6 +2602,61 @@ def test_unrelated_histories_fall_back_to_the_tree_diff_and_say_so(self): self.assertTrue(any("no common ancestor" in w for w in res["warnings"]), res["warnings"]) + def _sha(self, rev): + return subprocess.run(["git", "-C", self.repo, "rev-parse", rev], + capture_output=True, text=True, check=True).stdout.strip() + + def test_a_head_behind_base_is_not_an_empty_diff(self): + """Review round 1: head an ancestor of base makes the merge-base head + itself, so the diff was empty by construction — a confident NONE.""" + self._write("db/001.sql") + res = sel.select(self.repo, "HEAD", "HEAD~1", self.db, self.registry) + self.assertIn("db/001.sql", res["diff_files"]) + self.assertTrue(res["recall_degraded"]) + self.assertTrue(any("is behind" in w for w in res["warnings"]), res["warnings"]) + self.assertNotIn("diff_base", res) + + def test_the_push_hook_diffs_the_exact_range(self): + """A push moves the remote from its old tip to the new one. After a + rebase that dropped a change, the fork point no longer shows it; the + exact tree diff does. The hook (and its `full:` command) keep that.""" + self.run("git", "switch", "-qc", "feature") + self._write("db/dropped.sql") + old_tip = self._sha("HEAD") + self.run("git", "reset", "-q", "--hard", "main") + self._write("README.md") + new_tip = self._sha("HEAD") + res = sel.select(self.repo, old_tip, new_tip, self.db, self.registry, + exact_range=True) + self.assertIn("db/dropped.sql", res["diff_files"]) + self.assertNotIn("diff_base", res) + calls = [] + real = sel.select + + def spy(*a, **kw): + calls.append(kw) + return real(a[0], a[1], a[2], self.db, self.registry, **kw) + + os.makedirs(os.path.join(self.repo, ".codegraph")) + shutil.copy(self.db, os.path.join(self.repo, ".codegraph", "codegraph.db")) + with unittest.mock.patch.object(sel, "select", spy): + text, record = hook.run(self.repo, old_tip, new_tip, + registry_path=self.registry) + self.assertEqual(calls, [{"exact_range": True}]) + self.assertIn("db/dropped.sql", text + str(record)) + self.assertIn("--exact-range", text) + + def test_only_exit_1_means_no_common_ancestor(self): + """git merge-base exits 1 for unrelated histories; 128 is a bad rev or + a missing object, which must not read as "unrelated" (#112 review).""" + fake = subprocess.CompletedProcess(["git"], 128, "", "fatal: bad object") + with unittest.mock.patch.object(sel.subprocess, "run", return_value=fake): + with self.assertRaises(subprocess.CalledProcessError): + sel.diff_base(self.repo, "main", "HEAD") + none = subprocess.CompletedProcess(["git"], 1, "", "") + with unittest.mock.patch.object(sel.subprocess, "run", return_value=none): + self.assertIsNone(sel.diff_base(self.repo, "main", "HEAD")) + def test_an_unknown_revision_still_fails_loudly(self): with self.assertRaises(subprocess.CalledProcessError): sel.select(self.repo, "no-such-rev", "HEAD", self.db, self.registry)