From 394c0d77b5d45110ab3e48bc683652b633a2e9e4 Mon Sep 17 00:00:00 2001 From: lane Date: Thu, 12 Nov 2026 05:25:40 -0400 Subject: [PATCH 1/2] tool_help_check: a tool that answers --help by WRITING is a failure too I found this by doing it: two `oga_*` tools take their output path from argv, so probing them wrote a 172 KB file named `--help` into the repo root and a directory `--help/dd_metastate_...` beside it. The file sat there ten minutes and made one of my own fixtures pass for the wrong reason. `check()` now snapshots the tree around every tool and fails on any change. Measured before/after: this tree, all 96 tools under tools/ --help leaves the tree unchanged 0 dirty a planted tool with argv[1] as an output path rc 1, named by file a missing-dependency tool still a SKIP, not a write **The snapshot had to be fixed twice, and the arms caught both:** 1. Scanning `root` (tools/) missed the real defect, because a tool writes relative to the **cwd** -- which is where `--help` landed. `tree_state` now asks `git status --porcelain --untracked-files=all` first (one process, and it covers the whole work tree) and falls back to an rglob digest. 2. It ran that `git` in the CHECKER's cwd, so the self-test arm passed vacuously: the fixture wrote inside its temp dir, git reported the repo, nothing changed. `cwd` is now threaded through and git runs where the tool ran. **With the fix removed the arm fails** -- verified by reverting it. Self-test 8 -> 9 arms, all ok; `ctest tool_help_check` 23.78 s and `_selftest` 0.26 s, both Passed; `copyright --check` rc 0. --- tools/ci/tool_help_check.py | 51 +++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tools/ci/tool_help_check.py b/tools/ci/tool_help_check.py index be886afa8..0e20660a2 100644 --- a/tools/ci/tool_help_check.py +++ b/tools/ci/tool_help_check.py @@ -30,6 +30,7 @@ """ import ast +import hashlib import os import pathlib import subprocess @@ -58,6 +59,39 @@ def is_cli(path): return has_main and 'sys.argv' in pathlib.Path(path).read_text(errors='replace') +def tree_state(root, cwd): + """A digest of the tree a tool could write into. + + `git status` when the cwd is inside a work tree -- a tool writes relative to the cwd, not to the + directory it lives in, so scanning `root` alone misses the exact defect this is for: the two + `oga_*` tools wrote a file named `--help` (172 KB of JSON) into the repo ROOT and a directory + `--help/dd_metastate_...` beside it. git reports both as untracked, and it costs one process + instead of hashing every path. + """ + try: + # git in the SAME directory the tool runs in -- a write lands relative to that cwd, and asking + # git from the checker's own cwd reports the wrong tree (the self-test arm caught this). + r = subprocess.run(['git', 'status', '--porcelain', '--untracked-files=all'], + cwd=str(cwd), capture_output=True, text=True, timeout=30) + if r.returncode == 0: + return 'git:' + hashlib.sha256(r.stdout.encode()).hexdigest()[:16] + except (OSError, subprocess.TimeoutExpired): + pass + h = hashlib.sha256() + scan = pathlib.Path(cwd) + for f in sorted(scan.rglob('*')): + rel = str(f.relative_to(scan)) + if '.git' in rel.split('/'): + continue + h.update(rel.encode()) + if f.is_file(): + try: + h.update(str(f.stat().st_size).encode()) + except OSError: + h.update(b'?') + return 'tree:' + h.hexdigest()[:16] + + 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): @@ -118,7 +152,15 @@ def check(root): for f in tools_under(root): rel = f.relative_to(root) n += 1 + before = tree_state(root, pathlib.Path.cwd()) state, detail = crashes(f, root) + after = tree_state(root, pathlib.Path.cwd()) + if before != after: + # A tool that writes while answering `--help` is a defect whether or not it also printed + # something: the file it wrote is named after an argument, and nothing will clean it up. + bad.append((rel, f'wrote into the tree while answering --help ' + f'({before} -> {after})')) + continue if state == 'silent': if is_cli(f): bad.append((rel, detail)) @@ -192,6 +234,15 @@ def chk(want, got, what): '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") + # The side-effect rule, in the shape that produced it: a tool whose argv[1] is an output path. + (root / 'writer.py').write_text( + "import sys, pathlib\nif __name__ == '__main__':\n" + " pathlib.Path(sys.argv[1]).write_text('x')\n") + before = tree_state(root, root) + crashes(root / 'writer.py', root, cwd=root) + chk(False, before == tree_state(root, root), + 'a tool that writes its argument while answering --help changes the tree digest') + 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") From 49cb79f8cdbb11d7eb7a3f561cc2a43654d40b1d Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Thu, 12 Nov 2026 08:53:35 -0400 Subject: [PATCH 2/2] tool_help_check: remove what a misbehaving tool wrote, so the gate does not leave the artefact behind --- tools/ci/tool_help_check.py | 44 +++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/tools/ci/tool_help_check.py b/tools/ci/tool_help_check.py index 0e20660a2..e1ee3e25b 100644 --- a/tools/ci/tool_help_check.py +++ b/tools/ci/tool_help_check.py @@ -60,7 +60,7 @@ def is_cli(path): def tree_state(root, cwd): - """A digest of the tree a tool could write into. + """A digest of the tree a tool could write into, and the untracked paths inside it. `git status` when the cwd is inside a work tree -- a tool writes relative to the cwd, not to the directory it lives in, so scanning `root` alone misses the exact defect this is for: the two @@ -74,22 +74,44 @@ def tree_state(root, cwd): r = subprocess.run(['git', 'status', '--porcelain', '--untracked-files=all'], cwd=str(cwd), capture_output=True, text=True, timeout=30) if r.returncode == 0: - return 'git:' + hashlib.sha256(r.stdout.encode()).hexdigest()[:16] + untracked = {line[3:] for line in r.stdout.splitlines() if line.startswith('?? ')} + return 'git:' + hashlib.sha256(r.stdout.encode()).hexdigest()[:16], untracked except (OSError, subprocess.TimeoutExpired): pass h = hashlib.sha256() scan = pathlib.Path(cwd) + paths = set() for f in sorted(scan.rglob('*')): rel = str(f.relative_to(scan)) if '.git' in rel.split('/'): continue + paths.add(rel) h.update(rel.encode()) if f.is_file(): try: h.update(str(f.stat().st_size).encode()) except OSError: h.update(b'?') - return 'tree:' + h.hexdigest()[:16] + return 'tree:' + h.hexdigest()[:16], paths + + +def remove_created(cwd, created): + """Delete what a misbehaving tool wrote, deepest first, and only inside the tree it was run in. + + A checker that reports "this tool writes a file while answering --help" and then leaves that file in the + developer's tree is only half a check: the next `git status` blames them, and a second run reports the same + tool against a tree that is now dirty for a reason nobody chose. + """ + scan = pathlib.Path(cwd) + for rel in sorted(created, key=lambda r: r.count('/'), reverse=True): + p = scan / rel + try: + if p.is_file() or p.is_symlink(): + p.unlink() + elif p.is_dir(): + p.rmdir() # only succeeds when the tool's own contents are gone/empty + except OSError: + pass def tools_under(root): @@ -152,10 +174,11 @@ def check(root): for f in tools_under(root): rel = f.relative_to(root) n += 1 - before = tree_state(root, pathlib.Path.cwd()) + before, before_paths = tree_state(root, pathlib.Path.cwd()) state, detail = crashes(f, root) - after = tree_state(root, pathlib.Path.cwd()) + after, after_paths = tree_state(root, pathlib.Path.cwd()) if before != after: + remove_created(pathlib.Path.cwd(), after_paths - before_paths) # A tool that writes while answering `--help` is a defect whether or not it also printed # something: the file it wrote is named after an argument, and nothing will clean it up. bad.append((rel, f'wrote into the tree while answering --help ' @@ -238,10 +261,17 @@ def chk(want, got, what): (root / 'writer.py').write_text( "import sys, pathlib\nif __name__ == '__main__':\n" " pathlib.Path(sys.argv[1]).write_text('x')\n") - before = tree_state(root, root) + before, before_paths = tree_state(root, root) crashes(root / 'writer.py', root, cwd=root) - chk(False, before == tree_state(root, root), + after, after_paths = tree_state(root, root) + chk(False, before == after, 'a tool that writes its argument while answering --help changes the tree digest') + # And the check undoes the write: the file the tool created is gone, and so is its digest change. + remove_created(root, after_paths - before_paths) + chk(False, (root / '--help').exists(), + 'the file such a tool wrote is removed, so the tree is left as it was found') + chk(True, before == tree_state(root, root)[0], + 'after the removal the tree digest is the one from before the run') chk('skip', crashes(root / 'dep.py', root)[0], 'a missing dependency is not asked, not a defect')