chore: revert duplicate fix logic for search and replace array args
This commit is contained in:
Dallin Romney
parent
d9e60cffc1
commit
3adf4b62a7
|
|
@ -1,304 +0,0 @@
|
|||
import { beforeEach, describe, expect, it } from "vitest";
|
||||
import { AssistantChatMessage, ChatMessage, PromptLog } from "../..";
|
||||
import { interceptSystemToolCalls } from "./interceptSystemToolCalls";
|
||||
|
||||
describe("interceptSystemToolCalls - Complex Tool Call Test", () => {
|
||||
let abortController: AbortController;
|
||||
|
||||
beforeEach(() => {
|
||||
abortController = new AbortController();
|
||||
});
|
||||
|
||||
const createAsyncGenerator = async function* (
|
||||
messages: ChatMessage[][],
|
||||
): AsyncGenerator<ChatMessage[], PromptLog | undefined> {
|
||||
for (const messageGroup of messages) {
|
||||
yield messageGroup;
|
||||
}
|
||||
return undefined;
|
||||
};
|
||||
|
||||
function isAssistantMessageWithToolCalls(
|
||||
message: ChatMessage,
|
||||
): message is AssistantChatMessage {
|
||||
return (
|
||||
message.role === "assistant" &&
|
||||
"toolCalls" in message &&
|
||||
message.toolCalls !== undefined &&
|
||||
message.toolCalls.length > 0
|
||||
);
|
||||
}
|
||||
|
||||
async function collectAllResults(
|
||||
generator: AsyncGenerator<ChatMessage[], PromptLog | undefined>,
|
||||
): Promise<ChatMessage[][]> {
|
||||
const results: ChatMessage[][] = [];
|
||||
while (true) {
|
||||
const result = await generator.next();
|
||||
if (result.done) break;
|
||||
if (result.value) results.push(result.value);
|
||||
}
|
||||
return results;
|
||||
}
|
||||
|
||||
it("handles complex multi-line tool arguments with nested content and special characters", async () => {
|
||||
// This test case is based on the example.txt which has complex nested content
|
||||
// that includes JSON arrays, code blocks, escape characters, and the word "assistant"
|
||||
const complexDiffContent = `I'll add an exp method to the Calculator class in test.js. This method will raise the current result to the power of a given number.
|
||||
|
||||
\`\`\`tool
|
||||
TOOL_NAME: search_and_replace_in_file
|
||||
BEGIN_ARG: filepath
|
||||
test.js
|
||||
END_ARG
|
||||
BEGIN_ARG: diff
|
||||
[
|
||||
"------- SEARCH
|
||||
/**
|
||||
* Divide the current result by a number
|
||||
* @param {number} number - The number to divide by
|
||||
* @
|
||||
assistant
|
||||
returns {Calculator} - Returns this instance for method chaining
|
||||
* @throws {Error} - Throws error if attempting to divide by zero
|
||||
*/
|
||||
divide(number) {
|
||||
if (number === 0) {
|
||||
throw new Error(\"Cannot divide by zero\");
|
||||
}
|
||||
this.result /= number;
|
||||
return this;
|
||||
}
|
||||
|
||||
=======
|
||||
/**
|
||||
* Divide the current result by a number
|
||||
* @param {number} number - The number to
|
||||
assistant
|
||||
divide by
|
||||
* @returns {Calculator} - Returns this instance for method chaining
|
||||
* @throws {Error} - Throws error if attempting to divide by zero
|
||||
*/
|
||||
divide(number) {
|
||||
if (number === 0) {
|
||||
throw new Error(\"Cannot divide by zero\");
|
||||
}
|
||||
this.result /= number;
|
||||
return this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Raise the current result to the power of a number
|
||||
*
|
||||
assistant
|
||||
@param {number} number - The exponent to raise to
|
||||
* @returns {Calculator} - Returns this instance for method chaining
|
||||
*/
|
||||
exp(number) {
|
||||
this.result = Math.pow(this.result, number);
|
||||
return this;
|
||||
}
|
||||
|
||||
+++++++ REPLACE"
|
||||
]
|
||||
END_ARG
|
||||
\`\`\``;
|
||||
|
||||
const messages: ChatMessage[][] = [
|
||||
[
|
||||
{
|
||||
role: "assistant",
|
||||
content:
|
||||
"I'll add an exp method to the Calculator class in test.js.\n\n",
|
||||
},
|
||||
],
|
||||
[{ role: "assistant", content: "```tool\n" }],
|
||||
[
|
||||
{
|
||||
role: "assistant",
|
||||
content: "TOOL_NAME: search_and_replace_in_file\n",
|
||||
},
|
||||
],
|
||||
[{ role: "assistant", content: "BEGIN_ARG: filepath\n" }],
|
||||
[{ role: "assistant", content: "test.js\n" }],
|
||||
[{ role: "assistant", content: "END_ARG\n" }],
|
||||
[{ role: "assistant", content: "BEGIN_ARG: diffs\n" }],
|
||||
[{ role: "assistant", content: complexDiffContent }],
|
||||
[{ role: "assistant", content: "\nEND_ARG\n" }],
|
||||
[{ role: "assistant", content: "```" }],
|
||||
];
|
||||
|
||||
const generator = interceptSystemToolCalls(
|
||||
createAsyncGenerator(messages),
|
||||
abortController,
|
||||
);
|
||||
|
||||
const allResults = await collectAllResults(generator);
|
||||
|
||||
// Should have processed intro text
|
||||
expect(allResults[0]).toEqual([
|
||||
{
|
||||
role: "assistant",
|
||||
content: [
|
||||
{
|
||||
type: "text",
|
||||
text: "I'll add an exp method to the Calculator class in test.js.",
|
||||
},
|
||||
],
|
||||
},
|
||||
]);
|
||||
|
||||
// Should have detected and parsed the tool call
|
||||
const toolCallResults = allResults.filter(
|
||||
(r): r is AssistantChatMessage[] =>
|
||||
r && r[0] && isAssistantMessageWithToolCalls(r[0]),
|
||||
);
|
||||
|
||||
expect(toolCallResults.length).toBeGreaterThan(0);
|
||||
|
||||
// Tool name should be correct
|
||||
const toolNameResult = toolCallResults.find(
|
||||
(r) =>
|
||||
r[0].toolCalls?.[0]?.function?.name === "search_and_replace_in_file",
|
||||
);
|
||||
expect(toolNameResult).toBeTruthy();
|
||||
|
||||
// Reconstruct the full arguments from all deltas
|
||||
let fullArgs = "";
|
||||
for (const toolResult of toolCallResults) {
|
||||
fullArgs += toolResult[0].toolCalls?.[0]?.function?.arguments || "";
|
||||
}
|
||||
|
||||
console.log("Full args reconstructed:", fullArgs);
|
||||
|
||||
// Should be valid JSON
|
||||
expect(() => JSON.parse(fullArgs)).not.toThrow();
|
||||
|
||||
const parsed = JSON.parse(fullArgs);
|
||||
expect(parsed.filepath).toBe("test.js");
|
||||
expect(parsed.diffs).toBeTruthy();
|
||||
|
||||
// The diffs should contain the complex content including special markers
|
||||
const diffContent1 =
|
||||
typeof parsed.diffs === "string"
|
||||
? parsed.diffs
|
||||
: JSON.stringify(parsed.diffs);
|
||||
expect(diffContent1).toContain("SEARCH");
|
||||
expect(diffContent1).toContain("REPLACE");
|
||||
expect(diffContent1).toContain("Cannot divide by zero");
|
||||
expect(diffContent1).toContain("assistant"); // Word should be preserved in content
|
||||
});
|
||||
|
||||
it("handles arguments with embedded JSON and special escape sequences", async () => {
|
||||
// Test with JSON array containing embedded quotes and backslashes
|
||||
const jsonArrayContent = `["first item", "second \\"quoted\\" item", "third\\nitem"]`;
|
||||
|
||||
const messages: ChatMessage[][] = [
|
||||
[{ role: "assistant", content: "```tool\n" }],
|
||||
[{ role: "assistant", content: "TOOL_NAME: test_tool\n" }],
|
||||
[{ role: "assistant", content: "BEGIN_ARG: json_array\n" }],
|
||||
[{ role: "assistant", content: jsonArrayContent }],
|
||||
[{ role: "assistant", content: "\nEND_ARG\n" }],
|
||||
[{ role: "assistant", content: "```" }],
|
||||
];
|
||||
|
||||
const generator = interceptSystemToolCalls(
|
||||
createAsyncGenerator(messages),
|
||||
abortController,
|
||||
);
|
||||
|
||||
const allResults = await collectAllResults(generator);
|
||||
|
||||
// Should have parsed the tool call
|
||||
const toolCallResults = allResults.filter(
|
||||
(r): r is AssistantChatMessage[] =>
|
||||
r && r[0] && isAssistantMessageWithToolCalls(r[0]),
|
||||
);
|
||||
|
||||
expect(toolCallResults.length).toBeGreaterThan(0);
|
||||
|
||||
// Reconstruct the full arguments
|
||||
let fullArgs = "";
|
||||
for (const toolResult of toolCallResults) {
|
||||
fullArgs += toolResult[0].toolCalls?.[0]?.function?.arguments || "";
|
||||
}
|
||||
|
||||
// Should be valid JSON
|
||||
expect(() => JSON.parse(fullArgs)).not.toThrow();
|
||||
|
||||
const parsed = JSON.parse(fullArgs);
|
||||
expect(parsed.json_array).toBeTruthy();
|
||||
|
||||
// The parsed array should be correct
|
||||
const arrayValue =
|
||||
typeof parsed.json_array === "string"
|
||||
? JSON.parse(parsed.json_array)
|
||||
: parsed.json_array;
|
||||
|
||||
expect(Array.isArray(arrayValue)).toBe(true);
|
||||
expect(arrayValue).toHaveLength(3);
|
||||
expect(arrayValue[0]).toBe("first item");
|
||||
expect(arrayValue[1]).toBe('second "quoted" item');
|
||||
expect(arrayValue[2]).toBe("third\nitem");
|
||||
});
|
||||
|
||||
it("simulates the exact user scenario that causes parsing issues", async () => {
|
||||
// This simulates the exact scenario from example.txt that was causing issues
|
||||
const userProblemDiff = `[
|
||||
"------- SEARCH
|
||||
/**
|
||||
* Divide the current result by a number
|
||||
* @param {number} number - The number to divide by
|
||||
* @returns {Calculator} - Returns this instance for method chaining
|
||||
* @throws {Error} - Throws error if attempting to divide by zero
|
||||
*/
|
||||
divide(number) {
|
||||
if (number === 0) {
|
||||
throw new Error(\"Cannot divide by zero\");
|
||||
}
|
||||
this.result /= number;
|
||||
return this;
|
||||
}
|
||||
=======
|
||||
/**
|
||||
* Divide the current result by a number
|
||||
* @param {number} number - The number to divide by
|
||||
* @returns {Calculator} - Returns this instance for method chaining
|
||||
* @throws {Error} - Throws error if attempting to divide by zero
|
||||
*/
|
||||
divide(number) {
|
||||
if (number === 0) {
|
||||
throw new Error(\"Cannot divide by zero\");
|
||||
}
|
||||
this.result /= number;
|
||||
return this;
|
||||
}
|
||||
|
||||
/**
|
||||
* Raise the current result to the power of a number
|
||||
* @param {number} number - The exponent to raise to
|
||||
* @returns {Calculator} - Returns this instance for method chaining
|
||||
*/
|
||||
exp(number) {
|
||||
this.result = Math.pow(this.result, number);
|
||||
return this;
|
||||
}
|
||||
+++++++ REPLACE"
|
||||
]`;
|
||||
|
||||
// Test that the JSON string can be properly parsed and used
|
||||
let parsed;
|
||||
expect(() => {
|
||||
parsed = JSON.parse(userProblemDiff);
|
||||
}).not.toThrow();
|
||||
|
||||
expect(Array.isArray(parsed)).toBe(true);
|
||||
expect(parsed!.length).toBe(1);
|
||||
|
||||
const diffContent = parsed![0];
|
||||
expect(diffContent).toContain("------- SEARCH");
|
||||
expect(diffContent).toContain("=======");
|
||||
expect(diffContent).toContain("+++++++ REPLACE");
|
||||
expect(diffContent).toContain("Cannot divide by zero");
|
||||
});
|
||||
});
|
||||
|
|
@ -10,45 +10,13 @@ export const searchReplaceToolImpl: ClientToolImpl = async (
|
|||
toolCallId,
|
||||
extras,
|
||||
) => {
|
||||
const { filepath } = args;
|
||||
let { diffs } = args;
|
||||
|
||||
// Handle legacy parameter name and system message tools edge cases
|
||||
if (!diffs && (args as any).diff) {
|
||||
diffs = (args as any).diff;
|
||||
}
|
||||
|
||||
if (!diffs) {
|
||||
throw new Error("Missing required parameter: diffs (or diff)");
|
||||
}
|
||||
const { filepath, diffs } = args;
|
||||
|
||||
const state = extras.getState();
|
||||
const allowAnonymousTelemetry = state.config.config.allowAnonymousTelemetry;
|
||||
|
||||
const streamId = uuid();
|
||||
|
||||
// Handle the case where system message tools provide diffs as a JSON string
|
||||
// instead of an actual array (due to JSON parsing/stringifying in system message tools)
|
||||
let processedDiffs: string[];
|
||||
if (typeof diffs === "string") {
|
||||
try {
|
||||
const parsed = JSON.parse(diffs);
|
||||
if (Array.isArray(parsed)) {
|
||||
processedDiffs = parsed;
|
||||
} else {
|
||||
processedDiffs = [diffs];
|
||||
}
|
||||
} catch (e) {
|
||||
processedDiffs = [diffs];
|
||||
}
|
||||
} else if (Array.isArray(diffs)) {
|
||||
processedDiffs = diffs;
|
||||
} else {
|
||||
throw new Error(
|
||||
"diffs parameter must be an array of strings or a JSON string representing an array",
|
||||
);
|
||||
}
|
||||
|
||||
// Resolve the file path
|
||||
const resolvedFilepath = await resolveRelativePathInDir(
|
||||
filepath,
|
||||
|
|
@ -60,16 +28,11 @@ export const searchReplaceToolImpl: ClientToolImpl = async (
|
|||
|
||||
// Parse all search/replace blocks from all diff strings
|
||||
const allBlocks = [];
|
||||
for (let diffIndex = 0; diffIndex < processedDiffs.length; diffIndex++) {
|
||||
const blocks = parseAllSearchReplaceBlocks(processedDiffs[diffIndex]);
|
||||
for (let diffIndex = 0; diffIndex < diffs.length; diffIndex++) {
|
||||
const blocks = parseAllSearchReplaceBlocks(diffs[diffIndex]);
|
||||
if (blocks.length === 0) {
|
||||
throw new Error(
|
||||
`No complete search/replace blocks found in diff ${diffIndex + 1}. Each diff must contain at least one complete SEARCH/REPLACE block with the format:
|
||||
------- SEARCH
|
||||
[content to find]
|
||||
=======
|
||||
[replacement content]
|
||||
+++++++ REPLACE`,
|
||||
`No complete search/replace blocks found in diff ${diffIndex + 1}`,
|
||||
);
|
||||
}
|
||||
allBlocks.push(...blocks);
|
||||
|
|
|
|||
Loading…
Reference in New Issue