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
41 changes: 41 additions & 0 deletions scripts/check-role-discipline.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@
cmake/, CMakeLists.txt) can no longer be pushed straight to main. Integration
paths (scripts/, .agents/, docs/, .github/) still can, deliberately, so the
operator can fix a gate or repair the record without a round trip.

ARRIVAL IS JUDGED ONCE, ON THE COMMIT THAT LANDS THE CHANGE. A squash-merge
lands one commit carrying "(#N)". A real merge commit lands the merge plus the
branch commits it brings in; the merge is the arrival, and `merged_pr_content`
exempts the content underneath it rather than re-judging each branch commit on a
message that was never required to name the PR.
"""

from __future__ import annotations
Expand Down Expand Up @@ -195,6 +201,37 @@ def policy_commit_violations(
]


def merged_pr_content(commits: list[str]) -> frozenset[str]:
"""The commits that reached main as the reviewed CONTENT of a row/* PR merge.

A PR landed with a real merge commit ("Merge pull request #N from
mudler/row/X") pushes the merge AND the branch commits it brings in, so both
appear in one `before..after` range. The merge names the PR; the branch
commits under it do not, and reading each of them on its own message alone
called every merge-landed PR a direct push -- the gate reddened main for
doing exactly what the gate asks for. Squash-merges are unaffected: their one
commit carries "(#N)" and passes on its own message.

Only the SIDE parents count. `parents[0]` is main's existing first-parent
history, so merging something on top cannot launder a commit that was pushed
straight to main: it is excluded by `--not parents[0]`. A merge naming no row
and no PR exempts nothing, which is the case the gate exists for.
"""
content: set[str] = set()
for commit in commits:
parents = git("rev-list", "--parents", "-n", "1", commit).split()[1:]
if len(parents) < 2:
continue
subject = git("log", "-1", "--format=%s", commit)
body = git("log", "-1", "--format=%b", commit)
merged = tuple(git("log", "-1", "--format=%s%n%b", p) for p in parents[1:])
if not arrives_via_row_pr(parents, subject, body, merged):
continue
brought_in = git("rev-list", *parents[1:], "--not", parents[0])
content.update(line for line in brought_in.splitlines() if line)
return frozenset(content)


def commit_paths(commit: str) -> list[str]:
parents = git("rev-list", "--parents", "-n", "1", commit).split()[1:]
if parents:
Expand Down Expand Up @@ -301,8 +338,12 @@ def main() -> int:
print(f"ERROR: {exc}", file=sys.stderr)
return 1

reviewed = merged_pr_content(commits)
failures, reported = [], []
for commit in commits:
# Already judged, once, on the merge commit that carries it.
if commit in reviewed:
continue
for problem in inspect(commit):
# A row head has not reached main yet, so it is reportable pending
# integration rather than a false claim that unmerged work already
Expand Down
100 changes: 100 additions & 0 deletions tests/scripts/test_agent_role.py
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,33 @@ def test_landed_detached_commit_remains_strict_without_pending_evidence(self) ->
finally:
sys.argv = saved

def test_the_real_push_that_reddened_main_now_passes(self) -> None:
"""`3bbee96e..0cf3dbbb` is the exact CI range that failed for PR #178.

The unit checks above own the rule; this one owns the fact that the rule
answers THE push CI ran. Skipped rather than failed where the history is
absent (a shallow clone), because the checkers themselves need depth.

`has_reached_main` and `enforced` are pinned TRUE on purpose: run from a
`row/*` worktree they report every commit as pending PR disposition, so
main() would return 0 without judging arrival at all and this test would
pass against the very defect it exists to catch.
"""
base, head = "3bbee96ea8649cefd748bf3b979f91ae4f31d08b", "0cf3dbbb"
try:
discipline.git("cat-file", "-e", f"{base}^{{commit}}")
discipline.git("cat-file", "-e", f"{head}^{{commit}}")
except subprocess.CalledProcessError:
self.skipTest("history for the #178 push range is not present")
saved = sys.argv
sys.argv = [saved[0], "--base", base, "--head", head]
try:
with mock.patch.object(discipline, "has_reached_main", return_value=True), \
mock.patch.object(discipline, "enforced", return_value=True):
self.assertEqual(discipline.main(), 0)
finally:
sys.argv = saved

def test_a_pr_number_in_the_body_does_not_decide_this_gate(self) -> None:
"""Regression: the case that made the test above fail in CI.

Expand All @@ -526,6 +553,79 @@ def test_a_pr_number_in_the_body_does_not_decide_this_gate(self) -> None:
sys.argv = saved


class MergeLandedPrContent(unittest.TestCase):
"""A PR landed with a REAL merge commit pushes the branch commits too.

Those commits were never required to name the PR -- the merge above them
does -- so judging each one on its own message called every merge-landed PR
a direct push. Every main push that merged a PR was red for it (#178's
`6603356a`, #204's `e73cbbae`, #196's `1a02ab4f`). These build real git
history rather than hand-fed parents, because the defect was in WHICH
commits get judged, not in how one commit's message reads.
"""

def setUp(self) -> None:
self.tmp = tempfile.TemporaryDirectory()
self.repo = Path(self.tmp.name)
self.addCleanup(self.tmp.cleanup)
self.git("init", "-q", "-b", "main")
self.git("config", "user.email", "t@example.com")
self.git("config", "user.name", "T")
self.commit("docs: seed", "docs/STATUS.md")

def git(self, *args: str) -> str:
return subprocess.check_output(
["git", *args], cwd=self.repo, text=True, stderr=subprocess.DEVNULL
).strip()

def commit(self, message: str, path: str) -> str:
target = self.repo / path
target.parent.mkdir(parents=True, exist_ok=True)
target.write_text(f"{message}\n{path}\n")
self.git("add", path)
self.git("commit", "-q", "-m", message)
return self.git("rev-parse", "HEAD")

def land_via_merge(self, merge_message: str, branch_message: str) -> tuple[str, str]:
"""Build `main -- merge(row branch)` and return (branch head, merge)."""
self.git("checkout", "-q", "-b", "row/ENG-FOO")
head = self.commit(branch_message, "src/vllm/a.cpp")
self.git("checkout", "-q", "main")
self.git("merge", "-q", "--no-ff", "-m", merge_message, "row/ENG-FOO")
return head, self.git("rev-parse", "HEAD")

def content(self, *commits: str) -> frozenset[str]:
with mock.patch.object(discipline, "ROOT", self.repo):
return discipline.merged_pr_content(list(commits))

def test_a_row_pr_merge_exempts_the_branch_commits_it_brings_in(self) -> None:
head, merge = self.land_via_merge(
"Merge pull request #12 from mudler/row/ENG-FOO", "perf: faster kernel"
)
self.assertIn(head, self.content(merge))

def test_the_exemption_does_not_reach_mains_own_history(self) -> None:
"""`--not parents[0]`: the first-parent side is main, not PR content."""
seed = self.git("rev-parse", "HEAD")
_, merge = self.land_via_merge(
"Merge pull request #12 from mudler/row/ENG-FOO", "perf: faster kernel"
)
self.assertNotIn(seed, self.content(merge))

def test_a_direct_push_is_not_laundered_by_a_later_row_pr_merge(self) -> None:
"""The hole this must not open: merging a PR on top of a direct push."""
pushed = self.commit("perf: hand-edit a kernel", "src/vt/cuda/x.cu")
_, merge = self.land_via_merge(
"Merge pull request #12 from mudler/row/ENG-FOO", "perf: faster kernel"
)
self.assertNotIn(pushed, self.content(pushed, merge))

def test_a_merge_naming_no_row_anywhere_exempts_NOTHING(self) -> None:
head, merge = self.land_via_merge("Merge branch 'wip'", "perf: hand-edit")
self.assertEqual(self.content(merge), frozenset())
self.assertNotIn(head, self.content(merge))


class ReadOnlyAndModeTests(unittest.TestCase):
def test_claimable_roles_stay_exactly_two(self):
# read-only must never become a third claimable role: it takes no lock
Expand Down
59 changes: 59 additions & 0 deletions tests/scripts/test_check_pr_size.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import sys
import unittest
from pathlib import Path
from unittest import mock


ROOT = Path(__file__).resolve().parents[2]
Expand Down Expand Up @@ -497,6 +498,64 @@ def test_pending_pr_range_requires_the_exact_event_head(self) -> None:
with self.assertRaises(ValueError):
role.pending_pr_commits(base, head, pending)

def test_a_merge_landed_pr_carries_the_commits_it_brings_in(self) -> None:
"""Arrival is judged ONCE, on the commit that lands the change.

A PR landed with a real merge commit pushes the merge AND its branch
commits in one range. The merge names the PR; the branch commits under it
never had to, so judging each on its own message called every merge-
landed PR a direct push -- main went red for `6603356a` (#178),
`e73cbbae` (#204) and `1a02ab4f` (#196), in a gate about arriving through
exactly the PR that had just been merged.

The exhaustive cases live in tests/scripts/test_agent_role.py; this is the
evidence CHECKER_EVIDENCE_OVERRIDES names for the role-discipline checker,
so it pins the rule and the hole it must not open: only the SIDE parents
count, and a merge naming no row and no PR exempts nothing.
"""
role = checker.load_role_discipline()
with tempfile.TemporaryDirectory() as tmp:
repo = Path(tmp)

def git(*args: str) -> str:
return subprocess.check_output(
["git", *args], cwd=repo, text=True, stderr=subprocess.DEVNULL
).strip()

def commit(message: str, path: str) -> str:
target = repo / path
target.parent.mkdir(parents=True, exist_ok=True)
target.write_text(f"{message}\n")
git("add", path)
git("commit", "-q", "-m", message)
return git("rev-parse", "HEAD")

git("init", "-q", "-b", "main")
git("config", "user.email", "t@example.com")
git("config", "user.name", "T")
commit("docs: seed", "docs/STATUS.md")
pushed = commit("perf: hand-edit a kernel", "src/vt/cuda/x.cu")
git("checkout", "-q", "-b", "row/ENG-FOO")
reviewed = commit("perf: faster kernel", "src/vllm/a.cpp")
git("checkout", "-q", "main")
git("merge", "-q", "--no-ff", "-m",
"Merge pull request #12 from mudler/row/ENG-FOO", "row/ENG-FOO")
merge = git("rev-parse", "HEAD")

with mock.patch.object(role, "ROOT", repo):
content = role.merged_pr_content([pushed, merge])
self.assertIn(reviewed, content)
# Merging a PR on top must not launder a direct push below it.
self.assertNotIn(pushed, content)

git("checkout", "-q", "-b", "wip", merge)
commit("perf: hand-edit again", "src/vllm/b.cpp")
git("checkout", "-q", "main")
git("merge", "-q", "--no-ff", "-m", "Merge branch 'wip'", "wip")
self.assertEqual(
role.merged_pr_content([git("rev-parse", "HEAD")]), frozenset()
)


if __name__ == "__main__":
unittest.main()
43 changes: 42 additions & 1 deletion tests/vllm/models/test_qwen27_dense_forward.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -227,13 +227,54 @@ TEST_CASE("qwen27 loader packs GDN in_proj_ba in exact b,a row order") {
doctest::Contains("missing tensor"), std::runtime_error);
tensors["a"].dtype = "F32";
CHECK_THROWS_WITH_AS(LoadMergedBf16RawNK(get, {"b", "a"}),
doctest::Contains("expected BF16"), std::runtime_error);
doctest::Contains("unsupported dtype 'F32'"),
std::runtime_error);
tensors["a"].dtype = "BF16";
tensors["a"].shape = {3, 5};
CHECK_THROWS_WITH_AS(LoadMergedBf16RawNK(get, {"b", "a"}),
doctest::Contains("share input width"), std::runtime_error);
CHECK_THROWS_WITH_AS(LoadMergedBf16RawNK(get, {}),
doctest::Contains("at least one shard"), std::runtime_error);

// nvidia/Qwen3.6-27B-NVFP4 publishes the GDN in-projections as per-tensor FP8
// beside BF16 siblings, so one merged parameter may MIX dtypes. The FP8 shard
// is materialized first (nvfp4_dequant.h:83 -- `bf16(f8(w) * scale)`), and the
// b,a row order plus the nk=true orientation stay exactly as the all-BF16 case
// above. E4M3: 0x38 = 1.0, 0x40 = 2.0, 0x3c = 1.5.
const std::vector<uint8_t> a_f8 = {
0x38, 0x40, 0x3c, 0x38,
0x40, 0x3c, 0x38, 0x40,
0x3c, 0x38, 0x40, 0x3c,
}; // [3,4]
const float a_scale = 2.0F;
tensors["a"].dtype = "F8_E4M3";
tensors["a"].shape = {3, 4};
tensors["a"].data = a_f8.data();
tensors["a"].nbytes = a_f8.size();
add("a_scale", "F32", {1}, &a_scale, sizeof(a_scale));

const OwnedTensor mixed = LoadMergedBf16RawNK(get, {"b", "a"});
REQUIRE(mixed.rank == 2);
CHECK(mixed.shape[0] == 5);
CHECK(mixed.shape[1] == 4);
CHECK(mixed.dtype == DType::kBF16);
REQUIRE(mixed.bytes.size() == (b.size() + a_f8.size()) * sizeof(uint16_t));
CHECK(std::memcmp(mixed.bytes.data(), b.data(), b.size() * sizeof(uint16_t)) == 0);
const uint16_t* merged_rows =
reinterpret_cast<const uint16_t*>(mixed.bytes.data());
// 1.0,2.0,1.5 * 2.0 -> 2.0,4.0,3.0 -> bf16 0x4000,0x4080,0x4040.
const uint16_t want[3] = {0x4000, 0x4080, 0x4040};
for (size_t i = 0; i < a_f8.size(); ++i) {
CHECK(merged_rows[b.size() + i] == want[i % 3]);
}

// A per-output-channel scale read as per-tensor would be silently wrong, so
// any other element count is REJECTED rather than reinterpreted.
const float bad_scale[2] = {2.0F, 2.0F};
add("a_scale", "F32", {2}, bad_scale, sizeof(bad_scale));
CHECK_THROWS_WITH_AS(LoadMergedBf16RawNK(get, {"b", "a"}),
doctest::Contains("per-tensor or one value per output row"),
std::runtime_error);
}

TEST_CASE("qwen27 GDN loader retains one merged BA owner and no split copies") {
Expand Down
Loading