test(chat): ratchet the bounded slot-detail disk-vs-window disagreement

[chenmingwei23]

What is the problem?

The bounded branch of GET /api/chat/slots/{slot} and the unbounded branch do not agree on which store decides a row's content inside the live window's region. The unbounded branch returns older + list(slot.messages), so the window decides. The bounded branch reads the chained disk corpus and merges only the rows disk is missing, so disk decides -- and a row that was rewritten in place is not missing, it is present at the same meta.mid holding stale text.

Two triggers reach it, both ordinary user actions:

• A variant switch. api_chat_slot_switch_variant assigns target_dict["content"] and target_dict["ts"] on the existing window dict and never touches meta.mid. Its inline save is wrapped in a try/except that swallows the failure without setting _pending_rewrite, so the next periodic flush takes the append path rather than the rewrite path and disk keeps the previous variant indefinitely -- not for one flush interval.
• A regenerate. chat_regenerate does del slot.messages[u_idx + 1:] and only then saves. A failed save leaves the deleted turns on disk.

Refs #7526 -- this PR does not resolve that issue. It carries the reproduction only.

Why this issue matters to the user

Both are user-visible transcript corruption. Selecting a variant paints the one you switched away from straight back over it, because chat_variant_switch is broadcast and the client's handler dispatches refreshSlot -- which is bounded as of #6947. Regenerating can bring deleted turns back. Neither is hypothetical and neither needs a race to observe.

How our fix solves it

It does not fix it, deliberately. It ratchets it.

The contract decision -- which store is authoritative for the bounded branch's window region -- is not a local change, and that is the finding that kept production code out of this PR. It spans three surfaces:

1. The bounded handler itself (chat_handlers.py), including the total and next_before it reports.
2. api_chat_fork. It resolves a client-supplied at_message_index against exactly this corpus and mirrors it on purpose: "Mirror that corpus here, or every index sent after the reader paged past a rotation boundary resolves short by the archived visible-row count, silently forking the WRONG message." Change the bounded read's corpus without changing fork in lockstep and a fork lands on a different message.
3. The client. pagingCursorAfterKeptHead treats next_before as a row offset in the server's corpus index space and shifts it by the kept head's own serverRowCount. retainServerTotal keeps total as a warm-read baseline, and its own comment records the cost of a wrong one: a bad baseline "makes the next warm read that ordinary collapse as a truncation and suppress the rescue, dropping a live row."

So the ratchet is the part that is safe to land now: four xfail(strict=True) tests that pin today's answer. strict is the point -- the day the contract is settled they turn into an xpass and force whoever fixed it to delete the marker, so the fix cannot land while leaving prose behind claiming the old behaviour.

It also defuses a live trap. The eleven tests preserved in c9979c43 no longer reproduce anything: they stub read_messages_chained, and the bounded branch now reads read_messages_chained_full, so the disk side is never consulted and eight of them pass vacuously against an unpatched handler. Anyone who picks the issue up and runs them concludes the bug is gone. These stub both readers with one corpus, so a pass cannot come from an empty tmp history.

What tests we did

test/test_slot_detail_disk_window_authority_7526.py, +150/-0, no other file touched.

Red-before was established against the unpatched handler without the markers, and each failed for the right reason rather than by construction:

Test	Unmarked result on main
test_bounded_read_returns_the_selected_variant	AssertionError: assert 'm11' == 'SELECTED VARIANT'
test_both_branches_agree_on_content	bounded 'm11' vs unbounded 'SELECTED VARIANT'
test_bounded_read_does_not_resurrect_the_deleted_tail	'm10' present after truncation
test_both_branches_agree_on_length	bounded and unbounded row lists differ

TestTheFrozenPrefixSurvives is not marked: it passes today and guards the opposite error -- a future fix that swallows the older-session prefix the bound exists to page into. A ratchet that only pins the broken direction lets an over-correction through.

