fix(mental-models): don't overwrite content when reflect retrieved nothing

[chethanuk]

Problem

refresh_mental_model refuses to write only when the rendered content is empty:

if not final_content.strip():

A reflect run that retrieves nothing does not produce an empty answer. The agent
answers from the prompt alone, with a non-empty generic refusal along the lines of
"I don't have information about that", which passes that check and replaces a good
mental model with garbage. In delta mode it is worse than a one-off: the refusal
text becomes the baseline the next refresh builds on.

The existing no_new_facts skip does not cover this. It is gated on
use_delta and current_doc is not None, so full mode has no protection at all,
and full is the default.

Fixes #2894

Fix

Count the grounding reflect actually retrieved, from based_on, before the delta
merge folds the previous based_on into the payload. Directives are excluded from
that count: they are bank-config injection, not retrieval evidence, so a
based_on carrying only directives is not grounding.

The guard fires only when retrieval regressed:

fresh_grounding_count == 0 and prior_grounding_count > 0 and has_delta_baseline

prior_grounding_count > 0 matters. A model that has never been grounded (fresh
seed content, no prior reflect_response) has nothing to lose, so its first
refresh still writes normally. Only a model that had grounding and now has none is
protected.

On that path the previous content and structured payload are left untouched, the
failure is recorded as reflect_response.refresh_skipped = 'no_memories_found'
alongside the source-query tracking, and MentalModelRefreshError is raised.

The same counter also replaces not supporting_facts in the delta no-new-facts
skip, which until now let directives alone count as new evidence.

What this is and is not

This is defense in depth against an empty recall from any cause. It is not a fix
for the consolidation or embedding-index race the reporter suspected. Sanderhoff-alt
could not reproduce that race: he dropped all three bank HNSW indexes and held
ACCESS EXCLUSIVE on memory_units, and the overwrite never happened. He did
confirm the defensive gap itself, via a mismatched strict tag scope. PR #2997 was
closed as not reproducible for claiming the former, so this claims only the latter.

#2959 covers a sibling case, the "No answer provided." placeholder taking the
same path. That is deliberately not touched here; PR #2960 owns it, and keeping
the two apart keeps both diffs composable.

Behaviour change worth flagging

Refreshes that previously "succeeded" while writing a refusal will now raise and
surface as failed operations. That is the point, but it is visible: bank operators
who were silently accumulating garbage documents will start seeing failures
instead. The alternative, returning the previous content quietly, would hide
upstream retrieval failures from workers and tests.

Only one internal caller is affected, _handle_refresh_mental_model via
execute_task. The HTTP route enqueues and returns an operation id, so the new
raise cannot become a user-facing 500.

Merged PR #3078 named this issue as a blocker on widening auto-refresh to
strict-tagged models.

Test evidence

Check	Result
mental-model suite, -n0 -m "not hs_llm_core"	95 passed, 12 deselected, 111s

The 12 deselected are hs_llm_core tests, which CI runs in its own core-LLM job.

New cases are deterministic (MockLLM and monkeypatch, no judge), parametrized over
empty based_on with existing content, based_on carrying only directives, a
never-grounded model whose first refresh must still write, and delta versus full
mode.

[ebarkhordar]

Full mode loses this guard after one cycle, because the skip path clears the counter the guard depends on.

prior_grounding_count is read from the stored reflect_response.based_on (memory_engine.py:11768), and the skip path persists reflect_response_payload wholesale (:11941). In full mode that payload holds this run's based_on only: the accumulate-with-previous merge at :11773 sits inside if use_delta:. So the first ungrounded refresh preserves content and writes back a based_on with zero grounding, and the next identical refresh reads prior_grounding_count == 0, fails the > 0 clause at :11929, and writes the refusal.

Measured at d46f682, full mode (no trigger.mode), reflect_async stubbed to return the refusal text with an empty based_on, stub embeddings, real Postgres:

REFRESH 1 (grounded, 1 observation): no raise
  content='# Team\n\nAlice is the lead.\n'   stored grounding=1  refresh_skipped=None

REFRESH 2 (retrieval empty, refusal text): RAISED MentalModelRefreshError
  content='# Team\n\nAlice is the lead.\n'   stored grounding=0  refresh_skipped='no_memories_found'

REFRESH 3 (identical conditions to refresh 2): NO RAISE
  content="I don't have information about this topic."   stored grounding=0  refresh_skipped=None

Refresh 3 also resets refresh_skipped to None, so the audit record of the first failure goes out with the document.

Delta mode is not affected. The same sequence with trigger={"mode": "delta"} returns early at :11838 and the merge keeps the accumulated based_on, so grounding stays at 1 through refreshes 2, 3 and 4.

Worker-driven refreshes retry, so the gap between the two runs is just the retry interval. One way to close it: on the skip path, carry the previous based_on into the persisted payload rather than replacing it, which is what :11773-11782 already does in delta mode. Not blocking if you would rather the audit payload stay a snapshot of the failed run, though then the counter likely needs to read from something the skip path does not rewrite.

[chethanuk]

Confirmed and fixed in 99725e5. I reproduced your sequence first: the guard held for exactly one cycle, and refresh 3 cleared refresh_skipped along with the content. Also:

• The skip path now writes only the failure marker and carries the stored reflect_response forward, i.e. your first option. It reads as the honest record rather than a compromise, since content is preserved on this path too, so text and based_on still describe the document that is actually stored. A later successful refresh writes a fresh payload and clears the marker.
• I tried your second option first, keying on stored content. Durable by construction, but too broad: content cannot tell a retrieval-built document from hand-authored seed text, so a model grounded only in directives or bank background context could never refresh again. It broke two unrelated plumbing tests. It would also need to exclude NO_ANSWER_TEXT or it strands any model already holding the refresh_mental_model overwrites a healthy mental model with the "No answer provided." placeholder when reflect fails #2959 stub.
• Regression test test_full_refresh_guard_survives_consecutive_skips: three consecutive skipped refreshes in full mode, asserting content and marker survive each. Verified it fails on the previous commit. No existing test refreshed after a skip, which is how this got through.
• Left alone: the skip path still passes last_refreshed_source_query. fix(refresh): preserve mental model content when reflect returns no usable answer #2960 removes that from both failure paths for its own reason, and it is pre-existing on main rather than new here, so I would rather not collide with it.