fix(runtime): keep providerExecuted on non-streamed tool results - #3446
Merged
Conversation
…sults Anthropic's buildAnthropicGenerateResult and OpenAI's Responses normalizer both return doGenerate content parts carrying providerExecuted: true for server-side tools. The streamed result builder propagates that flag; buildDirectGenerateResult does not. This test drives the non-streamed path with that provider shape and fails, proving the flag is lost before it reaches the agent loop.
buildDirectGenerateResult dropped providerExecuted while the streamed builder propagated it, so the agent loop saw undefined for every provider-executed tool result on the doGenerate path. That cost more than telemetry: persistGeneratedToolResult passes the flag to createToolResultMessage, so a genuinely provider-executed result was persisted into conversation history as not provider-executed. Propagate the flag with the same conditional spread the streamed builder uses, so it stays omitted rather than set to false.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe runtime bridge now accepts and preserves ChangesProvider execution metadata
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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.
A provider-executed tool result loses its
providerExecutedmarker on the non-streamed generate path, and is then persisted into conversation history as an ordinary client-side tool result.The bug
src/runtime/runtime-bridge.tshas two result builders that should agree:buildGenerateResultFromStream) propagatesproviderExecutedin both itstool-resultandtool-errorcases.buildDirectGenerateResult) never set it.The type guard
isDirectToolResultPartnarrowed to a shape that omittedproviderExecutedentirely, so the field was invisible at the type level and the omission never surfaced as a compile error.It is on the default path, not an edge case
Providers really do send it.
extensions/ext-llm-anthropic/src/anthropic-provider.ts:275-282setsproviderExecuted: trueunconditionally on everyweb_search/web_fetch/ MCP tool-result block, typed as the literaltrue.extensions/ext-llm-openai/src/openai-provider.ts:837-844does the same forweb_search_call.And the non-streamed path is the common one:
shouldGenerateViaStreamis justmodel._generateViaStream === true, and that flag is set in exactly one place —src/provider/veryfront-cloud/provider.ts:17. So veryfront-cloud models divert to the stream builder (which was correct), while direct-key Anthropic and OpenAI models takedoGenerateand hit the buggy builder.Consequences
persistGeneratedToolResultpassesgeneratedToolResult.providerExecuted === trueintocreateToolResultMessage, whose parameter defaults tofalseand drives a conditional spread (tool-result-continuation.ts:17,28). A genuinely provider-executed result was therefore written into conversation history with the marker absent entirely. Downstream code keys on that marker — e.g.src/chat/conversation.ts:376, and the completeness rule exercised byfinalized-message.test.ts:62("fails local web_fetch input-available tools without providerExecuted").traceProviderExecutedToolfires only underproviderExecuted === true, so provider-executed tools were never traced in generate mode.Link 1 is traced through the file:line hops above rather than proven end-to-end; link 2 is covered by the test below.
The fix
Two lines in
runtime-bridge.ts: widenisDirectToolResultPart's narrowed type withproviderExecuted?: boolean, and add the same conditional spread the streamed builder uses, so the field is omitted rather than set tofalse.Proof, in commit order
The two commits are deliberately ordered test-then-fix so the red/green is visible in history rather than asserted here.
Against the unfixed code, the new test fails on exactly the missing field:
All 26 pre-existing steps in that file passed at the test-only commit, so the failure is attributable to the bug and nothing else. After the fix: 27 passed, 0 failed.
Why this survived until now
The neighbouring
uses the direct generate path for provider-native toolstest feeds model content carrying noproviderExecutedfield at all — an unrealistic fixture, since the real Anthropic provider always sets it on server-tool blocks. Nothing could observe the field being dropped because nothing ever supplied it. That test still passes unchanged (the conditional spread omits the field when absent) and was not modified.Evidence
deno task test:unit: 3801 passed / 27916 steps / 0 failed / 1 ignored, exit 0. No pre-existing test broke; none needed updating.deno task verify:quickexit 0;deno check src/index.tsclean; fmt and lint clean.docs/api-reference/veryfront/embedding.mdis a one-line change: it pins a source line intoruntime-bridge.ts, and the fix shifted#L973→#L975. This was not assumed pre-existing — the base versions were checked out anddocs:api-reference:checkreported "current (43 files)", confirming the staleness came from this change. Regenerated withdeno task docson Deno 2.7.7 (the CI-pinned version), never hand-edited.Known adjacent issue, deliberately not fixed here
tool-callparts dropproviderExecutedon the same path (runtime-bridge.ts:568-573), while both providers set it on the matching tool-call part and the streamed path carries it. Same asymmetry, adjacent lines. WhetherRuntimeGenerateTextResult["toolCalls"]should carry it depends on whether any consumer reads it, which hasn't been investigated — so it is flagged rather than swept in.Summary by CodeRabbit
Bug Fixes
Documentation
similarityfunction in the API reference.