fix(harness): keep the runtime error message on failed provider-executed tool results - #18902
fix(harness): keep the runtime error message on failed provider-executed tool results#18902gdaybrice wants to merge 3 commits into
Conversation
…ted tool results A harness `tool-result` event with `isError` was translated into a plain AI SDK `tool-result` part whose output happened to be the error. Because that part is not marked as a provider-executed error, `toUIMessageChunk` never takes the provider-executed branch, and the runtime's own failure text is replaced by the generic string returned from `onError` — the real reason (a failed shell command, an MCP timeout) never reaches the consumer. Translate it into a `tool-error` part with `providerExecuted: true` instead, carrying `providerMetadata` and `dynamic` through unchanged so the error keeps matching the UI part opened by its tool call. `tool-error` parts are now also appended to the current step content, so a failed provider-executed tool stays visible in `step.content` the way its successful counterpart does (this also covers the host-tool `tool-error` parts the agent loop already emitted).
… call Review follow-up to the previous commit. `providerExecuted: true` was hardcoded on the new `tool-error` branch, but host tools travel the same wire events: their `tool-call` is emitted with `providerExecuted: false` and their failures are submitted back to the runtime and echoed as a `tool-result` with `isError`. Hardcoding the flag labelled those host failures provider-executed, which bypasses the `onError` redaction consumers rely on. `translateStreamPart` now takes an `isProviderExecuted` lookup, resolved in `run-prompt.ts` from the `rawToolCallsByToolCallId` map it already maintains, so only genuine provider-executed failures become `tool-error` parts. Also collapse the five identical arms of `appendToCurrentStepContent` into one fall-through group, matching how `stream-text.ts` accumulates the same set of part types, and trim a redundant unit test.
An `execute` that only throws infers `Promise<never>`, which propagates into the `tool()` generic and makes `inputSchema` fail to match `FlexibleSchema<never>`. Annotate the return type, matching the existing throwing-tool test in this file.
Bugfix reviewOutcome: changes-required Fixes issueStatus: partially-addresses Provider-executed failures explicitly marked true are correctly translated to tool-error, but legitimate host calls with providerExecuted omitted are incorrectly treated as provider-executed. Concerns:
Side effectsRisk: medium Failed host tools whose calls omit providerExecuted change from tool-result to provider-executed tool-error, altering downstream UI and error handling. Concerns:
PerformanceRisk: none The change adds a constant-time lookup in an existing per-turn map and does not materially increase memory use. Backwards compatibilityRisk: none The change does not modify or migrate existing persisted data formats; it only changes newly emitted stream parts. Breaking changesRisk: medium No public types or exports change, but an already accepted input—host tool calls with providerExecuted omitted—now produces a different stream and UI output shape. Concerns:
ArchitectureRisk: low The implementation remains localized to harness stream translation and result accumulation, with no cross-package source imports or dependency-boundary violations. Change scopeStatus: minimal The production changes, regression tests, refactoring of identical switch arms, and patch changeset are all directly related to the claimed fix. SecurityRisk: low No new execution or parsing primitive is introduced, but misclassified host errors bypass the non-provider error redaction path. Concerns:
TestingStatus: needs-more Tests cover explicit true and false values but omit the contractually equivalent host case where providerExecuted is undefined. Concerns:
VerificationReviewed every changed hunk and the relevant provider, harness, UI conversion, adapter, and step-accumulation code. Harness Node and Edge suites, package type checking, package build, and diff validation passed. A focused translation probe confirmed that a known host call with providerExecuted omitted is emitted as providerExecuted tool-error. |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@gdaybrice Thank you for the PR!
This looks reasonable, but it needs some iteration to account for the variety of tool kinds AI SDK supports.
| */ | ||
| const translateOptions = { | ||
| isProviderExecuted: (toolCallId: string): boolean => | ||
| rawToolCallsByToolCallId.get(toolCallId)?.providerExecuted ?? true, |
There was a problem hiding this comment.
See the agent review in #18902 (comment), the true here isn't a reasonable fallback. Certain tools, including client-side tools, may omit the providerExecuted property altogether.
2 participants
Background
When a provider-executed tool fails, the harness runtime already sends the real reason across the wire — a failed shell command's stderr, an MCP server's timeout — as a
tool-resultevent withisError: true.translateStreamPartdropped that flag, projecting the event into a plain AI SDKtool-resultpart whose output happened to be the error text. Consumers saw a tool that succeeded and returned an error-shaped payload — the UI message stream emittedtool-output-available— so nothing downstream could distinguish a failed provider-executed tool from a successful one without inspecting its output.Summary
tool-resultcarryingisErrorinto atool-errorpart, so the failure is typed as one and reaches consumers astool-output-error. Marking itproviderExecuted: trueis load-bearing:toUIMessageChunkredacts non-provider-executed errors throughonError, which would replace the runtime's own message with a generic string.providerMetadataanddynamicpass through unchanged, keeping the error on the same UI part as its tool call.providerExecutedfrom the originatingtool-callrather than hardcoding it. Host tools travel the sametool-resultevents — their calls are emitted withproviderExecuted: falseand their failures are submitted back to the runtime and echoed withisError— so hardcoding the flag would have marked host failures provider-executed and bypassed theonErrorredaction consumers rely on. Host tool failures keep their existingtool-resultprojection.tool-errorparts to the current step content, so a failed provider-executed tool stays visible instep.contentthe way its successful counterpart does. This also covers the host-tooltool-errorparts the agent loop already emitted and that were being dropped.appendToCurrentStepContentinto one fall-through group, matching howstream-text.tsaccumulates the same set of part types.End-to-End Verification
Ran a harness agent against a local sandbox and forced a provider-executed tool to fail. The client now receives that failure typed as a tool error carrying the runtime's own message. Previously the same failure arrived as a completed tool result whose output happened to hold the error text, with nothing marking the tool as having failed; and typing it as an error without
providerExecutedset replaces the message with a generic placeholder, which is what the flag prevents.The run exercised this change together with other harness changes in the same batch rather than in isolation.
Checklist
pnpm changesetin the project root)Future Work
tool-resultwhose output is an error payload, rather than as atool-error. This PR deliberately leaves that path untouched; aligning it with core would be a separate, consumer-visible change.appendToCurrentStepContentkeeps preliminary tool results in step content, wherestream-text.tsexcludes them. Pre-existing drift between the two accumulators, not addressed here.