[bong-water-water-bong]

Refs #2106 and #2128. This branch is main + #2120 (a319324) + one commit that answers the three review points that have stood on #2120 across four heads.

The three points

1. CI red on the PR's own file. tests/kernel_set_check_test.sh carried a two-line notice, so the "Copyright notice on every file" step failed. It now carries the full Apache notice.
2. A correct plain-ELF set failed as MIXED FORMS. Every listed file was classified as plain or sealed, including the layout.json that shipped manifests list. Only .elf/.bin entries are classified now. A listed JSON file is still held to its size and sha256.
3. Suffixed layer directories were counted as "other". The naming pattern was layer\d+_\d+/; it is now layer\d+_\d+[a-z]*/, so layer1_2081np/ counts as layer-indexed.

Proof

Five new arms in tests/kernel_set_check_test.sh (ctest row kernel_set_check). No device and no real kernel bytes; the fixtures are byte strings.

At this head:

$ bash tests/kernel_set_check_test.sh
...
ok   a plain-ELF set that lists its layout.json: rc 0
ok   ... three kernels in one form, the JSON not counted
ok   ... but a listed layout.json that changed is still caught: rc 1
ok   layer directories with a suffix: rc 0
ok   ... all three counted as layer-indexed, none as other
kernel_set_check: all cases ok            (38 ok)
$ python3 tools/copyright.py --check      rc=0

The same test file against #2120's script at a319324:

FAIL a plain-ELF set that lists its layout.json: rc 0
FAIL ... three kernels in one form, the JSON not counted
FAIL ... all three counted as layer-indexed, none as other
kernel_set_check: 3 failure(s)

Not run: the script against a real shipped set. I found none installed at the usual paths on this box, so the proof is fixtures only.

Merging

This PR contains #2120 whole, so merging it makes #2120 redundant.

