diff --git a/CMakeLists.txt b/CMakeLists.txt index 73db2e28d..1759dbe7b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -219,6 +219,14 @@ add_test(NAME kernel_set_check COMMAND bash ${CMAKE_SOURCE_DIR}/tests/kernel_set # gets one row. add_test(NAME dupdef_check COMMAND python3 ${CMAKE_SOURCE_DIR}/tools/ci/dupdef_check.py --check WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) +# engine#2255: `--help` must not be a traceback. Five tools indexed sys.argv directly, so the flag +# arrived as a filename -- one with IndexError, the others with `FileNotFoundError: '--help'`, and two +# of those on `--help/dd_metastate_...`, which is what a path join with the flag looks like. Fixed; this +# row keeps the next one from landing. +add_test(NAME tool_help_check COMMAND python3 ${CMAKE_SOURCE_DIR}/tools/ci/tool_help_check.py --check --root tools + WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) +add_test(NAME tool_help_check_selftest COMMAND python3 ${CMAKE_SOURCE_DIR}/tools/ci/tool_help_check.py --self-test + WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) add_test(NAME bump_hrx_report COMMAND bash ${CMAKE_SOURCE_DIR}/tests/bump_hrx_report_test.sh ${CMAKE_SOURCE_DIR}/scripts/bump-hrx-report.sh) add_test(NAME metrics_serve COMMAND ${CMAKE_SOURCE_DIR}/tests/metrics_serve.sh $) diff --git a/tools/check_pins.py b/tools/check_pins.py index 359d7d016..08f27886c 100755 --- a/tools/check_pins.py +++ b/tools/check_pins.py @@ -38,7 +38,12 @@ def git(*args): - return subprocess.run(["git", *args], capture_output=True, text=True, check=True).stdout + r = subprocess.run(["git", *args], capture_output=True, text=True) + if r.returncode != 0: + # check=True turned a typo in the base ref into an uncaught CalledProcessError traceback. + # A one-line message and a non-zero exit is what a command-line check should do. + raise SystemExit(f"check_pins: `git {' '.join(args)}` failed: {r.stderr.strip().splitlines()[-1] if r.stderr.strip() else r.returncode}") + return r.stdout def hrx_validation_error(path, new, validated): @@ -63,8 +68,16 @@ def compare(repo, old, new): def main(): + # `--help` used to pass `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. It is the worst shape of the three found in this family -- a crash at least says + # it broke, and this says "all fine". + if len(sys.argv) == 2 and sys.argv[1] in ("-h", "--help"): + print(__doc__) + return 0 if len(sys.argv) != 2: - sys.exit(__doc__) + print(__doc__, file=sys.stderr) + return 2 base = sys.argv[1] cfg = git("config", "-f", ".gitmodules", "--get-regexp", r"submodule\..*\.(path|url)") subs = {} diff --git a/tools/ci/tool_help_check.py b/tools/ci/tool_help_check.py new file mode 100644 index 000000000..be886afa8 --- /dev/null +++ b/tools/ci/tool_help_check.py @@ -0,0 +1,219 @@ +#!/usr/bin/env python3 +# Copyright 2026 bong-water-water-bong +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""tool_help_check.py -- `--help` must not be a traceback. + +Four tools in this tree indexed sys.argv directly, so `--help` arrived as a filename: one died with +IndexError, three with `FileNotFoundError: '--help'` (two of those on +`--help/dd_metastate_Llm_Token_Token_modellayers.0…`, which is what a path join with the flag looks +like). A user asking a tool what it does got a stack trace, and nothing in CI noticed because nothing +runs `--help`. + +The rule this checks is deliberately narrow: **no traceback**. A tool that prints a usage line and +exits 2 is fine -- that is a style choice, and several tools here use it. A traceback is not. + +Usage: + tool_help_check.py --check [--root DIR] + tool_help_check.py --self-test +""" + +import ast +import os +import pathlib +import subprocess +import sys + +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. +TIMEOUT_S = 30 + + +def is_cli(path): + """A tool that has a `__main__` block and reads sys.argv. A pure module is not a CLI, so silence + on --help from one is not a defect -- and three of the six silent files under tools/ are exactly + that (tools/q4nx_pack.py, tools/q4nx_t2s.py, tools/laya_finetune/head.py have no entry point).""" + try: + tree = ast.parse(pathlib.Path(path).read_text(errors='replace')) + except (SyntaxError, OSError): + return False + has_main = False + for node in ast.walk(tree): + if isinstance(node, ast.If): + src = ast.dump(node.test) + if '__name__' in src and '__main__' in src: + has_main = True + return has_main and 'sys.argv' in pathlib.Path(path).read_text(errors='replace') + + +def tools_under(root): + for f in sorted(pathlib.Path(root).rglob('*.py')): + if any(part in SKIP_DIR_NAMES for part in f.parts): + continue + if f.name == pathlib.Path(__file__).name: + continue + yield f + + +def crashes(path, root, cwd=None): + """(crashed, detail) for `python3 --help`. A timeout is not a crash: it is a tool we cannot + ask cheaply, and reporting it as broken would be a claim we did not measure.""" + env = dict(os.environ, PYTHONDONTWRITEBYTECODE='1') + # Absolute, because cwd is the scan root: a relative path would resolve to / and + # python would exit 2 with "can't open file" and NO traceback, which this function would have + # read as a clean run. That false pass is the defect this check exists to catch, in its own body. + cwd = pathlib.Path.cwd() if cwd is None else cwd + script = pathlib.Path(path).resolve() + try: + # cwd is where the caller stands, not the scan root: these tools are documented to run from + # the repo root, and running them from tools/ changes their answer (check_pins.py's + # `git config -f .gitmodules` fails there and it looked unaskable rather than silent). + p = subprocess.run([sys.executable, str(script), '--help'], cwd=str(cwd), env=env, + capture_output=True, text=True, timeout=TIMEOUT_S) + except subprocess.TimeoutExpired: + return ('skip', f'timed out after {TIMEOUT_S}s') + err = p.stderr or '' + if "can't open file" in err: + # The script we globbed could not be started. That is never a pass and never a skip: it means + # this checker handed python a path it cannot open, i.e. a defect in here. Kept distinct from + # a crash so an arm can tell "asked and crashed" from "never asked", which a two-valued + # result cannot (the first version of this arm passed with the bug reintroduced). + return ('internal', 'could not be started: ' + err.strip().splitlines()[-1][:80]) + if 'ModuleNotFoundError' in err or 'ImportError' in err: + # A tool whose dependency is not installed on this box cannot be asked for help. That is + # absence of a result, not a defect: reporting it would be a claim we did not measure. + return ('skip', 'a dependency is not installed here') + if 'Traceback (most recent call last)' in err: + last = [ln for ln in err.strip().splitlines() if ln.strip()][-1].strip() + # The subject is "`--help` was treated as an argument", so the *evidence* has to be the flag + # appearing as a path or value. A tool that dies for another reason -- a missing dependency, + # a data file that is not in the tree, a git call at import -- cannot answer --help here and + # is not this check's finding. Attributing those to --help would be a claim the measurement + # does not support (each of the three was misattributed before this clause existed). + if '--help' in last or "'-h'" in last: + return ('fail', last[:120]) + return ('skip', 'cannot answer here: ' + last[:100]) + if p.returncode == 0 and not (p.stdout or '').strip() and not err.strip(): + # Asked a CLI for help and it said nothing, successfully. A crash at least tells you it broke; + # this tells you nothing at all and looks like a clean run -- the shape check_pins.py had + # (--help became a base ref, no diff, exit 0). + return ('silent', 'exited 0 and printed nothing') + return ('clean', f'rc {p.returncode}') + + +def check(root): + bad, skipped, n = [], [], 0 + for f in tools_under(root): + rel = f.relative_to(root) + n += 1 + state, detail = crashes(f, root) + if state == 'silent': + if is_cli(f): + bad.append((rel, detail)) + else: + skipped.append((rel, 'not a CLI (no __main__), so --help is not its question')) + continue + if state == 'skip': + skipped.append((rel, detail)) + elif state in ('fail', 'internal'): + bad.append((rel, detail)) + for rel, detail in bad: + print(f"tool_help: {rel} -- `--help` raises: {detail}") + for rel, detail in skipped: + print(f"tool_help: {rel} -- not asked ({detail})") + if bad: + print(f"tool_help_check: {len(bad)} tool(s) crash on --help, of {n - len(skipped)} asked " + f"({len(skipped)} not asked)") + return 1 + print(f"tool_help_check: no tool crashes on --help ({n - len(skipped)} asked, {len(skipped)} not asked)") + return 0 + + +def self_test(): + 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 / 'crashes.py').write_text("import sys\nopen(sys.argv[1]).read()\n") + (root / 'clean.py').write_text("import sys\nprint('usage: clean.py ')\nsys.exit(2)\n") + (root / 'helps.py').write_text("print('usage: helps.py')\n") + chk(('fail', 'clean'), (crashes(root / 'crashes.py', root)[0], crashes(root / 'clean.py', root)[0]), + 'a traceback where the flag is the opened path is a crash; a usage line with rc 2 is not') + chk('clean', crashes(root / 'helps.py', root)[0], 'a tool that prints usage and exits 0 is clean') + + # The false pass that this checker shipped with: cwd is the scan root, so a RELATIVE path + # resolved to /, python exited 2 with "can't open file" and no traceback, and the + # tool was counted as clean without ever being asked. Resolved to absolute now. + sub = root / 'sub' + sub.mkdir() + (sub / 'dirs.py').write_text("import sys\nopen(sys.argv[1]).read()\n") + # The exact shape that produced the false green: the glob yields paths prefixed by the scan + # root while cwd IS the scan root, so / is //... . The arm has to + # reproduce that (a relative path with cwd = root), not pass an absolute one -- the first + # version of this arm did the latter and passed with the bug reintroduced. + cwd0 = os.getcwd() + os.chdir(root.parent) + try: + rel = pathlib.Path(root.name) / 'sub' / 'dirs.py' + state = crashes(rel, root)[0] + finally: + os.chdir(cwd0) + chk('fail', state, + 'a path relative to the invocation cwd, scanned with cwd=root, is still asked ' + '(with the bug it is INTERNAL: a different state, which fails this arm)') + + (root / 'silentcli.py').write_text( + "import sys\nif __name__ == '__main__':\n _ = sys.argv[1:]\n") + (root / 'module.py').write_text("print('imported, not run')\n") + chk('silent', crashes(root / 'silentcli.py', root)[0], + 'a CLI that exits 0 on --help without printing is silent, not clean') + chk((True, False), (is_cli(root / 'silentcli.py'), is_cli(root / 'module.py')), + 'a file with __main__ and sys.argv is a CLI; a plain module is not') + + (root / 'dep.py').write_text("import not_a_real_module_xyz\n") + chk('skip', crashes(root / 'dep.py', root)[0], 'a missing dependency is not asked, not a defect') + + (root / 'other.py').write_text("open('data-that-is-not-here.bin').read()\n") + chk('skip', crashes(root / 'other.py', root)[0], + 'a crash whose traceback never names the flag is not this check\'s finding') + print(f"tool_help_check --self-test: {'ok' if rc == 0 else 'FAIL'}") + return rc + + +def main(argv): + if '--self-test' in argv: + return self_test() + if '--check' not in argv: + print(__doc__.strip().splitlines()[-1].strip()) + print('usage: tool_help_check.py --check [--root DIR] | --self-test') + return 2 + root = '.' + for i, a in enumerate(argv): + if a == '--root' and i + 1 < len(argv): + root = argv[i + 1] + return check(root) + + +if __name__ == '__main__': + sys.exit(main(sys.argv[1:])) diff --git a/tools/laya_finetune/metrics.py b/tools/laya_finetune/metrics.py index 66a5bdcb5..8278ec71a 100755 --- a/tools/laya_finetune/metrics.py +++ b/tools/laya_finetune/metrics.py @@ -29,5 +29,10 @@ def m(path): "log_loss": round(ll, 4), "min_p_true": round(min(r["probabilities"][r["label"]] for r in rows), 4), "ms_p50": round(st.median(r["decision_ms"] for r in rows))} if __name__ == "__main__": + # `--help` arrives in sys.argv[1:] like any path, so it was opened literally and died with + # FileNotFoundError: '--help'. A metrics printer takes file paths; say so instead of crashing. + if not sys.argv[1:] or "-h" in sys.argv[1:] or "--help" in sys.argv[1:]: + print(__doc__ or "usage: metrics.py [ ...]") + sys.exit(0 if sys.argv[1:] else 2) for p in sys.argv[1:]: print(p, m(p)) diff --git a/tools/make_tokenizer_golden.py b/tools/make_tokenizer_golden.py index 7a35688d6..3f3d969ee 100755 --- a/tools/make_tokenizer_golden.py +++ b/tools/make_tokenizer_golden.py @@ -43,6 +43,16 @@ "a" * 300, ] +USAGE = "usage: tools/make_tokenizer_golden.py [label]" +if "-h" in sys.argv[1:2] or "--help" in sys.argv[1:2]: + # The script indexes argv directly, so --help used to arrive as tok_path and fail with + # IndexError (no args) or an open() on a file named "--help". + print(USAGE) + sys.exit(0) +if len(sys.argv) < 3: + print(USAGE, file=sys.stderr) + sys.exit(2) + tok_path, out = sys.argv[1], sys.argv[2] label = sys.argv[3] if len(sys.argv) > 3 else tok_path tok = tokenizers.Tokenizer.from_file(tok_path) diff --git a/tools/mtp_depth_pool.py b/tools/mtp_depth_pool.py index 828455bc4..281a309f4 100755 --- a/tools/mtp_depth_pool.py +++ b/tools/mtp_depth_pool.py @@ -481,6 +481,11 @@ def refusal_selftest(): def main(argv): + # --help is a request, not a path: without this it arrives as argv[1] and the tool + # opens a file literally named "--help" (FileNotFoundError) instead of saying anything. + if "-h" in argv or "--help" in argv: + print(__doc__ or "usage: see the header of this file") + return 0 selftest = "--selftest" in argv paths = [a for a in argv if a != "--selftest"] if not paths: diff --git a/tools/oga_attn_tile_golden.py b/tools/oga_attn_tile_golden.py index 4f301229e..45dc13999 100644 --- a/tools/oga_attn_tile_golden.py +++ b/tools/oga_attn_tile_golden.py @@ -112,10 +112,19 @@ def tiles_q4_1(code, scale, zp): def main(argv): - if len(argv) < 2: + # --help is a request, not a path: without this it arrives as argv[1] and the tool + # opens a file literally named "--help" (FileNotFoundError) instead of saying anything. + if "-h" in argv or "--help" in argv: + print(__doc__ or "usage: see the header of this file") + return 0 + positional = [a for a in argv[1:] if not a.startswith("-")] + if not positional: + # The usage line is ` [--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(argv[1]), "--check" in argv + pack, check = Path(positional[0]), "--check" in argv tile = read_tile(pack) code, scale, zp, bias = decode(tile) q4 = tiles_q4_1(code, scale, zp) diff --git a/tools/oga_mlp_gu_golden.py b/tools/oga_mlp_gu_golden.py index 7fee62f00..7fc22bd5c 100644 --- a/tools/oga_mlp_gu_golden.py +++ b/tools/oga_mlp_gu_golden.py @@ -90,10 +90,19 @@ def half(code, scale, zp, up: bool): def main(argv): - if len(argv) < 2: + # --help is a request, not a path: without this it arrives as argv[1] and the tool + # opens a file literally named "--help" (FileNotFoundError) instead of saying anything. + if "-h" in argv or "--help" in argv: + print(__doc__ or "usage: see the header of this file") + return 0 + positional = [a for a in argv[1:] if not a.startswith("-")] + if not positional: + # The usage line is ` [--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(argv[1]), "--check" in argv + 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))