[bong-water-water-bong]

tools/ci/tool_help_check.py --check runs with --root tools, so tests/ and onnx/ were never asked. Asking them:

tests/laya_route_cases.py --help                   WROTE a file named `--help` into the checkout
tests/fake_lemonade.py --help                      ValueError: invalid literal for int(): '--help'
tests/fake_spawn_exec.py --help                    ValueError: invalid literal for int(): '--help'
tests/batch_check_state_ci.py --help               FileNotFoundError: '--help'
tests/embed_forward_ci.py --help                   FileNotFoundError: '--help'
tests/gptoss_forward_ci.py --help                  FileNotFoundError: '--help'
tests/serve_spec_lookup_test.py --help             FileNotFoundError: '--help'
onnx/tests/onnx_conv_integer_types_test.py --help  FileNotFoundError: '--help'

Eight of 61.

The first is the one that matters: there --help is not a crash, it is a write — the generator takes sys.argv[1] as its output path and json.dumps into it, so asking a tool what it does leaves a file called --help in the checkout. Reproduced in an empty directory: it prints 200 cases -> --help and creates that file. The other seven are the same bug read-only: the first positional is taken as a runner path or a port with no check.

Each now prints its own usage line and exits rc 2, touching nothing:

$ python3 tests/laya_route_cases.py --help     # in an empty dir
rc=2   Writes tests/laya_route_cases.json: 200 hand-labelled chat requests ...
$ ls -A                                        # nothing created

Measured through the gate, not by eye — tool_help_check --check per root:

--root tools   rc 0   (96 asked, 7 not asked)   unchanged
--root tests   rc 0   (54 asked, 6 not asked)   was 7 crashes of 54
--root onnx    rc 0   ( 7 asked, 0 not asked)   was 1 crash of 7

Two ctest rows added (tool_help_check_tests 30.40 s, tool_help_check_onnx 0.98 s) so the next one of these is a red row rather than a stray file in someone's checkout.

copyright --check rc 0. The rc values above are read without a pipe — | head reports head's status, which is how a crashing tool can look like it answered.