With the markers: 1 passed, 4 xfailed. flake8, isort and mypy clean on the file. Single-file runs only (-n0); the suite was not run, by fleet instruction -- another track on the host was measured at 26G on full-suite runs tonight.

Any other suggestions on the work

Two things found while establishing the premise that are worth the record but are not this PR's to change:

• The issue's stated obstacle is misdiagnosed. It says _disk_older_count "describes the current session's file while the corpus is a chained read". It does not -- both load sites compute it as len(read_messages_chained(key)) - len(window) (chat_persistence.py:1110, :1586), already in chained-corpus units. The boundary is exact for any session with no rotated archive, so "cannot reconcile" is stronger than the code supports.
• Where it genuinely is not computable, the reason is different: the bounded branch reads read_messages_chained_full, which prepends each chain key's archive block and applies drop_persisted_tail_prefix, which drops live rows and reports how many to nobody. Rotation needs _SESSION_MAX_BYTES = 10 * 1024 * 1024, so that shape needs a real 10MB rotate segment in the chain and a later member carrying an archive segment. Real, but not the common path -- and I could not measure its production frequency, which is why the fail-closed fallback's blast radius stays unquantified.

Both are recorded in full on the issue.

Pattern harvest

Rule candidate: a preserved test kept as documentation of intended behaviour rots into a false all-clear the moment the code under it changes which collaborator it calls -- these stubbed read_messages_chained and kept passing after the handler moved to read_messages_chained_full, reporting green for a bug that was never fixed. A reproduction must stub every reader the path can reach, or assert the stub was actually consulted, so that "passes" cannot mean "never ran".

Refs #7526

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of aff869e9fda8a7981e8f211bcd83b34ddd31187d and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] aff869e

False positive or not applicable? A repository writer can comment:
/ai-review override gpt aff869e9fda8a7981e8f211bcd83b34ddd31187d: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of aff869e9fda8a7981e8f211bcd83b34ddd31187d — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real, user-visible corruption pinned with strict xfails that cannot rot silently — deliberately deferring the cross-surface contract ruling is the right scope call.

The four strict=True markers do encode one direction (window-authoritative) for a contract the PR itself says is undecided; if the ruling lands disk-authoritative these become permanently-xfailed dead weight rather than a tripped ratchet. That is acceptable here because the unbounded branch already answers window-side and the unmarked TestTheFrozenPrefixSurvives guards the over-correction, but whoever settles #7526 should treat this file as part of the decision, not just a marker to delete.

[DESIGN-REVIEWED] aff869e

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed aff869e9fda8a7981e8f211bcd83b34ddd31187d — this comment is updated in place on each push.

Review details

No candidates were proposed, and this is a test-only diff. The added tests stub both disk readers on ConversationLog, use tmp_path-backed state, neutralize push_slots_update, and spawn no processes — no host side effects. The xfail(strict=True) markers document a known bug (#7526) as expected-failing and ratchet correctly. I re-examined the handler wiring and helpers and could not ground any concrete input → call path → wrong-outcome defect in the changed lines at 80+.

No findings.

[OPUS-REVIEWED] aff869e

Verdict parsed from the review's SHA-scoped output markers for commit aff869e9fda8a7981e8f211bcd83b34ddd31187d.

False positive or not applicable? A repository writer can comment:
/ai-review override fable aff869e9fda8a7981e8f211bcd83b34ddd31187d: <one-sentence reason>

[dwu96]

Tier 1 auto-approve: test (1 file). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: adds one new test-only file (test/test_slot_detail_disk_window_authority_7526.py) as a strict-xfail positive-control ratchet for the bounded slot-detail disk-vs-window disagreement; no production code touched, no runtime impact.

[iamwhatever]

Tier 1 auto-approve: test (1 file). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: test-only ratchet -- adds test/test_slot_detail_disk_window_authority_7526.py pinning the bounded vs unbounded slot-detail branches' disagreement over which store decides a row inside the live window; no production file touched, no behaviour change.