Repository navigation
fix(reflect): a shallow budget shortens the answer, not the search (#4567) - #5250
Merged
Merged
Conversation
…4567) On low/mid budgets the depth guidance told the agent to stop as soon as the mental models gave "a reasonable answer". Combined with the short-circuit that releases forced lower-level retrieval once the pages come back fresh and non-empty, that let a reflect answer "the bank holds nothing about X" from the page layer alone, while recall() on the same bank returns the facts (#4567). Freshness is not coverage. Reword low and mid so the budget bounds the length of the answer and the number of searches, not whether the lower layers get consulted: - a result that merely shares the topic is not an answer; when it does not cover the question, go on to the next level; - never report that the bank holds nothing until recall() has run with the question's key terms verbatim; - on low, vary the query instead of repeating one that already ran (the report shows three page searches, two returning a byte-identical payload). Top-k and the short-circuit itself are unchanged.
…freshness (#4567) The first commit reworded the low/mid depth guidance; measuring it showed the wording was not the operative instruction. Two places decided the stop, and both keyed on the wrong thing: - the Workflow checklist said "if no mental model or it's stale, try search_observations()". A fresh page that says nothing about the question satisfies neither clause, so the procedure itself told the agent it was finished after one search; - the `done` tool's own description said to use it "when you have gathered enough information", and never said what enough means. That description is what the model weighs when it picks a tool. Both now turn on whether the level STATES the answer, and `done` carries the same gate plus the rule that an absence needs recall() first. Measured, and the result does not support calling this a fix: on gemini-3.1-flash-lite (the core-LLM job's model) the new story passes against the pre-change prompt too, 3/3; on 2.5-flash-lite it fails with the change and without it, across three wordings. The reported incident was a capable model (gpt-5.6-luna, effort low) over a 546k-char page set, and the payload's size and plausibility is what the fixture cannot stage. The test's docstring says so, and #4567 carries the ask to measure it on the reporter's banks.
…times
Review of the two commits above. No behaviour change intended; the
assembled prompt says the same thing in fewer tokens.
- The absence rule ("never report that the bank holds nothing until
recall() has run on the question's key terms") appeared in four places
in one prompt: the low block, the mid block, the Workflow steps and the
done gate. Every line is paid for on every reflect, and this package
spent a release cutting reflect input 39% (#4716). It now lives once at
its source -- the RAW FACTS level, whose MANDATORY rule fired only on a
*zero-result* search and so never covered the case in #4567 -- and once
on `done`, the decision point. The depth blocks keep only the short
coverage line.
- Comment the `done` description change; the prompts.py half had one and
this half did not (code-review step 8).
- Wrap the lines that ran past the file's width convention.
Tests: prompt-builder expectations follow the assembled text (26 passed);
the three real-LLM short-circuit stories pass on gemini-3.1-flash-lite,
including the uncovering-page one, which still descends to recall.
…s-only prompt Parity pass. The mental-model configurations now descend when a level does not state the answer, but the configuration with observations and no mental models still said 'if search_observations returns 0 results', which is the exact gap #4567 is about -- a level that answers with content but not with the answer. Same for its Workflow step. Prompt-builder expectations updated; 26 passed.
nicoloboschi
added a commit
that referenced
this pull request
Oct 6, 2026
…g-page test (#5293) #5250 added test_real_fresh_but_uncovering_mental_model_digs_instead_of_denying after #4776 had moved the agent callbacks into a shared mock_functions fixture, so the new test never requested the fixture and called _stub_functions without its 'base' argument. The Core LLM job has failed on it ever since with '_stub_functions() missing 1 required positional argument: base'. Takes the fixture and passes it positionally, matching the two sibling tests in the same class.
This was referenced Oct 8, 2026
Merged
nicoloboschi
added a commit
that referenced
this pull request
Oct 16, 2026
) On a low/mid reflect, fresh mental models release the forced lower layers, and the model could then answer "the bank holds nothing about X" off the page layer alone while recall held the facts. Prompt rules (#5250, a bank directive) were measured on the reporter's banks and did not move it. `done` now carries a required boolean, `answer_found`. When the model sets it to false and recall has not run yet, the loop refuses that `done` once, tells the model why, and forces `recall` on the next turn; the next `done` is accepted. A missing or unreadable field reads as found, so models that skip it behave as before. Tests: loop tests for the refused-then-recalled path and for a negative after recall (accepted), the lenient field parsing, every `done` variant requiring the field, and an hs_llm_core test that stages the early negative and checks the real model recalls and answers from the raw fact.
This was referenced Oct 16, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re #4567 — guidance, not a verified fix. Please read the measurement before merging.
What the prompt was actually telling the agent
The short-circuit releases the forced lower layers as soon as a page search returns fresh, non-empty pages, and the "you MUST call recall() before giving up" rule only fires on a search returning zero results. A page that shares the question's topic but says nothing about it is neither, so nothing made the agent look deeper. Three separate places told it to stop:
low/middepth guidance — "if mental models or observations provide a reasonable answer, stop there";donetool's own description — "use this when you have gathered enough information", with no definition of enough. That text is what the model weighs at the moment it chooses to stop.All three now turn on whether the level states the answer, and
doneadditionally carries the rule that reporting an absence requiresrecall()on the question's key terms first.Measurement — and it does not support calling this fixed
So the change has no measured effect on either model. On the weaker one, none of the three wordings moved the behaviour — the trace was identical every time: one page search, then
done, answering with the process page read back. On the stronger one, the scenario never failed to begin with.The reported incident was not a weak model:
gpt-5.6-lunaat effortlowover a 546k-char page set. The size and plausibility of the page payload is the part a five-line fixture can't stage, which is why the local runs are flat in both directions. The test's docstring states this scope explicitly rather than implying it guards the incident.What this PR is, honestly: three statements in the prompt that were wrong on their own terms are now right, and the gate sits where the decision is made. Whether that moves a real bank is an open question.
What would settle it
@feniix — you offered to test a branch. This is it. Same snapshot and config as your #4566 rerun, and the number that matters is the one from your table: short-circuited, then called
recall(baseline 0/10, with the directive 3/10). If this still sits near zero, the conclusion is that prompt steering can't carry it and the fix has to be a floor in code — one raw-fact lookup before a short-circuited low/mid reflect may answer.Also worth knowing
Your original report noted the agent re-running the same page search with a near-identical query, twice returning a byte-identical payload. That's unaddressed here and is pure latency at every budget.
Tests
tests/test_reflect_agent.py::TestMentalModelShortCircuitRealLLM— newhs_llm_corejudge story for the fresh-but-uncovering page, with its measured scope in the docstring. 3 passed on 3.1-flash-lite.tests/test_reflect_prompt_builder.py— 26 passed; the asserted prompt text is updated../scripts/hooks/lint.shclean.