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: