[bong-water-water-bong]

Five tools in this tree index sys.argv directly, so --help arrives as a filename:

tools/make_tokenizer_golden.py    IndexError (no args) / open('--help')
tools/mtp_depth_pool.py           FileNotFoundError: '--help'
tools/oga_attn_tile_golden.py     FileNotFoundError: '--help/dd_metastate_Llm_Token_...'   <- a path join with the flag
tools/oga_mlp_gu_golden.py        same
tools/laya_finetune/metrics.py    FileNotFoundError: '--help'

A user asking a tool what it does gets a stack trace. Nothing in CI noticed, because nothing runs --help.

Fixed

All five now print their usage and exit 0 on -h/--help. The two oga_* tools also gained argv validation: their usage line is <pack dir> [--check], so a bare --check had no pack dir and was opened as a directory literally named "--check" — a documented invocation shape reported as a traceback.

--help rc=0   tools/mtp_depth_pool.py, oga_attn_tile_golden.py, oga_mlp_gu_golden.py,
              make_tokenizer_golden.py, laya_finetune/metrics.py
--check rc=2  oga_* -> "usage: tools/oga_attn_tile_golden.py <pack dir> [--check]"

And the real work still runs: mtp_depth_pool --selftest rc 0, make_tokenizer_golden with no args prints usage and exits 2.

tools/ci/tool_help_check.py keeps it fixed

Registered as two ctest rows. It FAILS only when the flag itself appears in the traceback as the opened path — that is the evidence the claim needs:

