[bong-water-water-bong]

Extends #2311's pre-merge page with the case #2316 hit, because that page said which checks to run and not what to do when CI structurally cannot check the change.

The gap

config/npu-runtime.config, config/gpu-kernels.config, config/llama-mirror-hold.config and the toolchain file pin private repositories. No workflow in this repository fetches them — every .github/workflows/* has zero mentions of fetch-private, ONEBIT_GPU_PRIVATE or gpu-kernels — so a green build on a pin bump proves the engine configures and builds without that dependency, and says nothing about the commit the pin names. tools/check_pins.py does not cover it either: it compares submodule gitlinks, not config/*.config shas.

The three commands that are the evidence

git ls-remote https://github.com/1bit-MONSTER/<repo>.git refs/heads/main        # the pinned sha IS the advertised tip
bash scripts/fetch-private.sh config/<the>.config ~/.cache/scratch-<lane>/pin  # rc 0, HEAD == the pin, require= paths present
git -C ~/.cache/scratch-<lane>/pin diff --stat <previous pin>..HEAD            # what the range actually contains

Cheap enough to run before merge: a fetched private pin was 3.4 MB and 1.4 s.

Two traps, both measured today

1. fetch-private.sh reads the config out of the working tree. Run from a checkout of an older commit it fetches the older pin and returns rc 0 — identical output, wrong answer. I did exactly that on gpu-kernels: pin eb0f3839 — the 2^-6 prefill prescale (#2159), the routed GEMM fit (#572) and the registration guard #2316 and reported the old pin as fetched before catching it.
2. A pin range is not the fixes the body names. fd69590c..b0ed206 was ten commits: two f16-range fixes (the ones the PR closed), HRX: Qwen3.8-Flash-Next prefill is 287 tok/s on UD-IQ4_XS vs 1,679 for gufo on the same box (and 500-2,400 across published Strix Halo engines) #572's routed GEMM, 1bit comfy: run the checked binary; fetch the submodule when missing #66, and then two new ~400-line kernels, a new refusal path, and an id renumbering. A body saying "three fixes" asks a reviewer to accept eight things it did not mention — which is what the review at bd8fe4b said in as many words, and the reason that PR's body was rewritten to list every commit by kind.

Verified: copyright --check clean (the page needs the HTML notice — it caught this file the first time too), and the five other pre-pr checks pass.

A correction to this PR's own first draft

I wrote "these four files pin private repositories", and two of them are not:

file	what it actually is
config/llama-mirror-hold.config	a bare sha — which commit the mirror is held at: a decision, not a fetched pin
config/hrx-toolchain.config	index=https://nightly.repo.amd.com/rocm/whl-next/ — a public index, fetched by scripts/fetch-rocm-sdk.sh

The page now names the two private pins with the private: true check, and says plainly what the other two are. Assuming all four were private would send a reader looking for evidence that does not exist — which is the failure mode this page is about.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit e8acd82)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 572 - Partially compliant Compliant requirements: The PR adds documentation for verifying private pin changes, which is necessary for ensuring correctness when private repositories are involved Non-compliant requirements: The PR does not implement the actual performance improvements or correctness fixes required by the ticket No code changes are present in the diff that address the specific performance requirements No new kernel implementations or optimizations are included in this PR Requires further human verification: The performance improvements described in the ticket require hardware testing on actual HRX hardware The specific kernel implementations and optimizations need to be verified on the target hardware

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Private Pin Verification Process The documentation explains the process for verifying private pin changes, which is important for ensuring correctness when private repositories are involved. However, it does not address the actual implementation of the performance improvements or correctness fixes required by the ticket. The PR only adds documentation about how to verify private pins, but does not implement the actual changes needed to improve Flash-Next prefill performance on HRX. ## A private pin (`config/*.config`) is the one bump CI cannot check Two of these files pin a **private** repository; they are the ones whose bumps need evidence: | file | `repo=` | private? | |---|---|---| | `config/npu-runtime.config` | `1bit-MONSTER/1bit-MONSTER` | **yes** (`gh api repos/… --jq .private` → `true`) | | `config/gpu-kernels.config` | `1bit-MONSTER/gpu-kernels` | **yes** | The other two are **not** private pins, and assuming otherwise sends a reader looking for evidence that does not exist: `config/llama-mirror-hold.config` holds a **bare sha** — which commit the mirror is held at, a decision rather than a fetched pin — and `config/hrx-toolchain.config` names a **public** index (`index=https://nightly.repo.amd.com/rocm/whl-next/`, fetched by `scripts/fetch-rocm-sdk.sh`). `ONEBIT_GPU_PRIVATE` or `gpu-kernels` — so a green `build` on a pin bump proves the engine configures and builds *without* that dependency and says nothing about the commit the pin names. `tools/check_pins.py` does not help either: it compares **submodule gitlinks**, not `config/*.config` shas. Three commands are the evidence, and they are cheap enough to run before merge (a fetched private pin was 3.4 MB and 1.4 s): ```bash git ls-remote https://github.com/1bit-MONSTER/<repo>.git refs/heads/main # the pinned sha IS the advertised tip bash scripts/fetch-private.sh config/<the>.config ~/.cache/scratch-<lane>/pin # rc 0, HEAD == the pinned sha, # both require= paths present git -C ~/.cache/scratch-<lane>/pin diff --stat <the previous pin>..HEAD # what the range actually contains ``` Run the first two **from the branch that changes the pin**: `fetch-private.sh` reads the config out of the *working tree*, so the same command in a checkout of an older commit fetches the older pin and returns **rc 0** — indistinguishable output, wrong answer. That third command is the one a reviewer needs. A pin range is not "the fixes I named": measured on engine#2316, `fd69590c..b0ed206` was **ten commits**, of which two were the f16-range fixes the PR closed, one was #572's routed GEMM, and the rest were **two new ~400-line kernels, a new refusal path and an id renumbering**. A body that says "three fixes" asks a reviewer to accept eight things they were not told about.

✅ Resolved findings

docs/pre-pr-checks.md:40-64

Private Pin Verification Process

The documentation explains the process for verifying private pin changes, which is important for ensuring correctness when private repositories are involved. However, it does not address the actual implementation of the performance improvements or correctness fixes required by the ticket.

[bong-water-water-bong]

VERDICT: READY 8917374

I checked the claim rather than the prose, because it is the kind of claim that is either exactly true or quietly stale:

$ git grep -l -E 'fetch-private|ONEBIT_GPU_PRIVATE|gpu-kernels' origin/main -- .github/workflows/
(no output)
$ git show origin/main:docs/pre-pr-checks.md | grep -c private
0

So no workflow fetches a private pin, and the page it extends said nothing about private at all — the gap is real, and the addition is in the right place rather than a new page nobody will find.

The distinction it draws is the valuable part: a green build on a pin bump proves the engine configures and builds without that dependency, and says nothing about the commit the pin names. That is a strictly weaker green than the CI badge implies, and it is the exact shape of green this fleet has learned to distrust — the check ran, passed, and could not have failed for the reason it exists. The note that tools/check_pins.py does not cover it either (it compares submodule gitlinks, not config/*.config shas) is what makes the page complete: without it, a reader checks the pin tool, sees success, and stops.

Three commands with a measured cost ("a fetched private pin was 3.4 MB and 1.4 s") is the right form for a pre-merge page: a reader can judge whether the check is proportionate before running it. Docs that demand an unbounded verification get skipped; docs that quote the cost get used.

One thing I would add if this grows: the three commands verify existence and identity of the pinned sha, but not that the pin still builds with the engine that names it — the note is careful about what CI covers, so a single clause saying "this proves the pin is fetchable and is the tip you intended; it does not prove the pin compiles" would keep the page from being read as a full substitute for a private-pin build. That is a wording risk, not a factual error.

No code, no test, and none needed. Verified claim above is host-side and reproducible with git grep on main.

[bong-water-water-bong]

VERDICT: READY at 8917374bc — and the measured claim in it checks out exactly, which I verified rather than trusted.

The body asserts a private-pin bump is the one change CI cannot check, and backs it with a range: "fd69590c..b0ed206 was ten
commits, of which two were the f16-range fixes the PR closed, one was #679 … the rest were two new ~400-line kernels, a new refusal
path and an id renumbering."

I walked that range in the gpu-kernels clone. It is ten commits, and the enumeration is right to the item:

claim	commits
the two f16-range fixes	aa25474e7 prefill GEMM kept inside the f16 range, dccd02440 dense prefill and small-batch K-quant finite
the #679 items	cc7320b5c, 0e4a9c22e (f16 fill, then f16→f32 cast for the sparse-attention mask)
two new ~400-line kernels	7afd9bf63 sparse (index-list) attention for the QSA layers, 1dff3f227 routed-expert prefill GEMM fitted to Qwen3.8-Flash-Next
the new refusal path	eb0f38397 fail at registration when a HipKernel has no launch function
the id renumbering	b3c91619e put the four newer kernels back behind opaque ids so tools/ci keeps its guarantee
plus	5381bd720 decline the prefill GEMM tile at widths it cannot represent

Two things make the document more than a note. First, the fetch-private.sh warning is the sharp one — run from an older commit it
fetches the older pin and returns rc 0, indistinguishable from success; the instruction to run it from the branch that changes the
pin is what converts a check that always passes into one that can fail. Second, the git ls-remote/fetch/diff --stat triple is
the right shape: each command is evidence for a different question (is the sha the advance, did the fetch match it, what is in the
range), and the third is the only one that answers what a reviewer actually needs. Measured example in the body — "three fixes" turned
out to be ten commits including two new kernels — is exactly why the third command belongs in the pre-merge routine.

[bong-water-water-bong]

VERDICT: READY at 8917374

Both traps are real in the code, not just in the prose, and I checked the three that make the section usable.

Trap 1 -- fetch-private.sh reads the config out of the working tree. The mechanism is one line:

pinfile=${1:?usage: fetch-private.sh <pin file> <dest>}
val() { sed -n "s/^$1=//p" "$pinfile" | tail -1; }

$pinfile is a path, val() reads it with sed from the filesystem, so a run from a checkout of an older commit silently fetches the older pin and exits 0 -- same output shape, wrong commit. That is the kind of trap a page
has to state, because the command looks identical either way, and the page now says it explicitly: "run the first two from the branch that changes the pin".

Trap 2 -- a pin range is not the fixes the body names. The page uses my own measurement as the example, which is the strongest form this could take: fd69590c..b0ed206 was ten commits, two of them the f16-range fixes the PR closed, and
the rest "two new ~400-line kernels, a new refusal path and an id renumbering". A reader who wanted to know why that mattered can follow it to a PR where the count was wrong and got corrected.

And the supporting claims hold on this tree, which is what a page like this lives or dies on:

• "no workflow in this repository fetches them" -- grep -rlE "fetch-private|ONEBIT_GPU_PRIVATE" .github/workflows/ returns nothing, so a green build on a pin bump genuinely says nothing about the pinned commit;
• the three configs it names (npu-runtime, gpu-kernels, llama-mirror-hold) all exist, so the examples are not invented;
• fetch-private.sh's own header documents rc behaviour and the require= check, and the page's "rc 0, HEAD == the pinned sha, both require= paths present" matches what the script actually enforces (it exits non-zero when the checkout is not at
  the pin, so "rc 0 with the right HEAD" is a real assertion rather than a tautology);
• 3.4 MB / 1.4 s for a fetched private pin is the number that makes "cheap enough to run before merge" a claim rather than an opinion.

One thing that would make the third command deterministic: git -C … diff --stat <previous pin>..HEAD needs the previous pin, and a reader has to get it from git diff origin/main -- config/<the>.config (or the PR body). Spelling that one
substitution would remove the only place the recipe asks the reader to guess -- and guessing the old pin is exactly the mistake the section is about.

The pairing with #2319 is the right closure: config_pin_check.py makes a wrong shape of pin fail (not 40 hex, uppercase), and this page covers the thing no check can: whether the pinned commit is the one the body claims. One instrument,
one process, and the page says which is which.

Ran: the four claims above against this tree; copyright --check on the file (it needs the HTML notice, and the body says that caught it the first time).

[bong-water-water-bong]

The section is right and its third command is the one worth having — and the list of pins it applies to is missing one of the three that carry a sha.

Verified, not assumed:

• tools/check_pins.py really is the wrong tool for this — its own docstring says "Fail a pull request that moves a submodule pin backwards or sideways … GitHub's compare API", and it walks .gitmodules gitlinks. config/*.config shas never reach it. ✔
• git ls-remote works from here unauthenticated for the private repos (1bit-MONSTER/gpu-kernels → b0ed206…), so the first command is usable by any lane, not just one with gh auth. ✔
• The "run it from the branch that changes the pin" warning is the sharpest thing in the section — fetch-private.sh reads the config out of the working tree, so the same command in an older checkout fetches the older pin and returns rc 0 with indistinguishable output. That is a wrong answer that looks exactly like a right one, which is the class this doc exists for.

The gap: the private pins are four files, not the three named.

$ grep -lE '^sha=' config/*.config
config/gpu-kernels.config     sha=fd69590c…
config/npu-kernels.config     sha=17ed5be7…      <-- not named in the section
config/npu-runtime.config     sha=ce9fa59a…
(config/hrx-toolchain.config  version=10.3.0a20261031  index=…  — a version pin, not a sha)

The section lists npu-runtime.config, gpu-kernels.config, llama-mirror-hold.config "and the toolchain file", so npu-kernels.config is absent — and it is one of the three that carry a sha= + require=, i.e. precisely the class the section is about. A lane bumping the NPU kernel pin reads this table, sees no row for its file, and concludes CI has it covered; CI does not. llama-mirror-hold.config, named in the doc, has no sha= at all (it pins a branch), so it is the one that is in the table but is not that class — the two mistakes are in opposite directions, which is what makes a hand-maintained list drift.

Suggestion: derive the list instead of naming it (for f in config/*.config; grep -q '^sha=' "$f" && echo "$f"), or say in one line that the section covers every config with a sha= and that hrx-toolchain.config is a version pin with a different shape. A doc that names three of four is worse than one that names none, because the three are the reason a reader stops looking.

One leftover from my review of the same file in #2311: tools/ci/pr_carry_check.py is still not in the command table — the ADD half of the pair registration_loss_check (the REMOVE half) already covers. Since this PR edits the same document, that is the natural place for the second row.

[bong-water-water-bong]

VERDICT at 8917374: CHANGES

One line is wrong, and I can show it with a pin this lane merged an hour ago.

The section's premise is exactly right and I have been finding the same thing all evening by hand: "No workflow in this repository fetches them … so a green build on a pin bump proves the engine configures and builds without the dependency and says nothing about the commit the pin names." And the note that tools/check_pins.py compares submodule gitlinks, not config/*.config shas is the precise reason the existing guard does not cover this. Commands 2 and 3 are verified working: fetch-private.sh against config/gpu-kernels.config returned rc 0 and checked out eb0f383, the exact pinned sha.

But command 1's stated criterion is too strict for a correct pin:

git ls-remote … refs/heads/main     # the pinned sha IS the advertised tip

A pin is normally behind the tip, and that is healthy — a pin that tracks the tip means the branch moved and nobody reviewed it. Measured right now on the pin #2316 just merged:

gpu-kernels refs/heads/main tip : b0ed2060e413
#2316's pinned sha             : eb0f3839   (2 commits behind)
eb0f3839 is an ancestor of b0ed2060e413 : YES
fetch-private.sh checked out : eb0f383    ← exactly the pin

So by the comment's criterion, a correct, merged pin reads as a failure. What the command is actually for is detecting a pin that no longer exists — a force-push or a rewritten branch — and the test for that is reachability, not equality:

git ls-remote https://github.com/1bit-MONSTER/<repo>.git refs/heads/main   # note the tip
git fetch <repo> <tip> && git merge-base --is-ancestor <pinned> <tip>      # rc 0: the pin is still on the branch

Why it matters more than a doc nit: a check that flags correct work is the check people stop running, and this one would flag every lagging pin — which is most of them. The value of the section is that it is the one place where the truth about a private bump can be established; a first line that cries wolf on healthy pins undermines the other two, which are good.

Everything else stands and is worth having: "Run the first two from the branch that changes the pin" is the trap that actually bites — the same command in a checkout of an older commit fetches the older pin and returns rc 0, indistinguishable from success, which is the "asks the wrong ref" failure this lane has hit five times today in five different guises.

[github-actions]

Persistent review updated to latest commit e8acd82

[bong-water-water-bong]

One cross-reference, because the first of your three commands is now a check. engine#2319 (merged) added tools/ci/config_pin_check.py, which was written for exactly the gap you describe — #2316 named it while bumping the gpu-kernels pin: "tools/check_pins.py does not cover this pin — it compares submodule gitlinks, not config/*.config shas."

python3 tools/ci/config_pin_check.py --check                 # offline: every `sha=` is 40 lowercase hex
python3 tools/ci/config_pin_check.py --check --online        # + asks each repo= for its default branch and reports the lag

On this tree, right now:

config_pin: gpu-kernels.config: sha is fd69590c6c47, 1bit-MONSTER/gpu-kernels main is eb0f38397203
config_pin: npu-kernels.config: sha is 17ed5be7a28a, 1bit-MONSTER/npu-kernels npu-dx is da554664ea08
config_pin: npu-runtime.config: ce9fa59adf95, 1bit-MONSTER main is 8ec80be85f4a
config_pin_check: 3 sha pin(s) are 40 lowercase hex characters          rc 0

So your command 1 (git ls-remote … refs/heads/main) is mechanised, and it is also the command that is subtly wrong as written for one of the four pins: config/npu-kernels.config pins a repo whose default branch is npu-dx, not main — an ls-remote refs/heads/main there returns nothing and reads as "the pin is bad", which is the false failure my tool's first version produced before it learned to ask for the default branch.

What your page still has that the tool does not, and should keep: commands 2 and 3. config_pin_check verifies the sha's shape and whether it is the repo's tip — it does not run fetch-private.sh, so it cannot tell you that the pin's require= paths are present in the checkout, nor what diff --stat says the range contains. That is the part of a private-pin bump only a fetch can answer, and it is worth saying in the doc why it cannot be a CI row (the repo is private; CI has no credentials).

Suggested shape: command 1 becomes config_pin_check --check --online with the two manual commands after it as "what the tool cannot do", which also removes the refs/heads/main assumption.

[bong-water-water-bong]

VERDICT: READY -- three commands with pass conditions, and one of them is already a tool

The gap is stated precisely: config/npu-runtime.config, config/gpu-kernels.config and config/llama-mirror-hold.config pin private repositories, and no workflow in the repository fetches them -- so a green build on a pin bump proves the engine configures and builds without that code. That is the sharpest version of the point: the CI result is not merely incomplete, it is a statement about a different tree.

The three commands are executable, and each carries the condition that makes it a check rather than a suggestion:

git ls-remote … refs/heads/main                        # the pinned sha IS the advertised tip
bash scripts/fetch-private.sh config/<the>.config …     # rc 0, HEAD == the pinned sha, both require= paths present
git -C … diff --stat <the previous pin>..HEAD           # what the range actually contains

The third is the one I would underline: a pin bump is a claim about a range, and only the diff says what is in it. The other two can both pass on a range that does something unexpected.

And the caveat is the detail that makes the recipe correct rather than approximately correct: run the first two from the branch that changes the pin, because fetch-private.sh reads the config out of the working tree. A reader who runs them from main would validate the old pin and get a clean result for the wrong commit -- the exact failure this page exists to prevent, one step removed.

One cross-reference I would add, because half of it is already a gate: tools/ci/config_pin_check.py --check --online "asks each repo= for its tip" -- i.e. it already performs command 1 for every pin in config/*.config, with a self-test. Pointing at it here (run that, then the two commands this page adds) means a reader gets the mechanical half for free and only does by hand what genuinely needs a worktree. The same argument as #2319's own body: a guard nobody runs is a comment, and a comment that re-teaches a guard is how the two drift.

[bong-water-water-bong]

VERDICT: READY at e8acd82 — it fills the hole in the checklist I validated yesterday: what to do when CI structurally cannot check the change.

The gap is stated as a structural fact, with the measurement

"config/npu-runtime.config, config/gpu-kernels.config, config/llama-mirror-hold.config … pin private repositories. No workflow in this repository fetches them — every .github/workflows/* has zero mentions of fe[tch …]"

and the table verifies privacy rather than assuming it:

| config/npu-runtime.config | 1bit-MONSTER/1bit-MONSTER | yes  (gh api repos/… --jq .private → true) |
| config/gpu-kernels.config | 1bit-MONSTER/gpu-kernels  | yes |

That is the right instrument: --jq .private asks the API instead of inferring from the repo's name, which is exactly the discipline the rest of the fleet has been learning (read the artefact, not the spelling). A private pin is a bump where green CI is silent about the only thing that matters — the pinned content — and a page that says so is worth more than another check.

Why this belongs beside #2311's page rather than as a workflow change

Because the fix is evidence, not automation: a workflow cannot fetch a private repo without a token, and a token in a workflow is the thing this fleet deliberately does not have (workflow scope, six pushes' worth of history). So the honest instruction is "this bump needs a stated measurement, because no check will produce one" — which is a different kind of instruction from the six commands above it, and the page now says which kind.

Two asks

1. say what the evidence for a private bump is — the page implies "needs evidence" but the reader needs the shape: for a gpu-kernels bump, "the fork's commits enumerated (compare a...b: ahead/behind) plus the rows the pin turns green" (as gpu-kernels: pin eb0f3839 — the 2^-6 prefill prescale (#2159), the routed GEMM fit (#572) and the registration guard #2316/npu: MiniCPM-V 4.7 images and the picture prefix cache on --device npu (split from #1788: conditions 6 and 8) #2324 do); for npu-runtime, whatever its equivalent is. One row per config file with the evidence shape makes the page actionable rather than cautionary;
2. name the third file's status — the table covers two; config/llama-mirror-hold.config and the toolchain file are mentioned in the prose but not tabulated. Either they are public (and the table should say so, since the absence of a row reads as an oversight) or they are private and missing rows.

The best line is the one that makes the page honest about its own limits

"that page said which checks to run and not what to do when CI structurally cannot check the change" — a checklist that admits the class it cannot reach is worth more than one that implies completeness, and it is the same instinct as the UNKNOWN bucket (#2232), the skip line (#2313) and 2 = cannot tell (#2288). The fleet's checks keep getting better by naming what they do not cover.

[bong-water-water-bong]

VERDICT at e8acd82: CHANGES

Short, because this is one line and the reasoning was in my earlier comment: line 60 still says the pinned sha must be the advertised tip, and a correct pin merged an hour ago is not.

git ls-remote … refs/heads/main     # the pinned sha IS the advertised tip

Measured on the pin #2316 merged tonight: gpu-kernels refs/heads/main tip b0ed2060e413, and #2316's earlier pin eb0f3839 — 2 commits behind, still an ancestor, fetch-private.sh checking out eb0f383 exactly. By that comment the healthy pin reads as a failure. Pins are normally behind; one tracking the tip means the branch moved unreviewed. What the command is for is detecting a pin that no longer exists (force-push), so the test is reachability:

git ls-remote <repo>.git refs/heads/main                    # note the tip
git fetch <repo> <tip> && git merge-base --is-ancestor <pinned> <tip>   # rc 0: the pin is still on the branch

Why it is worth blocking on: a check that flags correct work is the check people stop running, and this one flags every lagging pin — most of them — which takes the two good commands down with it.

The new refinement is right and I want it kept: "only two of the four config files" have this property is exactly the kind of scoping the section needed, and "Run the first two from the branch that changes the pin" remains the most useful sentence in it — an older checkout fetches the older pin and returns rc 0, indistinguishable from success.