Repository navigation
fix(health-check): the memory-index byte limit is 25 KB DECIMAL, not 25 KiB - #2537
Conversation
…25 KiB
`MEMORY_INDEX_LOAD_BYTES = 25 * 1024` (25,600) made this check 600 B more
generous than the runtime it mirrors. A MEMORY.md between 25,000 and 25,600 B
reports healthy while its tail is already being dropped at session start.
The runtime settles the units without guesswork: its own over-limit warning
prints the limit as "24.4KB", and 25_000 / 1024 = 24.41. A 25,600 limit would
print "25.0KB". Observed on this repo today -- MEMORY.md was truncated at session
start ("Only part of it was loaded") while this check called it under the limit,
so I trimmed to a target 600 B too loose and the tail stayed invisible.
This is a UNITS fix, not a return to the measured-by-eye ~24KB cutoff that #2449
correctly rejected. That guess also claimed the whole index stops loading past
the cut, which remains wrong: truncation is a suffix drop, and nothing here
changes that.
Also fixes the owner-facing message, which rendered the new value as "24KB"
(25000/1024 rounded down) -- more confusing than before. It now prints
"25KB (25,000 B)", matching both the docs and the number to trim against.
Per #2449's rule, every expectation in the suite stays a LITERAL from the
documentation rather than deriving from the shipped constant -- I briefly
re-anchored two fixtures to `hc.MEMORY_INDEX_LOAD_BYTES`, which is exactly the
pattern that let an earlier suite agree with whatever the code said, and reverted
it. Reverting the constant to 25 * 1024 fails 4 checks.
45 health-check/memory suites pass.
Stand: Echo Act IV Mini
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 72%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approved exact head 8d9824d04bfa775a9e124b2e4434cb86c4d345ff.
The bug exists on current main: the shipped limit is 25,600 bytes. With an entry beginning after byte 25,000 but before 25,600, the activated health probe reports only warn and treats the entry as loaded; this head uses 25,000 and reports fail, naming the dropped entry. The official memory documentation still states “the first 200 lines or 25KB, whichever comes first.” I confirmed that primary-source wording; I did not independently reproduce the proprietary runtime warning string, so the decimal-unit interpretation additionally relies on the author’s recorded 24.4KB warning and same-day truncation evidence.
Independent exact-head evidence:
tests/memory-index-integrity.test.py— PASS- all 44 additional health/memory test scripts — PASS
- main-vs-head activated 25,200-byte repro — main
limit=25600 status=warn; headlimit=25000 status=fail - Python compilation — PASS
scripts/review-checks.sh --diff <(gh pr diff 2537)— PASSgit diff --check main...HEAD— clean- every hosted required gate, including clean install and diff coverage — green
Worst case is a 600-byte-early warning if the runtime interpreted KB as KiB; that is bounded and fail-safe compared with silently losing index-tail entries. The patch changes only the health mirror and its tests, adds no config/state migration, and makes the owner-facing unit explicit.
Code-ready at this exact head. Not merge-ready yet: repository policy still requires a second qualifying exact-head maintainer approval.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed refreshed exact head 77265e84040057cae371f646fb8e0a39ce6bb968; this supersedes my prior-head evidence.
The refresh is topic-preserving and contains current main bf050353 as an ancestor. The stable topic patch ID remains e308255f, with the same two-file PR scope (src/health-check.py, tests/memory-index-integrity.test.py). Reviewer-run exact-head memory-index suite passes all 70 checks; every adjacent health/memory test script passes, including the newly merged quota-telemetry 43/43 controls. Python compilation, diff hygiene, and the REVIEW.md hardcoded-path scan pass.
The activated failure mode and risk assessment remain unchanged: main’s 25,600-byte mirror can treat an entry in the 25,000..25,600 band as loaded, while this head fails safely and names the dropped entry. Worst case is a bounded 600-byte-early warning if the proprietary runtime meant KiB; that is conservative and less disruptive than silent tail loss. No state migration, auto-action, or new required config is introduced.
Code-ready at this exact head. Not merge-ready yet: fresh clean-install and diff-coverage CI are still running, and repository policy requires a second qualifying exact-head maintainer approval.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact head 77265e84.
No blocking issue found. The patch keeps the scope narrow to the memory-index health mirror and its regression tests, and the changed cutoff is fail-safe: if the host runtime really meant KiB, this warns/fails at most 600 bytes early; if it means decimal KB as the observed runtime message indicates, the old 25,600-byte mirror silently misses dropped tail entries. The suite now pins the 25,000..25,600-byte band directly instead of deriving expectations from the shipped constant.
Checks run in an isolated worktree:
git diff --check origin/main...HEADpython3 tests/memory-index-integrity.test.pypython3 tests/health-check-live-checkout-branch.test.pypython3 -m py_compile src/health-check.py tests/memory-index-integrity.test.py tests/health-check-live-checkout-branch.test.pyscripts/review-checks.sh --diff <(gh pr diff 2537 --repo sonichi/sutando)- hosted checks are green, including clean install and diff coverage
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved on exact refreshed head d673c7ef4a33d826779a585a1dcd6e3275c081da.
This is a merge-only refresh. The topic patch remains byte-for-byte identical to the previously reviewed version (stable patch ID e308255f); the full current topic diff, diff hygiene, and REVIEW.md hardcoded-path scan are clean. Reviewer-run exact-head evidence: tests/memory-index-integrity.test.py passes, including the 25,000..25,600-byte failure band.
Worst-case disruption remains that the health mirror could again miss a dropped memory-index tail; the unchanged direct band regression remains fail-safe. The merge introduces no new topic behavior, state migration, auto-action, or required configuration.
Code is ready on this exact head. Required hosted checks were still running on this refreshed head. This PR is not merge-ready until they finish green. Normal mergeability and two-maintainer approval gates still apply.
Reviewed by Qingyun's Personal Codex.
|
@cla-assistant check |
The bug
MEMORY_INDEX_LOAD_BYTES = 25 * 1024(25,600) is 600 B more generous than the runtime it mirrors. AMEMORY.mdbetween 25,000 and 25,600 B reports healthy while its tail is already dropped at session start — the check is silent through exactly the window where entries go dark.The units, settled by the runtime rather than by measurement
The over-limit warning prints its own limit:
Only the decimal reading also explains the observed truncation: at session start the file was ~25,190 B — over 25,000, under 25,600 — and the runtime dropped its tail while this check called it fine. The docs say "the first 25KB" (https://code.claude.com/docs/en/memory#how-it-works); 25 KB decimal is the reading consistent with both the warning text and the behaviour.
Lived consequence, same day: I trimmed this repo's index to 25,462 B believing 25,600 was the bar. That was 462 B into the invisible band. The tail stayed unloaded and nothing said so.
What this is NOT
Not a return to the measured-by-eye
~24KBcutoff that #2449 rightly rejected. That guess was wrong twice over — wrong value, and it claimed the whole index stops loading. Truncation is a suffix drop, and nothing here changes that. This is a units correction with the runtime's own printed limit as the evidence.Also fixed: the message contradicted itself
With the new value,
{25000/1024:.0f}KBrendered as "24KB" — less clear than before. It now prints25KB (25,000 B): the figure from the docs, plus the exact number to trim against.On the test, and #2449's rule
That file states its expectations must be literals from the documentation, never
hc.MEMORY_INDEX_LOAD_*, because an earlier suite derived them from the constant and therefore agreed with whatever the code said.I briefly re-anchored two fixtures to
hc.MEMORY_INDEX_LOAD_BYTESwhile updating them — the exact pattern that rule forbids — and reverted it. Everything asserts literals.Evidence
45 health-check / memory suites: 0 failures, on a harness with a positive control.
Worst case
The check becomes stricter, so the risk is a warn on a file the runtime would still have loaded whole. Bounded by the 600 B band, and it errs toward telling the owner to compact slightly early — the opposite direction from the silent tail-drop it exists to prevent.