Tool capability is additive to grounding, not a replacement for it - #181
Merged
Conversation
Six witnesses against WorkflowEngine.run, three of them failing. A knowledge context the user explicitly selected does not reach the agent path at all: llm.generic validates context_id, retrieves for it and injects the chunks, while the agent planner never received context_id and offered file_search only when the conversation held a searchable attachment. The model here is tool-capable and calls no tool on purpose. A context the user selected must not depend on the model guessing that it should go looking for it, so "the model could have searched" is not accepted as equivalent to being grounded. Parameterized over two triggers, web and a published MCP server, because the loss belongs to tool routing rather than to web: an operator with web off and a server published loses the same context. The shipped web settings (web_tools_enabled=true, web_search_provider=none) only make it universal, and that premise is asserted against config.py rather than assumed. One witness exists to refuse a fix rather than to require one. Narrowing the selector to `provider != "none"` also turns the reds green, and pays for it by making web_fetch unreachable: it needs no provider and is offered whenever web tools are on. Measured on this code, the tool list for that configuration is exactly ['web_fetch'], so that patch would restore grounding by removing the turn's only capability.
The fix is at agent-context construction, not at routing. context_id now reaches the planner, is authorized by _validate_context_scope — the same check llm.generic uses, so the two paths cannot answer "may this user read this" differently — and its chunks go into the system block before the first model call, behind the digest and the recall and in the same "Context:" shape the plain path injects. The same snippets ride in the plan so the worker returns them among its own. That is what makes the turn report the grounding it actually used rather than only what a tool call went and fetched, on the batch and streaming paths alike; both assemble the agent prompt through the same function and both had the same hole. Retrieval sits beside prompt assembly rather than inside it because those snippets have two destinations, the prompt and the reported context_snippets, and retrieving twice to tell two callers the same thing is how the two answers begin to differ. file_search is offered whenever a valid ordinary context_id exists. _run_file_search already resolved an explicit context_id, so the tool worked the whole time and was simply never offered unless the conversation happened to hold a searchable attachment. That is the additive half: search beyond the initial top-k, never the only route to the context. _build_agent_context keeps its four-tuple return, so the sixteen existing call sites are untouched. Three mutations, each killed by a different witness: dropping the propagation kills both selected-context cases and the file_search offer; dropping the grounding while leaving file_search offered kills only the selected-context cases; suppressing web_fetch when the provider is none kills the capability-preservation witness. Measured on the running product against a live model and the shipped configuration: the same probe went from three misses to three hits, with the no-context control missing in both, and the browser flow from 13/14 to 14/14.
Two gaps review found in the previous commit, both narrow. The streaming half was implemented and not witnessed. The red was six cases against WorkflowEngine.run; the green separately changed _stream_agent_files_node, which calls _explicit_context_grounding itself, passes its own arguments into _build_agent_context, and seeds its own worker plan. Sharing the assembly function does not make any of that shared, so both paths being fixed rested on reading the code rather than on a failing test. One run_streaming case now covers it, and two mutations confirm the separation: removing the streaming retrieval, or the two arguments it passes down, kills that case alone and leaves every batch case green. Grounding was also exempt from the prompt budget. _apply_prompt_budget drops context from its low-priority end before it drops any history, then refuses the turn if the prompt still does not fit; appending the chunks onto system_content and passing [] as the context made them part of an indivisible system block, so the pruner reached past them and evicted conversation turns instead. Tool routing may add capabilities, but it does not promote retrieved knowledge above the ordinary budget rules. Grounding is now passed as context and appended only after budgeting. _build_agent_context therefore returns the grounding that survived rather than the grounding that was retrieved, and the plan and the streamed result carry that surviving set. context_snippets is a claim about what the model was shown, so reporting the pre-pruning retrieval would name chunks that never reached it. The four-tuple was worth keeping while it cost sixteen unpackings; it was not worth recomputing the surviving set or parsing it back out of the system prompt. Seven mutations, all applied, all killed, and each of the four new ones falls to a different witness. Two earlier attempts did not apply at all — a stale anchor and a cooked string that turned a literal backslash-n into a newline — so the driver now reports an unmatched anchor as loudly as a survivor. A mutation that does not apply measures nothing. make test-xdist: 2869 passed, 27 skipped. Live against the shipped configuration and a real model, unchanged: three hits, control still missing.
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.
Found by running the product, not by reading it. A file was uploaded into a knowledge context through the browser, the context was selected, and the question was asked. The model answered that it had not been given any notes.
What was wrong
The retriever was never at fault. Called directly,
rag.retrieve()returned the chunk for all four phrasings tried, and the lexical channel alone returned it. The loss was one layer up._turn_needs_tools()decides whether a turn takes the tool-agent workflow or the plain one. It asks about capability — an attachment, web tools, a published MCP server — and it answered correctly. But the two workflows differ in more than capability:llm.genericvalidatescontext_id, retrieves for it, and injects the chunks.context_idat all, and offeredfile_searchonly when the conversation held a searchable attachment.So a knowledge context the user explicitly selected entered neither the first prompt nor the tool list. Choosing the capable path silently removed the grounding, leaving the model to infer that a context it could not see needed searching.
Not a web bug, though the shipped settings make it universal
web_tools_enabledshipsTrueandweb_search_providerships"none", so_turn_needs_tools()is true on every fresh installation and every turn takes the agent path. That is why this reproduced immediately. It is not the cause: an attachment or a published MCP server loses the same context with web off. The witness is parameterized over two triggers for that reason — a fix that repaired only the web case would leave an operator with an MCP server still losing every selected context.The tempting fix, and what it costs
Narrowing the selector to
web_tools_enabled and provider != "none"also turns the reds green, which is what makes it dangerous."none"disables web search;web_fetchneeds no provider and is offered whenever web tools are on. Measured on the unfixed code, the tool list for that configuration is exactly['web_fetch']— so that patch would restore grounding by removing the turn's only capability, and nothing would have said so.One witness exists to refuse it rather than to require anything: with web enabled and no provider,
web_fetchmust be offered andweb_searchmust not.The fix
At agent-context construction, not at routing.
context_idreaches the planner and is authorized by_validate_context_scope— the same checkllm.genericuses, so the two paths cannot answer "may this user read this" differently.file_searchis offered whenever a valid ordinarycontext_idexists. Not a new capability:_run_file_searchalready resolved an explicitcontext_id, so the tool worked the whole time and was simply never offered. Iterative search is the additive half, never the only route to the context — which is why the model in the witness makes no tool call.Retrieval sits beside prompt assembly rather than inside it, because those snippets have two destinations — the prompt and the reported
context_snippets— and retrieving twice to tell two callers the same thing is how the two answers begin to differ.Grounding obeys the prompt budget
_apply_prompt_budgetdrops context from its low-priority end before it drops any history, then refuses the turn if the prompt still does not fit. Appending the chunks ontosystem_contentand passing[]as context would put them inside an indivisible system block, so the pruner would reach past them:Routing may add capability; it does not rearrange priority. Grounding is passed as context and appended only after budgeting.
_build_agent_contexttherefore returns the grounding that survived rather than the grounding that was retrieved, and the plan and streamed result carry that set.context_snippetsis a claim about what the model was shown, so reporting the pre-pruning retrieval would name chunks it never saw.Both paths, each witnessed
_stream_agent_files_nodecalls_explicit_context_groundingitself, passes its own arguments into_build_agent_context, and seeds its own worker plan. Sharing the assembly function does not make any of that shared, so the streaming half has a witness that drivesrun_streamingrather than resting on inference from the batch cases.Verification
Nine witnesses, three failing before the fix — including the MCP trigger, which failed identically to the web one.
Seven mutations, all applied, all killed:
web_fetchgated on providerTwo earlier attempts did not apply at all — a stale anchor, and a cooked string that turned a literal backslash-n into a newline. A mutation that does not apply measures nothing, so the driver now reports an unmatched anchor as loudly as a survivor.
Lanes:
make test-xdist— 2869 passed, 27 skipped, 0 failed. CI's lint selection (ruff check liminallm/ --select=E,F,W,I --ignore=E501) clean.On the running product, against the shipped configuration and a live model: the same probe went from three misses to three hits, with the no-context control missing in both — which is what makes the hits mean anything. The browser flow went from 13/14 to 14/14.
Not in this tranche
_turn_needs_tools, the web defaults, and RAG retrieval itself are untouched.summarize_preferencescallslist_artifactswithoutowner_user_id, so the visibility clause collapses tovisibility='global'and a user's own private adapter never appears inGET /v1/preferences/insights. Measured, no cross-user leak — the fail-safe direction. It is a separate owner-scoping call-site defect and is deliberately left out of this tranche.🤖 Generated with Claude Code
https://claude.ai/code/session_01DQtPsg9YSUXaStGXyUjozA
Generated by Claude Code
Note
Overview
Fixes a regression where choosing a knowledge context did nothing on the tool-agent path. When
_turn_needs_tools()sent a turn toagent.files_v1(common on fresh installs because web tools default on with no search provider), the planner never receivedcontext_id, so selected corpora were neither in the first prompt nor eligible forfile_search—whilellm.genericstill retrieved and injected them.Agent turns now ground the same way as plain chat.
_explicit_context_groundingvalidates scope and retrieves chunks before assembly; batch and streaming agent paths pass that into_build_agent_context, seed the worker plan with budget-survivingcontext_snippets, and the tool worker reports those snippets instead of only tool-fetched text.file_searchis offered when the turn names an owned context (additive search), not as the only way to see the selection.Grounding follows §20.3 prompt budgeting: chunks go through
_apply_prompt_budgetas context (appended after pruning), and returned/reported snippets are the subset actually shown—not the full retrieval.SPEC §21 and
docs/ISSUES.mddocument the invariant;tests/test_agent_grounding_is_additive.pycovers web/MCP triggers, streaming, budget/reporting, and refusing a “disable web_fetch” workaround.Reviewed by Cursor Bugbot for commit 6fe34ca. Bugbot is set up for automated code reviews on this repo. Configure here.