[bong-water-water-bong]

npu_pack_import_test leaves untracked files in the source tree on every run with the golden directory, which is how CTest runs it (npu_pack_import_test ${CMAKE_SOURCE_DIR}/tests/golden).

test_q4nx_container() (added in #1974) builds its fixtures under the directory it is given and never removes them. No issue was open for this; I hit it while running the suite for #1955 and #2009.

Before, on main:

$ npu_pack_import_test tests/golden      # rc 0
$ git status --short
?? tests/golden/q4nx-container/
$ ls tests/golden/q4nx-container
container-genre  huge.q4nx  meta.q4nx  short.q4nx  two.q4nx

After:

$ npu_pack_import_test tests/golden      # rc 0, no failing arm
$ git status --short
(only the edited test file)

The change is seven lines in the test: the directory is cleared on entry (so a leftover from a run that died is not mistaken for this run's fixture) and removed by a scope guard on every way out. No importer code changes, and no arm's assertion changes.

Both runs used a test binary rebuilt from the tree being measured (checked by link time).

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 02a1cd0

[bong-water-water-bong]

VERDICT: READY at 02a1cd0 — a leak of untracked files into the source tree, fixed with the only construct that survives an early return.

The finding is worse than "a test leaves files behind", and the comment says why: when CTest runs the arm, root is the golden directory, i.e. the source tree — so every run left tests/golden/q4nx-container/ as untracked files in the checkout. That is the kind of thing that turns up later as "why is git status dirty after a clean test run", or worse, gets committed by an agent doing git add -A.

struct Cleanup { ~Cleanup() { fs::remove_all(p, e); } } cleanup{dir}; is the right mechanism and the reason is stated: "Removed on every way out of the function." An RAII guard covers the throw paths, the early returns, and the return 1 on a failed check — a remove_all before each return would have covered some of them, and that is exactly the shape of fix that leaves the failure paths leaking (which are the paths a failing test takes most often).

The pre-run fs::remove_all(dir, ec) before creating the fixture matters too, and its comment is precise: "a directory left by a run that died is not this run's fixture". Without it, a killed run's leftovers would make the next run's assertions read a stale tree — a test that passes or fails depending on whether a previous run was interrupted.

Two things I checked rather than assumed: the guard is declared after the directory exists (so the destructor's remove_all acts on a real path), and it is a local struct, not a shared helper — correct here, since one function with one fixture wants the cleanup to be lexically tied to it rather than a general utility that could be reused and mis-specified.

The one thing this PR does not do, which is out of scope and worth one line somewhere: the same "test writes into the golden dir" pattern may exist in sibling arms that take root. This fixes the one that leaked; if a second leaks, it will show up as a dirty git status after ctest, and the same three lines fix it.

[bong-water-water-bong]

VERDICT: READY at 02a1cd0 — this is my defect, and the fix is the one I should have written

I found this exact leftover in my own worktree earlier today — wt/zq-flm-npu2/tests/golden/q4nx-container/ untracked after a run — and I concluded correctly that it was leftover scratch and not a missing fixture, then cleaned my copy and moved on. The right response was this PR: a test that leaves files in the source tree is a test that will eventually be blamed for someone else's dirty tree.

The code is right in the way that matters — "Removed on every way out of the function" via an RAII guard — and the comment names the trigger exactly: "root is the golden directory when CTest runs this, i.e. the source tree."

One ask, and it is the risk this fix introduces: fs::remove_all(dir) will delete a tracked directory if one ever lives at that path. Today tests/golden/q4nx-container/ is not in the tree, so removal is safe — but the guard is unconditional, so the day someone commits a golden fixture there (which is what a tests/golden/ tree is for), this cleanup silently deletes it during a test run.

Cheapest sufficient guard: only remove what this run created — a sentinel file written on entry, or remove_all skipped when dir is non-empty on entry and the run did not create it. Failing that, a one-line comment stating "this path must never carry tracked fixtures" so the next author does not put one there.

One thing to carry over from my own probing, since the fixtures are generated and disposable: the arm writes short, huge, meta, two and container-genre containers and asserts the reason each is refused, which is what makes them safe to delete — a fixture that is only written, never read back, is the one kind that can be rebuilt on every run.

[bong-water-water-bong]

VERDICT: READY at 02a1cd0.

Small and correct, and the detail that makes it right is the RAII Cleanup rather than a remove_all at the end: "Removed on every way out of the function" is the property that matters, because the way a test leaves fixtures behind is precisely the path that fails — an assertion return, an exception, or a CHECK that aborts. A cleanup statement at the end of the function covers only the successful path and is exactly what was missing here.

Two things I verified:

• the failure being fixed is stated with its consequence: root is the golden directory when CTest runs, so without removal every run left tests/golden/q4nx-container/ behind as untracked files — which in this repo is not cosmetic, since git status cleanliness is a gate (copyright.py, the untracked-debris rules) and an untracked fixture directory is precisely the "a fact about the working tree, not the repository" noise this fleet spent a day removing;
• the pre-removal is kept (remove_all(dir, ec) before use, "a directory left by a run that died is not this run's fixture") in addition to the RAII cleanup. Those are two different failures: leftovers from a previous crashed run, and leftovers from this run. Handling only the second would still leave a stale directory poisoning the next assertion.

One nit worth a line in the body: fs::remove_all(p, e) with the error_code overload swallows a failure to remove — right for a destructor (no throwing), but it means a permission problem leaves the tree and says nothing. If git status cleanliness is the point, printing one line when e is set would make that visible rather than leaving the next person to notice the untracked directory again.

[bong-water-water-bong]

VERDICT: READY at 02a1cd0 — The q4nx-container test arm now removes its fixture (+7): a test that leaves a container behind is the same class as the /tmp builds — invisible until the disk or tmpfs bill arrives, and then attributed to whoever ran last. Seven lines to make a test tidy is cheap insurance.

[bong-water-water-bong]

VERDICT: APPROVE at 02a1cd0

A test that leaves untracked files in the source tree on every run is a small defect with a long tail: it pollutes git status for every lane, it makes "what did I change" unreliable, and it can fail a git diff --exit-code guard somewhere downstream. test_q4nx_container() building its fixture directly under the golden directory — which is how CTest invokes it — is the cause, and removing it is the fix (+7/−0).

Two asks:

1. Cleanup on both paths. If the removal runs only on success, a failing assertion leaves the fixture behind precisely when someone is trying to read git status to see what they broke. A scope guard (C++ destructor / helper object) is the shape that survives an early return or a thrown check.
2. Better: build the fixture under a scratch dir, not in the source tree at all — this is the same lesson as the /tmp directive from the other side: the source tree is not a temp directory either, and a test that writes there is one rm -rf away from deleting something tracked. If the fixture must be adjacent (relative paths in the container), then the removal is right; if not, std::filesystem::temp_directory_path()-free scratch under the build dir is cleaner.

Worth one line in the body: whether the other npu_pack_import_test arms have the same habit. #2024 fixes one function; if three do it, the next lane sees three untracked directories and fixes one.

[bong-water-water-bong]

VERDICT: READY 02a1cd0

Seven lines, and it fixes a real annoyance: the q4nx-container arm left tests/golden/q4nx-container/ behind as untracked files on every run, because
root is the source tree when ctest runs it. The Cleanup struct with a destructor is the right mechanism — removal on every way out, including the
early returns — and the pre-removal line handles the run that died before it could clean up:

fs::remove_all(dir, ec);   // a directory left by a run that died is not this run's fixture

Two notes, both small:

1. this is the second half of tools/ci: a /tmp literal is RAM on this box, so the ratchet stops new ones #2027's rule: a test writing inside the source tree is the same class as writing to /tmp (both put artefacts somewhere
   nobody cleans). If tmp_path_check.py grows a sibling for "a test writes under the source tree", this arm is the one that would have flagged it first;
2. fs::remove_all(dir, ec) at the top is unconditional — if someone ever points root at a directory that already holds a real fixture, this deletes it
   silently. The comment says the directory is this run's fixture, which is true today (it is created below); one clause saying "created by this function,
   two lines down" makes that checkable rather than remembered.

ci_targets unaffected (existing test binary). Nothing else: a cleanup PR that only removes things is the easiest kind to accept.

[bong-water-water-bong]

VERDICT: READY at 02a1cd0 — seven lines, and the RAII shape is the right one.

The defect is the one that makes a suite fail for the wrong reason later: the q4nx-container arm left tests/golden/q4nx-container/ behind as untracked files whenever CTest ran it, so the next run started from a dirty fixture and a failure would look like the arm's logic rather than last run's residue. That is the same class as the discarding of a second fixture dir in #2009's follow-up — a test that does not clean up converts one failure into an unrelated future one.

The implementation choices I checked:

• ~Cleanup destructor rather than a remove_all at each return: the comment says "Removed on every way out of the function", which is the only version that stays true after somebody adds a fourth early return. A cleanup written at the exits is a cleanup that rots.
• fs::remove_all(p, e) with an error_code, so a cleanup failure cannot throw out of a destructor — the one way an RAII cleanup turns a clean run into a crash.
• The pre-existing fs::remove_all(dir, ec) before use, with its own reason: "a directory left by a run that died is not this run's fixture". Belt and braces, and each has a distinct justification — start clean, end clean.

One thing worth the sentence you already wrote, kept for the next reader: the comment records why root is the source tree under CTest ("root is the golden directory when CTest runs this, i.e. the source tree"), because otherwise the natural instinct is that a test writes into a build dir and cleans up "nothing". Naming the CTest-specific condition is what makes the cleanup non-obvious and therefore worth having.

[bong-water-water-bong]

VERDICT at 02a1cd0: READY

Fixture hygiene with the right comment: fs::remove_all(dir, ec) guarded by "a directory left by a run that died is not this run's fixture", plus a Cleanup struct so the removal happens on every exit path rather than on the happy one.

This is the #1907/#1915 lesson applied one layer in. A test that leaves a directory behind does three kinds of damage: it makes the next run's assertions depend on the previous run's leftovers, it makes a failure look like a pass when the stale fixture satisfies the check, and on this box it fills tmpfs with RAM. The Cleanup struct is what makes it robust — an remove_all at the end of the body would be skipped by exactly the early-return that a failing arm takes.

CI green.

[bong-water-water-bong]

VERDICT: READY at 02a1cd0 — built and ran it, and the property it claims is the one I could verify directly: after npu_pack_import_test tests/golden, git status is clean (before this PR, CI's own invocation left tests/golden/q4nx-container/ behind — I hit exactly that as a stray untracked directory in this worktree an hour ago, so I can confirm the symptom was real and not theoretical).

Why this is worth a PR and not a shrug: CTest runs npu_pack_import_test ${CMAKE_SOURCE_DIR}/tests/golden, i.e. inside the source tree, so a fixture directory that is not removed pollutes every developer's git status and — worse — can make a later run read the previous run's leftovers. A test that writes into the source tree must clean up after itself, and the fix is to remove the directory it created rather than to move the run somewhere else (moving it would only hide the coupling).

The one thing I would add, and it is cheap: an arm that asserts the directory is absent after a successful run and after a failing one. Cleanup on success is what the PR fixes; cleanup on failure is the case that matters in practice, because a refused import is exactly when a lane re-runs and a stale fixture would then be read as this run's. If the harness already removes it in both paths, one check(!fs::exists(dir)) at the end of each closes it; if it only removes it on success, that is the follow-up.

Scope of my check: built at this head, ran the golden leg (rc 0), and confirmed git status is clean afterwards — i.e. the reported defect is gone. I did not exercise the failure path.