Skip to content

Commit c099d41

Browse files
committed
fix: improve AssistantMessageParser with validation, consistent trimming, and test coverage
- Add tool name validation to prevent invalid casts - Ensure consistent text content trimming for both new and updated blocks - Add test coverage for MAX_ACCUMULATOR_SIZE and MAX_PARAM_LENGTH limits - Fix auto-approve regression by keeping blocks partial until finalization
1 parent d9a986e commit c099d41

File tree

2 files changed

+68
-5
lines changed

2 files changed

+68
-5
lines changed

src/core/assistant-message/AssistantMessageParser.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -159,10 +159,19 @@ export class AssistantMessageParser {
159159

160160
for (const toolUseOpeningTag of possibleToolUseOpeningTags) {
161161
if (this.accumulator.endsWith(toolUseOpeningTag)) {
162+
// Extract and validate the tool name
163+
const extractedToolName = toolUseOpeningTag.slice(1, -1)
164+
165+
// Check if the extracted tool name is valid
166+
if (!toolNames.includes(extractedToolName as ToolName)) {
167+
// Invalid tool name, treat as plain text and continue
168+
continue
169+
}
170+
162171
// Start of a new tool use.
163172
this.currentToolUse = {
164173
type: "tool_use",
165-
name: toolUseOpeningTag.slice(1, -1) as ToolName,
174+
name: extractedToolName as ToolName,
166175
params: {},
167176
partial: true,
168177
}
@@ -179,8 +188,6 @@ export class AssistantMessageParser {
179188
.slice(0, -toolUseOpeningTag.slice(0, -1).length)
180189
.trim()
181190

182-
this.currentTextContent.content = this.currentTextContent.content.trim()
183-
184191
// No need to push, currentTextContent is already in contentBlocks
185192
this.currentTextContent = undefined
186193
}
@@ -207,7 +214,7 @@ export class AssistantMessageParser {
207214
// Create a new text content block and add it to contentBlocks
208215
this.currentTextContent = {
209216
type: "text",
210-
content: this.accumulator.slice(this.currentTextContentStartIndex),
217+
content: this.accumulator.slice(this.currentTextContentStartIndex).trim(),
211218
partial: true,
212219
}
213220

@@ -216,7 +223,7 @@ export class AssistantMessageParser {
216223
this.contentBlocks.push(this.currentTextContent)
217224
} else {
218225
// Update the existing text content
219-
this.currentTextContent.content = this.accumulator.slice(this.currentTextContentStartIndex)
226+
this.currentTextContent.content = this.accumulator.slice(this.currentTextContentStartIndex).trim()
220227
}
221228
}
222229
}

src/core/assistant-message/__tests__/AssistantMessageParser.spec.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,62 @@ describe("AssistantMessageParser (streaming)", () => {
325325
})
326326
})
327327

328+
describe("size limit handling", () => {
329+
it("should throw an error when MAX_ACCUMULATOR_SIZE is exceeded", () => {
330+
// Create a message that exceeds 1MB (MAX_ACCUMULATOR_SIZE)
331+
const largeMessage = "x".repeat(1024 * 1024 + 1) // 1MB + 1 byte
332+
333+
expect(() => {
334+
parser.processChunk(largeMessage)
335+
}).toThrow("Assistant message exceeds maximum allowed size")
336+
})
337+
338+
it("should gracefully handle a parameter that exceeds MAX_PARAM_LENGTH", () => {
339+
// Create a parameter value that exceeds 100KB (MAX_PARAM_LENGTH)
340+
const largeParamValue = "x".repeat(1024 * 100 + 1) // 100KB + 1 byte
341+
const message = `<write_to_file><path>test.txt</path><content>${largeParamValue}</content></write_to_file>After tool`
342+
343+
// Process the message in chunks to simulate streaming
344+
let result: AssistantMessageContent[] = []
345+
let error: Error | null = null
346+
347+
try {
348+
// Process the opening tags
349+
result = parser.processChunk("<write_to_file><path>test.txt</path><content>")
350+
351+
// Process the large parameter value in chunks
352+
const chunkSize = 1000
353+
for (let i = 0; i < largeParamValue.length; i += chunkSize) {
354+
const chunk = largeParamValue.slice(i, i + chunkSize)
355+
result = parser.processChunk(chunk)
356+
}
357+
358+
// Process the closing tags and text after
359+
result = parser.processChunk("</content></write_to_file>After tool")
360+
} catch (e) {
361+
error = e as Error
362+
}
363+
364+
// Should not throw an error
365+
expect(error).toBeNull()
366+
367+
// Should have processed the content
368+
expect(result.length).toBeGreaterThan(0)
369+
370+
// The tool use should exist but the content parameter should be reset/empty
371+
const toolUse = result.find((block) => block.type === "tool_use") as ToolUse
372+
expect(toolUse).toBeDefined()
373+
expect(toolUse.name).toBe("write_to_file")
374+
expect(toolUse.params.path).toBe("test.txt")
375+
376+
// The text after the tool should still be parsed
377+
const textAfter = result.find(
378+
(block) => block.type === "text" && (block as TextContent).content.includes("After tool"),
379+
)
380+
expect(textAfter).toBeDefined()
381+
})
382+
})
383+
328384
describe("finalizeContentBlocks", () => {
329385
it("should mark all partial blocks as complete", () => {
330386
const message = "<read_file><path>src/file.ts"

0 commit comments

Comments
 (0)