From 194dcc0e3e3651d1e25777f95d9e12fa0aa90a92 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Fri, 14 Aug 2026 16:40:42 +0000 Subject: [PATCH] fix(GATE-FORK-ANCESTRY): diff a PR from its merge base, so CI finally validates an outside contributor's trailers (#773) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check-pr-size.py` and `check-commit-trailers.py` both demanded the base revision be an ANCESTOR of head. CI passes `github.event.pull_request.base.sha` -- the TIP of the base branch -- which stops being an ancestor the moment main advances after the branch was cut. On this repo that is continuous. Measured on three open PRs before touching anything. Base is not an ancestor in any of them, and a merge base exists in all three: #506 ancestor=NO merge-base=e1087a881 #523 ancestor=NO merge-base=fdd452637 #559 ancestor=NO merge-base=fafa16f0f The consequence is not a noisy red check. Both checkers abort BEFORE examining anything, so: CI has never validated commit trailers on an external contribution. The gate that enforces FOLLOWING_AGENTS_PROTOCOL and Assisted-by: exits before reading a single commit. On the external PRs reviewed this week, hand-checking by a reviewer was the only verification those trailers received. `pr-size` aborts identically, so path classification and the checker-evidence contract went unenforced on forks too -- while contributors saw two permanently red checks carrying no information, which is how a gate teaches people to ignore it. THE FIX. Use what a pull request actually IS: `git diff A...B` is defined as `git diff $(git merge-base A B) B`, and it is what GitHub shows. Two-dot diffing against a moved main is not merely stricter, it is WRONG -- main's own commits render as reversions inside the contributor's diff, so paths they never touched get classified and charged to them. The new test asserts both halves: the PR's file is present and main's is absent. `executable_evidence` gets the same treatment: the BASE version of a checker, for the red-before half, is the one at the merge base, not at a tip that has moved past the branch. WHAT DELIBERATELY DOES NOT MOVE. The old rule conflated two situations, and only one of them was wrong: ordinary divergence (merge base exists) -> now examines merge_base..head unrelated histories (no merge base) -> STILL RAISES Absence of information must never look like absence of work -- the script's own `require_origin_main()` docstring already states that principle for the other input. `test_missing_and_nonancestor_objects_fail_closed` uses an ORPHAN branch, so it still raises; only its regex changed, because the message now names what is actually wrong instead of calling it "not an ancestor". Its `assertRaises(ValueError)` is untouched. The trailers case was SPLIT rather than deleted. Its missing-revision half is unchanged; its divergent-branch half built two branches off a common root -- which share a merge base and are the ordinary shape of every PR -- so that half moved to a case asserting it now validates, and the genuinely-unrelated case is asserted separately in `test_unrelated_histories_still_fail_closed`. Nothing that used to fail closed stopped failing closed. Range changed, contract unchanged: `test_a_bad_trailer_in_the_merge_base_range_ is_still_reported` puts a trailerless commit inside the new range and requires it still be reported, so this cannot be mistaken for a way to smuggle a non-conforming commit past the gate. RED before, on the unmodified checkers, for the intended reason: ValueError: base must be an ancestor of head (check-pr-size.py) ValueError: range base must be an ancestor of range head (check-commit-trailers.py) 1 failed, 45 passed | 2 failed, 26 passed GREEN after: 74 passed, 148 subtests. Stop conditions §8 checked individually rather than inferred -- orphan still raises in both suites, a bad trailer in the new range is still reported, and each checker accepts its own range. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code] --- .agents/roadmap_v1.md | 1 + .agents/specs/gate-fork-ancestry.md | 133 ++++++++++++++++++++ scripts/check-commit-trailers.py | 25 +++- scripts/check-pr-size.py | 44 +++++-- tests/scripts/test_check_commit_trailers.py | 85 ++++++++++++- tests/scripts/test_check_pr_size.py | 59 ++++++++- 6 files changed, 329 insertions(+), 18 deletions(-) create mode 100644 .agents/specs/gate-fork-ancestry.md diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index 7c2772ab4..b76cdfa52 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -156,6 +156,7 @@ issue is not yet placed. Keyed record: update in place, never append. | [#644](https://github.com/mudler/vllm.cpp/issues/644) | `ROAD-V1-LTX25` | LTX-2.5 FULL PORT campaign. Row 0 `LTX25-PROMPT-ADALN` (spec [`ltx25-prompt-adaln.md`](specs/ltx25-prompt-adaln.md)): `use_prompt_adaln_single` defaults TRUE in BOTH references and the shipped DiT carries its 18 tensors, but `ltx2_loader.cpp:988` cleared the flag unconditionally, so every render dropped the timestep-conditioned half of the prompt K/V modulation — invisible to every shape and finiteness check | feature | | [#615](https://github.com/mudler/vllm.cpp/issues/615) | `GATE-PR-SIZE-BINARY` | `check-pr-size` fail-closes on every binary path with no exemption route, so no golden-bearing PR can merge: it blocks #431 and post-dates the golden precedent it rejects, spec [`gate-pr-size-binary.md`](specs/gate-pr-size-binary.md) | bug | | [#670](https://github.com/mudler/vllm.cpp/issues/670) | `BACKEND-TENSTORRENT-MISTRAL` | Tenstorrent: allowlist `MistralForCausalLM` and gate it on-device; goldens are `transformers`-teacher-forced because vLLM has no TT backend, spec [`tenstorrent-mistral.md`](specs/tenstorrent-mistral.md) | feature | +| [#773](https://github.com/mudler/vllm.cpp/issues/773) | `GATE-FORK-ANCESTRY` | `check-pr-size` and `check-commit-trailers` abort on every fork PR because the base SHA stops being an ancestor once main moves, so CI has never validated an outside contributor's trailers, spec [`gate-fork-ancestry.md`](specs/gate-fork-ancestry.md) | bug | | [#238](https://github.com/mudler/vllm.cpp/issues/238) | `SAMPLE-LOGPROB-TOKEN-IDS` | `logprobs_mode`: three of four modes are runtime-refused stubs | bug | | [#264](https://github.com/mudler/vllm.cpp/issues/264) | `SAMPLE-LOGPROB-TOKEN-IDS` | `logprob_token_ids`: generative scoring over an explicit token set is unported | feature | | [#365](https://github.com/mudler/vllm.cpp/issues/365) | `PERF-27B-DENSE-MARLIN-GATEUP` | 27B gap decomposed vs vLLM's own decode profile; dense W4A16 MLP bypassed the fused gate_up seam (spec `specs/perf-27b-dense-marlin-gateup.md`) | bug | diff --git a/.agents/specs/gate-fork-ancestry.md b/.agents/specs/gate-fork-ancestry.md new file mode 100644 index 000000000..57bcf7a48 --- /dev/null +++ b/.agents/specs/gate-fork-ancestry.md @@ -0,0 +1,133 @@ +# GATE-FORK-ANCESTRY — diff a PR from its merge base, not from a moved branch tip + +**Row:** `GATE-FORK-ANCESTRY` +**Issue:** [#773](https://github.com/mudler/vllm.cpp/issues/773) +**Base:** `origin/main` `5ddfca6f6` +**Status:** ACTIVE, 2026-08-14 + +## 1. Scope + +Two checkers stop requiring the base revision to be an *ancestor* of head, and +instead compute the range from `git merge-base base head`: + +- `scripts/check-pr-size.py` — `require_ancestor()` / `changed_paths()` +- `scripts/check-commit-trailers.py` — `validate_range()` + +**In scope.** Those two range computations, the two existing tests that pin the +current behaviour, and new tests for both halves of the distinction in §3. + +**Out of scope.** Every rule either checker enforces once the range exists: path +classification, the checker-evidence contract, the role checks, the trailer +contract itself, cutover handling. None of them changes. This alters *which +commits are examined*, never *what is demanded of them*. + +Also out of scope: `audit-live-rows` (#726) and `check-windows-portability` +(#774) are being repaired concurrently and this change touches neither. + +## 2. Anchors + +Local governance checkers; no vLLM counterpart. + +| What | Where | +|---|---| +| The pr-size guard | `scripts/check-pr-size.py` `require_ancestor()` | +| Its caller | `scripts/check-pr-size.py` `changed_paths()` | +| The trailer guard | `scripts/check-commit-trailers.py` `validate_range()` | +| What CI passes as base | `.github/workflows/ci.yml`, `github.event.pull_request.base.sha` | +| Test pinning current pr-size behaviour | `tests/scripts/test_check_pr_size.py::test_missing_and_nonancestor_objects_fail_closed` | +| Test pinning current trailer behaviour | `tests/scripts/test_check_commit_trailers.py::test_missing_unreachable_and_non_ancestor_revisions_fail_closed` | + +## 3. Design + +`github.event.pull_request.base.sha` is the **tip of the base branch**. It stops +being an ancestor of head the moment `main` moves after the branch was cut, +which on this repo is continuous. Measured on three open PRs (#506, #523, #559): +base is not an ancestor in any of them, and a merge base exists in all three. + +Both checkers therefore abort **before validating anything**. The consequence is +not merely a red check: + +> CI has never validated commit trailers on an external contribution. The check +> that enforces `FOLLOWING_AGENTS_PROTOCOL` and `Assisted-by:` exits before it +> reads a single commit. + +The fix is to use what a pull request actually *is*. `git diff A...B` (three-dot) +is defined as `git diff $(git merge-base A B) B`, and it is what GitHub shows. +Two-dot diffing against a moved `main` is not stricter, it is **wrong**: main's +own commits appear as reversions inside the contributor's diff, so paths the +contributor never touched get classified and charged to them. + +**The distinction that must survive.** "Base is not an ancestor" currently +conflates two situations: + +| Situation | Merge base | Correct behaviour | +|---|---|---| +| Ordinary divergence — branch cut from main, main moved on | exists | **Examine `merge_base..head`.** This is every PR. | +| Unrelated histories — orphan branch, wrong repo, garbage revision | none | **Fail closed.** Absence of information is not absence of work. | + +Only the first changes. The second keeps raising, and the existing orphan-branch +test keeps asserting it — which is why that test is preserved verbatim rather +than relaxed. + +**Why not "fetch more in CI instead".** That would make the base an ancestor +again only by luck of timing, and would leave the checkers wrong for anyone +running them locally against a branch cut before the last merge. The range +computation is the defect; the fetch is not. + +## 4. Risks and decisions + +| Risk | Assessment | +|---|---| +| Relaxing an assertion to make a gate pass | The orphan/unrelated-histories case still raises, and its test is kept unchanged. What is removed is a demand that no pull request in this repository can satisfy — a rule nothing can meet is not enforcing a standard. Argued in the commit message per the no-waiver-registry rule. | +| A contributor hides a change behind an old merge base | They cannot. `merge_base..head` contains exactly the commits the PR adds; anything they touch is in it. What leaves the range is main's own work, which is precisely what should not be charged to them. | +| The trailer checker examines fewer commits and misses one | It examines *more* correctly-scoped commits: today it examines **zero** on a fork PR, because it aborts. Any commit the PR introduces is reachable from head and not from the merge base, so it is in range by construction. | +| Cutover handling drifts | `cutover` is validated against `head`, not against `base`, and that check is untouched. | +| Divergent-history test now passes where it failed | Deliberate, and the reason the row exists — see §3's table. Split into two cases so the surviving half is asserted explicitly rather than deleted. | + +## 5. Tests + +RED-first in both suites. + +1. `test_pr_size_uses_the_merge_base_when_main_moved` — build a repo, cut a + branch, advance `main` past it, then call `changed_paths(main_tip, branch)`. + **RED before:** raises `base must be an ancestor of head`. **After:** returns + exactly the branch's own paths, and *not* the paths main added. +2. `test_trailers_validate_from_the_merge_base_when_main_moved` — same shape + against `validate_range()`. **RED before:** raises. **After:** validates the + branch's commits, and a bad trailer among them is still reported — proving + the range change did not disarm the contract. +3. Existing `test_missing_and_nonancestor_objects_fail_closed` (pr-size) is + **kept unchanged**: it uses an `--orphan` branch, so there is no merge base + and it must still raise. +4. `test_missing_unreachable_and_non_ancestor_revisions_fail_closed` (trailers) + is **split**. Its missing-revision half is unchanged. Its divergent-branch + half moves to a new unrelated-histories case, because the divergent pair it + built *shares* a merge base and is the situation §3 says must now work. + +## 6. Gates + +- Both suites green, with cases 1 and 2 shown RED on the unmodified checkers. +- `check-pr-size.py --base --head ` classifies this PR's own diff. +- `check-commit-trailers.py` validates this PR's own range. +- Both are `governance_checker` paths, so this PR must itself carry executable + mutation evidence in both recognized test files. +- `scripts/agent-preflight.sh`, and `pytest tests/scripts/` with + `test_cpu_kernel_bench.py` ignored (it needs a built benchmark binary and + fails collection on main). + +## 7. Evidence + +Recorded on completion: RED output from both checkers before the change, GREEN +after, the orphan case still raising, and each checker's verdict on this PR's +own range. + +## 8. Stop conditions + +- Stop if the orphan/unrelated-histories case stops raising. That is the half + that must not move. +- Stop if a trailer defect inside the new range goes unreported — the range may + change, the contract may not. +- Stop if either checker cannot classify or validate its own diff after the + change. +- Stop if making the range correct requires touching any rule in §1's + out-of-scope list. diff --git a/scripts/check-commit-trailers.py b/scripts/check-commit-trailers.py index 1acf7f50d..d98355b3f 100755 --- a/scripts/check-commit-trailers.py +++ b/scripts/check-commit-trailers.py @@ -253,6 +253,22 @@ def _resolve_commit(repo: Path, revision: str) -> str: return resolved[0] +def _merge_base(repo: Path, a: str, b: str) -> str: + """The merge base of two revisions; raises when they share no history.""" + result = subprocess.run( + ["git", "-C", str(repo), "merge-base", a, b], + capture_output=True, + text=True, + check=False, + ) + if result.returncode != 0: + raise ValueError("range base and head have no merge base (unrelated histories)") + oid = result.stdout.strip() + if not re.fullmatch(r"[0-9a-f]{40}", oid): + raise ValueError("merge base did not resolve to one commit") + return oid + + def _is_ancestor(repo: Path, older: str, newer: str) -> bool: result = subprocess.run( ["git", "-C", str(repo), "merge-base", "--is-ancestor", older, newer], @@ -277,8 +293,13 @@ def validate_range( base_oid = _resolve_commit(repo, base) head_oid = _resolve_commit(repo, head) - if not _is_ancestor(repo, base_oid, head_oid): - raise ValueError("range base must be an ancestor of range head") + # From the MERGE BASE, not the base tip (#773). CI passes + # `pull_request.base.sha`, which stops being an ancestor of head as soon as + # main advances past the branch -- so this used to raise and return WITHOUT + # READING A SINGLE COMMIT, meaning the trailer contract was never enforced + # on any external contribution. Unrelated histories still fail closed: no + # merge base means no range, and inventing one would be worse than refusing. + base_oid = _merge_base(repo, base_oid, head_oid) cutover_oid = _resolve_commit(repo, cutover) if cutover is not None else None if cutover_oid is not None and not _is_ancestor(repo, cutover_oid, head_oid): raise ValueError("cutover must be reachable from range head") diff --git a/scripts/check-pr-size.py b/scripts/check-pr-size.py index 1b3eebaf0..aa2864943 100755 --- a/scripts/check-pr-size.py +++ b/scripts/check-pr-size.py @@ -548,16 +548,39 @@ def resolve_commit(repo: Path, revision: str) -> str: return oid -def require_ancestor(repo: Path, base_oid: str, head_oid: str) -> None: +def range_base(repo: Path, base_oid: str, head_oid: str) -> str: + """The merge base of base and head -- i.e. what a pull request actually is. + + This used to demand that `base_oid` be an ANCESTOR of head (#773). CI passes + `pull_request.base.sha`, the TIP of the base branch, which stops being an + ancestor the moment main advances after the branch was cut -- continuously, + on this repo. So the checker aborted BEFORE classifying anything, and no + fork PR was ever checked. The sibling trailer gate aborted the same way, + which is why CI has never validated an external contributor's trailers. + + Diffing two-dot against a moved main is not merely stricter, it is WRONG: + main's own commits appear as reversions inside the contributor's diff, so + paths they never touched get classified and charged to them. `git diff + A...B` is defined as `git diff $(git merge-base A B) B` and is what GitHub + itself shows. + + Unrelated histories still fail closed. No merge base means there is no range + to compute, and reporting one would be an invention -- absence of + information must never look like absence of work. + """ result = subprocess.run( - ["git", "-C", str(repo), "merge-base", "--is-ancestor", base_oid, head_oid], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, + ["git", "-C", str(repo), "merge-base", base_oid, head_oid], + capture_output=True, + text=True, timeout=EVIDENCE_TIMEOUT_SECONDS, shell=False, ) - if result.returncode == 1: - raise ValueError("base must be an ancestor of head") + if result.returncode != 0: + raise ValueError("base and head have no merge base (unrelated histories)") + oid = result.stdout.strip() + if re.fullmatch(r"[0-9a-f]{40}", oid) is None: + raise ValueError("merge base did not resolve to one commit") + return oid if result.returncode != 0: raise ValueError("could not establish base/head ancestry") @@ -565,9 +588,10 @@ def require_ancestor(repo: Path, base_oid: str, head_oid: str) -> None: def changed_paths(base: str, head: str, *, repo: Path = ROOT) -> list[ChangedPath]: base_oid = resolve_commit(repo, base) head_oid = resolve_commit(repo, head) - require_ancestor(repo, base_oid, head_oid) + # From the MERGE BASE, not the base tip -- see range_base() (#773). return parse_numstat( - git("diff", "--no-renames", "--numstat", base_oid, head_oid, repo=repo) + git("diff", "--no-renames", "--numstat", + range_base(repo, base_oid, head_oid), head_oid, repo=repo) ) @@ -667,7 +691,9 @@ def executable_evidence( base_oid = resolve_commit(repo, base) head_oid = resolve_commit(repo, head) - require_ancestor(repo, base_oid, head_oid) + # The BASE version of a checker, for the red-before half, is the one at the + # merge base -- not at a base tip that has moved past this branch (#773). + base_oid = range_base(repo, base_oid, head_oid) changed = {item.path for item in changes} checkers = sorted( item.path diff --git a/tests/scripts/test_check_commit_trailers.py b/tests/scripts/test_check_commit_trailers.py index 627a7c5a0..9e84b036c 100755 --- a/tests/scripts/test_check_commit_trailers.py +++ b/tests/scripts/test_check_commit_trailers.py @@ -246,6 +246,78 @@ def test_post_cutover_legacy_commit_fails(self) -> None: ) self.assertTrue(any("Following-Agents-Protocol" in error for error in errors)) + def test_range_is_taken_from_the_merge_base_when_the_base_branch_moved(self) -> None: + """A PR branch diverges the moment main moves; that must still validate. + + RED before GATE-FORK-ANCESTRY (#773): CI passes + `pull_request.base.sha`, the TIP of the base branch, which stops being an + ancestor of head as soon as main advances. `validate_range` raised + "range base must be an ancestor of range head" and returned WITHOUT + READING A SINGLE COMMIT -- so the trailer contract was never enforced on + any external contribution. + """ + root = self.commit("root\n\nFOLLOWING_AGENTS_PROTOCOL\n") + # The PR branch, cut from root. + subprocess.run( + ["git", "-C", str(self.repo), "checkout", "-q", "-b", "pr", root], + check=True, + ) + head = self.commit(STRICT_MESSAGE) + # Main moves on after the branch was cut. This is the ordinary case. + subprocess.run( + ["git", "-C", str(self.repo), "checkout", "-q", "-B", "main", root], + check=True, + ) + moved_main = self.commit(STRICT_MESSAGE.replace("policy:", "mainline:")) + self.assertNotEqual(moved_main, root) + + errors = self.checker.validate_range( + self.repo, moved_main, head, cutover=None + ) + self.assertEqual(errors, [], "the PR's own commits must validate") + + def test_a_bad_trailer_in_the_merge_base_range_is_still_reported(self) -> None: + """Changing WHICH commits are read must not change what is demanded. + + Green on both sides of #773 in the ancestor case; the point is that it + stays green in the DIVERGED case too, so the range fix cannot be + mistaken for a way to smuggle a non-conforming commit past the gate. + """ + root = self.commit("root\n\nFOLLOWING_AGENTS_PROTOCOL\n") + subprocess.run( + ["git", "-C", str(self.repo), "checkout", "-q", "-b", "pr2", root], + check=True, + ) + self.commit("no trailers here at all\n") + head = self.commit(STRICT_MESSAGE) + subprocess.run( + ["git", "-C", str(self.repo), "checkout", "-q", "-B", "main2", root], + check=True, + ) + moved_main = self.commit(STRICT_MESSAGE.replace("policy:", "mainline2:")) + + errors = self.checker.validate_range( + self.repo, moved_main, head, cutover=None + ) + self.assertTrue(errors, "the offending commit must still be reported") + + def test_unrelated_histories_still_fail_closed(self) -> None: + """No merge base at all is absence of INFORMATION, and must still raise. + + This is the half of the old non-ancestor assertion that must NOT move: + an orphan branch shares no commit with the base, so there is no range to + compute and reporting one would be an invention. + """ + base = self.commit("base\n\nFOLLOWING_AGENTS_PROTOCOL\n") + subprocess.run( + ["git", "-C", str(self.repo), "checkout", "-q", "--orphan", "orphan"], + check=True, + ) + subprocess.run(["git", "-C", str(self.repo), "rm", "-qrf", "."], check=True) + orphan = self.commit(STRICT_MESSAGE) + with self.assertRaises(ValueError): + self.checker.validate_range(self.repo, base, orphan, cutover=None) + def test_missing_unreachable_and_non_ancestor_revisions_fail_closed(self) -> None: base = self.commit("base\n\nFOLLOWING_AGENTS_PROTOCOL\n") head = self.commit(STRICT_MESSAGE) @@ -253,12 +325,13 @@ def test_missing_unreachable_and_non_ancestor_revisions_fail_closed(self) -> Non self.checker.validate_range( self.repo, "missing", head, cutover=head ) - subprocess.run(["git", "-C", str(self.repo), "checkout", "-q", "--detach", base], check=True) - side = self.commit(STRICT_MESSAGE.replace("policy:", "side:")) - with self.assertRaises(ValueError): - self.checker.validate_range( - self.repo, side, head, cutover=head - ) + # The divergent-but-RELATED half of this case moved to + # test_range_is_taken_from_the_merge_base_when_the_base_branch_moved + # (#773): two branches off a common root share a merge base, which is + # the ordinary shape of every pull request and must validate rather than + # raise. The genuinely unrelated case -- no shared history at all -- is + # asserted in test_unrelated_histories_still_fail_closed, so the + # fail-closed behaviour this case was written for is still pinned. def test_ambiguous_revision_name_fails_closed(self) -> None: base = self.commit("base\n\nFOLLOWING_AGENTS_PROTOCOL\n") diff --git a/tests/scripts/test_check_pr_size.py b/tests/scripts/test_check_pr_size.py index e14cdae75..dfd855105 100755 --- a/tests/scripts/test_check_pr_size.py +++ b/tests/scripts/test_check_pr_size.py @@ -685,6 +685,58 @@ def test_real_git_rename_is_delete_plus_add_with_no_rename_syntax(self) -> None: ) self.assertFalse(any("{" in change.path or "=>" in change.path for change in changes)) + def test_changed_paths_uses_the_merge_base_when_the_base_branch_moved(self) -> None: + """A PR diff is merge_base..head, not base_tip..head (#773). + + RED before GATE-FORK-ANCESTRY: CI passes `pull_request.base.sha`, the + TIP of the base branch, which stops being an ancestor of head the moment + main advances -- so `changed_paths` raised and classification never ran + on any fork PR. Worse than strict: two-dot diffing a moved main renders + MAIN's own commits as reversions inside the contributor's diff, so paths + they never touched get classified and charged to them. This asserts both + halves: the PR's path is present, main's is absent. + """ + with tempfile.TemporaryDirectory(dir="/dev/shm") as directory: + repo = Path(directory) + run = lambda *a: subprocess.run(["git", "-C", str(repo), *a], check=True) + subprocess.run(["git", "init", "-q", str(repo)], check=True) + run("config", "user.name", "Test") + run("config", "user.email", "test@example.com") + (repo / "src").mkdir() + (repo / "src" / "root.cpp").write_text("root\n", encoding="utf-8") + run("add", ".") + run("commit", "-qm", "root") + root = subprocess.check_output( + ["git", "-C", str(repo), "rev-parse", "HEAD"], text=True + ).strip() + + # The contributor's branch, cut from root. + run("checkout", "-q", "-b", "pr", root) + (repo / "src" / "from_pr.cpp").write_text("pr\n", encoding="utf-8") + run("add", ".") + run("commit", "-qm", "pr work") + head = subprocess.check_output( + ["git", "-C", str(repo), "rev-parse", "HEAD"], text=True + ).strip() + + # main moves on afterwards -- the ordinary case, not an edge case. + run("checkout", "-q", "-B", "main", root) + (repo / "src" / "from_main.cpp").write_text("main\n", encoding="utf-8") + run("add", ".") + run("commit", "-qm", "mainline work") + moved_main = subprocess.check_output( + ["git", "-C", str(repo), "rev-parse", "HEAD"], text=True + ).strip() + self.assertNotEqual(moved_main, root) + + paths = {c.path for c in checker.changed_paths(moved_main, head, repo=repo)} + self.assertIn("src/from_pr.cpp", paths) + self.assertNotIn( + "src/from_main.cpp", + paths, + "main's own commit must not appear in the contributor's diff", + ) + def test_missing_and_nonancestor_objects_fail_closed(self) -> None: with tempfile.TemporaryDirectory(dir="/dev/shm") as directory: repo = Path(directory) @@ -705,7 +757,12 @@ def test_missing_and_nonancestor_objects_fail_closed(self) -> None: side = subprocess.check_output(["git", "-C", str(repo), "rev-parse", "HEAD"], text=True).strip() with self.assertRaises(ValueError): checker.changed_paths("missing", side, repo=repo) - with self.assertRaisesRegex(ValueError, "ancestor"): + # Still RAISES -- the fail-closed half of the old non-ancestor rule + # is deliberately preserved (#773). `side` here is an ORPHAN branch, + # so there is no merge base and therefore no range to compute. Only + # the message changed: what used to be reported as "not an ancestor" + # is now named for what it actually is. + with self.assertRaisesRegex(ValueError, "no merge base"): checker.changed_paths(base, side, repo=repo) def test_production_pr_classifier_covers_every_governed_path_class(self) -> None: