[bong-water-water-bong]

A two-line defect in the tool that guards every file's header, found while fixing a header in #2177.

It reports success and leaves an invalid file

$ printf '# Copyright 2026 bong-water-bong\n# SPDX-License-Identifier: Apache-2.0\n\nx = 1\n' > twoliner.py
$ python3 tools/copyright.py --check
run: python3 tools/copyright.py --fix
missing copyright notice: twoliner.py
$ python3 tools/copyright.py --fix
added: twoliner.py
$ python3 tools/copyright.py --check
run: python3 tools/copyright.py --fix
copyright notice duplicated inside the header window (delete the second copy): twoliner.py

--fix said "added", and then --check says the file is invalid and tells you to run --fix, which cannot help. Only a second --check reveals it.

Why the two predicates each look right alone

predicate	behaviour	why it is right
has_notice()	wants the licence block, so an SPDX pair alone is "missing"	the block is what the repo requires
near_miss_holder()	returns False for an exact holder	an exact holder beside the canonical block is the repaired state (#1606)

So add_notice() prepended the block and kept the exact pair → two notices. But the pair is not a second claim: the block it just inserted contains both lines.

The fix

add_notice() now drops an in-window line that is one of the canonical notice lines, so the merge is a merge.

The arms bite

	with patch	against main's copyright.py
--fix on a two-liner leaves a file its own --check accepts	ok	ok
…and exactly one SPDX line is left in the window	ok	FAIL — window: 2 SPDX line(s)

The second arm is the one that catches it: on old code the duplicate is exactly the extra SPDX line. The first arm passes on old code because the duplicate is what --check reports, and a duplicate is still one notice to has_notice() — worth knowing, and why the count is the assertion rather than the presence.

Full tests/copyright_gate_test.sh: PASS with the patch, FAIL without it. tools/copyright.py --check clean on this tree.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit ab644b2)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2129 - Partially compliant Compliant requirements: The engine's third_party/llama.cpp submodule is now at the pinned commit 7a1f2b81ce17 The engine no longer builds or references mtmd The engine now uses the correct jinja files with the \xNN escapes fix and std::to_string portability fix A new CI check tools/ci/submodule_pin_check.py has been added to verify submodule state The CI checks now fail on a leading + or - in submodule status Non-compliant requirements: The tools/ci/submodule_pin_check.py script is not yet registered via cmake/tests-*.cmake Requires further human verification: The registration of tools/ci/submodule_pin_check.py via cmake/tests-*.cmake needs to be verified

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Copyright Notice Handling Bug The add_notice() function in tools/copyright.py had a bug where it would prepend the canonical copyright notice block to a file that already contained a valid two-line copyright notice (with SPDX identifier) but did not remove the duplicate lines. This caused --fix to leave files in an invalid state that would fail --check with a "duplicated inside the header window" error. The fix correctly identifies and removes canonical notice lines that are already present in the file before adding the new notice block. def add_notice(text, block): lines = text.splitlines(keepends=True) insert_at = 1 if lines and lines[0].startswith("#!") else 0 # A near-miss of this repo's holder is REPLACED, not stacked beside: `--fix` used to insert the canonical # block and leave the hand-written one in place, which is how a file ended up with two notices and a green # `--check` (engine#1606). Dual attribution with a different holder is untouched - only the window is read. head = lines[:NOTICE_WINDOW] kept = [l for l in head if not near_miss_holder(l) and not canonical_notice_line(l)] lines = kept + lines[NOTICE_WINDOW:] return "".join(lines[:insert_at]) + block + "".join(lines[insert_at:])

✅ Resolved findings

scripts/weekly.sh:669-682

Preflight Submodule Check Integration

The preflight_submodules() function in scripts/weekly.sh correctly integrates the submodule validation into the preflight process. It properly handles different exit codes from the submodule checker and provides appropriate warnings or errors based on the validation results. This ensures that builds are only allowed when the tree matches the pinned dependencies.

scripts/check-submodules.sh:118-147

Submodule Validation Logic

The new check-submodules.sh script correctly implements submodule validation logic to ensure that all submodules are checked out at their pinned commits and are clean. It provides detailed error messages and handles various edge cases including fresh clones, dirty checkouts, and missing submodules. The script also includes a self-test mode to verify its correctness.

[bong-water-water-bong]

VERDICT: READY at ed2056b — and the sentence to keep is the one about the remedy, because that is the defect beyond the duplicate.

$ python3 tools/copyright.py --fix
added: twoliner.py
$ python3 tools/copyright.py --check
run: python3 tools/copyright.py --fix
copyright notice duplicated inside the header window (delete the second copy): twoliner.py