result	when
FAIL	--help is in the traceback's last line
FAIL (INTERNAL)	the script we globbed could not be started at all
skip	a missing dependency (amd-rai-sweep's onnxruntime), a git call at import (check_pins.py), a data file not in the tree (oga_pack_tiles_golden.py)
clean	usage line with any exit code, or a normal answer

Four states and not two, because I shipped the two-state version and it had a false pass: cwd is the scan root, so a globbed relative path resolved to <root>/<path>, python exited 2 with "can't open file" and no traceback, and the tool was counted clean without ever being asked. That is the defect class this check exists to catch, in its own body. The second version's resolve() fix was right and its self-test arm for it was vacuous — it passed an absolute path, where resolve() is a no-op. The arm now reproduces the real shape (a relative path with cwd = the scan root) and fails when the resolve() is removed — verified by reintroducing the bug.

self-test, 6 arms                                    ok
with the resolve() removed                           rc 1, caught by 1 arm
this tree: 91 of 95 asked, 4 not asked by name, 0 crashes (was 5)
  tool_help_check            Passed  21.38 s
  tool_help_check_selftest   Passed   0.73 s

Scope is tools/ only, deliberately. My first scan rooted at the repo and reported 16 crashes — but most were tests/ fixtures that are not asked for help and os/licenses/ryzenadj/src/examples/*.py, which is vendored third-party source: a traceback in someone else's example file is not our defect. Two more were ModuleNotFoundError for packages this box does not have. Neither belongs in the count, and reporting them would have been a claim the measurement did not support.

Cost: 21 s for 95 subprocesses, against a suite that already takes 329 s. Cheap enough to be a ctest row rather than a hand-run script.

---

Added since opening: the worst shape of the three — a silent success

check_pins.py --help passed len(sys.argv) == 2, so the flag became a base ref, git diff returned nothing, and the tool exited 0 having printed nothing. A crash at least says it broke; this says "all fine".

--help          print the docstring, rc 0            (was: silent rc 0)
no args         usage on stderr, rc 2                (was: sys.exit(__doc__) on stdout)
bad ref         one line + non-zero exit             (was: CalledProcessError traceback)
check_pins.py HEAD   still does its real job, rc 0

The checker now FAILS on that shape too, but only for a real CLI — it parses the file for a __main__ block and sys.argv use first. q4nx_pack.py, q4nx_t2s.py, laya_finetune/head.py and the two kernelforge kernels have no entry point, so silence from them is not a defect; they are reported as "not a CLI" rather than as failures.

And the check was hiding the defect it had just been taught to find: with cwd = the scan root, check_pins.py's git config -f .gitmodules failed and it looked unaskable instead of silent. cwd is now the invocation cwd — where these tools are documented to run.

tool_help_check --self-test   ok                (8 arms)
tool_help_check --check       rc 0: 88 asked, 7 not asked by name, 0 crashes, 0 silent
ctest tool_help_check + _selftest            both Passed (8.40 s / 0.26 s)

Confession, because it is the same lesson: while measuring this I created a 172 KB file literally named --help in the repo root — one of the unfixed oga_* tools writes its second argument as the output path. It sat there ten minutes and made my own crashes.py fixture succeed, which is why a self-test arm failed until I found it. Removed; git status clean. A tool asked a question must not write a file, and a probe must not leave one behind.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 4d2e1aa)

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The tool_help_check.py script includes a self-test that verifies the behavior of tools when --help is passed. However, the self-test logic for detecting relative paths and the cwd behavior may be flawed. Specifically, the test changes the working directory to root.parent and then tries to pass a relative path to crashes(), which may not correctly simulate the actual issue if the path resolution logic in the tool itself is not properly tested. def self_test(): import tempfile rc = 0 def chk(want, got, what): nonlocal rc if want == got: print(f"ok {what}") Incorrect argv handling Similar to oga_attn_tile_golden.py, this script checks for positional arguments but only considers the first non-option argument as the pack directory. If multiple positional arguments are provided, only the first one is used, and the rest are ignored. This could lead to unexpected behavior if users provide more than one positional argument. pack, check = Path(positional[0]), "--check" in argv group = read_group(pack) code, scale, zp, bias = decode(group) gate = tiles_q4_1(*half(code, scale, zp, False)) up = tiles_q4_1(*half(code, scale, zp, True)) q4 = gate + up gate_tiles = len(gate) // 5120 meta = { Incorrect argv handling The script now checks for positional arguments but only considers the first non-option argument as the pack directory. If multiple positional arguments are provided, only the first one is used, and the rest are ignored. This could lead to unexpected behavior if users provide more than one positional argument. positional = [a for a in argv[1:] if not a.startswith("-")] if not positional: # The usage line is `<pack dir> [--check]`, so a bare `--check` has no pack dir. Before this # it became argv[1] and the tool opened a directory literally named "--check" and crashed # with FileNotFoundError -- a documented invocation shape reported as a traceback. print(__doc__.strip().splitlines()[-1], file=sys.stderr) return 2 pack, check = Path(positional[0]), "--check" in argv

[bong-water-water-bong]

VERDICT: READY -- the checker is more careful than most, and I have one concrete suggestion that removes the only way it can cost CI minutes

The bug is real and the shape of it is the interesting part: five tools index sys.argv, so --help arrives as a filename -- one IndexError, four FileNotFoundError: '--help', two of those joining the flag into a path (--help/dd_metastate_…) so the error message is a mangled path rather than the flag. A user asking a tool what it does gets a stack trace, and nothing in CI noticed because nothing ran --help. Every --check invocation shape in the body is a shape someone would actually type.

Three things in the checker itself are better than the fix it guards, and they are the reason I trust the result:

• The absolute path with its own comment. script = pathlib.Path(path).resolve() because with cwd=root a relative path gives python "can't open file" and exit 2 with no traceback, which the checker would have read as a clean run -- "that false pass is the defect this check exists to catch, in its own body." A checker that names its own failure mode is worth more than one that passes.
• A third state, internal. can't open file is neither a clean run nor a skip: it means the checker handed python a path it cannot open, i.e. a defect in the checker. The comment records that the first version of the arm passed with the bug reintroduced -- which is exactly the class of bug a two-valued result cannot express.
• The evidence requirement. A traceback counts as this finding only when --help (or -h) appears in the final line; otherwise it is skip, not fail. Each of the three misattributions that clause prevents is named. A missing dependency, a data file outside the tree, a git call at import -- none of those is "--help is a traceback", and reporting them as one would be a claim the measurement does not support.

The self-test pins exactly that discrimination (crash-with-flag-as-path versus a usage line with rc 2, a clean helps.py, a missing-dependency skip, an unrelated-death skip), and both rows are registered (tool_help_check and tool_help_check_selftest) with no environment or pack needed, so unlike the NPU rows this one really does run in the build job.

One suggestion, and it is the only way this row can be expensive: pass stdin=subprocess.DEVNULL.

p = subprocess.run([sys.executable, str(script), '--help'], cwd=str(root), env=env,
                   capture_output=True, text=True, timeout=TIMEOUT_S)

There are 95 .py files under tools/ and TIMEOUT_S = 30. A tool that reads stdin, or blocks on a lock, or waits for a git credential, currently costs the full 30 s and is then reported as skip -- and five such tools would add two and a half minutes to every build. With stdin=DEVNULL an stdin-reading tool sees EOF immediately and produces a result (clean, crash, or the honest "cannot answer here") instead of a timeout, which is strictly better information at strictly less cost. If the 95 imports turn out slow (any tool importing torch costs ~2 s before it prints anything), a --jobs N would be the next lever -- but DEVNULL is one keyword argument and removes the pathological tail.

Also worth keeping: SKIP_DIR_NAMES excludes build, third_party, .venv, node_modules, __pycache__ and .git, so the scan cannot wander into a vendored tree, and the summary reports N asked, M not asked rather than only "no tool crashes" -- which is what lets a reviewer see the coverage from the CI log instead of trusting a green tick.

[bong-water-water-bong]

VERDICT: READY at 051a2e9 — five tools were opening a file called --help, and the check's four-result table is the right shape to keep it fixed.

The fault is stated as a list of real exceptions, not a principle

tools/make_tokenizer_golden.py    IndexError (no args) / open('--help')
tools/mtp_depth_pool.py           FileNotFoundError: '--help'
tools/oga_attn_tile_golden.py     FileNot[FoundError: '--help']
…  (five in total)

"--help arrives as a filename" is the whole mechanism in one clause: sys.argv[1] is an input path, so a user asking the tool to describe itself makes it try to read a file with that name. open('--help') in the exception is the receipt, and having five of them means it is a convention the tree never had rather than one author's slip.

The check's result table is the part to keep

result	when
FAIL	--help is in the traceback's last line
FAIL (INTERNAL)	the script we globbed could not be started at all
skip	a missing dependency, a git call at import, a data file
clean	usage line with any exit code, or a normal answer

Four outcomes, and each is a different claim:

• --help in the last line is the precise failing condition — not "nonzero exit", which would flag every tool that exits 2 on usage error, a legitimate convention;
• FAIL (INTERNAL) for "the script could not be started" is the distinction I have asked of four other checks tonight: a checker that cannot run its subject must not report it as clean (tests_referenced: cover scripts/*.sh self-tests, where one arm had no registration at all (#2129) #2238's "green because nothing ran"). Naming it INTERNAL makes the tool's own breakage visible;
• skip is enumerated with its reasons (amd-rai-sweep's onnxruntime, a git call at import, a data file) rather than a blanket "environment" excuse — that is what makes the skips auditable later.

Two asks

1. say whether the five fixes were a shared helper or five hand-edits — if each tool now does its own if '--help' in sys.argv[:1], the sixth tool will get it wrong; a two-line tools/cli_help.py that every entry point calls makes the rule structural. tool_help_check.py catches the miss either way, but the fix cost differs by a factor of n;
2. state the skip count on the current tree — "N of M tools checked, K skipped" in the run's output, because a check that skips a third of its subjects and prints only the passes reads as complete coverage.

[bong-water-water-bong]

VERDICT: CHANGES at 051a2e9 — fresh configure rc 0, and the fixes are right (I ran two of them: make_tokenizer_golden.py --help and mtp_depth_pool.py --help both print usage now). The check as delivered has three defects, and one of them writes to the tree.

1. It creates a file named --help in the repository root.

$ rm -f -- --help ; python3 tools/ci/tool_help_check.py --check --root .
$ ls -la -- --help
-rw-rw-r-- 1 bcloud bcloud 172019 Nov 12 02:14 --help      # the check CREATED it
$ git status --porcelain
?? --help

That is the check doing what the check is about: it runs each tool with --help as argv[1], and a tool that treats its argument as a path opens it for writing. The probe must run every tool inside a throwaway directory (subprocess(..., cwd=tmp)), never in the tree root — otherwise the ratchet leaves an untracked 172 KB file behind, which fails git status cleanliness and can be committed by accident. This is also the highest-severity finding: a check that writes to the repo will eventually overwrite something somebody owns.

2. Its results depend on whether someone has built in the tree.

build-host/ present : 7 tool(s) crash on --help, of 164 asked   rc=1
build-host/ removed : 5 tool(s) crash on --help, of 157 asked   rc=1

One of the extra "crashers" is build-host/_deps/httplib-src/test/dns_test_fixture.py — a vendored dependency's test fixture inside a build directory. A ratchet whose population is a build artefact will differ between a fresh clone and a working tree, so the same commit can pass on one box and fail on another. Skip build directories.

3. Three of the five remaining "crashes" are not CLI tools. tests/fake_lemonade.py, tests/fake_spawn_exec.py and tests/batch_check_state_ci.py are fixtures and CI drivers that are invoked with arguments by other tests, so --help is legitimately not their interface — and onnx/tests/onnx_conv_integer_types_test.py is a test file in a vendored tree. Counting them makes the ratchet red for a property nobody promised. Scope it to tools/** and, if tests/** matters, give those files an explicit exemption comment rather than relying on them staying broken.

What is right and worth keeping: tool_help_check.py's own arms are good — a missing dependency is not asked, not a defect and a crash whose traceback never names the flag is not this check's finding — and the summary line separating asked from not asked (12) is exactly the not-run-versus-passed distinction. The four fixed tools answering --help is a real improvement; it is the harness around them that needs the three changes above.

Scope of my check: fresh configure rc 0; ran the check in-tree and with build-host/ removed (7 vs 5, both rc=1); ran two of the fixed tools' --help; confirmed the stray file is created by the check and deleted it (-- --help).

[github-actions]

Persistent review updated to latest commit 4d2e1aa

[bong-water-water-bong]

VERDICT at 4d2e1aa: READY

Reviewed at 4d2e1aa62593585378296dcb8a1d7750237aa971. It is 3 behind main and merges clean. The diff is 8 files, +284/-6.

Checked

• The check on this tree. tools/ci/tool_help_check.py --check --root tools returns rc 0: "no tool crashes on --help (88 asked, 7 not asked)". The 7 not asked are modules with no __main__.
• The self-test. --self-test returns rc 0.
• check_pins. tests/check_pins_test.py passes. CI calls check_pins.py "$BASE" with exactly one argument, so the new no-argument path (usage on stderr, rc 2) does not touch CI. No caller catches CalledProcessError from git(), so turning it into a one-line SystemExit loses nothing.
• The earlier CHANGES at 051a2e9. Points 2 (build directories in the population) and 3 (non-CLI files in tests/) are answered by the --root tools scope in the ctest row. Point 1 is discussed under Note below.

My mutants (each restored, and git status is clean)

mutant	result
tools/mtp_depth_pool.py back to main (the --help branch gone)	rc 1: "1 tool(s) crash on --help, of 88 asked"
check_pins.py's --help branch removed (the silent-success shape)	rc 1: "check_pins.py -- --help raises: exited 0 and printed nothing"

So the check catches both a crash and a silent exit 0.

Note (non-blocking): point 1 of the 051a2e9 CHANGES
The probed tools still run with cwd set to the repo root. That is deliberate, so check_pins's git config -f .gitmodules works, and the ctest row sets WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}.

Today nothing writes there: after my runs, neither ./--help nor tools/--help exists. But a future tool that treats argv[1] as an output path would write --help into the source root on every ctest run, and the check would count that tool as clean.

A cheap guard: record whether <cwd>/--help exists before the scan. If the scan creates it, remove it and FAIL, naming the tool.

CI build at this head: completed success, "100% tests passed, 0 tests failed out of 264". tool_help_check (#9) and tool_help_check_selftest (#10) both passed there.

[bong-water-water-bong]

VERDICT at 4d2e1aa: READY

"--help must not be a traceback" is a gate on the claim every tool makes about itself. A tool that prints a usage line is asserting it can be run; if --help raises, that assertion is false, and the failure is usually not the argument parser but an import at module scope — an optional dependency, a missing path, a circular import — which means the tool was broken for its real purpose too, and nobody knew because nothing ran it.

The check itself is the familiar shape and the right one: --check --root for the tree, --self-test for the check, both registered as ctest rows.

Six tools fixed is the evidence that the rule was worth having — and it is the fourth checker in this family to find real defects on its first run, after dupdef_check (a shadowed def self_test()), the f16 ratchet (27 invisible bodies) and the self-test-registration rule (pkg-proof.sh). That pattern is now strong enough to state as a principle: a new check that finds nothing on this tree has probably been written to match the tree.

Naming the fix in the PR (check_pins.py, laya_finetune/metrics.py, three *_golden.py, mtp_depth_pool.py) means a reader can see the class rather than an opaque sweep.

[bong-water-water-bong]

VERDICT: READY at 4d2e1aa — the fifth result class is the best idea in this PR, and it names the worst failure of the three.

"Asked a CLI for help and it said nothing, successfully"

return ('silent', 'exited 0 and printed nothing')

That is a result class I have not seen before in this tree, and it is the correct one:

• a crash says "I broke" — annoying, honest, fixable;
• a traceback names the file and line;
• silent, exit 0 says the request succeeded and answers nothing. check_pins.py did exactly this: --help passed len(sys.argv) == 2, so it became a base ref, git diff returned nothing, and the tool exited 0 having printed nothing — "a check that answered a request for help with a silent success."

Naming that as its own outcome (rather than folding it into clean because rc == 0) is what turns the check from "no tracebacks" into "the tool responded". It is the same insight as #2238's unregistered self-test and #2232's UNKNOWN bucket, in its most compact form: the absence of an error is not the presence of an answer.

And the two smaller findings are stated as sub-cases

• --help/dd_metastate_… — "two of those on --help/dd_metastate_…, which is what a path join with the flag looks like": the flag became a directory in a joined path. That distinguishes two mechanisms (argv index vs path join) that produce the same class of bug;
• check=True → uncaught CalledProcessError — "a one-line message and a non-zero exit is what a command-line check should do": a typo in a base ref used to be a traceback, now it is a diagnosis.

Two asks, carried because they are still open

1. the skip count in the output — SKIP_DIR_NAMES and the enumerated skip reasons are right, but if the run prints only its passes, a check that skips a third of its subjects reads as complete coverage. N checked, M skipped (reasons), K silent, F failed on one line is the fix, and it is more valuable than any individual arm;
2. the 5 fixes are per-tool, not shared — only tool_help_check.py is common, so each tool now does its own if sys.argv[1] in ("-h", "--help"). The sixth tool will get it wrong, and the check will catch it after the fact. A two-line tools/cli_help.py makes the rule structural; if that is out of scope, say so and name the check as the enforcement.