fix(providers): synthetic tool-result text must say the outcome is unknown, not that it failed - #174
Closed
jasonkneen wants to merge 1 commit into
Closed
jasonkneen wants to merge 1 commit into
jasonkneen wants to merge 1 commit into
Conversation
Invariant: recovery closes an interrupted tool call with a structured error that says the side effect may or may not have happened; it never re-executes. transform-messages.ts synthesized "No result provided" (isError true) for orphaned tool calls left after a crash/kill mid-tool or an abort mid-batch (agent-loop.ts sequential/parallel executors break on signal.aborted). That wording reads as "nothing happened" and gave the model no reason to verify state before blindly retrying non-idempotent commands like bash. Replaced the literal string with synthesizeInterruptedToolResultText(), which names the interrupted tool, states the call was interrupted (not failed), says its side effects may or may not have happened, and tells the model to verify current state before retrying. isError and message shape are unchanged. agent-loop.ts's abort break semantics are untouched -- only the synthesized wording changed.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation and tests are covered with no unresolved blocking issues.
Review effort: Lite
Findings: None
What changed in this PR
Updates synthetic tool-result messages to clarify that interrupted calls may have unknown outcomes.
Changes:
- Added reusable interrupted-result wording with verification guidance.
- Updated tests for wording, error status, and de-duplication.
| File | Summary |
|---|---|
packages/providers/src/api/transform-messages.ts |
Generates explicit unknown-outcome messages for interrupted tool calls. |
packages/providers/test/transform-messages-copilot-openai-to-anthropic.test.ts |
Verifies recovery wording and duplicate prevention. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Problem
packages/providers/src/api/transform-messages.ts(~line 171, ininsertSyntheticToolResults) synthesizes a placeholdertoolResultfor any tool call left without a real result — this happens after a crash/kill mid-tool, or when the agent loop aborts mid-batch (packages/agent-core/src/agent-loop.tssequential/parallel executorsbreakonsignal?.aborted, lines ~554 and ~573, leaving later tool calls with no result in the in-memory transcript). The synthesized text was the literal string"No result provided"withisError: true.That wording reads as "the tool call didn't execute" / "this failed cleanly." It does not tell the model that the side effect may already have happened. On resume, a model reading this synthetic result for e.g. a
bashcall has no signal to re-check state before retrying — it just sees an error and retries the (possibly non-idempotent) command blindly, which is exactly the invariant this recovery path is supposed to prevent: recovery must close an interrupted call with a structured error that says the outcome is unknown, never re-execute or imply nothing happened.Fix
Replaced the literal string with a new exported helper,
synthesizeInterruptedToolResultText(toolName), inpackages/providers/src/api/transform-messages.ts:isError: trueand the existing message structure (role/toolCallId/toolName/content/timestamp) unchanged.packages/agent-core/src/agent-loop.ts— its abortbreaks were already correct (they stop issuing new tool calls and leave the gap for this synthesis step to fill); execution semantics are untouched, only the synthesized wording changed.packages/providers/src/index.ts, so this stays package-internal surface, not new public API.Grepped the whole repo (
grep -rn "No result provided" . --exclude-dir=node_modules --exclude-dir=dist --exclude-dir=.git) — the only two hits were the implementation site and the one test file that pinned the old string; both are covered by this change. No other synthesis site or snapshot/doc pinned the old text.Test
packages/providers/test/transform-messages-copilot-openai-to-anthropic.test.ts:"No result provided"string (assertion lines 158 and 188) to assert againstsynthesizeInterruptedToolResultText(toolName)instead. Before the fix, these two tests failed against the new implementation output (actual text was the new interrupted-wording string, expected was still"No result provided") — that failure is the evidence the implementation changed the wording as intended. (Note: this is "old test pinned the buggy string, updated it to the new correct string, and confirmed it fails against the new impl if reverted" rather than "wrote a new failing test first" — being explicit about that since it's the more relevant evidence here.)role: "toolResult",isError: true, the text contains the tool name ("bash"), matches/interrupted/i, matches/may or may not have happened/i, and matches/verify/i. This test would fail against the old"No result provided"text (none of those patterns match it).toolResultproduces exactly onetoolResultmessage (no synthetic duplicate) — coversinsertSyntheticToolResults'sexistingToolResultIdsde-dup path.All 6 tests in the file pass. Full
packages/providerssuite: 424 passed, 6 skipped, 0 failed. Fullpackages/agent-coresuite: 465 passed, 1 skipped, 0 failed.packages/coding-agentfull suite (--maxWorkers=2): 9 failed / 3465 passed / 51 skipped — matches the documented pre-existing baseline (context-projection.test.ts x3, resource-loader.test.ts x3, step-tool-profile.test.ts x1, 2791-fswatch-error-crash.test.ts x1, subagent-invocation.test.ts x1); none touch transform-messages or agent-loop and none are new.Risk / behaviour change
Purely a user-facing string change inside a synthetic tool-result message;
isError, message shape, and control flow are unchanged. Slightly longer text is sent to the model on resume after a crash/abort — no other behavioural change. No changes to provider registry/catalog/defaults, no provider add/remove, no web search touched.https://claude.ai/code/session_01GUdnnHEaDThHUATSwXBpV9