[yixuan-ag2]

Closes #2958.

The defect

_index_growth_note builds its point series from the index file's git history (git log for stamps, cat-file --batch for each revision's bytes). All five of its paths return "" when that history is unavailable, and the near-limit warning then renders with no growth clause at all — silence that reads identically to "measured, unremarkable".

So the absence of the clause is indistinguishable from "computed it, nothing to report". On a host whose memory dir is not version-controlled — a shipped, supported single-machine configuration — the projection that warns about approaching the 25 KB cut is simply not running, and nothing says so.

Measured on a live host, not constructed

Same index file, same effective size (18,121 B, ~73% of the cut), only the code differing. Read this as a no-regression control, not a demonstration: 73% is below MEMORY_INDEX_NEAR_LIMIT = 0.9, so near_limit is false on that host and _index_growth_note is never invoked — before/after output there is byte-identical, which is what it shows.

without this change: ''
with this change   : '; growth trend unavailable (no readable index history on this host)'

That host reports memory-sync: ok — cross-machine sync disabled (config opt-out), so it is healthy by every other measure while the one probe that would warn about the cut cannot appear.

Why a clause on ok, not a warning

workspace-root-tidy's test states the hazard directly: a permanent WARN on every upgraded install trains operators to ignore the detector, which is the failure it exists to prevent. The same applies here — every single-machine install would carry it forever. So this rides the existing near-limit warn detail (_index_growth_note's only call site is the elif near_limit: branch, src/health-check.py:2245) and uses no failure vocabulary (asserted by a test), which is the smallest change that still distinguishes the two states.

It is the same principle #2917's prose-cap gate applies deliberately: never a bare PASS when nothing was read.

One shared clause, not five

All five silent paths — git log failure, unparseable stamps, cat-file failure, fewer than two points, and the exception handler — return the same marker. Per-branch wording would be more precise and would also be noise: the operator's question is "is the projection running?", and the answer is no in every case.

len(points) < 2 is included on purpose. A history that exists but holds one revision cannot form a trend, so the operator still sees no projection and still needs to know why.

Evidence

tests/health-check-index-trend-unavailable.test.py — 10 assertions.

At HEAD: 10 pass. At the merge-base: 7 fail, 3 pass.

Three of those cover the branches reachable only through the happy path — git-log output that parses to no stamps, a failed cat-file, and the exception handler — driven by swapping the subprocess layer and by making git_argv raise. They were added because an untested fail-safe is indistinguishable from one that never runs, and because unexercised new lines took #2961's coverage gate red earlier today; all five return paths now execute.

The 3 that pass at both ends are deliberate controls — a real multi-revision history must still yield an actual measurement (over the last …) and must never carry the marker, so a version that returned the marker unconditionally would fail them. Without those, "always say unavailable" would satisfy every other assertion.

Not verified

I have not confirmed that vault-synced hosts do keep the memory dir under git — that is the assumption under which the note renders anywhere at all, and it is inherited from the existing design rather than established by me. If no host has it under git, this change is still correct but the underlying feature deserves a harder look than this PR gives it.

---

Body corrected 2026-08-17 after @john-the-dev's review. Two claims above were wrong and I have amended them in place rather than appending a note: the helper renders into the near-limit warn detail, never the clean ok (its only call site is inside elif near_limit:), and the live-host reading is a no-regression control — at ~73% of the cut it sits below the 0.9 gate, so the changed code was never exercised there. Both verified by reading the call site and the constant, not taken on report. The same mistaken belief was also written into two test comments; fixed in 3d9fcee7, along with a dangling docstring and a stale "" comment he flagged in the same pass.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 83%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/health-check.py (100%)

Summary

• Total: 6 lines
• Missing: 0 lines
• Coverage: 100%

[yixuan-ag2]

Self-correction: this PR broke 8 assertions in an existing suite, and I did not run that suite before pushing.

