[PraveenKumarSridhar]

fix(reflect): preserve required recall when context fills (#4563)

Fixes #4563.

Problem

During a mental-model refresh, a large search_observations result can trip the Reflect context guard before the required raw-fact recall step. The agent then synthesizes from observations alone, potentially missing newer facts and corrections.

Change

• Compact earlier tool results in the model-facing conversation only when a required retrieval step needs room. Keep the full results for final synthesis, citations, and tracing.
• Run the required retrieval before synthesizing. After compaction, synthesize from the complete evidence history rather than a shortened conversation.
• Fail explicitly when the required step cannot fit, the provider rejects its context, or the iteration limit would skip it.
• Reject a provider response that ignores the pinned recall choice, returns done or prose instead, or returns an invalid/error recall result. A named tool choice alone does not prove raw facts were retrieved.
• Use the splitter's bounded single chunk for final synthesis, so an oversized individual result is not silently dropped.
• Add red-first regressions for oversized observations, early recall, invalid recall output, provider noncompliance, context overflow, iteration limits, prompt caching, and final evidence retention.

Validation

• uv run --no-sync pytest tests/test_reflect_agent.py tests/test_reflect_split_synthesis.py -q -n 0 -m 'not hs_llm_core' --disable-warnings --tb=short -o log_cli=false: 96 passed, 4 live LLM tests deselected.
• Full blackbox system suite: 287 passed, including mental-model refresh and Reflect.
• API-wide Ruff check/format, ty check hindsight_api, and git diff --check: passed.
• Full local API run: 9,790 passed, 252 skipped, 5 xfailed, 9 failed, 10 setup errors. The failures were outside the changed agent/tests: five live-LLM tests lacked local credentials, several pg0 tests hit invalid macOS C.UTF-8 locale or stale local Alembic revision, and parallel database tests hit a deadlock/audit timing failure. The three database failures passed in isolation; the Reflect-adjacent per-operation config test passed when rerun with LC_ALL=en_US.UTF-8.

Related work

PR #4582 restores retrieval after fresh mental models. This change handles the context guard when a required retrieval step has not yet run. The two patches touch the Reflect agent and tests, so they may need conflict resolution if either merges first.

Limitations

No live LLM test was run in this worktree, and the full local API run was not green for the environment and unrelated failures noted above. A fresh mental model can still release the forced sequence under the existing policy, which PR #4582 addresses separately. The existing splitter still truncates a single indivisible result that exceeds the synthesis budget; this change keeps the complete tool result in history and trace, and ensures the bounded prefix reaches synthesis.

[nicoloboschi]

Thanks for this! We're tracking this problem in #4563 and will fix it from there, so I'm closing this PR.

We've stopped accepting pull requests from outside the team (see CONTRIBUTING.md). Any extra detail or steps to reproduce on the issue are very welcome.

[nicoloboschi]

Superseded by #5292. It keeps this PR's idea (blank earlier results so the required recall still fits, then answer from the full evidence) and drops the new hard failures, which broke 3 existing tests and could fail every reflect under a lowered step limit. Retrying an ignored forced step already landed on main in #5236.