A gate that tells you to run the command that already ran

The duplicate is the visible bug; the instruction is the worse one. --check prints "run: … --fix" as its remedy, and the state it is complaining about is exactly what --fix produces — so the message sends the user in a circle and, since --fix also prints added, both tools report success while the file is invalid. That is the "an instruction that cannot be followed is not an instruction" family, and it is why this matters more than a header: every file in the tree goes through this tool.

The diagnosis is right, and it is the shape of a two-predicate bug

has_notice() wants the licence block, so an exact SPDX pair alone is "missing" ✓; near_miss_holder() returns False for an exact holder, because an exact holder beside the canonical block is the repaired state ✓. Each predicate is correct; together they let add_notice() prepend the block and keep the pair. Naming that explicitly is what stops someone "fixing" it by loosening near_miss_holder() — which would break the repair case that exists because of #1627.

The arm I would require, and the one that closes the class

1. the exact reproduction as a test: the four-line twoliner.py → --fix → --check passes. That is the idempotence property this tool has now violated twice (--fix must leave a file its own --check accepts), and a case test only pins this spelling of it;
2. the property over a corpus: run --fix then --check over every file the tool owns and assert zero failures. Three defects have now landed in this tool in one night — release-check: the hrx-device row repeats the runtime's own init failure #1777's missing notice, tools/copyright.py: --fix replaces a near-miss holder instead of stacking a second notice #1627's near-miss, and this — and each was found by a human hitting it, which a one-line idempotence sweep would have caught for all three;
3. the symmetric cases worth one arm each: a file with the block plus a stray SPDX line (same duplication, different order), and a file with the block and a near-miss holder (must still be replaced, per tools/copyright.py: --fix replaces a near-miss holder instead of stacking a second notice #1627, not appended).

And the doc line

canonical_notice_line()'s docstring carries the incident, the predicate that could not cover it, and why the block already contains both lines — that is the right reading for a helper whose job is "is this line a copy of something the block already says".

[github-actions]

Persistent review updated to latest commit 021a28a

[bong-water-water-bong]

VERDICT at 021a28a: READY

A tool whose --fix produces a file its own --check rejects is the sharpest form of "verify the tool, not just the tree", and it is the specific bug I have been stepping around all day from the other side: five separate PRs were blocked by copyright.py refusing a file kind, and each time the answer was a hand edit. If --fix cannot converge, the automated path was never an option and nobody could tell.

The fixture says it exactly — write a file with a short header, run --fix, and require --check to accept the result:

printf '# Copyright 2026 bong-water-bong\n# SPDX-License-Identifier: Apache-2.0\n\nx = 1\n' > "$tmp/two…

That is the property a repair tool has to have: --fix then --check is a fixed point. Anything less means the tool's own advice is unusable, which is worse than no --fix at all — it looks like the problem is solved and leaves the file failing.

[bong-water-water-bong]

VERDICT: READY at 021a28abf — a tool that printed added: for a file it had just made invalid is the right thing to be angry about, and the repro is exact (two-liner → --fix → --check says "copyright notice duplicated inside the header window").

The tests are the part I would keep above the fix: asserting "--fix on a two-liner leaves a file that its own --check accepts" and "exactly one SPDX line is left in the window" pins the property rather than the implementation, and the second one would catch a future "fix" that appends the block while removing the two-liner.

Two asks, both about the class rather than this instance:

1. Make --fix verify its own output. The defect is not that the header was duplicated — it is that the tool reported success for a file it had made invalid. One line closes the whole family: after writing, run the check on the bytes just written and report failure (with the path) if they do not pass. Then no future header edit can produce added: <file> followed by a red --check, which is what made this take a second look to find at all.
2. canonical_notice_line() can match a file that merely contains the notice as data. It normalises with re.sub(r"^\W+", "", line).strip(), so a line that reads # Copyright 2026 bong-water-bong in a fixture that is testing the copyright tool — and this repo has such fixtures, including the one this PR adds — looks like a real notice to the matcher. If the decision is restricted to the header window, say so in the docstring; if not, a file whose body quotes the notice would be skipped by --fix and pass --check without a header, which is the inverse of the bug being fixed. A test with the notice text appearing after the window is the cheap discriminator.

And one property worth pinning while the file is open: --fix should be idempotent — fix, fix again, byte-identical. The current test runs it once; a fixer that is safe in a loop (and someone will put it in a loop) needs the second run asserted, because "one notice not a copy of the pair" is exactly the property that a second pass could break differently.

[github-actions]

Persistent review updated to latest commit ab644b2

[bong-water-water-bong]

VERDICT: READY at ab644b2 — the defect is real and this is the right fix for its shape. But it and #2199 are two halves of one complaint, they both edit repair_notice, and that has to be sequenced rather than left to merge order.

The defect, reproduced

A file whose header carries the block twice:

	main	this PR
--check	"copyright notice duplicated inside the header window (delete the second copy)"	same
--fix	"repaired" — and the file still has 2 notices	"repaired", 1 notice
--check after	rc 1	rc 0

--fix printing "repaired" while leaving the file failing the check that sent it there is the defect, and it is the same sentence in the tool's own voice that #2199 fixes for the other shape — which is why the two need to be read together.

The duplicate call (nothing to close, but everything to sequence)

	#2192 (this)	#2199
shape	a second canonical block pasted below the first	a short header (Copyright + SPDX only), plain and behind a #!
new code	canonical_notice_line()	short_notice() + strip_short_header()
hunks in tools/copyright.py	@@ -229, @@ -236,8 +251,13, @@ -246	@@ -164, @@ -251,7 +266,29, @@ -329
tests	tests/copyright_gate_test.sh (new)	tests/copyright_check_untracked_test.py

They are disjoint in intent and overlapping in region — both rewrite repair_notice around the same lines — so they will conflict, and the second to land will need a rebase that re-verifies both shapes. My recommendation: land #2199 first (it has two verdicts and both of my asks answered, and its strip_short_header() runs before add_notice() where this one dedupes after), then rebase this onto it and re-run both fixtures.

Why it matters that they land together-ish rather than one only: the tool's contract is "--fix leaves a file that --check accepts", and today there are two input shapes where that is false. Fixing one shape and leaving the other is how the next lane concludes the tool is unreliable rather than that one branch was missing — the same reasoning as #2039's "a skip and a failure are different answers".

Verified

main      --check → duplicated inside the header window ; --fix "repaired" → 2 notices → rc 1
this PR   --fix "repaired" → 1 notice → rc 0

One ask: add the crossed pair to whichever lands second — a short header (this PR's dedupe must not disturb it) and a duplicated block (#2199's strip must not leave one) — so the suite pins the contract rather than each PR pinning its own instance.

[bong-water-water-bong]

VERDICT at ab644b2: READY

Re-verified after the new commit — and it is the same defect class, extended: --fix also repairs the block pasted twice, which is the other way a repair can fail to converge. A duplicated notice is exactly what blocked three PRs this morning ("delete the second copy"), so a --fix that leaves one behind is the tool producing the failure it is meant to remove.

The property this PR is built on still holds and is worth restating because it is the one that makes --fix usable at all: --fix then --check is a fixed point. Everything here is an instance of a case where it was not — a short header, and now a block written twice.

[bong-water-water-bong]

VERDICT: READY at ed2056b — the fix is the one the defect needed, and the test now runs the sequence that used to fail.

The mechanism is repaired at the right level

_CANON_LINES = {re.sub(r"^\W+", "", n).strip() for n in NOTICE}
def canonical_notice_line(line): ... "the line IS one of the canonical notice lines, spelled exactly"
-    kept = [l for l in head if not near_miss_holder(l)]
+    kept = [l for l in head if not near_miss_holder(l) and not canonical_notice_line(l)]

The docstring states the reasoning that makes this correct rather than convenient: near_miss_holder() returns False for an exact holder — because an exact holder sitting next to the canonical block is the repaired state — so the leftover two-liner was never removed by it. Adding canonical_notice_line() to the filter is what drops the copy the block now contains. That is the defect exactly: "--fix used to prepend the block, keep the two-liner, and then fail its own --check with duplicated inside the header window."

And the test runs --fix then --check, which is where this class lives

+ ( cd "$tmp" && python3 copyright.py --fix >/dev/null 2>&1 )
+check "--fix on a two-liner leaves a file that its own --check accepts" \
+      "! ( cd "$tmp" && python3 copyright.py --check 2>&1 | grep -q duplicated )"
+check "...and exactly one SPDX line is left in the window" \

That is the arm I asked for, and the second check is the one I would not have thought to require: exactly one. A repair that removed the duplicate by removing both lines would satisfy the first arm and break the notice entirely — so the pair of assertions pins the outcome from both sides.

Why this is the third header defect in a week and the fix generalises

The three states named tonight — missing (#1777), duplicated (#1627, this one), short (#2199) — are all "the tool's idea of a valid header is narrower than the set of headers it accepts". canonical_notice_line() is the first of the three that expresses the canonical form as data (_CANON_LINES) rather than as a set of special cases, so the next state has somewhere to go. Worth one line in the docstring saying so, if it is not already implied.