submodule_pin: a submodule at its pin with a dirty work tree passes as 'at their pinned commit' (#2129)

cmake/tests-submodule-pin.cmake

@@ -25,3 +25,8 @@
 # check's business); `--require-initialised` is the release-gate form, where a missing checkout is a failure too.
 add_python_test(submodule_pin_check ${CMAKE_SOURCE_DIR}/tools/ci/submodule_pin_check.py --check --root ${CMAKE_SOURCE_DIR})
 add_python_test(submodule_pin_check_self ${CMAKE_SOURCE_DIR}/tools/ci/submodule_pin_check.py --self-test)
+
+# engine#2129: the release preflight's use of the check -- a build tree whose submodules are not the pinned commits
+# BLOCKS the run, and "no checkout to compare" only WARNs so a fresh clone can still build. The function is cut out of
+# scripts/weekly.sh and run against stub checkers: no submodules, no network, no build.
+add_test(NAME weekly_submodule_preflight COMMAND ${CMAKE_SOURCE_DIR}/tests/weekly_submodule_preflight_test.sh)

scripts/weekly.sh

@@ -118,6 +118,11 @@ host_desc() {
 
 # --- build / test -------------------------------------------------------------------------
 build() {
+    # engine#2129: what this builds is $SRC, whose submodules `checkout` has just updated, and a build compiles whatever
+    # those work trees hold. This cannot live in preflight(): preflight runs before $SRC exists, so the check would find
+    # no file to run, skip, and report success -- a check that inspects nothing and says it passed. Here it is the tree
+    # that is about to be compiled, and the run stops before the first compile instead of after it.
+    preflight_submodules "$SRC/tools/ci/submodule_pin_check.py" "$SRC" || return 1
     # the private components compiled in (docs/releases.md, "Private components"): the NPU runtime
     # (into 1bit) and the HIP kernels (into the HRX llama.cpp), each a checkout at the commit its
     # config/*.config pins; the build stops here when one cannot be fetched at that commit
@@ -658,6 +663,33 @@ preflight_runner_units() {
     return 0
 }
 
+# The submodule row of the build (engine#2129). It runs at the top of build(), not in preflight(): the tree to inspect is
+# $SRC, which `checkout` creates, and preflight runs before that. A
+# build compiles whatever the submodules' work trees hold: tools/ci/submodule_pin_check.py is the check for whether those
+# checkouts ARE the commits this tree pins. It already exists as a ctest row, and a ctest row runs in CI -- nothing ran it
+# before a build on the box, which is where the drift was measured (7 of 7 submodules off-pin). Exit 1 stops the run: the
+# build would not be of the pinned sources. Exit 2 (nothing to compare -- no submodules initialised) only WARNS, because a
+# fresh clone must still be able to build and the warning says the check inspected nothing rather than implying a pass.
+# --require-clean is deliberately NOT passed here: it fails on a dirty submodule work tree, and a deliberate local patch
+# is a real thing a box does; the check reports those with their counts either way.
+# A function of its own so tests/weekly_submodule_preflight_test.sh can run it against a stub.
+preflight_submodules() {
+    local check=$1 src=$2 out rc=0
+    # An older ref does not carry the tool. That is not a failure, but it is not a pass either: say that the tree was not
+    # inspected, because a silent 0 here is a check that reports success for doing nothing.
+    [ -f "$check" ] || { echo "preflight: WARNING, not blocking: $src has no tools/ci/submodule_pin_check.py (a ref older than engine#2129), so submodule drift is not checked for this build"; return 0; }
+    out=$(python3 "$check" --check --root "$src" 2>&1) || rc=$?
+    case "$rc" in
+        0) printf '%s\n' "$out" | grep -v 'at their pinned commit' | sed 's/^/preflight: /' ;;
+        2) printf '%s\n' "$out" | sed 's/^/preflight: /'
+           echo "preflight: WARNING, not blocking: no submodule checkout to compare against the pin, so this build follows whatever the work trees hold (tools/ci/submodule_pin_check.py exit 2; git submodule update --init fills it)" ;;
+        *) printf '%s\n' "$out" | sed 's/^/preflight: /'
+           echo "preflight: a submodule is not at the commit this tree pins, so the build would not be of the pinned sources (tools/ci/submodule_pin_check.py exit $rc; the line above names the submodule and what to do)"
+           return 1 ;;
+    esac
+    return 0
+}
+
 # --- preflight ----------------------------------------------------------------------------
 # What the stages need from the machine, checked before the first one runs, so that a missing
 # tool stops the run at the start rather than after an hour of build (2026-10-19 rehearsal).

