fix(refresh): preserve mental model content when reflect returns no usable answer

hindsight-api-slim/hindsight_api/engine/memory_engine.py

@@ -103,6 +103,27 @@ class _BankTemplateImportAuthorizationState:
 MENTAL_MODEL_PENDING_CONTENT = "Generating content..."
 
 
+def is_populated_content(content: str | None) -> bool:
+    """Whether mental-model content is a real synthesis rather than a placeholder.
+
+    ``MENTAL_MODEL_PENDING_CONTENT`` (not refreshed yet) and the reflect agent's
+    fallback answers are all non-empty, so a length or emptiness check reads them
+    as populated. Both the refresh outcome metadata and the persistence guard use
+    this one predicate, so what gets reported and what gets stored cannot drift
+    apart.
+
+    Note this is the last-resort text check. The authoritative signal is
+    ``ReflectAgentResult.answer_failure_reason``, which is set where the failure
+    happens and does not depend on matching placeholder strings.
+    """
+    stripped = (content or "").strip()
+    return bool(stripped) and stripped not in (
+        MENTAL_MODEL_PENDING_CONTENT,
+        NO_ANSWER_TEXT,
+        ITERATION_LIMIT_TEXT,
+    )
+
+
 def get_current_schema() -> str:
     """Get the current schema from context (falls back to config default)."""
     schema = _current_schema.get()
@@ -392,6 +413,7 @@ def validate_sql_schema(sql: str) -> None:
 from .multi_llm import MultiLLMProvider
 from .query_analyzer import QueryAnalyzer
 from .reflect import run_reflect_agent
