[bong-water-water-bong]

Refs #2263. A follow-up to #2271 (merged), from that PR's review: "the sweep's own note is still "%.1f GiB -> %.1f GiB (bound %.1f GiB)" while the CLI summary uses the MiB/GiB formatter … worth one size() call in the same file."

Change

• One formatter. q4nx_cache_size_text() in npu/q4nx_cache_sweep.{h,cpp} prints GiB from 1 GiB up and MiB below. It replaces the CLI's local lambda.
• Where it is used. The sweep's note: line and every size 1bit q4nx-cache prints, including the "sweep is off (bound 0)" line, which still printed %.1f GiB.

Before → after, on npu_pack_import_test's sweep arm, which empties a 2 GiB older entry down to the new conversion:

note: q4nx cache: removed 1 entry, 2.0 GiB -> 0.0 GiB (bound 1.0 GiB, …)
note: q4nx cache: removed 1 entry, 2.0 GiB -> 5.3 MiB (bound 1.0 GiB, …)

Tests

• q4nx_cache_test has four new arms (6 MiB, 0, 1 GiB − 1 byte, 60 GiB), and all 36 arms pass.
• Mutant: with the formatter printing GiB for every size, the test returns rc 1 with 3 FAILs. Restored, it returns rc 0.
• npu_pack_import_test (golden legs) returns rc 0.
• Full build is green. ctest -j 6 passes 267 of 267, excluding npu_device_lock, device_holders and device_route. The NPU is off and this build has ONEBIT_NPU=OFF. tools/copyright.py --check returns rc 0.

