fix(chat): stat-failure listing goes silent, and compaction summary composes after the reset (#14248, #14322)

[mrveiss]

Closes #14248, #14322

Thinking Path

Both issues are the same failure mode on the chat session path: an ordering or
error-handling defect where nothing raises and nothing logs, so a caller sees
wrong or missing data instead of a visible failure.

#14248 — SessionListingMixin._build_session_entry (chat_history/session_listing.py)
already caught an os.stat failure and logged it, but the message was keyed on
the filename ("Error reading file stats for %s: %s") and read no differently
from any other I/O error. It did not name the session id in a way an operator
(or a log-scraping alert) could grep for, and it did not distinguish "this
conversation exists but is unreadable" from "Chat session %s not found" — the
wording used elsewhere in chat_history for a session that never existed.
Because the entry is dropped either way, distillation's _select_pending_sessions
never even sees a session_id for it, so it can't emit its own "no usable
timestamp" style warning (skill_distillation_scheduler.py:578-583) — the
listing layer is the only place this failure is observable at all, and it read
the same as routine noise.

#14322 — ContextOverflowProtection._create_summary computed retained_tokens
before reset_session, with a comment citing #14065 that the tracker must not
reset while the history it claims to have compressed is still uncompressed —
but _compose_summary (and through it _extract_state, _render_state_block,
_preserved_user_messages) ran after that reset, on the wrong side of the
invariant the comment states one statement above it. It doesn't raise today
because every helper is isinstance-guarded and falls back on an unexpected
shape, which is exactly why it needed an issue instead of a runtime crash: the
next edit to any of those three helpers that starts reading tracker state would
reintroduce #14065 one call deeper, past the caller's except SummarizationFailed.

The issue also flagged that PR #14249's multi-round idempotency test fed round
1's composed summary back into round 2 as {"role": "user", "content": first} —
a shape production never writes. overflow_integration.create_summary_message
actually injects {"sender": "system", "messageType": "context_summary", ...},
and _role_of reads role or sender, so the real artifact's role is "system".
Under the fake "user" role, _preserved_user_messages mistook the entire
round-1 summary blob for a user instruction and re-protected it wholesale — the
test passed for the wrong reason. Traced through the real shape: the guarantee
does not survive round 2. The issue offered two ways out — teach extraction to
recognise a prior summary's verbatim block, or scope the docstring claim to a
single round. Recognising a prior summary's rendered markdown block would mean
parsing prose back into structured data, which is precisely the "prose, not
guarantee" failure mode #14066 built the deterministic extraction path to
avoid, one layer removed. I scoped the claim instead, and turned the test into
a guard that pins the corrected scope so it can't quietly regress back to
"believed to work across every round."

What Changed

autobot-backend/chat_history/session_listing.py

• _build_session_entry's stat-failure log now names the session id explicitly
  and reads "Chat session %s exists (%s) but its file stats could not be read
  — excluded from this listing pass: %s" — grep-able by id, and worded so it
  cannot be confused with "not found".

autobot-backend/chat_history/session_listing_stat_failure_test.py (new)

• Drives the real list_sessions_fast() path over real files on disk, with
  os.stat monkeypatched to fail for exactly one of them (not a stubbed
  lister). Asserts: the healthy session still lists; the failure produces a
  logged error naming the session id in a "session <id> " position (not
  merely as an incidental filename substring); the wording differs from "not
  found"; the failing session is still excluded from that pass (matching the
  malformed-timestamp precedent — loud, then excluded, never silent).

autobot-backend/chat_history/context_overflow.py

• _create_summary: _compose_summary(...) now runs before
  self.tracker.reset_session(...), on the same side of the bug(chat): summarization failure returns a success-shaped placeholder — half the conversation is destroyed and reported as compacted #14065 invariant
  as retained_tokens.
• _preserved_user_messages docstring corrected: the verbatim guarantee is
  scoped to raw role == "user" turns within the round that summarizes them,
  not to a prior round's own composed summary swept into a later window.

autobot-backend/chat_history/context_overflow_boundary_test.py

• New TestCompositionRunsBeforeTheReset: patches the module-level
  _compose_summary to record call order against a reset_session AsyncMock,
  drives the real _create_summary path, and asserts compose precedes
  reset. _compose_summary can't raise today, so an ordering regression
  would not show up as a different composed string under this suite's mocked
  summarizer — only the call order proves it.
• test_two_successive_compactions_do_not_erode_the_instruction replaced with
  test_a_prior_rounds_summary_is_not_silently_re_protected_as_a_user_turn,
  which feeds round 2 the real {"sender": "system", "messageType": "context_summary", ...} shape and asserts the single-round guarantee holds
  while the cross-round one explicitly does not, with a comment steering a
  future author toward a real fix rather than relaxing the assertion.
