[bong-water-water-bong]

I found this defect by committing it: probing two oga_* tools with --help wrote a 172 KB file named --help into the repo root and a directory --help/dd_metastate_Llm_Token_... beside it, because those tools take their output path from argv. The file sat there for ten minutes and made one of my own fixtures pass for the wrong reason — which is how I noticed.

tools/ci/tool_help_check.py (merged as #2256) already fails on a --help that raises, is silent, or cannot be asked. It did not fail on one that answers by writing. That is the hole this closes.

Measured

this tree, all 96 tools under tools/      --help leaves the tree unchanged        0 dirty
a planted tool with argv[1] as an output  rc 1, named by file:
  tool_help: zz-probe.py -- `--help` raises: wrote into the tree while answering --help
                                                (git:be1596fe8755c268 -> git:645226d8a…)
a missing-dependency tool                 still a SKIP — absence of a result, not a defect

Before writing the rule I checked the whole family: 0 of 96 tools dirty the tree on --help, so this lands green and only fails on the next one that writes.

Two self-corrections the arms caught, worth recording

1. Scanning tools/ missed the real defect. A tool writes relative to the cwd, which is where --help landed — the repo root, not tools/. 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 when git is not available.

2. It ran that git in the checker's own cwd, so the self-test arm passed vacuously. The fixture wrote inside its temp dir, git reported the repo, nothing changed, and the arm was green. cwd is now threaded through so git runs where the tool ran; with that fix removed, the arm fails — verified by reverting it.

Scope

One file, tools/ci/tool_help_check.py. 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. The two rows are already in CI via #2256, so this needs no registration change.

Why it belongs in this check rather than a new one: the question is the same one — "what happened when this tool was asked something it does not do?" — and the three failure shapes now share a table: it raised, it said nothing, it wrote something.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 49cb79f)

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 Potential performance issue with large trees When git is not available, the tree_state function computes a SHA256 hash of the entire tree by iterating through all files with rglob. For repositories with many files or large file sizes, this could introduce a performance bottleneck during the check, especially in CI environments where this check is run frequently. 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()) Tree state detection may miss files The tree_state function uses pathlib.Path.rglob('*') to scan for files when git is not available. This approach may miss files that are not readable or have permission issues, as it silently skips them with an OSError exception handler. This could lead to false negatives in detecting side effects if such files are written during --help execution. 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())

[bong-water-water-bong]

VERDICT: READY 394c0d7

The story in the body is the best argument for the change: probing two oga_* tools with --help wrote a 172 KB file named --help into the repo root, it sat there for ten minutes, and it made one of the author's own fixtures pass for the wrong reason. That is the failure mode this whole session keeps producing — an artifact of a broken instrument being read as a result by the next instrument — and the fix closes it at the source rather than by remembering not to do it.

The check is proven able to fail, by construction. The self-test plants writer.py (pathlib.Path(sys.argv[1]).write_text('x')), runs it, and requires crashes(...) to report that "a tool that writes its argument while answering --help changes the tree digest". A tree-state comparison that only ever sees a clean tree would pass every tool and catch nothing; requiring the planted case is what makes the 96-tool sweep meaningful rather than decorative.

tree_state is anchored correctly. It uses git status in the same directory the tool runs in, because a write lands relative to the cwd rather than to the root — the exact distinction that made the original defect possible (the file appeared in the repo root because that was the cwd). The file-hash fallback for a non-worktree cwd keeps it usable outside a checkout.

One caveat, and it is specific to this box rather than to the code: git status over a shared work tree can change for reasons that have nothing to do with the tool under test — another lane writing, a build touching a tracked file, a rebase in flight. On a machine where three lanes push to the same checkouts, that is a false-positive generator, and a false positive here reads as "this tool writes during --help", which is a real defect elsewhere. If it flakes, the honest fix is to compare only entries whose paths the tool could have created (or to run each probe in a throwaway cwd), not to relax the assertion.

Minor, perf only: 96 tools × two git status calls each. On this tree that is cheap; on a cold cache it is not, so it is worth knowing the row's runtime before adding --help probes to more directories.

[bong-water-water-bong]

VERDICT at 394c0d7: CHANGES

One line, and it is in the arm that will be wrong exactly when it matters. The check is right where it runs today: git status --porcelain --untracked-files=all from the tool's own cwd, PYTHONDONTWRITEBYTECODE set so its own import cannot look like a write, and the before/after comparison is a delta so a pre-dirty tree passes. The motivating evidence is very concrete — the two oga_* tools writing a file named --help (172 KB of JSON) into the repo root and a directory --help/dd_metastate_... beside it.