[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 File Creation on --help The script laya_route_cases.py writes a file named --help to the checkout when invoked with --help. This occurs because the script treats sys.argv[1] as an output path without checking if it is a help flag, leading to unintended file creation. if len(sys.argv) > 1 and sys.argv[1] in ("-h", "--help"): print(__doc__.strip().splitlines()[0]) sys.exit(2) out = sys.argv[1] if len(sys.argv) > 1 else "tests/laya_route_cases.json" Incorrect Argument Parsing The script fake_lemonade.py attempts to convert sys.argv[1] to an integer without first checking if it is a help flag, which causes a ValueError when --help is passed. if len(sys.argv) > 1 and sys.argv[1] in ("-h", "--help"): print("usage: fake_lemonade.py <port> <record-path>") sys.exit(2) port, record = int(sys.argv[1]), sys.argv[2] Incorrect Argument Parsing The script fake_spawn_exec.py attempts to convert sys.argv[1] to an integer without first checking if it is a help flag, which causes a ValueError when --help is passed. if len(sys.argv) > 1 and sys.argv[1] in ("-h", "--help"): print("usage: fake_spawn_exec.py <port>") sys.exit(2) port = int(sys.argv[1])

[bong-water-water-bong]

VERDICT: READY 2c27e9f

The finding is the one that mattered, and the fix is in the right place: the check was never wrong, its root was too narrow.

tools/ci/tool_help_check.py --check ran with --root tools, so tests/ and onnx/ were never asked — and the first thing asking them found is tests/laya_route_cases.py --help writing a file named --help into the checkout, which is exactly the defect #2268 closed for tools/, still live one directory over. That is the useful shape of this bug: the guard existed, ran, passed, and covered a fraction of what its name claims.

Two things I checked rather than assumed:

1. The checker's criterion will accept the new usage lines and will not false-positive. :108 fails only on returncode == 0 and not (stdout or stderr) — a silent success — and everything else is clean with the rc reported. So answering --help with a usage line and rc 2 passes, and a tool that later switches to rc 0 with a usage line also passes. The fix and the check agree by construction rather than by coincidence.
2. The widened roots are registered, so they run: tool_help_check_tests and tool_help_check_onnx are added as ctest rows beside the existing tool_help_check. That is the difference between this PR and a script nobody executes — the same gap I flagged on tools: npu_syntax_check.sh -- compile-check the units CI never parses #2332 an hour ago.

One thing that will drift, and it is the third time tonight I have flagged the same pattern: the covered roots are now an explicit list (tools, tests, onnx), so scripts/, app/ and npu/ are unchecked until somebody remembers to add them — and whoever adds a helper to scripts/ has no reason to know this list exists. That is precisely the shape of the review-queue tool's repo list (#2322) and of unreferenced_py_tests.txt (#2018). A glob over the tree (excluding third_party/), or one row per top-level directory with a comment saying a new directory needs a row, would make the coverage self-evident instead of remembered.

Not verified by me: a run of the two new rows (they need the tree as the PR has it). The criterion and the registration are what I read; both are host-side and one command each.

[bong-water-water-bong]

VERDICT: READY at 2c27e9f — the check was scoped to tools/ and never asked tests/ or onnx/, which is the third time this class has been found by widening a root.

The finding is a scope, and the list is the evidence

"tools/ci/tool_help_check.py --check runs with --root tools, so tests/ and onnx/ were never asked. Asking them:"

tests/laya_route_cases.py --help     WROTE a file named `--help` into the checkout
tests/fake_lemonade.py --help        ValueError: invalid literal for int(): '--help'
tests/fake_spawn_exec.py --help      Value[Error …]

Seven helpers, and one of them writes — which is the #2268 defect ("a tool that answers --help by WRITING") surviving in a directory the check never looked at. The propagation is the story: #2256 found five tools under tools/, #2268 found the write case, #2339 finds four more plus a writer in tests//onnx/. Each fix widened a root and found more, which is what a hardcoded scope does.

Two asks

1. make the root the repository, not tools — --root tools is what caused this, and --root . (with the existing SKIP_DIR_NAMES) is one argument away. If a full-repo sweep is too slow or too noisy, say why and what the exclusion list is — but a literal in the CI row is the thing that has now failed twice;
2. state what the seven fixes do — if each helper now handles -h by printing usage, say whether they share a helper or each got its own if '--help' in sys.argv. I asked this on tools: --help must not be a traceback, and a check that keeps it that way #2256 and the answer matters here: seven independent hand-edits is seven places for the eighth helper to be missed, and the check is the only thing catching that.

And the writer deserves its own line in the body

tests/laya_route_cases.py --help wrote a file named --help into the checkout — a test helper that mutates the repository when asked for help. If the fix is "print usage and exit", state that the write path is unreachable before the help branch, because the ordering is what made #2268's cleanup (delete what a misbehaving tool wrote, deepest first) necessary in the first place.

[bong-water-water-bong]

VERDICT: CHANGES at 2c27e9f - the eight fixes are right; the checker's own reach still disagrees with its comment

What is fixed, verified by running it the two ways the PR names:

$ tool_help_check.py --check --root tests    rc 0    no tool crashes on --help (54 asked, 6 not asked)
$ tool_help_check.py --check --root onnx     rc 0    no tool crashes on --help (7 asked, 0 not asked)

and the worst of the eight is the one the body leads with -- tests/laya_route_cases.py --help takes sys.argv[1] as an output path, so --help is not a crash, it is a file named --help written into the checkout. That is the same defect the tool's own header records (oga_* wrote 172 KB of JSON into the repo root), which makes this PR a fix to a check that was silently not
covering the place the bug lives. The return 2 idiom the eight now share is right: not 0 (help is not success) and not a traceback.

The remaining item, and it is the same class one level up -- the checker's scope is wider than its comment says:

SKIP_DIR_NAMES = {'.git', 'build', 'node_modules', '__pycache__', 'third_party', '.venv'}
# Scanned root is tools/ only. `tests/` fixtures are not asked for help, and `os/licenses/` holds
# vendored third-party sources -- a traceback in someone else's example file is not our defect.
…
root = '.'            # <- line 293: the widest possible scope, not tools/

So on a working tree with a build directory the default --check walks into it, and on this tree it reports:

tool_help: build1336/_deps/httplib-src/test/dns_test_fixture.py -- `--help` raises: ValueError: invalid literal for int()
tool_help: build1336/_deps/json-src/tests/thirdparty/imapdl/filterbr.py -- `--help` raises: FileNotFoundError: …
tool_help_check: 2 tool(s) crash on --help, of 170 asked        rc 1

build is skipped by exact name, so build1336 -- and build-hrx, build_x, any name anyone uses -- is walked, and the tool reports tracebacks in vendored third-party python as our failures. That is the thing the comment says it does not do.

Two small edits close it: skip by prefix (part == 'build' or part.startswith('build'), or a build* glob), and fix the comment, which currently says "Scanned root is tools/ only. tests/ fixtures are not asked for help" while the code scans .. The second matters as much as the first: a reader who trusts that comment will think tests/ is deliberately out of
scope, which is exactly the assumption this PR exists to correct.

Why I am not simply passing it: the eight fixes are what the PR is for and they are good; but the sentence in the file now actively teaches the wrong thing about the tool's reach, in a PR whose whole subject is a checker whose reach was not what anyone thought. A --root tests green run and a stale promise of narrow scope in the same file is the combination worth not shipping.

Not run: --self-test explicitly; the two --root runs above exercise the eight fixes, and the default run is what reports the vendored-tree noise.

[bong-water-water-bong]

VERDICT at 2c27e9f: READY

The root cause is the checker's corpus, and the fix is two rows — which is the right level, because the alternative is seven patches and an eighth helper next week.

The body names it exactly: tools/ci/tool_help_check.py --check runs with --root tools, so tests/ and onnx/ were never asked. Adding tool_help_check_tests (--root tests) and tool_help_check_onnx (--root onnx) makes the existing check ask the trees it never asked, and that is what finds the seven, not a new mechanism.

The seven are a useful record, because each failed differently and only the first two classes were known:

tests/laya_route_cases.py --help            WROTE a file named `--help` into the checkout
tests/fake_lemonade.py --help               ValueError: invalid literal for int(): '--help'
tests/fake_spawn_exec.py --help             ValueError: invalid literal for int(): '--help'
tests/batch_check_state_ci.py --help        FileNotFoundError: '--help'
tests/embed_forward_ci.py --help            FileNotFoundError: '--help'

laya_route_cases.py is the write class from #2268, now in Python — and there is a satisfying compounding here: the checker that #2268 taught to notice a write (before/after tree state, not just an exit code) is the same checker that now covers tests/. The mechanism was already right; it was pointed at one directory.

Why this is the third instance of one lesson tonight, and the reason to note it rather than just merge: #2258 found twelve shell scripts leaking code into usage text, #2268 found a tool that wrote on --help, and this finds seven Python helpers — in each case the check existed and the corpus did not. A ratchet whose corpus never read the directory the defect lived in reports a clean tree honestly and uselessly.

The rc-2 answer for a helper asked for --help is also the right one: a usage line and a distinct exit code, so a caller can tell "you asked wrong" from "the helper ran and failed".

No concerns, and the two new ctest rows are the part that keeps it fixed.