• Module docstring's item 2 corrected to state the scope explicitly.

Verification

Static only — per instructions, application code and the test suite were not
executed; CI runs pytest. Verified by:

• python3 -m py_compile on all four touched files — clean.
• Manual line-by-line trace of _pick_boundary, _role_of,
  _preserved_user_messages and _compose_summary against each new test's
  fixture to confirm the asserted outcome (documented inline in the tests
  themselves).
• No lines over the project's 120-char limit; import ordering follows the
  profile = "black" / straight-imports-before-from-imports convention already
  used in this file.

Mutations — statically traced, not executed (no test run per instructions):

• fix(chat): a session whose stat fails vanishes from the listing silently, so it is never distilled and never reported #14248: reverting the log message to
  "Error reading file stats for %s: %s", filename, str(e) removes the
  "session <id> " phrase and the word "exists" — both
  test_unreadable_session_is_logged_with_its_id and
  test_unreadable_session_wording_differs_from_not_found go red. (A looser
  "id is a substring of the message" check would NOT catch this — chat_id is
  always a substring of its own filename by construction — which is why the
  first test asserts the id in a "session <id> " position instead.)
• bug(chat): compaction composes the summary after resetting the tracker, and its multi-round test uses a shape production never writes #14322 ordering: swapping summary_text = _compose_summary(...) back to
  below await self.tracker.reset_session(...) flips
  TestCompositionRunsBeforeTheReset's recorded call_order to
  ["reset", "compose"], red against the asserted ["compose", "reset"].
• bug(chat): compaction composes the summary after resetting the tracker, and its multi-round test uses a shape production never writes #14322 test-shape: reverting
  test_a_prior_rounds_summary_is_not_silently_re_protected_as_a_user_turn's
  injected_summary back to {"role": "user", "content": first} makes
  _role_of return "user", so _preserved_user_messages re-protects the
  entire round-1 blob (which contains "never touch the prod database" as
  quoted text) and the assert ... not in second goes red — reproducing the
  original mis-passing test.

Model Used

Claude Sonnet 5

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

Review — APPROVE

Both issues verified against their acceptance criteria, and the most valuable finding here is a test that was passing for the wrong reason.

#14248 — matches the criteria exactly

The issue asks for three things, not for the session to be re-included:

• logged, once, with the session id
• distinguishable from a session that does not exist
• a test drives the real listing path with a stat failure, not a stubbed lister

Keeping the session excluded from that listing pass while logging loudly is the right call — it matches the malformed-timestamp precedent in skill_distillation_scheduler.py that the issue itself cites as the model. Wording it so it cannot be mistaken for "Chat session %s not found" is the substance of criterion 2.

The note about the assertion is a good catch in its own right: a loose "id appears somewhere in the message" check would pass trivially, because chat_id is a filename substring by construction. Requiring the id in a "session <id> " position is what makes the test able to fail.

#14322 — ordering fix is correct, and it exposed a false test

Moving _compose_summary(...) above self.tracker.reset_session(...) puts it on the same side of the #14065 invariant as retained_tokens. Asserting on recorded call order (["compose", "reset"]) rather than on output is the right shape — output could coincidentally match while the ordering stayed wrong.

The real find: test_two_successive_compactions_do_not_erode_the_instruction fed round 1's summary back as {"role": "user", "content": first} — a shape production never writes. Under that fake role, _preserved_user_messages mistook the entire round-1 summary blob for a user instruction and re-protected it wholesale. The test passed while asserting a guarantee the system does not provide.

Production injects a summary as {"sender": "system", "messageType": "context_summary", ...}, and _role_of reads role or sender, so that artifact is "system" — excluded from _preserved_user_messages, which rescues only role == "user".

Narrowing the docstring to the scope that actually holds is the honest fix. And declining to "solve" it by re-parsing a prior summary's markdown is right: that would reintroduce the prose-not-guarantee fragility the deterministic path exists to remove, as a regex over model-authored text re-run every round.

One thing I added

The behaviour behind that narrowed guarantee is a real limitation — a standing user instruction can still erode across several compaction rounds, one probabilistic step at a time. Since the module previously claimed the stronger guarantee, that limitation should be tracked rather than live only in a docstring.

Filed as #14393, with the mechanism, why the obvious fix is wrong, and three fix directions (structured carry-forward alongside the summary rather than embedded in its prose). This PR is unchanged by it — the scope correction here is right; #14393 is about widening the behaviour later.

Note on evidence

Mutation table is statically traced, correctly labelled. Worth confirming session_listing_stat_failure_test.py actually collects in CI rather than being skipped.

No blocking findings.