tests/weekly_submodule_preflight_test.sh

@@ -0,0 +1,82 @@
+#!/usr/bin/env bash
+# Copyright 2026 bong-water-water-bong
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# weekly_submodule_preflight_test.sh: the release preflight BLOCKS when a submodule is not the commit the tree pins and
+# only WARNS when there is no checkout to compare against (engine#2129). The function is cut out of scripts/weekly.sh and
+# run against stub checkers, so no submodules, no network and no build.
+set -u
+root=$(cd "$(dirname "$0")/.." && pwd)
+tmp=$(mktemp -d); trap 'rm -rf "$tmp"' EXIT
+fail=0
+ok()  { printf 'ok   %s\n' "$1"; }
+bad() { printf 'FAIL %s\n' "$1"; fail=1; }
+
+sed -n '/^preflight_submodules() {$/,/^}$/p' "$root/scripts/weekly.sh" > "$tmp/fn.sh"
+grep -q 'submodule_pin_check.py exit' "$tmp/fn.sh" || { bad "could not cut preflight_submodules() out of scripts/weekly.sh"; echo "weekly_submodule_preflight: FAIL"; exit 1; }
+# WHERE the call is, not merely that it exists: this gate was a silent no-op once because it lived in preflight(), which runs
+# before `checkout` creates $SRC -- the file was not there to run, the function returned 0, and a green run meant nothing. A
+# "present anywhere in weekly.sh" assertion passes in exactly that state, so pin the function it belongs to.
+sed -n '/^build() {$/,/^}$/p' "$root/scripts/weekly.sh" > "$tmp/build_fn.sh"
+sed -n '/^preflight() {$/,/^}$/p' "$root/scripts/weekly.sh" > "$tmp/preflight_fn.sh"
+grep -q 'preflight_submodules "' "$tmp/build_fn.sh" || bad "scripts/weekly.sh's build() no longer runs the submodule gate (a call elsewhere is the no-op this test exists for)"
+if grep -q 'preflight_submodules "' "$tmp/preflight_fn.sh"; then bad "the submodule gate is inside preflight(), which runs before \$SRC exists: it would inspect nothing and report success"; else ok "the submodule gate is in build(), where \$SRC exists"; fi
+say() { echo "$*"; }
+# shellcheck disable=SC1091
+. "$tmp/fn.sh"
+
+# A stub standing in for tools/ci/submodule_pin_check.py. It is a Python file because the function runs the real tool as
+# `python3 <path>`: printing what the tool prints and exiting with the code under test is the whole stub.
+stub() {  # <exit code> <line>
+    {
+        printf 'import sys\nprint('
+        printf '%s' "$2" | sed 's/.*/"&"/'
+        printf ')\nsys.exit(%s)\n' "$1"
+    } > "$tmp/stub.py"
+    echo "$tmp/stub.py"
+}
+row() {  # <name> <want return> <want text> <rc> <line>
+    local name=$1 want=$2 text=$3 rc=$4 line=$5
+    local check out got=0
+    check=$(stub "$rc" "$line")
+    out=$(preflight_submodules "$check" "$tmp" 2>&1) || got=$?
+    if [ "$got" = "$want" ] && printf '%s\n' "$out" | grep -q -- "$text"; then
+        ok "preflight: $name -> return $got"
+    else
+        bad "preflight: $name: want return $want with '$text', got $got: $out"
+    fi
+}
+
+# A passing run prints nothing at all: the tool's summary line ("N submodule(s) at their pinned commit") is the one
+# thing the function drops, because a preflight that always prints a line trains its reader to skip the lines.
+check=$(stub 0 "submodule_pin: 7 submodule(s) at their pinned commit")
+out=$(preflight_submodules "$check" "$tmp" 2>&1)
+if [ -z "$out" ]; then ok "a tree whose submodules are their pins passes silently"; else bad "a passing run should print nothing, got: $out"; fi
+row "a submodule at a different commit BLOCKS the run" 1 "not at the commit this tree pins" 1 "submodule_pin: third_party/x is checked out at abc, which is NOT the commit this tree pins"
+row "no checkout to compare only WARNS" 0 "WARNING, not blocking" 2 "submodule_pin: no submodules reported - cannot tell, and that is not a pass"
+
+# A dirty submodule at its pin is reported and does NOT block: a deliberate local patch is a real thing a box does.
+row "a dirty submodule at its pin is reported, not blocking" 0 "changed path" 0 "submodule_pin: third_party/x is at the pinned abc but its work tree has 301 changed path(s)"
+
+# A checker that is not there at all (an older ref): not a failure, but it must SAY that nothing was inspected --
+# a silent pass here is a check that reports success for doing nothing, which is what its first version did.
+out=$(preflight_submodules "$tmp/no-such-check.py" "$tmp" 2>&1) &&     printf '%s' "$out" | grep -q 'is not checked for this build' && ok "a missing checker warns and does not stop the run"     || bad "a missing checker should warn (and not stop the run): $out"
+
+# The asymmetry, in one assertion: exit 2 must not block, exit 1 must.
+check=$(stub 2 "submodule_pin: cannot tell")
+if preflight_submodules "$check" "$tmp" >/dev/null 2>&1; then ok "exit 2 is not a block"; else bad "exit 2 should not block the release"; fi
+
+if [ "$fail" != 0 ]; then echo "weekly_submodule_preflight: FAIL"; exit 1; fi
+echo "weekly_submodule_preflight: ok"

