fix(agent): stop the grounding rule and compaction summaries from licensing fabricated answers - #2149
Conversation
The rule used to require an up-front disclaimer for unsourced figures but otherwise let the model present them, and scoped that disclaimer duty to figures and numbers specifically. A model could satisfy it by inventing rows and appending a label copied near-verbatim from the rule text. Replace the disclosure duty with a default prohibition: unsourced values of any enumerated kind (numbers, person or organization names, identifiers, dates, statuses, table rows) must be omitted and reported as missing. The only exception is a current user request that explicitly asks for a template or sample, and even then the model must state the nature of the content before presenting it rather than appending a caveat afterward. The exemption for wording the model must compose itself (search queries, code, document text) is unconditional so all four call sites -- including the three forced-answer sites that previously lacked any prompt covering this -- carry it, while facts written literally inside such composed text remain subject to the sourcing rule. Also fix the module and function docstrings, which claimed disclosure was the instructed default; that stopped being true once fabrication without a source became a prohibition rather than a labelling obligation.
Both patterns follow their grounding_rule() call with a local sentence
that named the old wording ("unsupported specifics", "illustrative
placeholder"). The rule no longer uses that vocabulary, so those
sentences pointed at wording the model would never see.
Auto's routing remedy is reworded to name the value kinds the rule
forbids without changing its own job (route to react so a tool can
supply the value). DAG's assessment sentence is reworded to describe
leaving out a value rather than labelling an illustrative placeholder,
matching the rule's gap-reporting instruction.
The compaction summary prompt asked the summarizer to "name the next action needed" and told it the next call should continue "without redoing completed tool calls." Both fabrication-incident summaries put a do-not-call-tools instruction in that slot; the summarizer has no visibility into what tools the next call will actually have. Remove the next-action instruction entirely and state explicitly that the summary must not instruct the next call on tool use. Require fact-carrying values for the records the current request points at (names, identifiers, statuses, dates, counts) to be copied character for character rather than paraphrased or invented to complete a pattern, with credentials and unrelated personal information excluded even when they would otherwise qualify. Forbid claiming a dataset is complete unless the history shows every item was both returned and is still described in the summary -- the incident's dataset was fully returned across nine tool calls, but four of those calls' raw payloads had already been dropped by an earlier compaction. Give the smallest output budget an explicit priority order for what survives, and cap the prompt itself so it cannot silently outgrow the budget it exists to fit inside. The trailer appended after the summary is unchanged; it already carries a correctly-conditioned "re-read or re-query the source" instruction and is not part of this text.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refines the LLM prompts for evidence compaction and grounding rules to strictly forbid unsourced values (such as names, dates, statuses, and identifiers) by default, requiring the model to report missing data as gaps rather than inventing values. It also updates the compaction prompt to prioritize verbatim preservation of requested records and adds comprehensive unit tests. The review feedback highlights an inconsistency in auto.py where the list of forbidden value kinds is narrower than the centralized VALUE_KINDS in grounding.py, and suggests aligning both the prompt and its corresponding test assertion to ensure consistent routing behavior.
…medy The auto pattern's routing-remedy sentence and its test assertion each hand-wrote a five-item value-kind list that had drifted from the grounding module's six-item VALUE_KINDS constant (missing "a status"). Both now interpolate VALUE_KINDS directly, so the routing remedy always matches the same value kinds the grounding rule itself forbids, with no second literal copy left to fall out of sync.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and strengthens the grounding rules and LLM compaction prompts across the codebase to prevent data fabrication and hallucination (mitigating issue #1235). It introduces a centralized VALUE_KINDS definition to cover numbers, names, identifiers, dates, statuses, and table rows, and updates the grounding prompt to strictly forbid inventing these values by default, requiring gaps to be reported instead of using illustrative placeholders or caveats. The LLM compaction prompt is also updated to ensure verbatim preservation of requested records, forbid unearned completeness claims, and prioritize content when under a tight token budget. Corresponding updates are made to the Auto and DAG execution patterns, along with comprehensive test coverage validating these new behaviors. No review comments were provided, so there is no additional feedback.
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
execution.py:1245,1412 (outside this diff, see pointer on execution.py:1480) — [major] compaction trailer / dropped-tool notice still only names "figure", not the broader value-kind scope fixed elsewhere in this PR.
grounding.py:98-105 — [minor] template/sample exception scoped to "the answer" doesn't reach tool-argument content.
grounding.py:74 — [minor] tool_argument_rule keeps its own literal list instead of reusing VALUE_KINDS.
execution.py:1485 — [minor] "remaining work" phrasing risks being read as an implicit next-step instruction.
test_context.py:1252 — [minor] docstring miscites COMPACT_SUMMARY_MIN_TOKENS as basis for the 330-word input-prompt cap.
grounding.py:96 — [minor] relative clause reads as binding only to the last VALUE_KINDS list item.
grounding.py:99 — [minor] "a sample" trigger phrase is ambiguous between mockup content and a real example.
test_grounding.py:226 — [minor] ordering assertion checks two hardcoded substrings with no reorderable code path behind them.
grounding.py:17 / test_grounding.py:262 — [minor] denylist checks exact phrase "instructed default"; a synonymous rewrite evades it.
test_grounding.py:577 — [minor] coverage regression: no current test positively asserts the page-size/result-limit clause on the can_call_tools=True path.
test_dag.py:423 — [minor] assertion checks a phrase that never existed in dag.py's prompt; vacuously true.
test_context.py:1100 — [minor] _build_llm_compact_prompt_texts duplicates existing setup at ~1046-1055 instead of a shared helper.
Blocking: yes — recommended event: REQUEST_CHANGES
execution.py:1245,1412 — [new] compaction trailer/dropped-tool notice still only names "figure", missing the name/org/date/status types the incident actually fabricated
| "hold all of this, keep, in this order: first state what is " | ||
| "missing and not listed here, with counts; artifact handles; " | ||
| "the identifiers and names the request points at; statuses " | ||
| "and dates; then the rest. Separate completed work from " |
There was a problem hiding this comment.
[Major, Blocking — pointer] This hunk correctly broadens the compaction summary to preserve names/orgs/statuses/dates, not just numeric figures. But two other prompt strings in this same file were NOT touched and still only mention "figure": the non-LLM compaction summary trailer at execution.py:1245 and _dropped_tool_results_notice at execution.py:1412 (both unchanged since base commit f69c4ec, confirmed outside this PR's diff so not directly commentable here). Both tell the model to treat a missing/unsupported value as unavailable using only "figure"/"value, figure, statistic, table row, quotation, or identifier" — missing the name/org/date/status types this PR fixes everywhere else it touches (grounding.py, auto.py, dag.py, and this hunk). Trigger: any compaction event in a long-running session. Fix: broaden both sentences to the same value-kind scope used here.
| "to the user reaches you as an argument too; it is wording you " | ||
| "compose, so this argument standard does not reach it, while the " | ||
| "sourcing rule above still governs every fact inside it. This clause " | ||
| "does not reach a default or inferred parameter value such as a page " |
There was a problem hiding this comment.
[Minor] tool_argument_rule (starting at line 62) does not use the shared VALUE_KINDS constant introduced elsewhere in this same file/PR; it keeps its own independently-worded list of fact-carrying value types. Minor duplication/drift risk within the same file, same PR, same author who introduced VALUE_KINDS specifically to solve this class of problem.
| "restricts every fact asserted inside that wording. A fact value " | ||
| "written literally inside such composed code or text is still " | ||
| "subject to the sourcing rule above: the text you compose is yours; " | ||
| f"{VALUE_KINDS} that you place inside it is not. The only case in " |
There was a problem hiding this comment.
[Minor] The template/sample exception is scoped to "in the answer" (stated twice, lines 98-105). When a user explicitly asks for template content that must go into a tool argument rather than the chat answer (e.g. "write a sample invoice and save it to a file"), the exception's literal wording doesn't reach that content, and no other clause broadens it. This is a newly-sharpened gap — the base wording wasn't scoped to "the answer". No test covers this scenario. Suggest extending the exception to cover composed tool-argument content too.
| "missing: never call a dataset complete, fully retrieved, or " | ||
| "fully processed unless the history shows every item was " | ||
| "returned and every one is still described here; say which " | ||
| "parts survive as prose only. Write no instruction to the " |
There was a problem hiding this comment.
[Minor] "Separate completed work from remaining work" (line 1480) sits immediately next to "Write no instruction to the next call about tool use or whether to answer" (this line). The next sentence clarifies "remaining work" means factual gap-reporting, not directives, but a weaker summarizer model could still blur "remaining work" into an implicit next-step instruction. No test can catch this since it requires observing live LLM behavior.
| """ | ||
| system, _ = _build_llm_compact_prompt_texts() | ||
|
|
||
| assert len(system.split()) <= 330 |
There was a problem hiding this comment.
[Minor] test_compact_prompt_stays_within_the_smallest_budget's docstring cites COMPACT_SUMMARY_MIN_TOKENS (256 tokens ≈ 190 words) as the rationale for the 330-word assertion threshold, but that constant only bounds the LLM's output summary token budget (_llm_compact_max_tokens), never the input system prompt's own length — a category-error comparison. The current prompt is 327 words, 3 under the 330 cap, with no runtime enforcement anywhere else. Suggest fixing the docstring to state the 330-word cap is an arbitrary ceiling tied to the current prompt's length, not derived from COMPACT_SUMMARY_MIN_TOKENS.
| """On the exception path, the disclosure must precede the content.""" | ||
| for rule in (grounding_rule(), grounding_rule(can_call_tools=False)): | ||
| assert "before any of that content appears in the answer, state" in rule | ||
| assert rule.index("before any of that content appears") < rule.index( |
There was a problem hiding this comment.
[Minor] rule.index("before any of that content appears") < rule.index("keep such content to what the request asked for") checks two hardcoded substrings, in fixed order, inside a single sentence in the source string — there's no code path that could reorder them, so this doesn't test enforceable ordering semantics, only that nobody swapped the words.
| exception. | ||
| """ | ||
| doc = grounding.__doc__ or "" | ||
| assert "instructed default" not in doc |
There was a problem hiding this comment.
[Minor] This test denylists the exact phrase "instructed default", but grounding.py's module docstring (line 17) already reads "the instructed response" (different wording, same meaning) — a synonymous rewrite would trivially evade this guard since it checks specific words, not the underlying claim.
| assert ( | ||
| "does not reach a default or inferred parameter value such as a page " | ||
| "size or result limit" in rule | ||
| assert "The answer you write to the user reaches you as an argument too" in rule |
There was a problem hiding this comment.
[Minor] Coverage regression: test_grounding_rule_scopes_the_answer_as_argument_exemption replaces the previously-existing positive assertion for the can_call_tools=True path (that tool_argument_rule states it "does not reach a default or inferred parameter value such as a page size or result limit") with only these new assertions plus a negative check for can_call_tools=False. No current test positively covers that page-size/result-limit clause's text on the True-path prompt anymore — if it were deleted from tool_argument_rule entirely, all current tests would still pass.
| # the rule no longer uses. | ||
| assert "name that missing data in reason" in system_prompt | ||
| assert "illustrative" not in system_prompt | ||
| assert "report that value as unavailable" not in system_prompt |
There was a problem hiding this comment.
[Minor] assert "report that value as unavailable" not in system_prompt — this exact phrase never existed in dag.py's prompt in either the base commit or the current PR head, so the assertion is vacuously true and cannot catch any real regression.
| assert ctx.messages[1].content == "current request" | ||
|
|
||
|
|
||
| def _build_llm_compact_prompt_texts() -> tuple[str, str]: |
There was a problem hiding this comment.
[Minor] _build_llm_compact_prompt_texts near-verbatim duplicates the context-building setup (same threshold, same tool_calls shape, same tool_result payload) already present at ~1046-1055 in the same file, instead of factoring into a shared helper/fixture.
3 participants
Summary
This PR rewrites three prompt texts used by the agent: the grounding rule shared by every pattern, the compaction summary prompt, and the two sibling sentences that follow the grounding rule call in the auto and dag patterns. No execution logic changes; every diff is inside a string literal or a docstring. The goal is to close two gaps that let a model present unsourced or fabricated content to a user as if it were real: a labelling loophole in the grounding rule, and a next-action instruction in the compaction summary prompt that neither summarizer nor caller could actually honor.
Behavior changes
Product decision disclosed
#1237 (proposal A of #1235) introduced a labelling clause for unsourced content and recorded "block outright vs allow with a label" as an open product decision. This PR resolves it in favor of blocking by default, with a single exception for a current user request that explicitly asks for a template or sample. Maintainers should weigh in on whether that is the right default.
Not in this PR
Verification
VALUE_KINDSreference for a hand-written five-item list missing one entry ->test_grounding_rule_keeps_literal_facts_inside_composed_text_sourcedtest_compact_prompt_ranks_what_to_keep_when_the_budget_is_shorttest_compact_prompt_excludes_credentials_even_when_also_an_identifiertest_dag_completion_assessment_prompt_includes_grounding_ruletest_grounding_module_docstring_states_the_default_as_a_prohibitiontest_compact_prompt_stays_within_the_smallest_budgettest_grounding_rule_scopes_the_answer_as_argument_exemptiontest_compact_prompt_excludes_credentials_and_unrelated_personal_dataPart of #2146