diff --git a/scripts/check-role-discipline.py b/scripts/check-role-discipline.py index 916bef1c0..fc2df8478 100755 --- a/scripts/check-role-discipline.py +++ b/scripts/check-role-discipline.py @@ -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 @@ -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: @@ -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 diff --git a/tests/scripts/test_agent_role.py b/tests/scripts/test_agent_role.py index 472805cdc..f8537bbef 100644 --- a/tests/scripts/test_agent_role.py +++ b/tests/scripts/test_agent_role.py @@ -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. @@ -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 diff --git a/tests/scripts/test_check_pr_size.py b/tests/scripts/test_check_pr_size.py index 5253a3498..c728dc773 100755 --- a/tests/scripts/test_check_pr_size.py +++ b/tests/scripts/test_check_pr_size.py @@ -10,6 +10,7 @@ import sys import unittest from pathlib import Path +from unittest import mock ROOT = Path(__file__).resolve().parents[2] @@ -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() diff --git a/tests/vllm/models/test_qwen27_dense_forward.cpp b/tests/vllm/models/test_qwen27_dense_forward.cpp index f8975ba44..98abc1495 100644 --- a/tests/vllm/models/test_qwen27_dense_forward.cpp +++ b/tests/vllm/models/test_qwen27_dense_forward.cpp @@ -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 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(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") {