+from .reflect.models import ITERATION_LIMIT_TEXT, NO_ANSWER_TEXT
 from .reflect.tools import tool_expand, tool_recall, tool_search_mental_models, tool_search_observations
 from .response_models import (
     VALID_RECALL_FACT_TYPES,
@@ -2585,17 +2607,11 @@ async def _write_refresh_outcome_metadata(self, operation_id: str | None, refres
         if not operation_id:
             return
 
-        from .reflect.agent import NO_ANSWER_TEXT
-
         content = refreshed.get("content") or ""
-        stripped = content.strip()
         based_on = (refreshed.get("reflect_response") or {}).get("based_on") or {}
         outcome = RefreshMentalModelOutcomeMetadata(
             content_len=len(content),
-            # The no-answer stub and the pending placeholder complete
-            # wire-successful but carry no real synthesis — a length check
-            # alone would read them as populated.
-            populated_content=bool(stripped) and stripped not in (MENTAL_MODEL_PENDING_CONTENT, NO_ANSWER_TEXT),
+            populated_content=is_populated_content(content),
             based_on_counts={fact_type: len(facts or []) for fact_type, facts in based_on.items()},
         )
         try:
@@ -10352,6 +10368,7 @@ async def expand_fn(memory_ids: list[str], depth: str) -> dict[str, Any]:
             # Return response (compatible with existing API)
             result = ReflectResult(
                 text=agent_result.text,
+                answer_failure_reason=agent_result.answer_failure_reason,
                 based_on=based_on,
                 structured_output=agent_result.structured_output,
                 usage=usage,
@@ -11767,6 +11784,46 @@ async def refresh_mental_model(
                 "mental_models": [],  # Mental models are included in based_on["mental-models"]
             }
 
+            # Reflect produced no usable answer: keep the existing content.
+            #
+            # `text` still holds a human-readable placeholder here (NO_ANSWER_TEXT or
+            # ITERATION_LIMIT_TEXT), so it is NOT empty and the empty-content guard
+            # further down does not fire. Storing it would replace a working document
+            # with a one-line placeholder while the operation reported success.
+            #
+            # This runs before the delta block on purpose: delta rendering can turn a
+            # placeholder candidate into non-empty structured content, which would then
+            # look like a normal render by the time the later guard sees it.
+            #
+            # reflect_response is still written (with this refresh's based_on and any
+            # accumulated delta provenance) so the failure is auditable and the next
+            # refresh does not start from an erased grounding set; only content and
+            # structured_content are left untouched.
+            if reflect_result.answer_failure_reason is not None:
+                logger.warning(
+                    "[MENTAL_MODELS] Refresh for %s produced no usable answer (%s); "
+                    "keeping previous content and raising MentalModelRefreshError.",
+                    mental_model_id,
+                    reflect_result.answer_failure_reason,
+                )
+                reflect_response_payload["refresh_skipped"] = reflect_result.answer_failure_reason
+                # Record the failure, but do not advance last_refreshed_source_query:
+                # nothing was synthesized for the current query. Advancing it would tell
+                # the next attempt the topic is unchanged, so a retry after a
+                # source-query change would run in delta mode against content written
+                # for the previous topic, with recall scoped to new memories only.
+                await self.update_mental_model(
+                    bank_id,
+                    mental_model_id,
+                    reflect_response=reflect_response_payload,
+                    request_context=request_context,
+                )
+                raise MentalModelRefreshError(
+                    f"Refresh produced no usable answer for mental_model_id={mental_model_id} "
+                    f"(reason: {reflect_result.answer_failure_reason}). Previous content preserved; "
+                    "see reflect_response.refresh_skipped for audit."
+                )
+
             # Delta-mode path: emit structured operations against the existing
             # structured doc, apply them, then re-render to markdown. Sections
             # not mentioned by any operation are physically untouched, so prose
@@ -11891,19 +11948,24 @@ async def refresh_mental_model(
             # failures from callers (workers, tests). So: preserve existing
             # content in the DB (and audit the failure via reflect_response),
             # then RAISE so the caller knows the refresh didn't happen.
-            if not final_content.strip():
+            # A placeholder reaching this point means delta rendering produced one from
+            # an otherwise-usable answer; the answer_failure_reason check above already
+            # caught the case where reflect itself failed.
+            if not is_populated_content(final_content):
                 logger.warning(
-                    f"[MENTAL_MODELS] Refresh for {mental_model_id} produced empty content; "
+                    f"[MENTAL_MODELS] Refresh for {mental_model_id} produced no usable content; "
                     "preserving previous content and raising MentalModelRefreshError."
                 )
                 reflect_response_payload["refresh_skipped"] = "empty_candidate"
-                # Persist the reflect_response (so the failure is auditable) and
-                # the source-query tracking, but do NOT touch content/structured.
+                # Persist the reflect_response so the failure is auditable, but do not
+                # touch content/structured_content or last_refreshed_source_query: no
+                # content was produced for the current query, and recording it as
+                # refreshed would put a retry into delta mode against the previous
+                # topic's document.
                 await self.update_mental_model(
                     bank_id,
                     mental_model_id,
                     reflect_response=reflect_response_payload,
-                    last_refreshed_source_query=current_source_query,
                     request_context=request_context,
                 )
                 raise MentalModelRefreshError(

hindsight-api-slim/hindsight_api/engine/reflect/agent.py

@@ -16,7 +16,17 @@
 
 from ...config import get_config
 from ..llm_interface import LLM_TOOL_CHOICE_AUTO, LLMToolChoice
-from .models import DirectiveInfo, LLMCall, ReflectAgentResult, StructuredOutputResult, TokenUsageSummary, ToolCall
+from .models import (
+    ITERATION_LIMIT_TEXT,
+    NO_ANSWER_TEXT,
+    AnswerFailureReason,
+    DirectiveInfo,
+    LLMCall,
+    ReflectAgentResult,
+    StructuredOutputResult,
+    TokenUsageSummary,
+    ToolCall,
+)
 from .prompts import (
     _extract_directive_rules,
     build_final_prompt,
@@ -50,10 +60,11 @@ def _build_directives_applied(directives: list[dict[str, Any]] | None) -> list[D
 
 DEFAULT_MAX_ITERATIONS = 10
 
-# Fallback answer when the LLM returns nothing usable. Consumers that need to
-# tell a real answer from this placeholder (e.g. refresh outcome metadata's
-# populated_content) compare against this constant rather than the literal.
-NO_ANSWER_TEXT = "No answer provided."
+# NO_ANSWER_TEXT / ITERATION_LIMIT_TEXT are defined in .models and imported above;
+# they stay importable from this module so existing callers of
+# agent.NO_ANSWER_TEXT keep working. Consumers should prefer
+# ReflectAgentResult.answer_failure_reason over comparing these strings: the
+# reason is set where the condition is known and survives a reworded fallback.
 
 
 def _normalize_tool_name(name: str) -> str:
@@ -807,6 +818,7 @@ def _log_completion(answer: str, iterations: int, forced: bool = False):
             _log_completion(answer, iteration + 1, forced=True)
             return ReflectAgentResult(
                 text=answer,
+                answer_failure_reason=None if answer.strip() else "empty_answer",
                 structured_output=structured_output,
                 iterations=iteration + 1,
                 tools_called=total_tools_called,
@@ -871,6 +883,7 @@ def _log_completion(answer: str, iterations: int, forced: bool = False):
             _log_completion(answer, iteration + 1, forced=True)
             return ReflectAgentResult(
                 text=answer,
+                answer_failure_reason=None if answer.strip() else "empty_answer",
                 structured_output=structured_output,
                 iterations=iteration + 1,
                 tools_called=total_tools_called,
@@ -1015,6 +1028,7 @@ def _log_completion(answer: str, iterations: int, forced: bool = False):
             _log_completion(answer, iteration + 1, forced=True)
             return ReflectAgentResult(
                 text=answer,
+                answer_failure_reason=None if answer.strip() else "empty_answer",
                 structured_output=structured_output,
                 iterations=iteration + 1,
                 tools_called=total_tools_called,
@@ -1094,6 +1108,7 @@ def _log_completion(answer: str, iterations: int, forced: bool = False):
                 _log_completion(answer, iteration + 1)
                 return ReflectAgentResult(
                     text=answer,
+                    answer_failure_reason=None if answer.strip() else "empty_answer",
                     structured_output=structured_output,
                     iterations=iteration + 1,
                     tools_called=total_tools_called,
@@ -1149,6 +1164,7 @@ def _log_completion(answer: str, iterations: int, forced: bool = False):
             _log_completion(answer, iteration + 1, forced=True)
             return ReflectAgentResult(
                 text=answer,
+                answer_failure_reason=None if answer.strip() else "empty_answer",
                 structured_output=structured_output,
                 iterations=iteration + 1,
                 tools_called=total_tools_called,
@@ -1384,11 +1400,14 @@ def _log_completion(answer: str, iterations: int, forced: bool = False):
                 # Keep context history for fallback final prompt
                 context_history.append({"tool": tc.name, "input": input_dict, "output": output})
 
-    # Should not reach here
-    answer = "I was unable to formulate a complete answer within the iteration limit."
+    # Iteration budget exhausted without a done() call. This is a failure, not an
+    # answer: the text below is a human-readable placeholder, so it must be
+    # reported as such or a persisting caller would store it over real content.
+    answer = ITERATION_LIMIT_TEXT
     _log_completion(answer, max_iterations, forced=True)
     return ReflectAgentResult(
         text=answer,
+        answer_failure_reason="iteration_limit",
         iterations=max_iterations,
         tools_called=total_tools_called,
         tool_trace=tool_trace,
@@ -1436,6 +1455,10 @@ async def _process_done_tool(
     # Extract and clean the answer - some LLMs leak structured output into the answer text
     raw_answer = args.get("answer", "").strip()
     answer = _clean_done_answer(raw_answer) if raw_answer else ""
+    # Record the failure here, where "the model returned nothing usable" is still
+    # known. Once NO_ANSWER_TEXT is substituted the result looks like a normal
+    # non-empty answer, and downstream emptiness checks no longer detect it.
+    answer_failure_reason: AnswerFailureReason = None if answer else "empty_answer"
     if not answer:
         answer = NO_ANSWER_TEXT
 
@@ -1500,6 +1523,7 @@ async def _process_done_tool(
     log_completion(answer, iterations)
     return ReflectAgentResult(
         text=answer,
+        answer_failure_reason=answer_failure_reason,
         structured_output=structured_output,
         iterations=iterations,
         tools_called=total_tools_called,

hindsight-api-slim/hindsight_api/engine/reflect/models.py

@@ -6,6 +6,20 @@
 
 from pydantic import BaseModel, Field
 
+# Fallback answers the reflect agent returns when it cannot produce a real one.
+# Both are deliberately NON-EMPTY so a read-path caller gets a readable reply —
+# which is exactly why persistence must not test emptiness to detect them. They
+# live here (not in agent.py) so memory_engine can import them without pulling in
+# the agent module.
+NO_ANSWER_TEXT = "No answer provided."
+ITERATION_LIMIT_TEXT = "I was unable to formulate a complete answer within the iteration limit."
+
+# Machine-readable cause of a missing answer. None means "the answer is real".
+# Prefer this over comparing text: it is set where the condition is known, it
+# distinguishes the two failure modes for auditing, and it cannot be defeated by
+# a reworded fallback string.
+AnswerFailureReason = Literal["empty_answer", "iteration_limit"] | None
+
 
 class ObservationSection(BaseModel):
     """A section within an observation with its supporting memories."""
@@ -110,6 +124,17 @@ class ReflectAgentResult(BaseModel):
     """Result from the reflect agent."""
 
     text: str = Field(description="Final answer text")
+    answer_failure_reason: AnswerFailureReason = Field(
+        ...,
+        description=(
+            "Why this result carries no usable answer, or None when the answer is real. "
+            "REQUIRED (no default) on purpose: a future terminal path that forgets to set it "
+            "raises a validation error instead of silently reporting success. Callers that "
+            "PERSIST a reflect result (mental-model refresh) must treat any non-None value as a "
+            "failed render and preserve the previous content — the fallback texts below are "
+            "non-empty, so an emptiness check does not catch them."
+        ),
+    )
     structured_output: dict[str, Any] | None = Field(
         default=None, description="Structured output parsed according to provided response_schema"
     )

hindsight-api-slim/hindsight_api/engine/response_models.py

@@ -6,7 +6,7 @@
 API stability even if internal models change.
 """
 
-from typing import Any
+from typing import Any, Literal
 
 from pydantic import BaseModel, ConfigDict, Field, field_validator
 
@@ -421,6 +421,16 @@ class ReflectResult(BaseModel):
         default_factory=list,
         description="Directive mental models that were applied during this reflection.",
     )
+    answer_failure_reason: Literal["empty_answer", "iteration_limit"] | None = Field(
+        default=None,
+        description=(
+            "Why no usable answer was produced ('empty_answer' when the model returned nothing, "
+            "'iteration_limit' when the agent ran out of iterations), or None when the answer is "
+            "real. `text` still carries a human-readable placeholder in the failure cases, so "
+            "callers that store the result must check this field rather than testing text for "
+            "emptiness."
+        ),
+    )
 
 
 class EntityObservation(BaseModel):