Keep architect tool data across turns, show reviews in full - #2559
Conversation
There was a problem hiding this comment.
The core approach (persisting a rendered, bounded tool-result digest across turns + showing unnamed records whole) directly addresses the hallucination sources described and the tests look on-point.
Main things to consider before merge:
- If the newest carried entry is too large for
carried_result_max_chars,_build_carried_tool_results()currently drops the entire digest (including older smaller entries) due to abreak. - The per-record
record_block_charsbudget for unnamed records isn’t strictly enforced because overhead isn’t counted (and there’s no aggregate counter), so worst-case prompt growth can exceed the intended bound.
Found 2 issues (0 critical, 2 improvements).
| for rendered in reversed([*self._carried_tool_results, *fresh]): | ||
| if len(selected) >= cfg.carried_result_max_entries: | ||
| break | ||
| if used + len(rendered) > cfg.carried_result_max_chars: |
There was a problem hiding this comment.
Improvement: _build_carried_tool_results stops building the digest entirely when the newest entry doesn’t fit the remaining char budget (break on used + len(rendered) > ...). If a single large rendered result (e.g. a compacted 20-row page) exceeds carried_result_max_chars, this will produce an empty digest and also drop any older smaller carried entries.
Fix: consider
continue-skipping oversized entries (so older data still survives), and/or truncating an oversized newest entry to fit with a clear marker (similar to_render_tool_result’s fallback truncation). A regression test for “one oversized newest entry doesn’t wipe existing digest” would lock this in.
There was a problem hiding this comment.
Fixed in 693b4be: oversized newest entries are now truncated/skipped instead of break-ing the whole build, and there’s a regression test for the “fat page doesn’t empty/wipe digest” case. 👍
| unnamed_count = sum(1 for item in shown if not (item.get("name") or item.get("title"))) | ||
| allowance = record_chars | ||
| if unnamed_count: | ||
| allowance = min( |
There was a problem hiding this comment.
Improvement: allowance is computed per unnamed record as min(record_chars, max(_MIN_RECORD_CHARS, record_block_chars // unnamed_count)), but the loop then appends f" - {compact_item}" without accounting for the JSON overhead/newline/prefix. That means the actual block size can exceed record_block_chars (sometimes by a lot), defeating the “shared across unnamed records” guarantee.
Fix: either (a) include overhead in the per-record budget (e.g.
allowance - len(" - ")), or (b) enforcerecord_block_charsas an aggregate counter while iterating and truncate/mark once exceeded.
There was a problem hiding this comment.
With the new per-record truncation marker, I’m less worried about strict record_block_chars adherence: worst-case overhead (bullet + marker) is small vs the 20k default, and the important contract (don’t drop rows; mark truncation) is covered by tests. I’d treat this as non-blocking unless you need a hard cap.
Two ways the architect ended up reporting on annotations it could not see, both of which it filled in from imagination. Tool results died with the turn. The backend rehydrates only conversation_history, so every user message builds a fresh agent with an empty execution history. A follow-up question about something already fetched — the exact words of a review, who left it — had nothing but the agent's own earlier prose to answer from. dump_state now emits a bounded digest of rendered tool results, persist_state stores it on agent_state, and it goes into both the iteration and streaming prompts under a header that says it may be stale. Review comments arrived clipped to 80 characters. desc_chars is a summary width for a named entity's description, where the rest is one get_* call away. It was also being applied to every string inside an unnamed record, where this rendering is the only place the content is ever shown — so a reviewer's comment reached the LLM as half a sentence, which it finished on their behalf. Unnamed records are now dumped whole, bounded per field and in aggregate by budgets far above real data, and the aggregate budget is shared across the page so detail degrades instead of rows disappearing. Also slices the streaming prompt to the current turn. Execution history accumulates in-process, so a third-turn response was handed turn one's results under "data you collected" as if freshly fetched. Refs #2402 Signed-off-by: Nicolai Bohn <nicolai@rhesis.ai>
ece62dd to
6606e99
Compare
There was a problem hiding this comment.
Main idea looks solid and the new tests capture the intended “quote real tool data across turns” contract well.
Blocking: the two open threads still apply:
_build_carried_tool_resultsbreaks out if the newest rendered entry doesn’t fit the remainingcarried_result_max_chars, which can yield an empty carried digest when a single compacted page exceeds 12k (exactly the “fat page” case). This undermines the MR’s goal._compact_list_result_for_history’s per-recordallowancedoesn’t account for per-line overhead / named-item lines, sorecord_block_charsisn’t a hard bound.
Once those are addressed, ship it.
_build_carried_tool_results walked newest-first and stopped at the first entry that did not fit carried_result_max_chars. A compacted page of unnamed records renders past that budget on its own, so one such page returned an empty digest and dropped the older entries behind it — losing exactly the annotation data the digest exists to carry. Truncate to fit with a marker instead of stopping, and cap a truncated entry at half the budget so the newest page cannot starve the rest. Signed-off-by: Nicolai Bohn <nicolai@rhesis.ai>
|
Looks good. The carried tool-result digest + full rendering for unnamed records should materially reduce invented review content, and the new tests cover the risky cases (fresh-agent follow-up, fat pages, streaming turn-slice). Ship it. |
Purpose
#2427 stopped annotations arriving at the LLM as a column of
- ?, but it did not stop the architect inventing review content. Two paths were left open, and both are data starvation rather than creativity — the agent is told reviews exist, shown nothing usable, and asked to report on them.Tool results died with the turn.
prepare_and_load_sessionrehydrates onlyconversation_historyfrom the DB, so every user message builds a fresh agent with an empty_execution_history. A follow-up question about something already fetched — the exact words of a review, who left it, which turn it was on — had nothing but the agent's own earlier prose to answer from, and older messages are cut to 500 chars. This is the likeliest source of invented reviewer names and turn numbers in a real conversation, and #2427 does not touch it.Review comments arrived clipped to 80 characters.
desc_charsis a summary width for a named entity's description, where the untruncated version is oneget_*call away. Since #2427 it was also the clip width for every string inside an unnamed record, where this rendering is the only place the content is ever shown. A reviewer's comment reached the model as half a sentence ending in…, next to an instruction to "expand on the seed with appropriate detail" — the setup for finishing someone else's sentence and presenting it as their review. Arguably worse than the bare- ?it replaced, because a partial quote reads as a real one.What Changed
Tool data survives the turn
dump_stateemitscarried_tool_results: the rendered tool results from this and earlier turns, newest-first within two caps, then back in chronological order.restore_stateloads it;persist_statestores it onagent_stateandbuild_agentreads it back. No migration — the key is additive and existing sessions fall back to[]._format_historyfor the reasoning call,_format_tool_results_for_streamingfor the response writer) under a header that tells the agent to quote from it rather than recall it, and to re-read the tool if the user needs current state.finishresult is the answer text, which is already the assistant message in conversation history.carried_result_max_entries(12) andcarried_result_max_chars(12_000). One review costs about 415 bytes of JSONB.Unnamed records are shown whole
_compact_list_result_for_historynow takesrecord_chars(4_000) andrecord_block_chars(20_000) separately fromdesc_chars, which keeps its 80 for named entities. An annotation, a test result, anything without a label is dumped in full.- ?. Anything actually cut carries a… [record truncated]marker.Measured across three page shapes:
Incidental
_format_tool_results_for_streamingnow slices from_turn_start_step. Execution history accumulates in-process, so a third-turn response was handed turn one's results under the heading "Tool Results (data you collected)" as if freshly fetched. Stale data presented as current is its own fabrication source. No effect on the backend path, where the agent is fresh each turn, but it is required for correctness now that data crosses turns at all.Additional Context
- ?).test_long_comment_is_clippedasserted exactly the behaviour this removes, so it is replaced by three tests covering the new contract.streaming_response.j2still instructs the model to "expand on the seed with appropriate detail" with no grounding rule for quoted human text;get_test_resultstill returnstest_reviewsinline as an undocumented second shape for the same reviews; blankcomments/userfields still have no vocabulary, so "no comment left" cannot be said; andtotal_countis returned as anX-Total-Countheader, so the tool payload cannot state a real total. Each is a separate change.workflow_pathis in the snapshot and emitted bydump_state, butpersist_statenever writes it andbuild_agentnever reads it, so it resets tounsetevery turn.Testing
443 SDK tests and 100 backend tests pass. Docker must be running for the backend suite.
New coverage:
json.dumps/loadsthe way the JSONB column does, and turn 2 — a fresh agent with an empty execution history — has the reviewer, the verbatim comment and the turn index in its prompt.finishresults stay out of the digest; both caps keep the newest entries; earlier data survives a turn that fetched nothing;reset()clears it.record_chars; a 20-row page of fat records keeps all 20 rows with the clipping marked and every review still identifiable.Worth a manual check: open the architect on a run with reviews, ask what people flagged, then ask a follow-up about one specific review without naming it again. The follow-up should quote the real comment rather than paraphrasing from its own previous answer.