🤖 Generated with Claude Code

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit d84842a)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2263 - Partially compliant Compliant requirements: The PR addresses formatting consistency between sweep notes and CLI output Non-compliant requirements: The PR does not implement a scheduler or in-repo invocation for the q4nx-cache evict command The PR does not address the core issue of the command not being invoked Requires further human verification: The need for external invocation (scheduler) still needs to be addressed by system configuration

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Size Formatting Logic The new q4nx_cache_size_text() function includes logic to handle the edge case where a value of exactly 1024 MiB is formatted as "1024.0 MiB" but should be displayed as "1.0 GiB" for consistency. This is a good improvement to prevent misleading display of cache sizes. std::string q4nx_cache_size_text(int64_t bytes) { char out[32]; if (bytes >= (int64_t(1) << 30)) { std::snprintf(out, sizeof out, "%.1f GiB", double(bytes) / (1 << 30)); return out; } std::snprintf(out, sizeof out, "%.1f MiB", double(bytes) / (1 << 20)); // Within half a MiB of a GiB the MiB reading rounds to "1024.0", which is a value in the unit it has just left -- the same // kind of misleading exchange this formatter exists to stop. Print the GiB it is about to become. if (std::string(out) == "1024.0 MiB") std::snprintf(out, sizeof out, "%.1f GiB", double(bytes) / (1 << 30)); return out; } Code Duplication Removed The PR removes duplicated size formatting logic from app/q4nx_cache.cpp by replacing it with a call to the newly introduced q4nx_cache_size_text() function. This improves code maintainability and ensures consistent formatting across the application. const auto size = [](int64_t b, char* out, size_t n) { std::snprintf(out, n, "%s", q4nx_cache_size_text(b).c_str()); };

✅ Resolved findings

npu/q4nx_cache_sweep.cpp:27-32

Size Formatting Inconsistency

The PR introduces a new q4nx_cache_size_text() function to provide consistent size formatting (MiB below 1 GiB, GiB above) across the q4nx cache system. This addresses a formatting inconsistency between the sweep's note and CLI output, ensuring that small caches don't appear as "0.0 GiB" which could be misinterpreted as the sweep being off.

[bong-water-water-bong]

Pushed d84842a: verified the note end to end, and the arm was pinning a misleading reading.

Built q4nx_cache_test from this head on the host (-DONEBIT_NPU_HOST=ON) and read the tool's own output rather than the exit code — 36 arms pass, and the note now says what the PR claims, on a real cache:

q4nx-cache: 1 entry removed, 16.0 MiB -> 8.0 MiB (bound 6.0 MiB)
q4nx-cache: .../cli is 16.0 MiB, the sweep is off (bound 0)

All three of those are 0.0 GiB under the old formatter, and a bound of 0.0 GiB is indistinguishable from bound 0, which means the sweep is off — so the bug this fixes is not cosmetic: it made a working sweep read like a disabled one.

The one defect I found, and fixed here: the arm pinned q4nx_cache_size_text((1 << 30) - 1) == "1024.0 MiB". %.1f of anything in the last half-MiB before a GiB rounds to 1024.0, so a sub-GiB size printed a value in the unit it had just left — the same misleading-exchange class this formatter exists to stop, caught in its own boundary arm and locked in as intended. That arm now reads:

check(q4nx_cache_size_text((int64_t(1) << 30) - 1) == "1.0 GiB", "size text: just under 1 GiB is 1.0 GiB, not \"1024.0 MiB\"");
check(q4nx_cache_size_text(int64_t(1023) << 20) == "1023.0 MiB", "size text: a real 1023 MiB still reads MiB");

The second arm is there so the boundary fix cannot over-reach: a genuine 1023 MiB still reads MiB. 36 arms still pass after the change.

[github-actions]

Persistent review updated to latest commit d84842a

[bong-water-water-bong]

VERDICT: READY d84842a

Consolidating two ad-hoc formats into one formatter is the right fix for the note, and the boundary handling is better than the note asked for.

It caught the "1024.0 MiB" trap on its own. A size within half a MiB below a GiB rounds to 1024.0 in MiB — a number expressed in the unit it has just left, which is exactly the misleading exchange this formatter exists to remove. Reprinting that one as 1.0 GiB is the correct resolution, and the comment says why rather than leaving a magic comparison unexplained.

The boundary cases are pinned, not sampled: 6 MiB → "6.0 MiB", 0 → "0.0 MiB", 1 GiB − 1 → "1.0 GiB", 1023 MiB → "1023.0 MiB". That last pair is the one that matters — it shows the rounding rule does not swallow a genuinely 1023-MiB value into a GiB reading, which a test that only checked "small is MiB, large is GiB" would have missed.

The reason for the change is the reason to keep it: a small cache or a zero bound printed as 0.0 GiB reads as "the sweep is off", which is a wrong diagnosis produced by a formatting choice. One formatter for every size the CLI and the sweep print means that reading cannot come back.

One suggestion, non-blocking: the 1024.0 MiB case is detected by comparing the formatted string (std::string(out) == "1024.0 MiB"). That works and is documented, but it silently stops working if the format ever changes (a different precision, a space, a locale-decimal), and it would stop quietly — the output would just go back to printing 1024.0 MiB. Comparing in bytes instead — the threshold is 1 GiB − 512 KiB for one decimal place — keeps the rule independent of the format string. Cheap either way; worth it because this is the kind of function nobody re-tests after a format tweak.

[bong-water-water-bong]

VERDICT: READY -- this is the ask from my #2264 review, and it is delivered one detail further than I asked

My note on #2264 was that the sweep's own note still printed %.1f GiB while the CLI used a MiB/GiB formatter, so a small cache showed 0.0 GiB -> 0.0 GiB one line above 2.0 MiB -> 1.0 MiB, and that the discrepancy got worse when #2271 moved the sweep into a library shared by three binaries. Confirmations:

• One formatter exists -- q4nx_cache_size_text(int64_t) -> std::string in the cache library, declared in its header -- and the only two %.1f GiB left in the cache sources are the two lines inside it. Every call site delegates: the library note's %s -> %s (bound %s), the CLI summary (whose local size lambda is now a one-line adapter rather than a second implementation), and the "the sweep is off (bound 0)" line.
• The two-units problem is gone by construction, so #2271's shared library cannot print GiB from the library and MiB from the CLI for the same sweep.

The detail I did not ask for and would have wanted: within half a MiB of a GiB, MiB formatting yields "1024.0 MiB", which is a value expressed in the unit it has just left -- the same class of misleading exchange this formatter exists to stop. The code notices and prints the GiB it is about to become. A formatter whose author thought about its own edge is worth more than the formatting.

One nit, and it is only about durability: that edge is detected by comparing the formatted string (if (std::string(out) == "1024.0 MiB")), which silently stops working if anyone changes "%.1f MiB" to two decimals. The numeric form costs the same and cannot break that way:

const int64_t kGiB = int64_t(1) << 30;
if (bytes >= kGiB - (int64_t(1) << 19)) { /* print GiB */ }

Not blocking: the current form is correct today, and the comment above it explains the intent, which is what a future editor will read.

Nothing else to raise: the change is small, it removes an inconsistency rather than adding a facility, and the root argument is still passed for the "(bound …, %s)" context that names which cache was swept.

[bong-water-water-bong]

VERDICT: READY at d84842a

Built and ran it: q4nx cache: 36 arm(s) over the sweep, all passing -- and the boundary arms are the ones this formatter needed, including the one that is wrong in the obvious implementation.

The arms, and why the third is the interesting one:

size text: 6 MiB is MiB, not 0.0 GiB                      (the complaint that started it)
size text: an empty cache is 0.0 MiB
size text: just under 1 GiB is 1.0 GiB, not "1024.0 MiB"   <- the boundary
size text: a real 1023 MiB still reads MiB                 <- its control
size text: 60 GiB is GiB

(1<<30)-1 -> "1.0 GiB" next to 1023 MiB -> "1023.0 MiB" is the pair that pins the rule. A formatter that switches on >= 1 GiB prints 1024.0 MiB for the byte just below the threshold, and a
formatter that switches on >= 1023 MiB prints 1.0 GiB for a genuine 1023 — the two arms kill opposite wrong implementations, and neither alone would. That is the shape I have been asking for all session:
a positive assertion next to a near-miss control.

The rationale is the one the review asked for, stated as a reader's problem rather than a preference: "A small cache or bound printed as '0.0 GiB' reads…" -- and the fix is a single formatter
(q4nx_cache_size_text()) used by both the CLI and the sweep's note, which is the one-selector-two-callers rule this session has now applied four times (where_rc, q4nx_cache_candidates,
short_notice, and this).

