Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .agents/roadmap_v1.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
133 changes: 133 additions & 0 deletions .agents/specs/gate-fork-ancestry.md
Original file line number Diff line number Diff line change
@@ -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 <base> --head <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.
25 changes: 23 additions & 2 deletions scripts/check-commit-trailers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand All @@ -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")
Expand Down
44 changes: 35 additions & 9 deletions scripts/check-pr-size.py
Original file line number Diff line number Diff line change
Expand Up @@ -548,26 +548,50 @@ 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")


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)
)


Expand Down Expand Up @@ -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
Expand Down
85 changes: 79 additions & 6 deletions tests/scripts/test_check_commit_trailers.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,19 +246,92 @@ 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)
with self.assertRaises(ValueError):
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")
Expand Down
Loading
Loading