mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-06 08:18:39 +00:00
fix: enable XML tool parsing when switching from Native to XML protocol mid-task
- Dynamically check tool protocol on each streaming chunk instead of caching at stream start - Ensures XML parser is activated immediately when switching profiles from Native to XML - Fixes issue where XML tool calls were treated as plain text after protocol switch Fixes #9509
This commit is contained in:
Roo Code
parent
cdc72750e4
commit
fa80b4f000
2 changed files with 307 additions and 6 deletions
|
|
@ -2240,8 +2240,6 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
this.cachedStreamingModel = this.api.getModel()
|
||||
const streamModelInfo = this.cachedStreamingModel.info
|
||||
const cachedModelId = this.cachedStreamingModel.id
|
||||
const streamProtocol = resolveToolProtocol(this.apiConfiguration, streamModelInfo)
|
||||
const shouldUseXmlParser = streamProtocol === "xml"
|
||||
|
||||
// Yields only if the first chunk is successful, otherwise will
|
||||
// allow the user to retry the request (most likely due to rate
|
||||
|
|
@ -2349,8 +2347,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
case "text": {
|
||||
assistantMessage += chunk.text
|
||||
|
||||
// Use the protocol determined at the start of streaming
|
||||
// Don't rely solely on parser existence - parser might exist from previous state
|
||||
// Dynamically check the current protocol on each chunk to handle mid-task profile switches
|
||||
// This ensures XML parsing activates immediately when switching from Native to XML
|
||||
const currentModelInfo = this.api.getModel().info
|
||||
const currentProtocol = resolveToolProtocol(this.apiConfiguration, currentModelInfo)
|
||||
const shouldUseXmlParser = currentProtocol === "xml"
|
||||
|
||||
if (shouldUseXmlParser && this.assistantMessageParser) {
|
||||
// XML protocol: Parse raw assistant message chunk into content blocks
|
||||
const prevLength = this.assistantMessageContent.length
|
||||
|
|
@ -2679,8 +2681,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
// this.assistantMessageContent.forEach((e) => (e.partial = false))
|
||||
|
||||
// Now that the stream is complete, finalize any remaining partial content blocks (XML protocol only)
|
||||
// Use the protocol determined at the start of streaming
|
||||
if (shouldUseXmlParser && this.assistantMessageParser) {
|
||||
// Check the current protocol (not cached) to handle mid-stream profile switches
|
||||
const finalModelInfo = this.api.getModel().info
|
||||
const finalProtocol = resolveToolProtocol(this.apiConfiguration, finalModelInfo)
|
||||
if (finalProtocol === "xml" && this.assistantMessageParser) {
|
||||
this.assistantMessageParser.finalizeContentBlocks()
|
||||
const parsedBlocks = this.assistantMessageParser.getContentBlocks()
|
||||
// For XML protocol: Use only parsed blocks (includes both text and tool_use parsed from XML)
|
||||
|
|
|
|||
297
src/core/task/__tests__/task-protocol-switch.spec.ts
Normal file
297
src/core/task/__tests__/task-protocol-switch.spec.ts
Normal file
|
|
@ -0,0 +1,297 @@
|
|||
import { describe, it, expect, beforeEach, vi } from "vitest"
|
||||
import { Task } from "../Task"
|
||||
import { AssistantMessageParser } from "../../assistant-message/AssistantMessageParser"
|
||||
import { resolveToolProtocol } from "../../../utils/resolveToolProtocol"
|
||||
import type { ClineProvider } from "../../webview/ClineProvider"
|
||||
import type { ApiHandler } from "../../../api"
|
||||
import { TOOL_PROTOCOL } from "@roo-code/types"
|
||||
|
||||
// Mock vscode module
|
||||
vi.mock("vscode", () => ({
|
||||
default: {},
|
||||
workspace: {
|
||||
createFileSystemWatcher: vi.fn(() => ({
|
||||
onDidCreate: vi.fn(),
|
||||
onDidChange: vi.fn(),
|
||||
onDidDelete: vi.fn(),
|
||||
dispose: vi.fn(),
|
||||
})),
|
||||
getConfiguration: vi.fn(() => ({
|
||||
get: vi.fn(),
|
||||
})),
|
||||
},
|
||||
RelativePattern: class RelativePattern {
|
||||
constructor(
|
||||
public base: string,
|
||||
public pattern: string,
|
||||
) {}
|
||||
},
|
||||
EventEmitter: class EventEmitter {
|
||||
fire = vi.fn()
|
||||
event = vi.fn()
|
||||
dispose = vi.fn()
|
||||
},
|
||||
}))
|
||||
|
||||
// Mock other dependencies
|
||||
vi.mock("../../../utils/resolveToolProtocol")
|
||||
vi.mock("../../assistant-message/AssistantMessageParser", () => ({
|
||||
AssistantMessageParser: vi.fn(() => ({
|
||||
processChunk: vi.fn(),
|
||||
finalizeContentBlocks: vi.fn(),
|
||||
getContentBlocks: vi.fn(),
|
||||
reset: vi.fn(),
|
||||
})),
|
||||
}))
|
||||
vi.mock("../../ignore/RooIgnoreController", () => ({
|
||||
RooIgnoreController: class RooIgnoreController {
|
||||
initialize = vi.fn().mockResolvedValue(undefined)
|
||||
dispose = vi.fn()
|
||||
getInstructions = vi.fn()
|
||||
},
|
||||
}))
|
||||
vi.mock("../../protect/RooProtectedController", () => ({
|
||||
RooProtectedController: class RooProtectedController {
|
||||
dispose = vi.fn()
|
||||
},
|
||||
}))
|
||||
vi.mock("../../context-tracking/FileContextTracker", () => ({
|
||||
FileContextTracker: class FileContextTracker {
|
||||
dispose = vi.fn()
|
||||
},
|
||||
}))
|
||||
vi.mock("../../../services/browser/UrlContentFetcher", () => ({
|
||||
UrlContentFetcher: class UrlContentFetcher {
|
||||
closeBrowser = vi.fn()
|
||||
},
|
||||
}))
|
||||
vi.mock("../../../services/browser/BrowserSession", () => ({
|
||||
BrowserSession: class BrowserSession {
|
||||
closeBrowser = vi.fn()
|
||||
isSessionActive = vi.fn().mockReturnValue(false)
|
||||
},
|
||||
}))
|
||||
vi.mock("../../../integrations/editor/DiffViewProvider", () => ({
|
||||
DiffViewProvider: class DiffViewProvider {
|
||||
reset = vi.fn()
|
||||
revertChanges = vi.fn()
|
||||
isEditing = false
|
||||
},
|
||||
}))
|
||||
vi.mock("../../../api", () => ({
|
||||
buildApiHandler: vi.fn(() => mockApi),
|
||||
}))
|
||||
vi.mock("../../tools/ToolRepetitionDetector", () => ({
|
||||
ToolRepetitionDetector: class ToolRepetitionDetector {},
|
||||
}))
|
||||
vi.mock("../../message-queue/MessageQueueService", () => ({
|
||||
MessageQueueService: class MessageQueueService {
|
||||
on = vi.fn()
|
||||
removeListener = vi.fn()
|
||||
dispose = vi.fn()
|
||||
isEmpty = vi.fn().mockReturnValue(true)
|
||||
messages = []
|
||||
},
|
||||
}))
|
||||
vi.mock("../../auto-approval", () => ({
|
||||
AutoApprovalHandler: class AutoApprovalHandler {
|
||||
checkAutoApprovalLimits = vi.fn()
|
||||
},
|
||||
}))
|
||||
vi.mock("@roo-code/telemetry", () => ({
|
||||
TelemetryService: {
|
||||
instance: {
|
||||
captureTaskCreated: vi.fn(),
|
||||
captureTaskRestarted: vi.fn(),
|
||||
captureConversationMessage: vi.fn(),
|
||||
captureLlmCompletion: vi.fn(),
|
||||
captureConsecutiveMistakeError: vi.fn(),
|
||||
},
|
||||
},
|
||||
}))
|
||||
|
||||
// Need to define mockApi outside beforeEach so it's available to the mock
|
||||
let mockApi: any
|
||||
|
||||
describe("Task Protocol Switching", () => {
|
||||
let mockProvider: any
|
||||
|
||||
beforeEach(() => {
|
||||
// Reset mockApi before each test
|
||||
mockApi = {
|
||||
getModel: vi.fn(() => ({
|
||||
id: "test-model",
|
||||
info: {
|
||||
supportsNativeTools: true,
|
||||
contextWindow: 100000,
|
||||
},
|
||||
})),
|
||||
}
|
||||
|
||||
// Setup mock provider
|
||||
mockProvider = {
|
||||
context: {
|
||||
globalStorageUri: { fsPath: "/test/storage" },
|
||||
extensionUri: { fsPath: "/test/extension" },
|
||||
},
|
||||
getState: vi.fn(),
|
||||
postStateToWebview: vi.fn(),
|
||||
postMessageToWebview: vi.fn(),
|
||||
on: vi.fn(),
|
||||
off: vi.fn(),
|
||||
log: vi.fn(),
|
||||
updateTaskHistory: vi.fn(),
|
||||
providerSettingsManager: {
|
||||
getProfile: vi.fn(),
|
||||
},
|
||||
}
|
||||
|
||||
// Setup default return values
|
||||
mockProvider.getState.mockResolvedValue({
|
||||
apiConfiguration: {
|
||||
apiProvider: "anthropic",
|
||||
toolProtocol: TOOL_PROTOCOL.NATIVE,
|
||||
},
|
||||
})
|
||||
})
|
||||
|
||||
it("should dynamically check protocol during streaming when switching from Native to XML", async () => {
|
||||
// Start with Native protocol
|
||||
const mockResolveToolProtocol = vi.mocked(resolveToolProtocol)
|
||||
mockResolveToolProtocol.mockReturnValue(TOOL_PROTOCOL.NATIVE)
|
||||
|
||||
// Create task with Native protocol
|
||||
const task = new Task({
|
||||
provider: mockProvider as any,
|
||||
apiConfiguration: {
|
||||
apiProvider: "anthropic",
|
||||
toolProtocol: TOOL_PROTOCOL.NATIVE,
|
||||
},
|
||||
task: "Test task",
|
||||
})
|
||||
|
||||
// Verify parser is not created for native protocol
|
||||
expect(task.assistantMessageParser).toBeUndefined()
|
||||
|
||||
// Simulate profile switch to XML protocol
|
||||
await task.updateApiConfiguration({
|
||||
apiProvider: "anthropic",
|
||||
toolProtocol: TOOL_PROTOCOL.XML,
|
||||
})
|
||||
|
||||
// Mock the protocol resolution to return XML after switch
|
||||
mockResolveToolProtocol.mockReturnValue(TOOL_PROTOCOL.XML)
|
||||
|
||||
// Verify parser is created after switch to XML
|
||||
expect(task.assistantMessageParser).toBeDefined()
|
||||
expect(task.assistantMessageParser).toBeInstanceOf(AssistantMessageParser)
|
||||
})
|
||||
|
||||
it("should handle XML tool calls correctly after switching from Native to XML mid-task", async () => {
|
||||
const mockResolveToolProtocol = vi.mocked(resolveToolProtocol)
|
||||
|
||||
// Start with Native protocol
|
||||
mockResolveToolProtocol.mockReturnValue(TOOL_PROTOCOL.NATIVE)
|
||||
|
||||
const task = new Task({
|
||||
provider: mockProvider as any,
|
||||
apiConfiguration: {
|
||||
apiProvider: "anthropic",
|
||||
toolProtocol: TOOL_PROTOCOL.NATIVE,
|
||||
},
|
||||
task: "Test task",
|
||||
})
|
||||
|
||||
// Switch to XML protocol mid-task
|
||||
mockResolveToolProtocol.mockReturnValue(TOOL_PROTOCOL.XML)
|
||||
|
||||
await task.updateApiConfiguration({
|
||||
apiProvider: "anthropic",
|
||||
toolProtocol: TOOL_PROTOCOL.XML,
|
||||
})
|
||||
|
||||
// Mock parser to simulate parsing XML tool calls
|
||||
const mockProcessChunk = vi.fn().mockReturnValue([
|
||||
{
|
||||
type: "tool_use",
|
||||
name: "read_file",
|
||||
params: { path: "test.txt" },
|
||||
partial: false,
|
||||
},
|
||||
])
|
||||
|
||||
if (task.assistantMessageParser) {
|
||||
task.assistantMessageParser.processChunk = mockProcessChunk
|
||||
}
|
||||
|
||||
// Simulate receiving XML text chunk that should be parsed as a tool call
|
||||
const xmlChunk = "<read_file><path>test.txt</path></read_file>"
|
||||
|
||||
// The protocol should be checked dynamically and use XML parser
|
||||
const result = task.assistantMessageParser?.processChunk(xmlChunk)
|
||||
|
||||
expect(mockProcessChunk).toHaveBeenCalledWith(xmlChunk)
|
||||
expect(result).toBeDefined()
|
||||
expect(result?.[0]).toMatchObject({
|
||||
type: "tool_use",
|
||||
name: "read_file",
|
||||
params: { path: "test.txt" },
|
||||
})
|
||||
})
|
||||
|
||||
it("should handle switching from XML to Native protocol", async () => {
|
||||
const mockResolveToolProtocol = vi.mocked(resolveToolProtocol)
|
||||
|
||||
// Start with XML protocol
|
||||
mockResolveToolProtocol.mockReturnValue(TOOL_PROTOCOL.XML)
|
||||
|
||||
const task = new Task({
|
||||
provider: mockProvider as any,
|
||||
apiConfiguration: {
|
||||
apiProvider: "anthropic",
|
||||
toolProtocol: TOOL_PROTOCOL.XML,
|
||||
},
|
||||
task: "Test task",
|
||||
})
|
||||
|
||||
// Verify parser is created for XML protocol
|
||||
expect(task.assistantMessageParser).toBeDefined()
|
||||
|
||||
// Switch to Native protocol
|
||||
mockResolveToolProtocol.mockReturnValue(TOOL_PROTOCOL.NATIVE)
|
||||
|
||||
await task.updateApiConfiguration({
|
||||
apiProvider: "anthropic",
|
||||
toolProtocol: TOOL_PROTOCOL.NATIVE,
|
||||
})
|
||||
|
||||
// Verify parser is removed after switch to Native
|
||||
expect(task.assistantMessageParser).toBeUndefined()
|
||||
})
|
||||
|
||||
it("should not recreate parser if protocol doesn't change", async () => {
|
||||
const mockResolveToolProtocol = vi.mocked(resolveToolProtocol)
|
||||
mockResolveToolProtocol.mockReturnValue(TOOL_PROTOCOL.XML)
|
||||
|
||||
const task = new Task({
|
||||
provider: mockProvider as any,
|
||||
apiConfiguration: {
|
||||
apiProvider: "anthropic",
|
||||
toolProtocol: TOOL_PROTOCOL.XML,
|
||||
},
|
||||
task: "Test task",
|
||||
})
|
||||
|
||||
const originalParser = task.assistantMessageParser
|
||||
|
||||
// Update configuration but keep same protocol
|
||||
await task.updateApiConfiguration({
|
||||
apiProvider: "anthropic",
|
||||
toolProtocol: TOOL_PROTOCOL.XML,
|
||||
apiKey: "new-key", // Different property
|
||||
})
|
||||
|
||||
// Parser should remain the same instance
|
||||
expect(task.assistantMessageParser).toBe(originalParser)
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Reference in a new issue