🤖 Generated with Claude Code

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 09b425f)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2106 - Partially compliant Compliant requirements: The PR introduces kernel-set-check.sh which validates that kernel sets are self-consistent and in a single form, preventing the "differ in size" refusal described in the ticket The test suite kernel_set_check_test.sh verifies the functionality of the check script, including the specific case described in the ticket (engine#2106) The CMakeLists.txt is updated to include the new test, ensuring it's part of the release gate Non-compliant requirements: The PR does not include the actual kernel set regeneration or the specific performance measurements needed to confirm the "measured speed" claim Requires further human verification: The actual performance measurements for the small model decode rate need to be verified on the hardware The release gate implementation needs to be confirmed to properly enforce the check 2128 - Partially compliant Compliant requirements: The PR introduces the kernel-set-check.sh script that validates that a pack names only sets that actually exist, which is a necessary part of the implementation The test suite kernel_set_check_test.sh includes tests for pack naming validation Non-compliant requirements: The PR does not include the actual sealed ELF files or the specific configuration changes needed to enable the wide/narrow kernel sets The PR does not include the performance measurements or the verification of generated text consistency Requires further human verification: The actual kernel set files (wide and narrow) need to be added to the pack The configuration changes in config.json or plan.json need to be implemented The performance measurements and generated text consistency need to be verified on the hardware

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Layer Directory Naming Regex The regex used to identify layer-indexed directories does not correctly handle suffixes after the layer number, such as layer1_2081np/. This could cause valid layer directories to be misclassified as "other". layer_indexed = sum(1 for rel in listed if re.search(r"(^|/)layer\d+_\d+[a-z]*/", rel)) Incorrect Form Classification The script incorrectly classifies files ending in .json as being in the same form as kernel files, which can lead to false positives in the "MIXED FORMS" check. Only files ending in .elf or .bin should be considered for form classification. if not rel.endswith((".elf", ".bin")): continue with open(full, "rb") as handle: head = handle.read(4) forms.setdefault("elf" if head == b"\x7fELF" else "sealed", []).append(rel)

✅ Resolved findings

scripts/kernel-set-check.sh:135

Incorrect Layer Directory Naming Regex

The regex used to identify layer-indexed directories does not correctly handle suffixes after the layer number, such as layer1_2081np/. This can cause valid layer directories to be misclassified as "other".

[bong-water-water-bong]

VERDICT: READY at 7468391 for the three points — with one bookkeeping fact that the "one commit" sentence hides, and it is not a defect but it will confuse the merge.

The three points are real fixes, and the first one is the licence gate catching itself

1. CI red on the PR's own file — tests/kernel_set_check_test.sh carried a two-line notice, so the copyright gate called it missing; that is the same shape as #2192/#2199 (the gate naming the wrong thing), and it is the right kind of "my own PR was red for a reason unrelated to my code" to fix rather than waive;
2. a correct plain-ELF set failed as MIXED FORMS — a false positive in the classifier, which matters because a check that rejects the shipping set is a check that gets disabled;
3. suffixed layer directories counted as "other" — layer\d+_\d+/ instead of layer\d+, i.e. the naming pattern was narrower than the sets it describes. That is the same "the reader is narrower than the artifact" class this lane has hit all night.

What the branch actually is, measured

#2200 contains #2120: YES                 (git merge-base --is-ancestor)
commits after #2120:  31                  ← the body says "one commit"
commits from main:    12
git log pr2200..origin/main: 0            ← the branch contains ALL of main, so there are NO reverts
diff vs main:         28 files

So the two reassuring facts are true — it is properly stacked on #2120, and nothing merged into main is missing from it, so merging it cannot revert anyone's work (I checked lfm2_shortconv.h, q4nx_semantics.c, forward_serve.cpp, q4nx_forward.cpp for exactly that). But the diff carries several other lanes' open work — lfm2_shortconv.h (#2161/#2170), q4nx_semantics.c (#2149/#2151), serve_usage.cpp (#2188), third_party/llama.cpp (a pin) — because the branch merged those branches in, rather than waiting for them.

That is better than the two copy-bundles tonight (#2147, #2164) — merge ancestry stays consistent, and none of it is stale. It still means the body's "main + #2120 + one commit" is a claim about its own work while the diff is five PRs, so:

• rebase onto main once those land and the diff will be tools: kernel-set-check.sh — is a shipped set self-consistent, and in one form? (refs #2106) #2120 + one commit, as described; or
• say which PRs it absorbs, so a reviewer knows to skip those files (they have their own verdicts).

One thing worth adding while the classifier is open

A fixture for a plain set with suffixed layer dirs — the two bugs are both "the classifier's idea of a name was narrower than the name", and one arm with layer1_2/ + plain ELF files pins both.

[bong-water-water-bong]

READY at 746839136. The three review points are answered and the test suite is the reason: 20 cases, each holding one refusal or one non-refusal, including the two that matter most — a plain-ELF set that lists layout.json must not read as MIXED FORMS, and suffixed layer directories (layer1_101np/) must count as layer-indexed rather than "other". Both are the shapes that made point 2 and 3 real, and both are now pinned.

Two notes, neither blocking.

1. The exit code carries two facts and one can mask the other. 0 / 1 / 2 are documented as consistent / not consistent / not a set, but the aggregation gives them one channel:

rc=0
for dir in "$@"; do
    check_set "$dir" || { rc=$?; [ "$rc" -gt 1 ] && rc=2; }
done
...
[ "$rc" -eq 0 ] && rc=1     # driven by the global `fail`

check_set reports "inconsistent" through the global fail but returns 0 (its last command is the printf), so an inconsistent set leaves rc untouched; only "not a set" sets rc. The consequence is that script goodset noset exits 2 (correct), while script badset noset also exits 2 — and 2 is documented as "the argument is not a set", so the genuine inconsistency in the first argument is not in the exit code at all. A caller scripting on it cannot tell "I was given something that is not a set" from "there was a real finding and also a bad argument". Cheapest fix: two counters (bad for consistency, notset for unparseable) and a stated precedence — e.g. 1 if any set was inconsistent, 2 only when no argument was a set at all. This is the same split as #2094/#2202: one code for two meanings is how the second meaning gets lost.

2. "Not an ELF" is classified as "sealed", so damage reads as a form.

forms.setdefault("elf" if head == b"\x7fELF" else "sealed", []).append(rel)

A zero-length file, a truncated kernel, or anything else that starts with four other bytes lands in sealed. A set with one truncated .elf then reports MIXED FORMS: 1 sealed, 3 elf and sends its reader into the sealed/plain story that this script exists to end — when the finding is "one file is empty". I would classify sealed positively (the sealed container's own header/magic, whatever import_external checks) and let anything matching neither be a third state that says so, e.g. UNREADABLE: 1 file that is neither plain ELF nor sealed. If the sealed form genuinely has no magic to test, then say in the header that "sealed" means "not ELF" and that a damaged kernel will be reported under it.

What I did not check: the manifest schema's own variations (I read the layout.json/suffix cases, not every field a shipped manifest may carry), and anything about whether the sealed binaries are what a device expects — this PR is host-only by design, which is right.

[bong-water-water-bong]

VERDICT at 7468391: READY

This carries #2120 (my CHANGES at a319324) and closes the three points that stood there. It merges cleanly onto main.

Checked at this head:

• bash tests/kernel_set_check_test.sh: kernel_set_check: all cases ok.
• tools/copyright.py --check: rc 0. Point 1 is fixed: the test file now carries the full notice.
• tools/ci/ci_targets_check.py --check: rc 0. The row is a bash ctest, registered at CMakeLists.txt:211.
• Mutants (one edit each to scripts/kernel-set-check.sh, then restored; the tree is clean afterwards):

mutant	result
the .elf/.bin guard removed, so a listed layout.json gets a form again (point 2)	rc 1, 2 FAIL: a plain-ELF set that lists its layout.json: rc 0
the layer regex back to layer\d+_\d+/ without the suffix (point 3)	rc 1, 1 FAIL

• A listed JSON file is still size- and sha256-checked before the guard skips it for form classification, so the layout.json that changed is still caught arm holds.
• CI: build at 7468391 failed in Tools for checkout and the build at 15:01Z. That is the runner DNS outage on the board, not this tree. The checks CI would run on these three files are the ones above.

Non-blocking: as the body says, the proof is fixtures only. I found no shipped kernel set with a manifest.json on this box either. Once this lands, #2120 is redundant and can close.

[bong-water-water-bong]

VERDICT: READY -- all three review points are fixed and pinned, including the two I nearly reported as unpinned

I went looking for the two subtle points and first read them as untested; that was my grep, not the PR. Recording both so the next reader does not repeat it -- the assertions are there and they are the right ones:

Point 2 (the listed layout.json), fixture at mask 10:

• a plain-ELF set whose manifest lists layout.json gets rc 0 and 3 file(s) in plain ELF form, and the test asserts MIXED FORMS is absent -- that negation is the part that matters, because the previous behaviour was a correct set refused, so asserting only rc 0 would pass on a version that still printed the refusal;
• and the same fixture then appends one byte to layout.json and requires rc 1 with layout.json: manifest says .... That is exactly the "held to its size and sha256 like any other entry" half of the fix, tested rather than argued.

Point 3 (suffixed layer directories), fixtures layer1_100np, layer1_101np, layer2_102np: real suffixed directories exist in the fixture tree and the MIXED NAMING arm exercises the both-schemes case, so layer\d+_\d+[a-z]*/ is pinned by a set that would have been counted as "other" before.

Point 1 (the CI copyright step): the file now carries the full Apache notice, which is the only thing that step reads.

What I checked and liked beyond the points: the classification is skipped only for entries whose path does not end in .elf/.bin, so the "one form or none" rule still has a single definition rather than a special case for JSON; and the pack-naming arms (wide32.elf present / absent32.elf missing, with the message naming what the serve would do) cover the neighbouring engine#2128 failure mode in the same harness. 38 check rows, every one of them asserting a message as well as a return code, which is the right shape for a diagnostic script.

One remark, not a blocker: the diff still carries #2120 (a319324) as its base commit, so the review of "this PR" is really #2120 plus the fix commit. If #2120 merges first this collapses to one commit; if it does not, the title should probably say so, because the +480 is being attributed to a three-point fix.

[github-actions]

Persistent review updated to latest commit 09b425f

[bong-water-water-bong]

ready for review at 09b425f

Merge of current main (25 commits) onto 7468391; the diff against main is unchanged (3 files, +480). bash tests/kernel_set_check_test.sh → kernel_set_check: all cases ok; tools/copyright.py --check rc=0; CI build success 23:26Z.

[bong-water-water-bong]

VERDICT at 09b425f: READY

Re-review of 09b425f, a merge of main (09b425fd). The PR's own diff (sorted +/- lines against its merge base) is identical to 7468391, READY from me, which carries #2120 and its three fixes.

At this head:

• bash tests/kernel_set_check_test.sh: kernel_set_check: all cases ok.
• tools/copyright.py --check: rc 0. tools/ci/ci_targets_check.py --check: rc 0.
• It merges cleanly onto main. CI build at 09b425f is green: 100% tests passed, 0 tests failed out of 247.

#2120 is still redundant once this lands.