From f99422eeb7c37e3fdad93738f008546c572642c9 Mon Sep 17 00:00:00 2001 From: Can Date: Mon, 14 Sep 2026 09:39:50 +0300 Subject: [PATCH 1/4] fix(task): clear nativeArgs when tool-call finalize fails (#1221) finalizeStreamingToolCall() returns null when a streamed native tool call's arguments are truncated (e.g. the model hits max_tokens mid write_to_file content). Task.ts reuses the same tool-use object the streaming phase had been mutating in place, which still carried nativeArgs built from the incomplete partial-JSON parse, and only set partial = false. presentAssistantMessage.ts already guards against exactly this case (isKnownTool && !block.nativeArgs && !customTool -> structured tool_result instead of execution), but the guard never fired because nativeArgs was never actually cleared. Truncated arguments (e.g. a cut-off content string) could therefore be executed instead of rejected. Clear nativeArgs alongside partial = false at the finalize-null site so the existing guard does what its own comment already said it did. params is left untouched - NativeToolCallParser always initializes it to {} for native tool calls and never puts real data there. Adds truncated-native-tool-args.spec.ts, mirroring the exact Task.ts logic in a small local function per the convention already established in duplicate-tool-use-ids.spec.ts. --- src/core/task/Task.ts | 12 +- .../truncated-native-tool-args.spec.ts | 113 ++++++++++++++++++ 2 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 src/core/task/__tests__/truncated-native-tool-args.spec.ts diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 55798437c3..8ac47ef448 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -3809,12 +3809,18 @@ export class Task extends EventEmitter implements TaskLike { /* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */ this.presentAssistantMessageSafe() } else if (toolUseIndex !== undefined) { - // finalizeStreamingToolCall returned null (malformed JSON or missing args) - // We still need to mark the tool as non-partial so it gets executed - // The tool's validation will catch any missing required parameters + // finalizeStreamingToolCall returned null (malformed JSON or missing args). + // existingToolUse is the same object the streaming phase was mutating in + // place, so it still carries nativeArgs built from the incomplete partial + // parse (e.g. a truncated write_to_file `content` string) - that value was + // only ever meant for live progress display, never for execution. Mark the + // tool as non-partial so it's presented as complete, and clear nativeArgs so + // presentAssistantMessage's `!block.nativeArgs` guard actually short-circuits + // it with a structured tool_result instead of executing the truncated value. const existingToolUse = this.assistantMessageContent[toolUseIndex] if (existingToolUse && existingToolUse.type === "tool_use") { existingToolUse.partial = false + existingToolUse.nativeArgs = undefined // Ensure it has the ID for native protocol ;(existingToolUse as any).id = event.id } diff --git a/src/core/task/__tests__/truncated-native-tool-args.spec.ts b/src/core/task/__tests__/truncated-native-tool-args.spec.ts new file mode 100644 index 0000000000..4cc6f313e6 --- /dev/null +++ b/src/core/task/__tests__/truncated-native-tool-args.spec.ts @@ -0,0 +1,113 @@ +/** + * Regression test for issue #1221: truncated tool-call arguments can be silently + * written to disk. + * + * When a streamed native tool call's arguments are cut off mid-value (e.g. the + * model hits max_tokens while still writing write_to_file's `content` string), + * NativeToolCallParser.finalizeStreamingToolCall() returns null. Task.ts + * (~line 3748) reuses the same tool-use object the streaming phase was mutating + * in place and only sets `partial = false` - before the fix it left `nativeArgs` + * (built from the incomplete partial parse) untouched. + * + * presentAssistantMessage.ts (~line 443) is supposed to short-circuit exactly + * this case with a structured tool_result instead of executing the tool - but + * its guard is `isKnownTool && !block.nativeArgs && !customTool`. With + * nativeArgs still populated, the guard never fired and the truncated content + * would be passed straight to write_to_file's execution path. + * + * The fix clears `existingToolUse.nativeArgs` alongside `partial = false` at + * the finalize-null site, so the pre-existing guard actually does what its own + * comment already claimed. + */ + +import { isValidToolName } from "../../tools/validateToolUse" +import type { ToolUse, WriteToFileToolUse } from "../../../shared/tools" + +describe("Truncated native tool-call args on finalize failure (issue #1221)", () => { + /** + * Simulates the finalize-null branch from Task.ts (~line 3748) as it exists + * after the fix: on finalizeStreamingToolCall() returning null, mark the + * tool non-partial and clear nativeArgs. + */ + function finalizeNullBranch(existingToolUse: ToolUse): ToolUse { + existingToolUse.partial = false + existingToolUse.nativeArgs = undefined + return existingToolUse + } + + /** + * Simulates the finalize-null branch as it existed *before* the fix, for a + * companion test proving the old behavior really was the bug (not just an + * assumption). + */ + function finalizeNullBranchBeforeFix(existingToolUse: ToolUse): ToolUse { + existingToolUse.partial = false + return existingToolUse + } + + /** + * Simulates the short-circuit guard from presentAssistantMessage.ts (~line + * 443): `isKnownTool && !block.nativeArgs && !customTool`. Returns true when + * the tool call would be blocked (a structured tool_result emitted, no + * execution), false when it would proceed to execution. + */ + function wouldBeBlocked(block: ToolUse, customTool: unknown = undefined): boolean { + const isKnownTool = isValidToolName(String(block.name)) + return Boolean(isKnownTool && !block.nativeArgs && !customTool) + } + + it("clears nativeArgs so a truncated write_to_file call is blocked instead of executed", () => { + // A write_to_file call whose `content` was cut off mid-stream - exactly + // the scenario in #1221. The streaming phase already populated nativeArgs + // from the incomplete partial-json parse before finalize failed. + const truncated: WriteToFileToolUse = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: true, + nativeArgs: { path: "src/config.json", content: '{"apiKey": "sk-live-abc123' /* cut off mid-string */ }, + } + + finalizeNullBranch(truncated) + + expect(truncated.partial).toBe(false) + expect(truncated.nativeArgs).toBeUndefined() + expect(wouldBeBlocked(truncated)).toBe(true) + }) + + it("companion: without the fix, the same truncated call would NOT have been blocked", () => { + const truncated: WriteToFileToolUse = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: true, + nativeArgs: { path: "src/config.json", content: '{"apiKey": "sk-live-abc123' }, + } + + finalizeNullBranchBeforeFix(truncated) + + // This is the bug: partial is false (presented as "complete"), but + // nativeArgs still carries the truncated value, so the guard's + // `!block.nativeArgs` never becomes true and the call would proceed to + // execution with the truncated content. + expect(truncated.partial).toBe(false) + expect(truncated.nativeArgs).toEqual({ path: "src/config.json", content: '{"apiKey": "sk-live-abc123' }) + expect(wouldBeBlocked(truncated)).toBe(false) + }) + + it("does not affect a normally-finalized (non-null) tool call", () => { + // When finalizeStreamingToolCall() succeeds, Task.ts replaces the block + // with the freshly-finalized one instead of taking this branch at all - + // this test just confirms a complete, valid nativeArgs is never touched + // by wouldBeBlocked's guard simulation. + const complete: WriteToFileToolUse = { + type: "tool_use", + name: "write_to_file", + params: {}, + partial: false, + nativeArgs: { path: "src/config.json", content: '{"apiKey": "sk-live-abc123xyz"}' }, + } + + expect(wouldBeBlocked(complete)).toBe(false) + }) +}) From 79b28e7fdfd2d5772e61043cf2469444b991f49f Mon Sep 17 00:00:00 2001 From: Can Date: Mon, 14 Sep 2026 10:30:51 +0300 Subject: [PATCH 2/4] test(task): add integration coverage for truncated write_to_file finalize (#1221) Adds an integration-level regression test alongside the existing simulation-based unit tests in truncated-native-tool-args.spec.ts. Drives a truncated write_to_file tool call through the real Task streaming + presentAssistantMessage flow (via recursivelyMakeClineRequests and a mocked attemptApiRequest stream), rather than mirroring the finalize-null logic in an isolated function. Spies on writeToFileTool.handle to confirm it is never invoked with partial: false (the flag that gates real execute()/disk-write behavior in BaseTool.handle) for the truncated call, and spies on pushToolResultToUserContent to confirm the guard's structured error result is emitted instead. Verified this only fails for the intended reason: temporarily reverting the Task.ts fix makes writeToFileTool.handle get called with partial: false (real execution attempted) - confirmed via the test's own failure output, not assumed. An earlier version of this test used a .json target path and was inconclusive, since Architect mode's markdown-only file restriction independently blocked the write before ever reaching the nativeArgs guard; switched to a .md path so the guard under test is what's actually being exercised. Full core/task suite: 27 files, 382 tests, all passing. --- src/core/task/__tests__/Task.spec.ts | 88 ++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index c35acf864d..e1d501bfc1 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -32,6 +32,7 @@ import type { ApiMessage } from "../../task-persistence" import { asyncStreamFrom } from "../../../test-utils/stream" import { McpHub } from "../../../services/mcp/McpHub" import { McpServerManager } from "../../../services/mcp/McpServerManager" +import { writeToFileTool } from "../../tools/WriteToFileTool" type TaskTestAccess = { getSystemPrompt: (requestState: ProviderState | undefined, requestModelInfo?: ModelInfo) => Promise @@ -694,6 +695,93 @@ describe("Cline", () => { }, ]) }) + + it("blocks a truncated write_to_file call instead of executing it (issue #1221)", async () => { + // Regression test for #1221: if the model's stream is cut off mid-way + // through a write_to_file tool call's `content` argument (e.g. it hits + // max_tokens), finalizeStreamingToolCall() can't parse the incomplete + // JSON and returns null. Task.ts must not let the truncated content + // reach writeToFileTool's execution path - it must clear nativeArgs so + // presentAssistantMessage's fail-closed guard emits a structured + // tool_result error instead. + // + // Unlike the simulation-based tests in truncated-native-tool-args.spec.ts, + // this drives the real streaming + presentAssistantMessage flow through + // Task, and spies on the actual tool handler to prove it is never invoked. + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "truncated tool call test", + startTask: false, + }) + + vi.spyOn(task.diffViewProvider, "reset").mockResolvedValue(undefined) + vi.spyOn(getTaskTestAccess(task), "safeEnsureModelFetched").mockResolvedValue(undefined) + // presentAssistantMessageSafe is intentionally left un-mocked here (unlike + // the other tests in this block) - the whole point is to exercise the real + // dispatch/guard logic, not just tool_use finalization. + + const writeToFileHandleSpy = vi.spyOn(writeToFileTool, "handle") + // Spy directly on the guard's own push, rather than inspecting + // userMessageContent/apiConversationHistory afterwards - the task + // recurses into a follow-up request once the tool_result is ready + // (see the second mocked stream below), which resets those arrays + // for the new turn before this function returns. + const pushToolResultSpy = vi.spyOn(task, "pushToolResultToUserContent") + + vi.spyOn(task, "attemptApiRequest") + .mockImplementationOnce(() => + asyncStreamFrom([ + { + type: "tool_call_partial", + index: 0, + id: "call_truncated", + name: "write_to_file", + }, + { + type: "tool_call_partial", + index: 0, + // Cut off mid-string: no closing quote/brace, and the stream + // ends here with no explicit tool_call_end - exactly what + // happens when the model hits max_tokens mid-argument. + // .md path deliberately used so the only thing that can block + // execution is the nativeArgs guard under test - an arbitrary + // extension could also get caught by unrelated mode-based file + // restrictions (e.g. Architect mode's markdown-only rule), + // which would produce a false pass/fail unrelated to this bug. + arguments: '{"path":"docs/config.md","content":"sk-live-abc123', + }, + ]), + ) + // The task recurses once the error tool_result makes the turn + // "ready" - this bounds that follow-up to a single harmless text + // reply instead of an unmocked second call. + .mockImplementationOnce(() => asyncStreamFrom([{ type: "text", text: "" }])) + + await task.recursivelyMakeClineRequests([{ type: "text", text: "truncated tool call test" }]) + + // handle() legitimately gets called with partial: true while the call is + // still streaming (BaseTool.handle short-circuits to a no-op preview hook + // in that case) - that's expected and safe. What must never happen is a + // call with partial: false, which is what actually reaches execute() and + // writes to disk. + const nonPartialCalls = writeToFileHandleSpy.mock.calls.filter( + ([, block]) => (block as { partial?: boolean }).partial === false, + ) + expect(nonPartialCalls).toHaveLength(0) + + // A structured, matching tool_result error must have been pushed for + // the truncated call's ID instead of letting it execute. + const truncatedCallResult = pushToolResultSpy.mock.calls.find( + ([result]) => result.tool_use_id === "call_truncated", + )?.[0] + expect(truncatedCallResult).toMatchObject({ + type: "tool_result", + tool_use_id: "call_truncated", + is_error: true, + }) + expect(JSON.stringify(truncatedCallResult)).toContain("missing nativeArgs") + }) }) describe("constructor", () => { From 02286d8b6d78b7fcfda4ce9004e2b813f10716d9 Mon Sep 17 00:00:00 2001 From: Can Date: Sat, 19 Sep 2026 09:15:08 +0300 Subject: [PATCH 3/4] fix(task): also clear params on truncated tool-call finalize (#1221) Clearing nativeArgs alone left a second gap: Task.ts records each tool_use into apiConversationHistory via toolUse.nativeArgs || toolUse.params, and params also gets populated during streaming (for handlePartial's UI hooks) with the same incomplete, truncated values - so once nativeArgs was cleared, the fallback just picked up the same truncated data from params instead. Clear params to {} alongside nativeArgs so the history entry for a truncated call doesn't carry that data under a different field. Execution itself was never at risk from this gap - BaseTool.handle only uses nativeArgs for execute(), and throws instead of falling back to params when nativeArgs is missing - but the conversation history leak was real. Adds an assertion to the Task.spec.ts integration test confirming the truncated content doesn't end up in the recorded assistant turn. --- src/core/task/Task.ts | 16 ++++++++++------ src/core/task/__tests__/Task.spec.ts | 9 +++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 8ac47ef448..d00cc5892f 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -3811,16 +3811,20 @@ export class Task extends EventEmitter implements TaskLike { } else if (toolUseIndex !== undefined) { // finalizeStreamingToolCall returned null (malformed JSON or missing args). // existingToolUse is the same object the streaming phase was mutating in - // place, so it still carries nativeArgs built from the incomplete partial - // parse (e.g. a truncated write_to_file `content` string) - that value was - // only ever meant for live progress display, never for execution. Mark the - // tool as non-partial so it's presented as complete, and clear nativeArgs so - // presentAssistantMessage's `!block.nativeArgs` guard actually short-circuits - // it with a structured tool_result instead of executing the truncated value. + // place, so it still carries nativeArgs AND params built from the incomplete + // partial parse (e.g. a truncated write_to_file `content` string) - both were + // only ever meant for live progress display, never for execution or for + // ending up in conversation history. Mark the tool as non-partial so it's + // presented as complete, and clear both so presentAssistantMessage's + // `!block.nativeArgs` guard short-circuits with a structured tool_result + // instead of executing the truncated value, and so the toolUse.nativeArgs || + // toolUse.params fallback used when recording history doesn't fall through to + // the same truncated data under a different name. const existingToolUse = this.assistantMessageContent[toolUseIndex] if (existingToolUse && existingToolUse.type === "tool_use") { existingToolUse.partial = false existingToolUse.nativeArgs = undefined + existingToolUse.params = {} // Ensure it has the ID for native protocol ;(existingToolUse as any).id = event.id } diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index e1d501bfc1..89a39ab13b 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -770,6 +770,15 @@ describe("Cline", () => { ) expect(nonPartialCalls).toHaveLength(0) + // Neither nativeArgs nor params should leak the truncated content into + // the recorded assistant turn - Task.ts builds that entry's `input` via + // `toolUse.nativeArgs || toolUse.params`, so clearing nativeArgs alone + // would just have shifted the leak to params instead of closing it. + const assistantEntry = task.apiConversationHistory.find( + (m) => m.role === "assistant" && Array.isArray(m.content) && m.content[0]?.type === "tool_use", + ) + expect(JSON.stringify(assistantEntry)).not.toContain("sk-live-abc123") + // A structured, matching tool_result error must have been pushed for // the truncated call's ID instead of letting it execute. const truncatedCallResult = pushToolResultSpy.mock.calls.find( From d5ba3bdc3fce028edf43b81c1f61642d9635a0e7 Mon Sep 17 00:00:00 2001 From: Can Date: Sat, 19 Sep 2026 09:20:19 +0300 Subject: [PATCH 4/4] test(task): update safeEnsureModelFetched mock after rebase safeEnsureModelFetched's return type changed to Promise upstream while this branch was in flight; the new integration test still had mockResolvedValue(undefined) from before that change, which the type checker caught after rebasing onto main. Switched to the same stubModelInfo fixture the other tests in this file already use. --- src/core/task/__tests__/Task.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 89a39ab13b..2495c20ff2 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -716,7 +716,7 @@ describe("Cline", () => { }) vi.spyOn(task.diffViewProvider, "reset").mockResolvedValue(undefined) - vi.spyOn(getTaskTestAccess(task), "safeEnsureModelFetched").mockResolvedValue(undefined) + vi.spyOn(getTaskTestAccess(task), "safeEnsureModelFetched").mockResolvedValue(stubModelInfo) // presentAssistantMessageSafe is intentionally left un-mocked here (unlike // the other tests in this block) - the whole point is to exercise the real // dispatch/guard logic, not just tool_use finalization.