tools/ci/submodule_pin_check.py

@@ -33,8 +33,13 @@
          missing the source or using a system copy)             -> fail with --require-initialised
     'U'  merge conflicts in the submodule                       -> fail
 
+`git submodule status` cannot report one more state a build cares about: a submodule that IS at the pinned commit and
+has uncommitted changes in its own work tree. It prints ' ' for that, and a build compiles the uncommitted files. On
+2026-11-11 the same box that motivated this check had 301 such files in `third_party/llama.cpp`. Those are reported
+always, and --require-clean turns them into a failure.
+
 Usage:
-  tools/ci/submodule_pin_check.py --check [--root .] [--require-initialised]
+  tools/ci/submodule_pin_check.py --check [--root .] [--require-initialised] [--require-clean]
   tools/ci/submodule_pin_check.py --self-test
 """
 from __future__ import annotations
@@ -65,14 +70,30 @@ def parse_status(text: str) -> list[tuple[str, str, str, str]]:
     return rows
 
 
-def verdict(rows: list[tuple[str, str, str, str]], require_initialised: bool) -> tuple[int, list[str]]:
-    """(rc, messages). rc 0 pass, 1 a submodule is not at its pin, 2 nothing readable."""
+def verdict(rows: list[tuple[str, str, str, str]], require_initialised: bool,
+            dirty: dict[str, int] | None = None, require_clean: bool = False) -> tuple[int, list[str]]:
+    """(rc, messages). rc 0 pass, 1 a submodule is not at its pin, 2 nothing readable.
+
+    `dirty` maps a path to the number of paths its work tree has changed (git status --porcelain). A submodule at its
+    pin with a dirty work tree compiles code the pin does not name; it is always reported and fails only under
+    --require-clean, because a deliberate local patch is a real thing a box does and the fix for it (move the pin) is
+    not always the caller's to make."""
     if not rows:
         return 2, ["submodule_pin: no submodules reported - cannot tell, and that is not a pass"]
     out: list[str] = []
-    bad = unknown = uninit = 0
+    bad = unknown = uninit = dirty_at_pin = 0
     for flag, sha, path, describe in rows:
         if flag == " ":
+            n = (dirty or {}).get(path, 0)
+            if n:
+                dirty_at_pin += 1
+                out.append(
+                    f"submodule_pin: {path} is at the pinned {sha[:12]} but its work tree has {n} changed "
+                    f"path(s), so the build compiles code the pin does not name. Repair: git -C {path} stash, or "
+                    f"move the pin if the change is meant to stay (git add {path})"
+                )
+                if require_clean:
+                    bad += 1
             continue
         if flag == "?":
             unknown += 1
