From bd9c3334c07d1f5047a17a7b560e2bdb78a349e2 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 18:51:17 -0400 Subject: [PATCH 1/4] scripts/pre-pr-checks.sh: run the six checks docs/pre-pr-checks.md lists, one line each engine#2311 puts the six host-side pre-merge checks on one page. Asking a lane to type six commands -- two of which need an argument, and one of which needs a ref that may not resolve -- is how one gets skipped: three stale-baseline incidents in two days, each a check that exists and was not run. ./scripts/pre-pr-checks.sh [--base origin/main] [--list] One line per check, an aggregate count, and a non-zero exit when any is red. A check that cannot run says 'skip' and why, and the summary keeps the count, because a skipped check is not a pass -- the same reason tmp_path_check prints its excluded files instead of dropping them silently. Measured on this tree: 6 green, 0 skipped. With one /tmp literal added to npu/forward/src/, rc 1 and TWO checks name it (copyright, tmp_path_check) -- 'one line per check' is what makes that visible rather than a paste someone skims. --base origin/nope reports 'skip ... does not resolve here' instead of a green registration row. --- scripts/pre-pr-checks.sh | 94 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100755 scripts/pre-pr-checks.sh diff --git a/scripts/pre-pr-checks.sh b/scripts/pre-pr-checks.sh new file mode 100755 index 000000000..857ec17df --- /dev/null +++ b/scripts/pre-pr-checks.sh @@ -0,0 +1,94 @@ +#!/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. +# +# pre-pr-checks.sh [--base REF] [--list] +# +# `docs/pre-pr-checks.md` (engine#2311) lists six host-side checks to run before asking for a merge. +# Asking a lane to type six commands, two of which need an argument, is how one gets skipped: three +# stale-baseline incidents in two days, each a check that exists and was not run. +# +# This runs all six and prints ONE line per check, so the answer is per-check rather than "the paste +# looked empty". A check that cannot run says `skip` and why -- a skipped check is not a pass, and the +# summary keeps the count so a green line cannot hide one (the reason `tools/ci/tmp_path_check.py` +# prints its excluded files instead of excluding them silently). +# +# Deliberate: a NEW file must be `git add`ed first. Three of these six read the index or the tracked +# tree, so an unadded file is invisible to them and the run reads green -- the same trap that made +# `tools/copyright.py --check` skip untracked files by design. +set -uo pipefail + +# usage(): the header comment as prose, anchored on the comment block rather than a line range -- two +# scripts in this repo drifted past the end of their header and printed code to whoever asked for +# help (engine#2129). +usage() { awk 'NR>1 && /^#/ {sub(/^# ?/,""); print; next} NR>1 {exit}' "$0"; } + +base=origin/main +list_only=0 +while [ $# -gt 0 ]; do + case "$1" in + --base) base=${2:-}; shift 2 || { usage >&2; exit 2; } ;; + --base=*) base=${1#--base=}; shift ;; + --list) list_only=1; shift ;; + -h|--help) usage; exit 0 ;; + *) echo "pre-pr-checks: unknown argument '$1'" >&2; usage >&2; exit 2 ;; + esac +done +[ -n "$base" ] || { echo "pre-pr-checks: --base needs a ref" >&2; exit 2; } + +# The checks, in the order docs/pre-pr-checks.md lists them: cheapest and most common first, the two +# that need a revision last. +checks=( + "tools/copyright.py --check" + "tools/ci/tests_referenced_check.py" + "tools/ci/ci_targets_check.py --check" + "tools/ci/tmp_path_check.py --check" + "tools/ci/submodule_pin_check.py --check" + "tools/ci/registration_loss_check.py --check --base $base" +) + +if [ "$list_only" = 1 ]; then + for c in "${checks[@]}"; do echo "python3 $c"; done + exit 0 +fi + +ok=0; failed=0; skipped=0; detail="" +for c in "${checks[@]}"; do + name=${c%% *} + # A check that needs a revision it cannot resolve is skipped WITH ITS REASON, not reported green. + if [ "$name" = "tools/ci/registration_loss_check.py" ] && ! git rev-parse --verify -q "$base" >/dev/null; then + printf 'skip %-42s %s does not resolve here\n' "$name" "$base" + skipped=$((skipped + 1)); continue + fi + out=$(python3 $c 2>&1); rc=$? + last=$(printf '%s\n' "$out" | grep -v '^$' | tail -1) + if [ $rc -eq 0 ]; then + printf 'ok %-42s %s\n' "$name" "$last" + ok=$((ok + 1)) + else + printf 'FAIL %-42s %s\n' "$name" "$last" + failed=$((failed + 1)) + detail="$detail$name: $last"$'\n' + fi +done + +echo +if [ $failed -gt 0 ]; then + echo "pre-pr-checks: $failed failed, $ok ok, $skipped skipped" + printf '%s' "$detail" + exit 1 +fi +echo "pre-pr-checks: all $ok check(s) green, $skipped skipped" +exit 0 From d33c0c64450b707029c26b55e85d2ad1a35c5ac5 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 19:11:32 -0400 Subject: [PATCH 2/4] pre-pr-checks: a check that compared nothing is a skip, not ok Review feedback, and the reviewer is right: submodule_pin_check --check exits 0 having compared NOTHING when submodules are declared but none is checked out, so the runner printed 'ok' over a check that ran no comparison -- the exact shape this script exists to stop. Matched on the tool's own summary (0 submodule(s) at their pinned commit, N not initialised), not on a re-derivation. Its other nothing-to-compare case, 'no submodules reported - cannot tell, and that is not a pass', already exits 2 and is left to FAIL: the tool refuses to call that a pass, so the runner must not either. Verified against both literals: the 0-at-pin line maps to skip, the 1-at-pin line stays ok, and a tree with no checked-out submodules reports FAIL from the tool's own rc 2. --- scripts/pre-pr-checks.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/scripts/pre-pr-checks.sh b/scripts/pre-pr-checks.sh index 857ec17df..e2ab19d89 100755 --- a/scripts/pre-pr-checks.sh +++ b/scripts/pre-pr-checks.sh @@ -75,6 +75,18 @@ for c in "${checks[@]}"; do out=$(python3 $c 2>&1); rc=$? last=$(printf '%s\n' "$out" | grep -v '^$' | tail -1) if [ $rc -eq 0 ]; then + # `submodule_pin_check --check` exits 0 having compared NOTHING when submodules are declared but + # none is checked out -- `0 submodule(s) at their pinned commit, N not initialised` -- which would + # print `ok` for a check that ran no comparison, the shape this script exists to stop. Matched on + # the tool's own summary rather than re-deriving the condition, because the tool is the thing that + # knows. Its OTHER "nothing to compare" case, `no submodules reported - cannot tell, and that is not + # a pass`, exits 2 and is left to fail: the tool already refuses to call that a pass. + # `--require-initialised` is where a release build turns this one into a failure. + case "$last" in + "submodule_pin: 0 submodule(s) at their pinned commit, "*) + printf 'skip %-42s nothing compared: %s\n' "$name" "${last#submodule_pin: }" + skipped=$((skipped + 1)); continue ;; + esac printf 'ok %-42s %s\n' "$name" "$last" ok=$((ok + 1)) else From 9e46859c90c6c60228187d64f13970111659befe Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Fri, 13 Nov 2026 02:41:09 -0400 Subject: [PATCH 3/4] pre-pr-checks: run from the toplevel, and a FAIL prints the whole output Review of #2313, three items, all of them mine: 1. From scripts/ the six commands were run with relative paths, so the run produced twelve 'can't open file' lines and rc 1 with every row red for the wrong reason. The script now resolves and enters the toplevel first (rc 2 if it is not in a work tree). 2. On a FAIL the detail block printed only the LAST line of the tool's output. That is where the wrong cause comes from: submodule_pin_check prints 'NOT the commit this tree pins' above a summary that only counts, so the last line names the count and not the commit. The block now prints the whole output -- verified with an injected copyright failure, where the repair hint (run: python3 tools/copyright.py --fix) is now visible and used to be dropped. Verified: from scripts/, rc 0, 0 lines of 'cannot open file', 6 green. With two injected failures, rc 1, both FAIL rows named, and each one's full output under it. The third item (a submodule at HEAD~1 must print 'NOT the commit this tree pins') I could not run: this worktree has no initialised third_party/llama.cpp, so that tool reports 'not initialised' rather than comparing. The mechanism the item needs is item 2's -- the detail line is now included -- and it is verified on a failure that does print one. --- scripts/pre-pr-checks.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/scripts/pre-pr-checks.sh b/scripts/pre-pr-checks.sh index e2ab19d89..282391d49 100755 --- a/scripts/pre-pr-checks.sh +++ b/scripts/pre-pr-checks.sh @@ -48,6 +48,11 @@ while [ $# -gt 0 ]; do done [ -n "$base" ] || { echo "pre-pr-checks: --base needs a ref" >&2; exit 2; } +# Run from the toplevel: the six commands name `tools/...` and `config/...` relative to it, so a caller +# in scripts/ got twelve `can't open file` lines and rc 1 with every row red for the wrong reason. +top=$(git rev-parse --show-toplevel 2>/dev/null) || { echo "pre-pr-checks: not inside a git work tree" >&2; exit 2; } +cd "$top" || { echo "pre-pr-checks: cannot enter $top" >&2; exit 2; } + # The checks, in the order docs/pre-pr-checks.md lists them: cheapest and most common first, the two # that need a revision last. checks=( @@ -92,7 +97,11 @@ for c in "${checks[@]}"; do else printf 'FAIL %-42s %s\n' "$name" "$last" failed=$((failed + 1)) - detail="$detail$name: $last"$'\n' + # The whole output, not its last line: the case that matters is a checker whose summary is + # generic and whose detail line names the commit or file (`submodule_pin_check` prints "NOT the + # commit this tree pins" above a summary that only counts), and the last line sends the reader + # to the wrong place. + detail="$detail$name:"$'\n'"$out"$'\n' fi done From 94fdc54a0ae6c559104d0bec3c6c0549a9f17a04 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Fri, 13 Nov 2026 02:50:44 -0400 Subject: [PATCH 4/4] pre-pr-checks: an unresolvable --base is a usage error, not a skip Review of #2313, and the reviewer is right about the distinction I had drawn wrong: a SKIP is for a check that cannot run in this environment (no submodules, no device), while an argument the caller passed and that cannot be resolved is the caller's to fix. The old behaviour printed 'all 5 check(s) green, 1 skipped' and exited 0 for --base origin/nope, which is the shape this script exists to stop. --base is now verified with git rev-parse --verify '$base^{commit}' before any check runs: rc 2, the ref named, and the reason (the registration check compares against it). The in-loop skip for that one case is removed rather than left unreachable. Verified: --base origin/nope -> rc 2 with the message; --base origin/main^{tree} -> rc 2 (a tree is not a commit); normal run rc 0, 6 green, 0 skipped; and from scripts/ still rc 0. --- scripts/pre-pr-checks.sh | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/scripts/pre-pr-checks.sh b/scripts/pre-pr-checks.sh index 282391d49..f18190d6e 100755 --- a/scripts/pre-pr-checks.sh +++ b/scripts/pre-pr-checks.sh @@ -53,6 +53,16 @@ done top=$(git rev-parse --show-toplevel 2>/dev/null) || { echo "pre-pr-checks: not inside a git work tree" >&2; exit 2; } cd "$top" || { echo "pre-pr-checks: cannot enter $top" >&2; exit 2; } +# A `--base` that does not resolve is a USAGE error, not a skip: the caller asked for a comparison that +# cannot happen, and a run that prints "all 5 check(s) green, 1 skipped" then exits 0 is the shape this +# script exists to stop (review of engine#2313). A skip is for a check that cannot run in this +# environment; an argument that cannot be resolved is the caller's to fix. +git rev-parse --verify -q "$base^{commit}" >/dev/null || { + echo "pre-pr-checks: --base '$base' does not resolve to a commit here" >&2 + echo " the registration check compares against it; pass a ref that exists (default: origin/main)" >&2 + exit 2 +} + # The checks, in the order docs/pre-pr-checks.md lists them: cheapest and most common first, the two # that need a revision last. checks=( @@ -72,11 +82,6 @@ fi ok=0; failed=0; skipped=0; detail="" for c in "${checks[@]}"; do name=${c%% *} - # A check that needs a revision it cannot resolve is skipped WITH ITS REASON, not reported green. - if [ "$name" = "tools/ci/registration_loss_check.py" ] && ! git rev-parse --verify -q "$base" >/dev/null; then - printf 'skip %-42s %s does not resolve here\n' "$name" "$base" - skipped=$((skipped + 1)); continue - fi out=$(python3 $c 2>&1); rc=$? last=$(printf '%s\n' "$out" | grep -v '^$' | tail -1) if [ $rc -eq 0 ]; then