[bong-water-water-bong]

Found while answering a review ask that repeated twice: tools/hrx_f16_narrowing_check.py should print which corpus it read. Adding that, I hit the reason nobody had noticed the tool is not run in CI — the file defines def self_test() twice.

The defect

def self_test():        # line 163 -- three scan arms: mul-updated / add-only / f32 control
    ...
def self_test():        # line 185 -- twelve classify arms
    ...

The second wins. The first is dead. Nothing warns: the file imports, --self-test prints twelve ok lines, and the three arms in the shadowed copy cannot fail because they never run. That is a check that cannot fail wearing the costume of two checks — the failure mode this tree has a rule about.

I removed the dead copy (22 lines, no assertion lost: its three cases exist in the surviving body as class 3: …) and added the thing that catches the next one.

tools/ci/dupdef_check.py

An AST walk for a top-level def/class defined more than once, reporting name (lines N, M) and the consequence. Scoped to tools/ and scripts/, skipping third_party/build/node_modules/.venv — a duplicate in vendored code is upstream's business.

Five self-test arms, and two of them are controls:

• two top-level defs of one name reported with both line numbers;
• a method defined twice inside a class is not a top-level shadow;
• distinct names are not a finding;
• an unparseable file is not this check's finding — absence of a result, not a pass;
• the exact hrx_f16_narrowing_check.py shape, reproduced as a fixture.

before the fix   dupdef_check --check --root tools/hrx_f16_narrowing_check.py   rc 1  (self_test lines 163, 185)
after            same command                                                    rc 0
whole tree       --check over tools/ + scripts/                                  rc 0, 0 shadowed definitions

The corpus is now named and fingerprinted

The original ask: a result is only comparable to another if the tree it read is identified.

CORPUS       /home/bcloud/.cache/scratch-trel3/fx
FINGERPRINT  8dc60499acc5444b (2 .loom file(s))

sha256 over each file's relative path and bytes, sorted — so a moved file changes it, not just an edited one. Four arms: the file count, the same corpus twice (identical), a one-byte edit (different), a rename (different).

Both run in CI without touching ci.yml

add_test rows, since the workflow is not editable from a lane:

1/2 Test #8: dupdef_check .....................   Passed    0.47 sec
2/2 Test #9: hrx_f16_narrowing_selftest .......   Passed    0.08 sec
100% tests passed, 0 tests failed out of 2

hrx_f16_narrowing_check.py --self-test is now 16 arms rc 0 (was 12). ci_targets_check --check still reports 9 baseline / none new, so the new rows add no drift.

One honest note on how I got here

My first attempt to add the hashlib import replaced a substring that did not exist and printed "hashlib imported" anyway — the --self-test then failed with NameError. A print that reports work it did not do is the same defect as an arm that cannot fail, which is why the arms above assert the fingerprint against a real temporary corpus rather than that the function exists.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit bbacf7b)

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 Shadowed Definition Detection The new tool dupdef_check.py correctly identifies shadowed top-level definitions in Python files, which is important for preventing dead code that may appear to pass tests but is never executed. The tool is scoped to tools/ and scripts/ directories and skips vendored or generated code, which is appropriate. def duplicates(path): """[(name, [lineno, ...])] for top-level def/class names defined more than once.""" try: tree = ast.parse(path.read_text(errors='replace')) except (SyntaxError, OSError): # A file we cannot parse is not this check's finding; whoever owns syntax reporting says so. return [] seen = {} for node in tree.body: if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): seen.setdefault((node.name, type(node).__name__), []).append(node.lineno) return [(name, lines) for (name, _kind), lines in sorted(seen.items()) if len(lines) > 1] Corpus Fingerprinting The addition of fingerprinting logic in hrx_f16_narrowing_check.py ensures that the corpus used in the check is explicitly named and identified, improving reproducibility and traceability of results. This is a good enhancement for the tool's usability. def fingerprint(root): """sha256 over the corpus: sorted (relative path, file digest), so a moved file changes it too.""" h = hashlib.sha256() n = 0 for p in sorted(root.rglob('*.loom')): h.update(str(p.relative_to(root)).encode()) h.update(hashlib.sha256(p.read_bytes()).digest()) n += 1 return f"{h.hexdigest()[:16]} ({n} .loom file(s))"

[github-actions]

Persistent review updated to latest commit bbacf7b

[bong-water-water-bong]

VERDICT at bbacf7b: READY

This catches a class that bit this fleet twice today, and neither time was it caught by anything automatic.

• #2086 registered the same add_test(NAME bump_hrx_report …) twice — comment and all, in one hunk. CTest does not reject a duplicate name; it registers two tests with that name, so it passed CI and would have left ctest -R running the fixture twice for ever, plus a duplicate row nobody would question.
• #1790/#1794 both added the same implicit-zero-point block, so merging both applied the work twice.

Both are "a second definition that shadows the first and nothing notices", which is exactly what dupdef_check is for — and it is the same shape as the ratchet problem the fleet solved earlier tonight: a check can only see what it looks for, and a duplicate definition reads as a valid file to every other check.

Running it via python3 … --check with WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} is right for a whole-tree scan, and "" in the title ("and the f16 …") suggests it found something already, which is the best sign a new checker is real.

[bong-water-water-bong]

VERDICT: READY at bbacf7b — fresh configure rc 0, dupdef_check.py --self-test passes, and --check --root . reports no shadowed top-level definition in 175 file(s).

The finding is the best kind of process bug: a check whose arms were silently not running. tools/hrx_f16_narrowing_check.py defined def self_test() twice; the second won and the first was dead — "nothing warns: the file imports, --self-test prints twelve ok lines, and the three arms [in the shadowed copy] never ran". That means a ratchet looked green while testing a subset of what it claimed, which is exactly the failure the ratchet was written to prevent, one level up.

The check is shaped to be checkable itself, which is the part I care about most:

• an unparseable file is not this check's finding (absence of a result, not a pass) — a file it cannot parse is reported as no result, not as clean, so the check cannot pass by failing to look (the same distinction as the 77-skips elsewhere tonight);
• the hrx_f16_narrowing_check.py shape (a shadowed self_test) is caught — the self-test carries the real-world instance it was built from, so a regression in the detector is caught by the case that motivated it.

Two asks:

1. Say what it cannot see. A shadowed definition inside a class, a conditional definition (if TYPE_CHECKING:), or a try/except ImportError fallback is legitimate Python that a top-level text/AST scan may flag or miss. One sentence in the module docstring listing the shapes it does not cover keeps the check from being read as "no duplicate definitions anywhere" — the same discipline as --require-initialised on the submodule pin.
2. A --fix-less stance is right, but say so. Nothing here rewrites a file, which is correct for a ratchet; one clause stating that decision stops the next lane from adding one.

Scope of my check: fresh configure rc 0, the --self-test (including the shadowed-self_test arm) and --check over 175 files (clean). No build beyond configure: this is a Python check and its own arms are the evidence.