0 -> "0.0 MiB" is the third case and worth naming: an empty cache printed as 0.0 GiB would read as "the bound is zero" rather than "nothing is cached", and the arms keep those apart.

One note: the boundary pair covers the threshold from both sides but not exactly 1 GiB (1<<30). 6 MiB, 1 GiB - 1 and 1023 MiB bracket it tightly enough that a > vs >= slip at the exact
value would still be caught by 1 GiB - 1 reading 1.0 GiB -- so this is a completeness note, not a gap.

Ran: q4nx_cache_test (36 arms). Not run: the real cache (a fixture directory is what the arms use).

[bong-water-water-bong]

VERDICT at d84842a: READY

The rounding guard is the mark of someone who read their own output: "Within half a MiB of a GiB the MiB reading rounds to 1024.0, which is a value in the unit it has just left" — and so it prints the GiB it is about to become. 1024.0 MiB is exactly the kind of number that makes an operator stop trusting a tool, and it costs one comparison to prevent.

The reason for the change is the better argument, though: a small non-zero cache printed as 0.0 GiB reads as "the sweep is off", which is a different state entirely. A unit that swallows a real quantity is a diagnostic that lies — the same defect family as a usage line that prints code (#2258) or a --dry-run that computes its own list (#2264). All three are cases where the output is not wrong about the number, it is wrong about the state.

Using one formatter for both the sweep's note and the CLI is what makes this hold: if the note and the command disagreed about how a size is spelled, an operator comparing them would have to know which one to believe. That is the same discipline #2264 applied to the selection, applied to the presentation — one implementation, so there is nothing to drift.

Small and well-scoped. No concerns.