@@ -102,6 +123,8 @@ def verdict(rows: list[tuple[str, str, str, str]], require_initialised: bool) ->
         return 1, out
     if unknown and not (len(rows) - unknown):
         return 2, out
+    if dirty_at_pin and not require_clean:
+        out.append(f"submodule_pin: WARNING, not a failure: {dirty_at_pin} submodule(s) at their pin with a dirty work tree (--require-clean makes this an error)")
     out.append(
         f"submodule_pin: {len(rows) - bad - unknown - uninit} submodule(s) at their pinned commit"
         + (f", {uninit} not initialised" if uninit else "")
@@ -123,6 +146,28 @@ def run_git(root: pathlib.Path) -> tuple[str, int]:
     return p.stdout, 0
 
 
+def run_dirty(root: pathlib.Path, rows: list[tuple[str, str, str, str]]) -> dict[str, int]:
+    """{path: changed-path count} for the submodules that ARE at their pin.
+
+    Only those: a '+' or 'U' row already fails and its dirty count would be noise. `git status --porcelain` inside the
+    submodule is the only thing that reports this state - `git submodule status` prints ' ' for it.
+    """
+    out: dict[str, int] = {}
+    for flag, _sha, path, _describe in rows:
+        if flag != " ":
+            continue
+        try:
+            p = subprocess.run(["git", "-C", str(root / path), "status", "--porcelain"],
+                               capture_output=True, text=True, check=False)
+        except OSError:
+            continue
+        if p.returncode == 0:
+            n = len([l for l in p.stdout.splitlines() if l.strip()])
+            if n:
+                out[path] = n
+    return out
+
+
 def main(argv: list[str]) -> int:
     ap = argparse.ArgumentParser(description=__doc__.splitlines()[0])
     g = ap.add_mutually_exclusive_group(required=True)
@@ -134,6 +179,11 @@ def main(argv: list[str]) -> int:
         action="store_true",
         help="treat a submodule that is not checked out as a failure (a release build should use this)",
     )
+    ap.add_argument(
+        "--require-clean",
+        action="store_true",
+        help="treat a submodule that is at its pin with a dirty work tree as a failure (CI uses this)",
+    )
     args = ap.parse_args(argv)
 
     if args.self_test:
@@ -143,7 +193,9 @@ def main(argv: list[str]) -> int:
     if rc != 0:
         print(f"submodule_pin: cannot tell: {text}")
         return 2
-    rc, messages = verdict(parse_status(text), args.require_initialised)
+    rows = parse_status(text)
+    rc, messages = verdict(rows, args.require_initialised, run_dirty(pathlib.Path(args.root).resolve(), rows),
+                           args.require_clean)
     for m in messages:
         print(m)
     return rc
@@ -189,6 +241,18 @@ def check(ok: bool, what: str) -> None:
         any("not initialised" in m for m in verdict(parse_status(uninit), True)[1]),
         "... and so does the uninitialised one",
     )
+    # engine#2129, the half `git submodule status` cannot report: at the pin, dirty work tree
+    check(verdict(parse_status(at_pin), False, {"third_party/llama.cpp": 301})[0] == 0,
+          "at the pin with 301 dirty paths -> pass by default (a deliberate local patch is a real thing)")
+    check(any("301 changed" in m for m in verdict(parse_status(at_pin), False, {"third_party/llama.cpp": 301})[1]),
+          "... but it IS reported, with its count")
+    check(verdict(parse_status(at_pin), False, {"third_party/llama.cpp": 301}, True)[0] == 1,
+          "... and FAILS with --require-clean")
+    check(verdict(parse_status(at_pin), False, {}, True)[0] == 0,
+          "a clean submodule at its pin still passes with --require-clean")
+    check(not any("changed" in m for m in verdict(parse_status(ahead), False, {"third_party/llama.cpp": 5})[1]),
+          "a dirty '+' row is not double-counted (the commit is the failure there)")
+
     # one bad among several still fails: the check is not 'all lines' or 'the first line'
     check(verdict(parse_status(at_pin + ahead + at_pin), False)[0] == 1,
           "one '+' among three lines still fails")