From 7fdde43c776778426687f6e5aac241845121a331 Mon Sep 17 00:00:00 2001 From: Tim Geary Date: Sat, 25 Jul 2026 01:20:39 -0400 Subject: [PATCH 1/2] fix(refresh): keep existing content when reflect returns no usable answer refresh_mental_model could replace a working mental model with a one-line placeholder while reporting success. The reflect agent substitutes human-readable text when it cannot produce an answer: NO_ANSWER_TEXT ("No answer provided.") when the model returns nothing, and an iteration-limit message when the agent runs out of iterations. Both are non-empty, so the "refuse to overwrite existing content with an empty render" guard in refresh_mental_model did not fire, and the placeholder was stored over the document. The operation completed without error, so nothing surfaced the loss; populated_content=false was recorded afterwards, but only as metadata on an already-overwritten model. The fix is a machine-readable signal rather than another text check: - ReflectAgentResult.answer_failure_reason ("empty_answer" | "iteration_limit" | None) is set at each terminal path in the agent, where the condition is known. It is REQUIRED, with no default, so a terminal path added later that forgets to set it fails validation instead of silently reporting success. - refresh_mental_model checks it immediately after reflect and BEFORE delta rendering, then preserves the previous content and raises. Order matters: delta can render a placeholder candidate into plausible-looking markdown, so a check placed after it would see ordinary content. - The failure record still stores 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. - is_populated_content() centralizes the placeholder check (empty, pending placeholder, both fallback answers). The persistence guard and the refresh outcome metadata now share it, so what is reported and what is stored cannot drift apart. MentalModelRefreshError is retryable, so the task layer re-raises it as RetryTaskAt and only exhausted retries settle as failed. Preservation is what makes retrying safe: each attempt leaves the existing document intact. tests/test_refresh_preserves_on_failed_answer.py covers both failure texts, evidence preservation, the delta path, the async/worker path, and a success canary. The tests patch reflect_async rather than refresh_mental_model so the real persistence path executes. Verified against pre-fix code: 5 of the 7 fail without the change, and the pre-existing test_mental_models.py errors are unrelated (they require live LLM credentials and occur on an unmodified tree). --- .../hindsight_api/engine/memory_engine.py | 74 +++++- .../hindsight_api/engine/reflect/agent.py | 38 ++- .../hindsight_api/engine/reflect/models.py | 25 ++ .../hindsight_api/engine/response_models.py | 12 +- ...test_refresh_preserves_on_failed_answer.py | 220 ++++++++++++++++++ 5 files changed, 352 insertions(+), 17 deletions(-) create mode 100644 hindsight-api-slim/tests/test_refresh_preserves_on_failed_answer.py diff --git a/hindsight-api-slim/hindsight_api/engine/memory_engine.py b/hindsight-api-slim/hindsight_api/engine/memory_engine.py index 48004e96d6..550ce42368 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,42 @@ 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 + 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( + 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,9 +11944,12 @@ 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" diff --git a/hindsight-api-slim/hindsight_api/engine/reflect/agent.py b/hindsight-api-slim/hindsight_api/engine/reflect/agent.py index f67185a787..cc4bc241ac 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 cc54af4e8a..d9b3481c1d 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 1610c3dc31..aa43111deb 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 0000000000..16660abeaa --- /dev/null +++ b/hindsight-api-slim/tests/test_refresh_preserves_on_failed_answer.py @@ -0,0 +1,220 @@ +"""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.""" + + def _install(memory: MemoryEngine, result: ReflectResult): + async def fake_reflect_async(**kwargs): + return result + + monkeypatch.setattr(memory, "reflect_async", fake_reflect_async) + + 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.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(bank_with_model, request_context, patch_reflect): + """Delta rendering must never get the chance to turn a placeholder into content. + + Delta operates on the candidate and re-renders the document, so a check placed + after it could see ordinary-looking markdown. The failure check therefore runs + before delta; this test drives the same refresh twice, since delta mode only + engages once a prior refresh has recorded a matching source query. + """ + memory, bank_id, mm = bank_with_model + + patch_reflect(memory, _reflect_result("# Real Synthesis\n\nEstablished baseline.", failure_reason=None)) + 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) + assert baseline["content"].startswith("# Real Synthesis") + + 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"] == baseline["content"] + + +@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 From 3fd24a0f1e3b56b0890319a4d668dc499b256e16 Mon Sep 17 00:00:00 2001 From: Tim Geary Date: Sat, 25 Jul 2026 01:45:48 -0400 Subject: [PATCH 2/2] fix(refresh): keep query tracking unchanged when a refresh fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the previous commit found that both failed-refresh audit writes still passed last_refreshed_source_query=current_source_query while deliberately preserving the old content, which corrupts the retry. Delta mode requires an unchanged source query, so changing a delta model's query correctly forces a full regeneration: the stored document is about the previous topic. But if that attempt failed, the audit write recorded the new query anyway. The retry then saw an unchanged query, selected delta, and passed created_after — amending the old topic's document from recall scoped to new memories only. Content was preserved on every individual attempt, yet a later successful retry could persist mixed-topic content. Both failure paths (the answer_failure_reason guard and the empty-candidate guard) now write only reflect_response. Nothing was synthesized for the current query, so no tracking state should say otherwise. The success path and the delta no_new_facts path still advance it, since in both cases the stored content really is current for that query. Also repairs the delta regression, which never enabled delta mode: the fixture omitted trigger.mode and refresh_mental_model defaults to "full", so the test performed two full refreshes and its name claimed a path it never exercised. There is now a delta fixture, the delta LLM call is instrumented, and the test asserts both that delta was actually selected (created_after present) and that the failed answer exits before any delta operation is emitted. The canned operation adds an ordinary prose section, so with the early guard removed delta produces normal-looking markdown — the laundering the guard exists to prevent. Verified by mutation: restoring the query-tracking write fails the new tracking test; disabling the early guard fails the delta test. Focused and related suites: 21 passed, 4 skipped. --- .../hindsight_api/engine/memory_engine.py | 14 +- ...test_refresh_preserves_on_failed_answer.py | 162 +++++++++++++++--- 2 files changed, 147 insertions(+), 29 deletions(-) diff --git a/hindsight-api-slim/hindsight_api/engine/memory_engine.py b/hindsight-api-slim/hindsight_api/engine/memory_engine.py index 550ce42368..5b4e2ce9d1 100644 --- a/hindsight-api-slim/hindsight_api/engine/memory_engine.py +++ b/hindsight-api-slim/hindsight_api/engine/memory_engine.py @@ -11807,11 +11807,15 @@ async def refresh_mental_model( 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, - last_refreshed_source_query=current_source_query, request_context=request_context, ) raise MentalModelRefreshError( @@ -11953,13 +11957,15 @@ async def refresh_mental_model( "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/tests/test_refresh_preserves_on_failed_answer.py b/hindsight-api-slim/tests/test_refresh_preserves_on_failed_answer.py index 16660abeaa..79e49d01ea 100644 --- a/hindsight-api-slim/tests/test_refresh_preserves_on_failed_answer.py +++ b/hindsight-api-slim/tests/test_refresh_preserves_on_failed_answer.py @@ -52,13 +52,62 @@ def _reflect_result(text: str, *, failure_reason: str | None, facts: list[dict] @pytest.fixture def patch_reflect(monkeypatch): - """Patch reflect_async only, so refresh_mental_model itself still runs for real.""" + """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] = [] - def _install(memory: MemoryEngine, result: ReflectResult): 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 @@ -79,6 +128,27 @@ async def bank_with_model(memory: MemoryEngine, request_context): 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.""" @@ -86,9 +156,7 @@ async def test_empty_answer_preserves_content_and_raises(bank_with_model, reques 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 - ) + 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 @@ -105,9 +173,7 @@ async def test_iteration_limit_preserves_content_and_raises(bank_with_model, req 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 - ) + 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 @@ -126,9 +192,7 @@ async def test_failed_refresh_preserves_supporting_evidence(bank_with_model, req 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 - ) + 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"]] @@ -136,31 +200,81 @@ async def test_failed_refresh_preserves_supporting_evidence(bank_with_model, req @pytest.mark.asyncio -async def test_delta_mode_cannot_launder_a_failed_answer(bank_with_model, request_context, patch_reflect): +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 operates on the candidate and re-renders the document, so a check placed - after it could see ordinary-looking markdown. The failure check therefore runs - before delta; this test drives the same refresh twice, since delta mode only - engages once a prior refresh has recorded a matching source query. + 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 = bank_with_model + 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) - assert baseline["content"].startswith("# Real Synthesis") - patch_reflect(memory, _reflect_result(NO_ANSWER_TEXT, failure_reason="empty_answer")) + 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 - ) + 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.""" @@ -179,9 +293,7 @@ async def test_successful_refresh_still_writes_content(bank_with_model, request_ @pytest.mark.asyncio -async def test_async_refresh_of_failed_answer_does_not_report_success( - bank_with_model, request_context, patch_reflect -): +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