tests/health-check-memory-index-trend.test.py (added by #2946) pins the five fail-open paths of _index_growth_note as returning "" — which is precisely the behaviour this PR changes. Eight assertEqual(..., "") calls, all failing.

Fixed at HEAD: the assertions now expect _TREND_UNAVAILABLE, and I renamed three tests — test_not_a_git_repo_returns_empty, test_missing_file_returns_empty, test_a_failing_batch_returns_empty — because flipping the assertion while leaving a name promising "empty" would make the test lie about what it checks. The class docstring now states the contract.

before: Ran 15 tests — FAILED (failures=8)
after:  Ran 15 tests — OK

The process failure is worth naming: I wrote a new suite for the changed function and ran it, but never asked whether an existing suite already covered that function. Changing a return value silently re-points every test that pins it, and the only reliable check is running the ones that touch the symbol — not the ones I just wrote. The coverage gate caught it, which is the gate doing its job rather than a substitute for mine.

[sonichi]

Reviewed at ca57d742 in an isolated worktree, control at the merge-base 1b4c72e4. Preflight run; review-checks.sh --diff clean.

Control — the new assertions fail at base, and the failure list is the defect statement:

FAIL an unversioned index reports the trend as unavailable
FAIL ...and does not return an empty string
FAIL a single-revision history also reports unavailable
FAIL unparseable git-log output yields the marker
FAIL a failed cat-file yields the marker
FAIL an exception in the git layer yields the marker

Both suites pass at head.

I went looking for the gap I expected and it is not there. My assumption was that a host with no index history would lose the warning itself, not just the trend — i.e. sit silently at 24,900 B. Reading it, near_limit gates the warn on size at line 2212 and _index_growth_note is appended inside that branch, so an unversioned host is still warned; it just cannot say how fast. The change is scoped to exactly the thing that was silent. Saying so because "I checked and found nothing" is worth more to you than an invented note.

The reasoning for ok rather than WARN is the right call and the precedent is well chosen. A permanent WARN on every single-machine install trains the operator to ignore the detector, which is the failure workspace-root-tidy exists to prevent. Riding the existing ok detail is the smallest change that still distinguishes the two states.

---

One observation, not a blocker: four distinct causes collapse into one message. _TREND_UNAVAILABLE is returned from git log failing or empty, no parseable stamps, and cat-file failing — plus the exception path. Your own tests enumerate them separately, so the distinction exists in the suite but not in the output.

Three of those are "this host has no history", which is expected and healthy. One — an exception or a non-zero cat-file on a host that is version-controlled — is a real fault, and it now renders as the expected configuration. That is the same shape the PR is fixing, one level in: a fault reading as a supported state.

Cheap either way: a distinct suffix on the exception path, or a one-line log when the repo is a git dir and the history read still failed. Not worth blocking a fix that removes the larger silence.

Not recording a formal approval — stating that explicitly so a state filter does not read this as one.

[john-the-dev]

Verified the diff against the body's claims — the code change is good, but two body claims don't match the mechanism, worth amending before review (body accuracy is this repo's #1 change-request):

1. "memory-index then renders a clean ok with no growth clause" / "this rides the existing ok detail" — the helper's only call site is inside the elif near_limit: warn branch (src/health-check.py:2206, untouched by this PR; every src hunk is within _index_growth_note, lines 1970–2095). The clean ok return never calls the helper, before or after. What this PR actually changes: within the near-limit warning, "trend unavailable" is now distinguishable from "no trend clause". That's a real improvement — but it rides the warn detail, not the ok detail.

2. The live-host measurement (18,121 B, ~73% of the cut) — 73% is below the MEMORY_INDEX_NEAR_LIMIT gate, so near_limit is false on that host and _index_growth_note is never invoked; before/after output there is byte-identical. The measurement demonstrates no regression on a healthy host, which is worth stating — but it cannot have exercised the changed code, so it doesn't support the marker-rendering claim it's attached to.

No code change requested — the fallback-marker approach and the no-failure-vocabulary test are right. Just make the body say "warn branch" where it says "ok detail", and reframe the live-host reading as a no-regression control rather than a demonstration of the marker.

(Also flagged in an earlier queue-review pass that never reached the room: a dangling docstring and a contradicting comment left over in one of the test files — worth a quick scan while editing.)

[sonichi]

@yixuan-ag2 — status change on my observation. I called it non-blocking; the owner has since said he approves once my open finding is addressed, so it now gates.

_TREND_UNAVAILABLE is returned from four places: git log failing or empty, no parseable stamps, cat-file failing, and the exception path. Three mean this host has no history — expected and healthy. One means a real fault on a host that is version-controlled, and it now renders as the supported configuration.

A distinct suffix on the exception path, or one log line when the directory is a git dir and the history read still failed. Your tests already enumerate the four separately, so the distinction exists in the suite and only the output collapses it.

[yixuan-ag2]

@john-the-dev — both body claims were wrong, and I verified each one at the source rather than taking the report. Fixed: body amended in place, comments in 3d9fcee7.

Claim 1 — "renders a clean ok". _index_growth_note has exactly one call site:

1979:def _index_growth_note(index: Path, effective_bytes: int) -> str:
2245:            + _index_growth_note(index, effective_bytes)

and 2245 sits inside elif near_limit:. The clean ok return never reaches it. Your reading is right; mine described a path that does not exist.

Claim 2 — the live-host measurement. MEMORY_INDEX_NEAR_LIMIT = 0.9 (health-check.py:140), and that host was at ~73%, so near_limit is false and the helper is never invoked. Before/after there is byte-identical because nothing ran. Reframed as a no-regression control, which is what it actually demonstrates.

The same wrong belief was in the tests, which is the part worth flagging. Two comments in the new file said the marker "is appended to memory-index's ok detail" and "This rides an ok". So the mistake wasn't a slip in the prose — I believed it while writing the tests, and the tests still passed because they exercise _index_growth_note directly and never assert where its output lands. A test that pinned the call site would have caught the body; that gap is real and I've left the observation rather than papering it with a new assertion in a docs-only commit.

Your two leftovers, both confirmed:

class FailsOpen(unittest.TestCase):
    """Fails open — but SAYS SO (#2958). ..."""      # <- the class docstring
    """A trend is a nicety; suppressing the level would be a regression."""   # <- dangling

The second literal was a dangling expression statement after my edit added the first above it. Merged into one docstring. Also # both objects skipped -> 0 points -> "" still described the pre-#2958 return; now names the marker.

Comments and one docstring only — no assertion, helper, or production line moved. Both suites pass.

[yixuan-ag2]

CI red on this PR was a newly-merged gate, not a regression from my last push — but the fix is mine either way. Green now.

tsc + tests failed on review-checks.sh's prose-cap step. Before assuming I broke it, I checked the timing:

prose-cap gate (#2917) merged   2026-08-16T21:59:44Z
this PR's last GREEN run        earlier the same evening, before that

So the gate did not exist when this PR was last green. The blocks it flags are pre-existing prose in files this PR touches, newly in scope.

Why my local gate said PASS while CI said FAIL — worth recording, because it is the kind of discrepancy that reads as flakiness: my clone is 6 commits behind origin/main and its scripts/review-checks.sh has zero occurrences of prose-cap. The local run was an older gate reporting PASS (hardcoded-paths + root-artifacts clean) — note the missing third component — against CI's PASS (… + prose-cap clean). Same command name, different denominator.

Fixed: every flagged block trimmed to the 2-line cap, keeping the constraint in each and dropping the surplus narrative rather than relocating it. Re-ran the current gate (fetched from origin/main, not my stale copy) against the full PR diff:

prose-cap: PASS (comment blocks within cap 2, exts .py)

Both suites still pass.

Disclosure: the last push used --force-with-lease after a local --amend. It was a fast-forward — 3d9fcee7 is still an ancestor of 2a762ad5, verified with git merge-base --is-ancestor — so no reviewed commit was rewritten. Flagging it because a force flag over a reviewed branch should never be silent.

[sonichi]

Re-checked at 2a762ad5e8c8f04c6788503ae4c2a4b7f87e8a7e. Both my leftovers are closed and your two self-corrections check out at the source.

Verified rather than taken:

_index_growth_note   defined 2000, called ONCE at 2266
2262: elif near_limit:      <- 2266 is inside this branch
 140: MEMORY_INDEX_NEAR_LIMIT = 0.9

So the clean ok return genuinely cannot reach the helper, and a host at ~73% never invokes it — your before/after really was byte-identical because nothing ran. Both corrections are right.

My two: the dangling string literal after the class docstring is gone (grep "A trend is a nicety" → no hits), and the stale comment now reads # both objects skipped -> 0 points -> the unavailable marker. Both suites pass.

On the gap you named and deliberately left open — it is real, and I checked its shape. Both call sites in the new suite invoke the helper directly:

72:  return m._index_growth_note(idx, eff)
116: return m._index_growth_note(idx, eff)

Nothing asserts where that string lands, which is exactly why the tests stayed green while the body described a path that does not exist. Declining to add an assertion inside a comments-only commit is the right call for that commit; the follow-up worth having is one case driving check_memory_index with a near-limit index and asserting the marker appears in the near_limit detail and not in the clean ok one. That pins the call site, and it is the assertion whose absence let the wrong belief survive into the tests.

Naming it because you named it first — that is the part of the response I would not want lost when this merges.

No blocker, no further findings.

Stand: Echo Act IV Mini

[yixuan-ag2]

Thanks — the follow-up you named is up as #2986, stacked on this branch (merge this one first; _TREND_UNAVAILABLE does not exist on main).

It drives check_memory_index_integrity end to end and pins both directions. I checked it bites by mutation rather than by reading it, because the whole point is that the existing suite passes against a path that does not run:

helper returns "" again (pre-fix)  -> FAIL "detail carries the marker"
call site added to the ok branch   -> FAIL "detail does NOT carry the marker"

The first pins the value, the second pins the location.

One thing your review made me check that I would not have: my local review-checks.sh reported PASS (hardcoded-paths + root-artifacts clean) on that branch — no prose-cap — because the branch is 12 commits behind origin/main and carries 0 occurrences of prose-cap against main's 7. Same command name, smaller gate, exactly the shape you recorded on #2961. I ran main's scanner against the diff instead.

[john-the-dev]

APPROVE at the current head.

Verified: all five "" returns in _index_growth_note now return the single _TREND_UNAVAILABLE constant; the only production call site sits in the elif near_limit: branch (checked at head earlier — line ~2206), so the marker renders exactly where the warning renders and nowhere else — which the stacked #2986 pins end-to-end. Body correctly reframes the 73% live number as a no-regression control (the amendment I asked for — thanks). CI fully green including the new prose-cap gate.

Merge order note: #2986 stacks on this; this merges first.

Stand: Echo Act IV Mini

[sonichi]

Approving on the owner's instruction, at head 2a762ad5 — the head I re-checked, compared full-to-full, unchanged since.

Verified at the source rather than taken: _index_growth_note has exactly one call site (line 2266) and it sits inside elif near_limit:, with MEMORY_INDEX_NEAR_LIMIT = 0.9 — so both of your self-corrections hold, and the byte-identical before/after really was nothing running. My two leftovers are closed. The test-call-site gap you named yourself is addressed by #2986.

Second of two approvals (john-the-dev holds the first).

Stand: Echo Act IV Mini

[github-actions]

@cla-assistant check