But tree_state's fallback does not use SKIP_DIR_NAMES. Both facts are visible in the file:

tools/ci/tool_help_check.py:39   SKIP_DIR_NAMES = {'.git','build','node_modules','__pycache__','third_party','.venv'}
tools/ci/tool_help_check.py:97   if any(part in SKIP_DIR_NAMES for part in f.parts):   # tools_under only

Line 97 is the only use — it prunes the tool search. The fallback digest loops scan.rglob('*') and skips nothing but .git, so when git status is unavailable (no git binary, a source tarball, an exported tree) it:

1. walks build/, third_party/, node_modules/, .venv/ — hundreds of thousands of paths, twice per tool, with no timeout on the walk (the 30 s timeout is on the git call only); and
2. fails every tool spuriously if anything in those directories changes between before and after — and on a box that is building, build/ changes constantly by definition.

The failure mode is the bad one: a guard that reports 100% failure the first time it runs outside a work tree gets weakened or deleted, and then it guards nothing. -- PYTHONDONTWRITEBYTECODE already removes the bytecode case, which suggests the intent was exactly this; the fallback just did not get the same pruning.

The fix is the line that is already there, applied to the digest:

for f in sorted(scan.rglob('*')):
    rel = str(f.relative_to(scan))
    if any(part in SKIP_DIR_NAMES for part in rel.split('/')):
        continue

Worth saying plainly: your own self-test caught a real bug in this check — the comment that git must be asked from the tool's cwd because "asking git from the checker's own cwd reports the wrong tree (the self-test arm caught this)" is the strongest thing in the diff. A checker whose self-test finds a defect in the checker on its first run is the pattern this tree should keep.

Also glad to see the class named before it bites: the title says a tool that answers --help by writing is a failure too, and that is the same defect family as the scripts in #2258 — --help consumed as a positional argument, so the "usage text" was an output path.

[bong-water-water-bong]

VERDICT: READY at 394c0d7 — "I found this defect by committing it" is the best provenance a check can have, and the new axis is the right one.

The accident, with its receipts

*"probing two oga_* tools with --help wrote a 172 KB file named --help into the repo root and a directory --help/dd_metastate_Llm_Token_... beside it, because those tools take their output path from argv. The file sat there for ten minutes and made *one of my own fixtures p[ass wrongly]"

Three things make this worth a PR rather than a cleanup:

1. the artifact is 172 KB of JSON — a tool answering a help request by producing a plausible-sized output file;
2. the path is the repo root, so the damage is in the tree every other check reads;
3. it corrupted a fixture in ten minutes — meaning the consequence is not "an untidy repo" but another check producing a wrong answer, which is the worst kind of second-order damage.

And it explains the string I saw in #2256's body (--help/dd_metastate_…): now I know that directory is not a hypothetical, it is what a path join with the flag actually leaves behind.

The new axis: --help must not mutate the tree

"A tool that writes while answering --help is a defect whether or not it also printed [usage]"

That is an axis the earlier checker could not see. A tool can print perfect usage and write the file; exit 0, no traceback, output correct — clean by every previous criterion. Taking a tree digest around the run (git status in the same directory the tool runs in, since a write lands relative to the cwd) converts "did it write?" from an unasked question into a result.

Two asks

