From 5103e3d827cbb1b34dcc9a0e9e1edd49be3ba468 Mon Sep 17 00:00:00 2001 From: Can Date: Mon, 14 Sep 2026 09:39:50 +0300 Subject: [PATCH 1/2] 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 92ee8184d6..aa53d16de1 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -3746,12 +3746,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 d56e3cd8d9c3486ef31d2150a92d6eb1376816f2 Mon Sep 17 00:00:00 2001 From: Can Date: Mon, 14 Sep 2026 10:30:51 +0300 Subject: [PATCH 2/2] 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 1bcacd459c..8309e0f8d8 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -29,6 +29,7 @@ import { processUserContentMentions } from "../../mentions/processUserContentMen import { MultiSearchReplaceDiffStrategy } from "../../diff/strategies/multi-search-replace" import type { ApiMessage } from "../../task-persistence" import { asyncStreamFrom } from "../../../test-utils/stream" +import { writeToFileTool } from "../../tools/WriteToFileTool" type TaskTestAccess = { getSystemPrompt: () => Promise @@ -645,6 +646,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", () => {