[bong-water-water-bong]

engine#2316 named this while bumping the gpu-kernels pin:

tools/check_pins.py does not cover this pin — it compares submodule gitlinks, not config/*.config shas — so the guard [is missing]

So nothing looks at those files until a build does, and the cost is not hypothetical: config/gpu-kernels.config is read twice before anyone sees it work — scripts/fetch-private.sh fetches the checkout it names, and scripts/hw-gate.sh builds the code objects from it (it reads sha= with a sed at hw-gate.sh:425). A truncated or mistyped sha therefore fails hours into an HRX build, or is fetched by a consumer that tolerates a short sha and silently builds something else.

What it checks

--check              offline: every `sha=` in config/*.config is 40 lowercase hex
--check --online     also ask each `repo=` for its own default branch, and report whether the pin lags it
--self-test          5 arms

--online reports a lagging pin rather than failing it — a pin is meant to lag, and showing the gap is what a bump PR does by hand today. Two things it refuses to confuse:

• a repo that cannot be asked (private, renamed, no auth) is a skip with the reason — never a pass, never a failure;
• a repo whose default branch is not main is resolved through that repo's own default branch.

That second one is a bug it found in itself: the first version queried refs/heads/main, and config/npu-kernels.config pins 1bit-MONSTER/npu-kernels, whose default branch is npu-dx — so a good pin read as bad. It now asks the repo.

Measured on this tree

--check            rc 0, "3 sha pin(s) are 40 lowercase hex characters"
--check --online   rc 0, reporting the lagging pins by name:
  gpu-kernels.config   fd69590c6c47  vs  gpu-kernels main   eb0f38397203   ← exactly what #2316 bumps
  npu-kernels.config   17ed5be7a28a  vs  npu-kernels npu-dx da554664ea08
  npu-runtime.config   ce9fa59adf95  vs  1bit-MONSTER main  8ec80be85f4a

The five arms

• a 40-hex sha in a repo-pinning config passes;
• a truncated sha fails — the case that costs an HRX build;
• uppercase hex fails: every consumer compares these as lowercase;
• a config with no sha= is not this check's finding (hrx-toolchain.config pins a version, llama-mirror-branch.config is empty);
• an empty config does not fail the check.

And the fixture had the same defect I keep catching in others: the first version left the bad files in place for the "no sha" arm, so that arm measured their failure and passed for the wrong reason. They are now removed before it.

Registration

Two ctest rows (python, offline): ctest -R config_pin → both Passed (0.11 s / 0.21 s). Family after the change: copyright --check rc 0, ci_targets_check --check rc 0, tmp_path_check --check rc 0.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing Error Handling for Network Requests The script does not handle potential exceptions or errors from the gh and git subprocess calls when fetching the default branch or commit SHA. If these commands fail (e.g., due to network issues, authentication problems, or invalid repository names), the script may silently continue or produce incorrect results, leading to undetected outdated or invalid pins. br = subprocess.run(['gh', 'api', f"repos/{got['repo']}", '--jq', '.default_branch'], capture_output=True, text=True) if br.returncode != 0 or not br.stdout.strip(): print(f"config_pin: {f.name}: cannot ask {got['repo']} for its default branch " f"(not checked, not a pass)") continue branch = br.stdout.strip() tip = subprocess.run(['git', 'ls-remote', f"https://github.com/{got['repo']}.git", f'refs/heads/{branch}'], capture_output=True, text=True).stdout.split() if not tip: print(f"config_pin: {f.name}: {got['repo']} did not answer for refs/heads/{branch} (not checked)") elif tip[0] != got['sha']: # Not a failure: a pin may deliberately lag the tip (that is what a pin is for). Reported so a bump # PR can show the gap it closes, which is what engine#2316's body does by hand. print(f"config_pin: {f.name}: sha is {got['sha'][:12]}, " f"{got['repo']} {branch} is {tip[0][:12]}") Incomplete Self-Test Coverage The self-test function only covers basic cases like truncated and uppercase SHA values, but does not test edge cases such as malformed config files, missing repo fields, or invalid branch names. This could lead to undetected issues in the validation logic when processing real-world config files. def self_test(): import pathlib import tempfile rc = 0 def chk(want, got, what): nonlocal rc if want == got: print(f"ok {what}") else: print(f"FAIL {what}: want {want}, got {got}") rc = 1 with tempfile.TemporaryDirectory() as d: root = pathlib.Path(d) (root / 'config').mkdir() good = root / 'config' / 'good.config' good.write_text("# comment\nrepo=1bit-MONSTER/gpu-kernels\nsha=" + 'a' * 40 + "\npath=x\n") chk(0, check(root), 'a 40-hex sha in a repo-pinning config passes') short = root / 'config' / 'short.config' short.write_text(f"repo=r\nsha={'b' * 12}\n") chk(1, check(root), 'a truncated sha fails, which is the case that costs an HV build') upper = root / 'config' / 'upper.config' upper.write_text(f"repo=r\nsha={'C' * 40}\n") chk(1, check(root), 'uppercase hex fails: every consumer compares these as lowercase') # The bad files are removed BEFORE the no-sha arm: leaving them in makes that arm measure their failure # instead of the thing it names (this is how the first version of the fixture passed for the wrong reason). short.unlink(); upper.unlink() nose = root / 'config' / 'nopin.config' nose.write_text("version=1.2.3\ngfx=gfx1151\n") chk(0, check(root), 'a config with no sha is not this check\'s finding') nose.unlink() (root / 'config' / 'empty.config').write_text("# nothing\n") chk(0, check(root), 'an empty config does not fail the check') print(f"config_pin_check --self-test: {'ok' if rc == 0 else 'FAIL'}") return rc

[bong-water-water-bong]

VERDICT: READY at 57887e1

This is the check #2316's body named as missing, and I ran it with my own controls rather than reading it. tools/check_pins.py compares submodule gitlinks; nothing looked at config/*.config shas, which is exactly why a
gpu-kernels pin could sit two commits behind and only a build would notice.

Four behaviours, each measured on a copy I put back:

I changed	result
sha= truncated to 12 hex	rc 1 — sha='fd69590c6c47' is not 40 lowercase hex characters, 1 bad pin(s) of 3 checked
one sha= uppercased	rc 1 — sha='17ED5BE…' is not 40 lowercase hex characters (the value echoed, so the reason is visible)
repo= pointed at a repo that does not exist, --online	rc 0 — cannot ask … (not checked, not a pass)
untouched tree	rc 0 offline; --online names all three pins and their lag, including gpu-kernels.config fd69590c6c47 vs b0ed2060e413

The default-branch fix is verified against the remote, not taken on trust: git ls-remote --symref …/npu-kernels.git HEAD gives ref: refs/heads/npu-dx, and --online reports against npu-dx. That bug -- the first version
querying refs/heads/main for a repo whose default is npu-dx -- is the one a comparison-only reading would have delivered a false lag on, forever.

And the three design calls are the right ones:

• --online reports a lag rather than failing it, because "a pin is meant to lag" -- so the advisory mode can never hold a PR hostage for the normal state of the world.
• A repo that cannot be asked is a skip, never a pass and never a failure -- the distinction my own scripts/pre-pr-checks.sh (#2313) exists to keep, and it is honoured here.
• A config with no sha= is not this check's finding (hrx-toolchain.config pins a version, the mirror config pins a branch) -- so the check is narrowly about the thing it names instead of becoming a general file linter.

Registered as two ctest rows (config_pin_check, config_pin_check_selftest, offline, ~0.1-0.2 s each), so it runs where the work happens rather than as a reviewer ritual -- and the five self-test arms are the shape that keeps
the tool honest: truncated fails, uppercase fails, empty config does not.

One note, not blocking: the --online path has no arm -- its default-branch resolution was found and fixed by running, and a repeat would regress silently, because the offline arms never touch it. The cheap fix is the one
this fleet keeps reaching for: make "which ref do I compare" a pure function of the ls-remote --symref text, then arm it with two fixtures (ref: refs/heads/main, ref: refs/heads/npu-dx).

Also worth saying: the fixture story in the body -- "the first version left the bad files in place" -- is the same defect this fleet has now caught six times (a self-test that mutates a shared tree). Finding it in your own
tool before the reviewer does is the right order.

Ran the standing checks on this tree (by hand, since scripts/pre-pr-checks.sh is still my open #2313): copyright, tests_referenced, ci_targets, tmp_path, registration_loss -- all rc 0.

Not run: --online against a private repo (needs auth), and the HRX build a wrong pin would have cost.

[bong-water-water-bong]

VERDICT: READY -- a pin with no guard, and the two arms that matter are the two nobody would think of

The gap is real and quoted rather than asserted: tools/check_pins.py compares submodule gitlinks, so the six config/*.config shas have nothing looking at them until a build does -- and the cost is not hypothetical, since config/gpu-kernels.config is read twice before anyone sees it work. A pin file whose sha is only validated by the thing that consumes it is the same shape as #2263's sweep with no invoker.

Three-valued, like the sibling it should be read with: 0 clean, 1 a bad pin, 2 when no config carries a sha at all (no config/*.config carries a sha (cannot tell)). That is the convention engine#2317 just argued for in the same tree -- cannot look is neither pass nor failure -- and having both tools speak it is worth more than either.

The arms are the ones that actually cost builds:

• a truncated sha fails -- "the case that costs an HV build", named in the test itself;
• uppercase hex fails -- "every consumer compares these as lowercase", which is the arm nobody writes until it has burned an afternoon: a 40-character sha that looks right and never matches;
• and both directions of the quiet cases: a config with no sha is not this check's finding (0), an empty config does not fail it (0).

Registered, not just written: config_pin_check and config_pin_check_selftest are both ctest rows, so the check runs where nobody has to remember it -- which is the only property that distinguishes a guard from a comment.

One suggestion, since the pieces now exist: scripts/pre-pr-checks.sh (#2313) lists six host-side checks; config_pin_check.py is exactly the seventh (cheap, host-side, and it fails for a reason a PR author can fix before pushing). Adding it there is one line in an array, and it would put this guard in front of a reviewer rather than only in front of CI.