mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: prevent write_to_file from executing with missing content parameter
When finalizeStreamingToolCall returns null (malformed/incomplete args from LLM), the existing partial ToolUse block was promoted to non-partial while retaining stale nativeArgs from partial parsing. The partial parser for write_to_file used OR logic (path || content), allowing nativeArgs to be set with only path present. This bypassed the safety check in presentAssistantMessage that guards against missing nativeArgs. Two fixes applied: 1. Clear nativeArgs when promoting failed finalization blocks to complete in Task.ts (both finalization paths). This ensures the safety check in presentAssistantMessage properly catches invalid tool calls. 2. Tighten partial write_to_file nativeArgs in NativeToolCallParser to require both path AND content (handlePartial reads from params, not nativeArgs, so this is safe). Closes #12079
This commit is contained in:
parent
7adbfec2a4
commit
4f0c59569f
3 changed files with 116 additions and 3 deletions
|
|
@ -465,7 +465,7 @@ export class NativeToolCallParser {
|
|||
break
|
||||
|
||||
case "write_to_file":
|
||||
if (partialArgs.path || partialArgs.content) {
|
||||
if (partialArgs.path !== undefined && partialArgs.content !== undefined) {
|
||||
nativeArgs = {
|
||||
path: partialArgs.path,
|
||||
content: partialArgs.content,
|
||||
|
|
|
|||
|
|
@ -313,6 +313,107 @@ describe("NativeToolCallParser", () => {
|
|||
})
|
||||
})
|
||||
|
||||
describe("write_to_file partial streaming", () => {
|
||||
it("should not set nativeArgs when only path is present (content missing)", () => {
|
||||
const id = "toolu_wtf_partial_1"
|
||||
NativeToolCallParser.startStreamingToolCall(id, "write_to_file")
|
||||
|
||||
// Simulate partial chunk with only path, no content yet
|
||||
const result = NativeToolCallParser.processStreamingChunk(id, JSON.stringify({ path: "test.txt" }))
|
||||
|
||||
// nativeArgs should NOT be set when content is missing
|
||||
expect(result).not.toBeNull()
|
||||
if (result) {
|
||||
expect(result.nativeArgs).toBeUndefined()
|
||||
// params should still have path for UI display
|
||||
expect(result.params.path).toBe("test.txt")
|
||||
}
|
||||
})
|
||||
|
||||
it("should set nativeArgs when both path and content are present", () => {
|
||||
const id = "toolu_wtf_partial_2"
|
||||
NativeToolCallParser.startStreamingToolCall(id, "write_to_file")
|
||||
|
||||
const result = NativeToolCallParser.processStreamingChunk(
|
||||
id,
|
||||
JSON.stringify({ path: "test.txt", content: "hello world" }),
|
||||
)
|
||||
|
||||
expect(result).not.toBeNull()
|
||||
if (result) {
|
||||
expect(result.nativeArgs).toBeDefined()
|
||||
const nativeArgs = result.nativeArgs as { path: string; content: string }
|
||||
expect(nativeArgs.path).toBe("test.txt")
|
||||
expect(nativeArgs.content).toBe("hello world")
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe("write_to_file parseToolCall (complete)", () => {
|
||||
it("should return null when content is missing from complete tool call", () => {
|
||||
const toolCall = {
|
||||
id: "toolu_wtf_complete_1",
|
||||
name: "write_to_file" as const,
|
||||
arguments: JSON.stringify({ path: "test.txt" }),
|
||||
}
|
||||
|
||||
// parseToolCall throws internally and returns null for invalid args
|
||||
const result = NativeToolCallParser.parseToolCall(toolCall)
|
||||
expect(result).toBeNull()
|
||||
})
|
||||
|
||||
it("should parse correctly when both path and content are present", () => {
|
||||
const toolCall = {
|
||||
id: "toolu_wtf_complete_2",
|
||||
name: "write_to_file" as const,
|
||||
arguments: JSON.stringify({ path: "test.txt", content: "hello world" }),
|
||||
}
|
||||
|
||||
const result = NativeToolCallParser.parseToolCall(toolCall)
|
||||
expect(result).not.toBeNull()
|
||||
expect(result?.type).toBe("tool_use")
|
||||
if (result?.type === "tool_use") {
|
||||
expect(result.nativeArgs).toBeDefined()
|
||||
const nativeArgs = result.nativeArgs as { path: string; content: string }
|
||||
expect(nativeArgs.path).toBe("test.txt")
|
||||
expect(nativeArgs.content).toBe("hello world")
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe("write_to_file finalization fallback", () => {
|
||||
it("should return null when finalized with missing content", () => {
|
||||
const id = "toolu_wtf_finalize_1"
|
||||
NativeToolCallParser.startStreamingToolCall(id, "write_to_file")
|
||||
|
||||
// Stream only path, no content
|
||||
NativeToolCallParser.processStreamingChunk(id, JSON.stringify({ path: "test.txt" }))
|
||||
|
||||
// Finalization should fail (return null) because content is missing
|
||||
const result = NativeToolCallParser.finalizeStreamingToolCall(id)
|
||||
expect(result).toBeNull()
|
||||
})
|
||||
|
||||
it("should finalize successfully when both path and content are present", () => {
|
||||
const id = "toolu_wtf_finalize_2"
|
||||
NativeToolCallParser.startStreamingToolCall(id, "write_to_file")
|
||||
|
||||
NativeToolCallParser.processStreamingChunk(
|
||||
id,
|
||||
JSON.stringify({ path: "test.txt", content: "file content" }),
|
||||
)
|
||||
|
||||
const result = NativeToolCallParser.finalizeStreamingToolCall(id)
|
||||
expect(result).not.toBeNull()
|
||||
expect(result?.type).toBe("tool_use")
|
||||
if (result?.type === "tool_use") {
|
||||
const nativeArgs = result.nativeArgs as { path: string; content: string }
|
||||
expect(nativeArgs.path).toBe("test.txt")
|
||||
expect(nativeArgs.content).toBe("file content")
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe("finalizeStreamingToolCall", () => {
|
||||
describe("read_file tool", () => {
|
||||
it("should parse read_file args on finalize", () => {
|
||||
|
|
|
|||
|
|
@ -2967,6 +2967,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
const existingToolUse = this.assistantMessageContent[toolUseIndex]
|
||||
if (existingToolUse && existingToolUse.type === "tool_use") {
|
||||
existingToolUse.partial = false
|
||||
// Clear stale nativeArgs from partial parsing so the safety check
|
||||
// in presentAssistantMessage (!block.nativeArgs) properly catches
|
||||
// this as an invalid tool call. Without this, incomplete partial
|
||||
// nativeArgs (e.g., path set but content undefined) would bypass
|
||||
// the check and cause tools to execute with missing parameters.
|
||||
existingToolUse.nativeArgs = undefined
|
||||
// Ensure it has the ID for native protocol
|
||||
;(existingToolUse as any).id = event.id
|
||||
}
|
||||
|
|
@ -3350,11 +3356,17 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
presentAssistantMessage(this)
|
||||
} 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
|
||||
// We still need to mark the tool as non-partial so it gets presented.
|
||||
// The presentAssistantMessage safety check will catch missing nativeArgs.
|
||||
const existingToolUse = this.assistantMessageContent[toolUseIndex]
|
||||
if (existingToolUse && existingToolUse.type === "tool_use") {
|
||||
existingToolUse.partial = false
|
||||
// Clear stale nativeArgs from partial parsing so the safety check
|
||||
// in presentAssistantMessage (!block.nativeArgs) properly catches
|
||||
// this as an invalid tool call. Without this, incomplete partial
|
||||
// nativeArgs (e.g., path set but content undefined) would bypass
|
||||
// the check and cause tools to execute with missing parameters.
|
||||
existingToolUse.nativeArgs = undefined
|
||||
// Ensure it has the ID for native protocol
|
||||
;(existingToolUse as any).id = event.id
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue