diff --git a/.github/buildcheck/patches.py b/.github/buildcheck/patches.py new file mode 100644 index 000000000..8fb2bd4e2 --- /dev/null +++ b/.github/buildcheck/patches.py @@ -0,0 +1,104 @@ +# SPDX-FileCopyrightText: 2026 Bernard Ladenthin +# +# SPDX-License-Identifier: MIT OR Apache-2.0 + +"""Fail when a patch in llama/patches/ declares a hunk size that does not match its body. + +A unified-diff hunk header `@@ -a,b +c,d @@` promises exactly `b` old-side and `d` new-side lines. +When the body holds MORE than that, `git apply` writes only the declared number and silently +truncates -- and `git apply --check` does NOT notice, because it validates that the context applies, +not that the counts add up. That is not hypothetical: editing the comment inside a new file added by +`patches/0017` left its header at `+1,25` for 27 lines, so the file was written without its closing +brace, every translation unit including it continued inside an unclosed function, and `quants.c` +failed with 19 x "function definition is not allowed here" on every platform. 23 jobs of one +Publish run went red for it, after `--check` had reported the patch as fine. + +The applier (`llama/cmake/apply-llama-patches.cmake`) cannot catch this either: a truncated file is +a successfully applied patch as far as git is concerned, and `.github/verify-patches-applied.sh` +only asserts that every patch is in the stamp and that the tree is dirty -- both true here. + +So this is a STATIC check of the patch files, cheap enough to run in the first minutes of a run +(the `code-style` job) and before a push. + +**The algorithm is the part that matters, and two obvious forms of it are wrong.** Counting body +lines until the next delimiter over-counts trailing context *and* accepts a header that +under-counts its body -- exactly the bug above. Only checking that the body is not SHORT misses it +as well. The correct form is: consume exactly as many lines as the header declares, then require +that a delimiter follows. A body that ends early is short, a body that continues is under-counted, +and both are reported with what `git apply` would do about it. + +Usage (the CLI is .github/check-patches.py): check-patches.py [] +""" + +import glob +import io +import os +import re +import sys + +HUNK = re.compile(r"^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@") +# "\ No newline at end of file" belongs to the line before it and counts for neither side. +NO_NEWLINE = chr(92) +DEFAULT_DIR = os.path.join("llama", "patches") + + +def _delimiter(line): + """Whether `line` ends a hunk body: the next hunk, the next file, or a git signature.""" + return line.startswith("@@") or line.startswith("diff --git") or line == "-- " + + +def problems(path): + """Every hunk of `path` whose declared size disagrees with its body, as messages.""" + lines = io.open(path, encoding="utf-8", newline="").read().splitlines() + name = os.path.basename(path) + found = [] + for i, line in enumerate(lines): + m = HUNK.match(line) + if not m: + continue + want_old = int(m.group(2)) if m.group(2) is not None else 1 + want_new = int(m.group(4)) if m.group(4) is not None else 1 + old = new = 0 + j = i + 1 + while j < len(lines) and (old < want_old or new < want_new): + body = lines[j] + if _delimiter(body): + break + if body.startswith(NO_NEWLINE): + pass + elif body.startswith("+"): + new += 1 + elif body.startswith("-"): + old += 1 + elif body.startswith(" ") or body == "": + old += 1 + new += 1 + else: + break # prose below the last hunk of a patch that carries a description + j += 1 + if (old, new) != (want_old, want_new): + found.append(f"{name}:{i + 1}: hunk declares -{want_old} +{want_new} but its body holds " + f"{old} old / {new} new lines -- it is SHORT, so `git apply` fails or " + f"writes a partial file") + continue + if j < len(lines) and not _delimiter(lines[j]) and lines[j][:1] in ("+", "-", " "): + found.append(f"{name}:{i + 1}: hunk declares -{want_old} +{want_new} but its body " + f"continues at line {j + 1} ({lines[j][:48]!r}) -- the header UNDER-counts, " + f"so `git apply` writes a TRUNCATED file and `--check` does not notice") + return found + + +def main(argv): + directory = argv[1] if len(argv) > 1 else DEFAULT_DIR + paths = sorted(glob.glob(os.path.join(directory, "*.patch")) + + glob.glob(os.path.join(directory, "*.diff"))) + if not paths: + print(f"::error::no patches found in {directory} -- is the path right?", file=sys.stderr) + return 2 + found = [] + for p in paths: + found += problems(p) + for f in found: + print(f"::error::{f}", file=sys.stderr) + print(f"{len(paths)} patches checked, {len(found)} hunk size mismatch(es)") + return 1 if found else 0 diff --git a/.github/buildcheck/tests/test_patches.py b/.github/buildcheck/tests/test_patches.py new file mode 100644 index 000000000..8ebfa56f4 --- /dev/null +++ b/.github/buildcheck/tests/test_patches.py @@ -0,0 +1,115 @@ +# SPDX-FileCopyrightText: 2026 Bernard Ladenthin +# +# SPDX-License-Identifier: MIT OR Apache-2.0 + +"""Tests for buildcheck.patches -- the hunk-size guard.""" + +import io +import os +import tempfile +import unittest + +from buildcheck import patches + +# A correct new-file hunk: 4 added lines, declared as such. +GOOD_NEW_FILE = """diff --git a/x.h b/x.h +new file mode 100644 +index 0000000..1111111 +--- /dev/null ++++ b/x.h +@@ -0,0 +1,4 @@ ++#pragma once ++static inline int f(void) { ++ return 1; ++} +""" + +# A correct modification hunk: 3 context + 1 removed + 1 added = -4 +4. +GOOD_EDIT = """diff --git a/y.c b/y.c +--- a/y.c ++++ b/y.c +@@ -10,4 +10,4 @@ int main(void) + a; +-old; ++new; + b; + c; +""" + + +class HunkSizeTest(unittest.TestCase): + def write(self, text): + path = os.path.join(self.dir, "p.patch") + with io.open(path, "w", encoding="utf-8", newline="") as f: + f.write(text) + return path + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.dir = self._tmp.name + self.addCleanup(self._tmp.cleanup) + + def test_a_correct_new_file_hunk_passes(self): + self.assertEqual(patches.problems(self.write(GOOD_NEW_FILE)), []) + + def test_a_correct_modification_hunk_passes(self): + self.assertEqual(patches.problems(self.write(GOOD_EDIT)), []) + + def test_an_under_counting_header_is_reported(self): + """The regression this guard exists for: the body is longer than the header says, so + `git apply` writes only the declared lines and truncates the file -- and --check is happy.""" + broken = GOOD_NEW_FILE.replace("@@ -0,0 +1,4 @@", "@@ -0,0 +1,2 @@") + found = patches.problems(self.write(broken)) + self.assertEqual(len(found), 1, found) + self.assertIn("UNDER-counts", found[0]) + self.assertIn("TRUNCATED", found[0]) + # it names the line the body continues at, which is the first line git would drop + self.assertIn("return 1", found[0]) + + def test_an_over_counting_header_is_reported(self): + over = GOOD_NEW_FILE.replace("@@ -0,0 +1,4 @@", "@@ -0,0 +1,9 @@") + found = patches.problems(self.write(over)) + self.assertEqual(len(found), 1, found) + self.assertIn("SHORT", found[0]) + + def test_a_wrong_old_side_count_is_reported(self): + wrong = GOOD_EDIT.replace("@@ -10,4 +10,4 @@", "@@ -10,9 +10,4 @@") + found = patches.problems(self.write(wrong)) + self.assertEqual(len(found), 1, found) + self.assertIn("SHORT", found[0]) + + def test_a_single_line_hunk_may_omit_its_count(self): + """`@@ -5 +5 @@` is legal shorthand for a count of 1 and must not be flagged.""" + one = "--- a/z\n+++ b/z\n@@ -5 +5 @@\n-a\n+b\n" + self.assertEqual(patches.problems(self.write(one)), []) + + def test_a_no_newline_marker_counts_for_neither_side(self): + marker = ("--- a/z\n+++ b/z\n@@ -1,2 +1,2 @@\n a\n-b\n" + chr(92) + + " No newline at end of file\n+c\n" + chr(92) + " No newline at end of file\n") + self.assertEqual(patches.problems(self.write(marker)), []) + + def test_prose_after_the_last_hunk_does_not_count(self): + """The patches in this repository carry a long description ABOVE the diff, and some end + with a git signature; neither may be read as hunk body.""" + self.assertEqual(patches.problems(self.write(GOOD_EDIT + "-- \n2.43.0\n")), []) + + def test_every_hunk_of_a_multi_hunk_patch_is_checked(self): + two = GOOD_EDIT + GOOD_NEW_FILE.replace("@@ -0,0 +1,4 @@", "@@ -0,0 +1,3 @@") + found = patches.problems(self.write(two)) + self.assertEqual(len(found), 1, found) + self.assertIn("UNDER-counts", found[0]) + + +class MainTest(unittest.TestCase): + def test_the_repositorys_own_patches_are_consistent(self): + here = os.path.dirname(os.path.abspath(__file__)) + repo = os.path.dirname(os.path.dirname(os.path.dirname(here))) + self.assertEqual(patches.main(["x", os.path.join(repo, "llama", "patches")]), 0) + + def test_no_patches_found_is_a_failure(self): + with tempfile.TemporaryDirectory() as empty: + self.assertEqual(patches.main(["x", empty]), 2) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/check-patches.py b/.github/check-patches.py new file mode 100644 index 000000000..d00e88fd1 --- /dev/null +++ b/.github/check-patches.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# SPDX-FileCopyrightText: 2026 Bernard Ladenthin +# +# SPDX-License-Identifier: MIT OR Apache-2.0 +"""Fail when a patch declares a hunk size that does not match its body. + +See buildcheck/patches.py -- `git apply --check` does not catch this and a truncated file is a +successful apply as far as git is concerned. Usage: check-patches.py [] +""" + +import os +import sys + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +from buildcheck import patches # noqa: E402 + +if __name__ == "__main__": + sys.exit(patches.main(sys.argv)) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index ccd400894..6628950e1 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -292,6 +292,12 @@ jobs: # .github/buildcheck/natives.py; its unit tests run in the shared-files job. - name: Natives list check (everything that names a natives jar agrees with natives.csv) run: python3 .github/check-natives.py + # .github/buildcheck/patches.py. Static, so it runs here rather than after a build: a hunk + # header that under-counts its body makes `git apply` write a TRUNCATED file, which + # `git apply --check` accepts and verify-patches-applied.sh cannot see (the patch IS applied + # and the tree IS dirty). That cost one Publish run 23 red jobs. + - name: Patch hunk sizes (a miscounted header truncates the file git apply writes) + run: python3 .github/check-patches.py - name: Spotless check (fail fast on format violations) run: mvn -B --no-transfer-progress -f llama/pom.xml spotless:check - name: SpotBugs check (fail fast on static-analysis findings) diff --git a/CLAUDE.md b/CLAUDE.md index fc19406eb..c72e1ec74 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -163,6 +163,7 @@ shapes this repo writes and rejects anything else loudly. | `sharedfiles.py` | `check-shared-files.py` (`shared-files`) | the files kept byte-identical across the four sibling repos | | `versions.py` | `check-versions.py` (`shared-files`) | **warns** where a Maven dependency or plugin is used in another version than in a sibling repo | | `runscripts.py` | `check-run-scripts.py` (`shared-files`) | every `run:` script of the workflows and composite actions that runs in bash parses (`bash -n`) | +| `patches.py` | `check-patches.py` (`code-style`) | every hunk header in `llama/patches/` declares the size its body actually has -- a header that **under-counts** makes `git apply` write a TRUNCATED file, which `git apply --check` accepts and `verify-patches-applied.sh` cannot see (the patch *is* applied, the tree *is* dirty). That shipped once: a comment edit left `0017`'s new-file hunk at `+1,25` for 27 lines, so `prefetch.h` arrived without its closing brace and `quants.c` failed 19x with "function definition is not allowed here" on **every** platform -- 23 red jobs in one Publish run, after `--check` had reported the patch as fine. Static, so it runs in the first minutes rather than after a build | **The release gate.** A job nothing waits for can go red while a release still ships — the natives builds `package` once forgot to wait for, and the aarch64 fat jars that were signed and attached