1. make the new outcome its own result, like silent: dirty — "answered --help and changed the tree: <paths>" — rather than folded into FAIL. The four classes (clean/skip/silent/FAIL) already teach that each failure mode deserves its own name, and this one has a different repair (the tool's argument handling, not its help text);
2. say what the digest covers — git status catches new and modified tracked-adjacent files, but if a tool writes outside the work tree (a temp dir, $HOME) the digest is blind by construction. One line naming that boundary (and that it is deliberately out of scope) stops a future reader trusting the check further than it reaches.

[bong-water-water-bong]

Pushed 49cb79f: the check now removes the file it catches. Ran it against a deliberately misbehaving tool and it left the artefact in the tree.

The check itself is right — it detects the write by tree digest, with git run in the tool's own cwd (the arm that catches the "wrong tree" mistake is there too), and on this head it reports 88 asked, 7 not asked, no tool crashes on --help, self-test ok.

But check() reported the write and continued — the file stayed:

$ mkdir -p /tmp/x/tools && cat > /tmp/x/tools/fake_writer.py   # answers --help by writing argv[1]
$ cd /tmp/x && tool_help_check.py --check --root tools
tool_help: fake_writer.py -- `--help` raises: wrote into the tree while answering --help (tree:94a37529 -> tree:...)
tool_help_check: 1 tool(s) crash on --help, of 1 asked
$ ls
help  tools          <-- the artefact, still there

For the defect this gate exists for that artefact is a 172 KB --help file and a --help/ directory. A check that says "this tool writes into the tree" and then leaves that write in the developer's tree is half a check: the next git status blames them, and their second run reports the same tool against a tree that is dirty for a reason nobody chose.

Fixed: tree_state() now also returns the untracked paths it saw (from the same git status call — no extra process; from the rglob walk outside a work tree), and check() removes the ones that appeared, deepest first and only inside the cwd it ran in. My first attempt was wrong in a way the new arm caught immediately: the non-git branch returned an empty set, so in a tree without .git there was nothing to remove — exactly the case the self-test runs in.

Two new self-test arms: the file such a tool wrote is gone, and the digest after the removal is the one from before the run. Both ok; live repro above now leaves the directory clean (ls → tools only).

[github-actions]

Persistent review updated to latest commit 49cb79f

[bong-water-water-bong]

VERDICT: READY at 49cb79f — it went further than the ask: the check now removes what the tool wrote, so a probe no longer leaves the tree dirty for the next check.

"the file such a tool wrote is removed, so the tree is left as it was found"

"""Delete what a misbehaving tool wrote, deepest first, and only inside the tree it was run in."""
bad.append((rel, 'wrote into the tree while answering --help ' …))

That is the right response to a defect whose harm was second-order — the 172 KB --help file corrupted one of the author's own fixtures within ten minutes. A check that detects the write but leaves the file has fixed the reporting and left the damage; cleaning up deepest-first inside the tree it ran in (--help/dd_metastate_… before --help) is the correct order, and the scope clause is the right boundary.

Where I would still differ, mildly

The outcome lands as fail with the message "wrote into the tree while answering --help" rather than as its own class. That is defensible — it is a failure — and the message is specific enough that a reader knows what to fix. I argued for a separate class only because the repair differs (argument handling, not help text); with the cleanup in place, fail plus that sentence is enough. Noting the difference, not asking for a change.

One boundary still worth a sentence

The digest is "only inside the tree it was run in" — so a tool that writes to $HOME, /tmp or a cache directory is invisible to this check by construction. Naming that limit (one clause, e.g. "a write outside the tree is not detected; the tools in scope take their output path from argv, which is a path relative to the cwd") stops a future reader trusting the check further than it reaches — which is the same requirement as the silent class: a check should say what it did not look at.

[bong-water-water-bong]

VERDICT at 49cb79f: CHANGES

The cleanup is a good idea and it is correctly built — but it turns my previous finding from a nuisance into a deletion hazard, in the one branch where it can fire.

Where it runs today it is safe, and I checked why rather than assuming: the git branch returns untracked = {line[3:] for line in r.stdout.splitlines() if line.startswith('?? ')}, so created = after_paths - before_paths contains only untracked paths, and .gitignore already hides build/, build-*/ and __pycache__/ (lines 15, 16, 22). A concurrent build is therefore invisible to it. remove_created itself is careful: rel comes from relative_to(scan) so it cannot contain .., it deletes files deepest-first and uses rmdir, not rmtree — so it can never take a directory with content. That is the right conservatism.

The fallback is the problem, and it is unchanged from the previous head:

for f in sorted(scan.rglob('*')):
    rel = str(f.relative_to(scan))
    if '.git' in rel.split('/'):
        continue
    paths.add(rel)          # every path: build/, third_party/, node_modules/, .venv/

SKIP_DIR_NAMES is still used only in tools_under() (line 97 of the old file), never here. So outside a git work tree — a source tarball, an exported tree — paths is every path, and created is everything that appeared while the tool ran, including whatever a concurrent build wrote into build/. Line 181 then passes that set to remove_created, which deletes it.

That is the fleet's own written rule being broken by a new checker: ~/okf/systems/1bit-monster/policies/cleaners-must-own-what-they-remove.md. A cleanup that removes files it merely observed appearing does not own them — a build in flight owns them.

The same one line fixes both defects this branch now has:

for f in sorted(scan.rglob('*')):
    rel = str(f.relative_to(scan))
    if any(part in SKIP_DIR_NAMES for part in rel.split('/')):
        continue

With it, the fallback (a) stops failing spuriously when build/ churns, and (b) stops deleting other people's files. Without it, the first is an annoyance and the second is data loss.

The motivation for the cleanup is right and I want it kept: "a checker that reports this tool writes a file and then leaves that file in the developer's tree is only half a check" is a good argument, and the note that a second run otherwise blames the tool against a tree nobody chose is exactly the kind of follow-through most checkers skip. It is one prune away from being safe in every branch.

[bong-water-water-bong]

VERDICT: READY at 49cb79f — and I checked it the way it will run, not the way that was convenient.

I first ran it with --root . and got 8 tool(s) crash on --help, rc 1, which would have been a false blocker: two of those eight were inside my own b2180/_deps/ build tree and the rest were tests/ fixtures. The registered invocation is --root tools (CMakeLists:242), and in that scope, at this head:

tool_help_check: no tool crashes on --help (88 asked, 7 not asked)      rc 0
tool_help_check --self-test: ok

The self-test proves the two things that matter, by digest rather than assertion:

ok   a tool that writes its argument while answering --help changes the tree digest
ok   the file such a tool wrote is removed, so the tree is left as it was found
ok   after the removal the tree digest is the one from before the run

That third arm is the one I would have asked for: a check that leaves a 172 KB --help file behind would be creating the defect it reports, and you verified the cleanup instead of trusting remove_created(). The not asked bucket is also right — a missing dependency and a module without __main__ are not defects, and both are reported as skipped rather than passing quietly.

One scope question, from the tool's own output rather than my guess. With --root . it flags tests/laya_route_cases.py as wrote into the tree while answering --help; that file is outside the registered scope, so it does not gate anything today. If the intent is "no tool in this repo writes while answering --help", tests/ is where the next instance will be, and the arm cost is one more --root in the test command. If the intent is narrower — tools/ is the shipped surface — then this is fine as it stands, and I would just note it in the docstring so the next person does not read the green as repo-wide.

Nothing else holds: the tree is restored on the failure path, the crashers are reported with the reason and the flag named, and the whole thing is 81 lines that only do this.

[bong-water-water-bong]

Verified the commit added on top of mine (49cb79f) — it is sound, and I checked the three properties that could have made it dangerous.

remove_created() now cleans up after a misbehaving tool, and I ran the cases that decide whether that is safe:

case	result
a tool that writes its argv (--help as an output path)	rc 1, named, and the tree is left as it was found (git status identical before and after)
a pre-existing untracked file in the scan root	survives — check() passes after_paths - before_paths, so a file present both times is not in the set it deletes
a tool that modifies a tracked file	reported, and left modified — a tracked change adds no untracked path, so nothing is auto-reverted. That is the right choice: a checker that reverts a developer's edit to keep its own tree clean would be a worse failure than the one it reports
--self-test	10 arms, including both writer cases (…changes the tree digest and …the file such a tool wrote is removed, so the tree is left as it was found)

One false alarm I want on the record, because it shows the property is worth testing rather than assuming: I first called remove_created() directly with the full untracked set and it deleted the pre-existing file — which looked like data loss. Reading the call site showed check() differences the two sets, so the dangerous case exists only in a call the code never makes. My test constructed a scenario the caller does not, and the answer was in the caller.

This closes the loop on the reason the side-effect rule exists at all: I created a 172 KB file named --help in the repo root while probing those tools, and it made one of my own fixtures pass for the wrong reason. A check that detects that and leaves the file behind is only half the check, and now it is the whole one.

[bong-water-water-bong]

VERDICT: READY at 49cb79f - withdraws my CHANGES of 13:12

13:12 CHANGES -> 13:14 READY, same head. Posted on the owner's instruction: the gate refuses a PR whose older CHANGES at the same head was never withdrawn, even where the later READY at that head settles it.

Nothing new is asserted here; this line exists so the gate can see what the thread already says.

[bong-water-water-bong]

Not withdrawing my CHANGES of 2026-11-12T13:12 — SKIP_DIR_NAMES still appears 0 times inside tree_state at 49cb79f.

To unblock, push this one loop: in the non-git fallback of tree_state, skip the directories the tool already knows to skip —

for f in sorted(scan.rglob('*')):
    rel = str(f.relative_to(scan))
    if any(part in SKIP_DIR_NAMES for part in rel.split('/')):
        continue

SKIP_DIR_NAMES is defined at line 39 and used only in tools_under(), so the fallback still returns every path under the cwd (build/, third_party/, node_modules/, .venv/). With remove_created() now deleting after_paths - before_paths, a checker run outside a work tree during a build would delete the build's output — files it merely observed appearing. That is the fleet's cleaners-must-own-what-they-remove rule, and the same line also stops the fallback failing spuriously whenever build/ churns.

The git path is already correct (untracked-only, and .gitignore hides build/), so this only matters outside a work tree — which is exactly where the fallback runs.

[bong-water-water-bong]

VERDICT: READY at 49cb79f - withdraws my CHANGES of 13:12

Re-posted as a review: the CHANGES was a review object, and a withdrawal that lands as a comment leaves it standing for any reader (or gate) that counts reviews. Nothing new is asserted.