From 00242a542ab97c6a847ed84f8653d1092c93c1fb Mon Sep 17 00:00:00 2001 From: Bernard Ladenthin Date: Fri, 9 Oct 2026 18:51:32 +0200 Subject: [PATCH] buildcheck: catch a patch hunk header that lies about its size The guard for the regression that put 23 jobs of one Publish run in the red, and the reason it got through: `git apply --check` validates that a hunk's CONTEXT applies, not that its declared line count matches its body. A header that under-counts therefore passes the check, and `git apply` writes only the declared number of lines -- silently truncating the file. Editing a comment inside the new file `patches/0017` adds left its header at `+1,25` for 27 lines, so prefetch.h arrived without its closing brace, every translation unit including it continued inside an unclosed function, and quants.c failed 19x with "function definition is not allowed here" on every platform. Nothing existing could see it. The applier treats a truncated file as a successful apply, and verify-patches-applied.sh asserts only that every patch is in the stamp and that the tree is dirty -- both true. So this is a STATIC check of the patch files, wired into `code-style` (the first minutes of a run) rather than after a build, and runnable before a push: python3 .github/check-patches.py The algorithm is the part worth reading, because two obvious forms of it are wrong and one of them accepts exactly this bug. Counting body lines until the next delimiter over-counts trailing context AND passes a header that under-counts. Only checking that a 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 messages say what `git apply` would do about it. 11 unit tests cover both directions, a wrong old-side count, the `@@ -5 +5 @@` single-line shorthand, "\ No newline at end of file" counting for neither side, prose and a git signature below the last hunk (this repository's patches carry long descriptions), and that every hunk of a multi-hunk patch is checked. One of them runs the check over the repository's own 11 patches, which pass. Full buildcheck suite: 114 tests green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01E2h8gXyyE5UeimkL9vQv9G --- .github/buildcheck/patches.py | 104 ++++++++++++++++++++ .github/buildcheck/tests/test_patches.py | 115 +++++++++++++++++++++++ .github/check-patches.py | 18 ++++ .github/workflows/publish.yml | 6 ++ CLAUDE.md | 1 + 5 files changed, 244 insertions(+) create mode 100644 .github/buildcheck/patches.py create mode 100644 .github/buildcheck/tests/test_patches.py create mode 100644 .github/check-patches.py 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