diff --git a/hindsight-api-slim/hindsight_api/engine/memory_engine.py b/hindsight-api-slim/hindsight_api/engine/memory_engine.py index 48004e96d..5b4e2ce9d 100644 --- a/hindsight-api-slim/hindsight_api/engine/memory_engine.py +++ b/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( diff --git a/hindsight-api-slim/hindsight_api/engine/reflect/agent.py b/hindsight-api-slim/hindsight_api/engine/reflect/agent.py index f67185a78..cc4bc241a 100644 --- a/hindsight-api-slim/hindsight_api/engine/reflect/agent.py +++ b/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, diff --git a/hindsight-api-slim/hindsight_api/engine/reflect/models.py b/hindsight-api-slim/hindsight_api/engine/reflect/models.py index cc54af4e8..d9b3481c1 100644 --- a/hindsight-api-slim/hindsight_api/engine/reflect/models.py +++ b/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" ) diff --git a/hindsight-api-slim/hindsight_api/engine/response_models.py b/hindsight-api-slim/hindsight_api/engine/response_models.py index 1610c3dc3..aa43111de 100644 --- a/hindsight-api-slim/hindsight_api/engine/response_models.py +++ b/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): diff --git a/hindsight-api-slim/tests/test_refresh_preserves_on_failed_answer.py b/hindsight-api-slim/tests/test_refresh_preserves_on_failed_answer.py new file mode 100644 index 000000000..79e49d01e --- /dev/null +++ b/hindsight-api-slim/tests/test_refresh_preserves_on_failed_answer.py @@ -0,0 +1,332 @@ +"""A refresh that produces no usable answer must keep the previous content. + +The reflect agent substitutes a human-readable placeholder when it cannot produce +an answer: ``NO_ANSWER_TEXT`` when the model returns nothing, ``ITERATION_LIMIT_TEXT`` +when the agent runs out of iterations. Both are non-empty, so ``refresh_mental_model``'s +"refuse to overwrite with an empty render" guard did not fire for them and the +placeholder was stored over the existing document — while the operation reported +success. + +The fix is ``ReflectAgentResult.answer_failure_reason``: set where the failure is +known, checked before any delta rendering, and independent of the placeholder text. +These tests pin that behavior end to end. They patch ``reflect_async`` rather than +``refresh_mental_model`` so the real persistence path — delta, guards, the write — +actually executes; stubbing the refresh itself would assert nothing about storage. +""" + +import uuid + +import pytest + +from hindsight_api import MemoryEngine +from hindsight_api.engine.memory_engine import MentalModelRefreshError, is_populated_content +from hindsight_api.engine.reflect.models import ITERATION_LIMIT_TEXT, NO_ANSWER_TEXT +from hindsight_api.engine.response_models import ReflectResult + +HEALTHY_CONTENT = "# Working Document\n\nThis is the real synthesized content that must survive." + +# Non-empty evidence, mirroring the production failures: recall returned plenty of +# facts and the answer still came back unusable. Preserving this distinguishes the +# defect from the empty-recall case and keeps grounding for the next refresh. +SUPPORTING_FACTS = [ + {"id": "f1", "text": "supporting fact one", "type": "observation", "context": None}, + {"id": "f2", "text": "supporting fact two", "type": "observation", "context": None}, +] + + +def _reflect_result(text: str, *, failure_reason: str | None, facts: list[dict] | None = None) -> ReflectResult: + return ReflectResult.model_validate( + { + "text": text, + "answer_failure_reason": failure_reason, + "based_on": { + "observation": facts if facts is not None else SUPPORTING_FACTS, + "world": [], + "experience": [], + "mental-models": [], + "directives": [], + }, + } + ) + + +@pytest.fixture +def patch_reflect(monkeypatch): + """Patch reflect_async only, so refresh_mental_model itself still runs for real. + + Returns the list of kwargs each call received, which lets a test see how the + refresh classified itself — notably whether ``created_after`` was passed, the + marker of an incremental (delta) recall. + """ + + def _install(memory: MemoryEngine, result: ReflectResult) -> list[dict]: + calls: list[dict] = [] + + async def fake_reflect_async(**kwargs): + calls.append(kwargs) + return result + + monkeypatch.setattr(memory, "reflect_async", fake_reflect_async) + return calls + + return _install + + +@pytest.fixture +def patch_delta_llm(monkeypatch): + """Patch the reflect LLM ``.call()`` that delta mode uses to emit operations. + + Records every invocation so a test can assert the delta call never happened. + The canned operation rewrites the document to ordinary markdown: if the early + failure check is removed, delta turns the placeholder candidate into content + that looks entirely normal, which is the laundering path being guarded. + """ + from hindsight_api.engine.reflect.delta_ops import DeltaOperationList + + def _install(memory: MemoryEngine) -> list[dict]: + calls: list[dict] = [] + canned = DeltaOperationList.model_validate( + { + "operations": [ + { + "op": "add_section", + "heading": "Delta Rendered Section", + "blocks": [ + { + "type": "paragraph", + "text": "Prose produced by delta that reads like a normal refresh.", + } + ], + } + ] + } + ) + + async def fake_call(*, messages, **kwargs): + calls.append({"messages": messages, **kwargs}) + return canned + + monkeypatch.setattr(memory._reflect_llm_config, "call", fake_call) + return calls + + return _install + + +@pytest.fixture +async def bank_with_model(memory: MemoryEngine, request_context): + """Bank holding one mental model with known-good content; unique per test for xdist.""" + bank_id = f"test-refresh-preserve-{uuid.uuid4().hex[:8]}" + await memory.get_bank_profile(bank_id, request_context=request_context) + mm = await memory.create_mental_model( + bank_id=bank_id, + name="Preservation Model", + source_query="What must survive a failed refresh?", + content=HEALTHY_CONTENT, + request_context=request_context, + ) + yield memory, bank_id, mm + await memory.delete_bank(bank_id, request_context=request_context) + + +@pytest.fixture +async def delta_bank_with_model(memory: MemoryEngine, request_context): + """Same, but with trigger.mode="delta". + + refresh_mental_model defaults the mode to "full", so a delta-path test that + omits this exercises full refreshes regardless of what its name says. + """ + bank_id = f"test-refresh-delta-{uuid.uuid4().hex[:8]}" + await memory.get_bank_profile(bank_id, request_context=request_context) + mm = await memory.create_mental_model( + bank_id=bank_id, + name="Delta Preservation Model", + source_query="What must survive a failed delta refresh?", + content=HEALTHY_CONTENT, + trigger={"mode": "delta"}, + request_context=request_context, + ) + yield memory, bank_id, mm + await memory.delete_bank(bank_id, request_context=request_context) + + +@pytest.mark.asyncio +async def test_empty_answer_preserves_content_and_raises(bank_with_model, request_context, patch_reflect): + """The no-answer placeholder must not replace a working document.""" + memory, bank_id, mm = bank_with_model + patch_reflect(memory, _reflect_result(NO_ANSWER_TEXT, failure_reason="empty_answer")) + + with pytest.raises(MentalModelRefreshError): + await memory.refresh_mental_model(bank_id=bank_id, mental_model_id=mm["id"], request_context=request_context) + + after = await memory.get_mental_model(bank_id, mm["id"], request_context=request_context) + assert after["content"] == HEALTHY_CONTENT + assert after["reflect_response"]["refresh_skipped"] == "empty_answer" + + +@pytest.mark.asyncio +async def test_iteration_limit_preserves_content_and_raises(bank_with_model, request_context, patch_reflect): + """The iteration-limit placeholder is a different failure and must also be caught. + + A guard written against the no-answer text alone would let this one through. + """ + memory, bank_id, mm = bank_with_model + patch_reflect(memory, _reflect_result(ITERATION_LIMIT_TEXT, failure_reason="iteration_limit")) + + with pytest.raises(MentalModelRefreshError): + await memory.refresh_mental_model(bank_id=bank_id, mental_model_id=mm["id"], request_context=request_context) + + after = await memory.get_mental_model(bank_id, mm["id"], request_context=request_context) + assert after["content"] == HEALTHY_CONTENT + assert after["reflect_response"]["refresh_skipped"] == "iteration_limit" + + +@pytest.mark.asyncio +async def test_failed_refresh_preserves_supporting_evidence(bank_with_model, request_context, patch_reflect): + """The failure record keeps this refresh's based_on rather than blanking it. + + Two reasons: it is the evidence that the answer failed *despite* real retrieval, + and delta refreshes accumulate grounding from the stored reflect_response, so + erasing it would degrade the next successful refresh. + """ + memory, bank_id, mm = bank_with_model + patch_reflect(memory, _reflect_result(NO_ANSWER_TEXT, failure_reason="empty_answer")) + + with pytest.raises(MentalModelRefreshError): + await memory.refresh_mental_model(bank_id=bank_id, mental_model_id=mm["id"], request_context=request_context) + + after = await memory.get_mental_model(bank_id, mm["id"], request_context=request_context) + stored_ids = [f["id"] for f in after["reflect_response"]["based_on"]["observation"]] + assert stored_ids == ["f1", "f2"] + + +@pytest.mark.asyncio +async def test_delta_mode_cannot_launder_a_failed_answer( + delta_bank_with_model, request_context, patch_reflect, patch_delta_llm +): + """Delta rendering must never get the chance to turn a placeholder into content. + + Delta applies operations to the stored document and re-renders it, so a guard + placed after that step would inspect ordinary-looking markdown and pass. The + failure check therefore runs before delta — asserted here by the delta LLM call + never being made. + + The model is created with trigger.mode="delta" (refresh defaults to full + otherwise) and given a successful refresh first, since delta also requires + existing content and an unchanged source query. + """ + memory, bank_id, mm = delta_bank_with_model + + patch_reflect(memory, _reflect_result("# Real Synthesis\n\nEstablished baseline.", failure_reason=None)) + delta_calls = patch_delta_llm(memory) + await memory.refresh_mental_model(bank_id=bank_id, mental_model_id=mm["id"], request_context=request_context) + baseline = await memory.get_mental_model(bank_id, mm["id"], request_context=request_context) + + reflect_calls = patch_reflect(memory, _reflect_result(NO_ANSWER_TEXT, failure_reason="empty_answer")) + delta_calls.clear() + with pytest.raises(MentalModelRefreshError): + await memory.refresh_mental_model(bank_id=bank_id, mental_model_id=mm["id"], request_context=request_context) + + # Delta mode really was selected for this attempt: recall was scoped to memories + # created since the last refresh. So the guard was reached on the delta path. + assert "created_after" in reflect_calls[0] + # ...and it stopped before delta could render anything. + assert delta_calls == [] + after = await memory.get_mental_model(bank_id, mm["id"], request_context=request_context) + assert after["content"] == baseline["content"] + + +@pytest.mark.asyncio +async def test_failed_refresh_does_not_advance_source_query_tracking( + delta_bank_with_model, request_context, patch_reflect, patch_delta_llm +): + """A failed refresh must not record the new query as the one that was synthesized. + + Changing a delta model's source query forces a full regeneration, because the + stored document is about the previous topic. If a failed attempt still recorded + the new query, the retry would see an unchanged query, switch to delta, and + amend the old topic's document using recall scoped to new memories only — + quietly producing mixed-topic content even though the guard preserved content + each time. + """ + memory, bank_id, mm = delta_bank_with_model + patch_delta_llm(memory) + + # Baseline: a successful refresh for the original query. + patch_reflect(memory, _reflect_result("# Original Topic\n\nBaseline content.", failure_reason=None)) + await memory.refresh_mental_model(bank_id=bank_id, mental_model_id=mm["id"], request_context=request_context) + + # The topic changes, so the next refresh must be a full regeneration. + await memory.update_mental_model( + bank_id, + mm["id"], + source_query="An entirely different question about a different topic", + request_context=request_context, + ) + + first_attempt = patch_reflect(memory, _reflect_result(NO_ANSWER_TEXT, failure_reason="empty_answer")) + with pytest.raises(MentalModelRefreshError): + await memory.refresh_mental_model(bank_id=bank_id, mental_model_id=mm["id"], request_context=request_context) + assert "created_after" not in first_attempt[0], "query change should force a full regeneration" + + # The retry must still be a full regeneration: the failure changed no tracking state. + retry = patch_reflect(memory, _reflect_result(NO_ANSWER_TEXT, failure_reason="empty_answer")) + with pytest.raises(MentalModelRefreshError): + await memory.refresh_mental_model(bank_id=bank_id, mental_model_id=mm["id"], request_context=request_context) + assert "created_after" not in retry[0], "a failed attempt must not reclassify the retry as delta" + + +@pytest.mark.asyncio +async def test_successful_refresh_still_writes_content(bank_with_model, request_context, patch_reflect): + """The guard must not block real answers — the failure path is the exception.""" + memory, bank_id, mm = bank_with_model + new_content = "# Updated Synthesis\n\nFresh content from a successful reflect." + patch_reflect(memory, _reflect_result(new_content, failure_reason=None)) + + refreshed = await memory.refresh_mental_model( + bank_id=bank_id, mental_model_id=mm["id"], request_context=request_context + ) + + assert refreshed["content"] == new_content + assert "refresh_skipped" not in refreshed["reflect_response"] + after = await memory.get_mental_model(bank_id, mm["id"], request_context=request_context) + assert after["content"] == new_content + + +@pytest.mark.asyncio +async def test_async_refresh_of_failed_answer_does_not_report_success(bank_with_model, request_context, patch_reflect): + """Through the task path, a failed refresh never settles as a successful one. + + This is the property the incident violated: the operation reported success while + the stored document had been replaced by a placeholder. The failure must surface + as an error and the content must be untouched. + + MentalModelRefreshError is classified retryable, so the task layer re-raises it as + RetryTaskAt: a transient provider hiccup gets another attempt, and only exhausted + retries settle as failed. Preservation is what makes that safe to retry — each + attempt leaves the existing document intact. The test backend runs the task inline + on submit, so that wrapper surfaces here. + """ + from hindsight_api.worker.exceptions import RetryTaskAt + + memory, bank_id, mm = bank_with_model + patch_reflect(memory, _reflect_result(NO_ANSWER_TEXT, failure_reason="empty_answer")) + + with pytest.raises(RetryTaskAt): + await memory.submit_async_refresh_mental_model( + bank_id=bank_id, mental_model_id=mm["id"], request_context=request_context + ) + + after = await memory.get_mental_model(bank_id, mm["id"], request_context=request_context) + assert after["content"] == HEALTHY_CONTENT + assert after["reflect_response"]["refresh_skipped"] == "empty_answer" + + +def test_is_populated_content_rejects_every_placeholder(): + """The text-level backstop knows all three placeholders, not just the empty string.""" + assert is_populated_content("# Real content") is True + assert is_populated_content("") is False + assert is_populated_content(" \n ") is False + assert is_populated_content(None) is False + assert is_populated_content(NO_ANSWER_TEXT) is False + assert is_populated_content(ITERATION_LIMIT_TEXT) is False + assert is_populated_content("Generating content...") is False