From 1458861fd22415f160f577ea51944f7aa80437d8 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 13 Apr 2026 21:56:55 +0100 Subject: [PATCH] fix: validate individual edit entries in parseEditFilesArgs (#24301) --- .../ChatElements/runtimeTypeUtils.ts | 11 ++ .../ChatElements/tools/utils.test.ts | 126 ++++++++++++++++++ .../components/ChatElements/tools/utils.ts | 32 +++-- 3 files changed, 161 insertions(+), 8 deletions(-) diff --git a/site/src/pages/AgentsPage/components/ChatElements/runtimeTypeUtils.ts b/site/src/pages/AgentsPage/components/ChatElements/runtimeTypeUtils.ts index 50f47e6c47..add1ec0510 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/runtimeTypeUtils.ts +++ b/site/src/pages/AgentsPage/components/ChatElements/runtimeTypeUtils.ts @@ -1,3 +1,5 @@ +import type { Schema } from "yup"; + export const asRecord = (value: unknown): Record | null => { if (!value || typeof value !== "object" || Array.isArray(value)) { return null; @@ -8,6 +10,15 @@ export const asRecord = (value: unknown): Record | null => { export const asString = (value: unknown): string => typeof value === "string" ? value : ""; +/** + * Type-narrowing wrapper around a Yup schema. Returns `true` + * (and narrows `value` to `T`) when `value` satisfies the + * schema. Strict mode is always enabled to prevent silent + * type coercion. + */ +export const isValid = (schema: Schema, value: unknown): value is T => + schema.isValidSync(value, { strict: true }); + export const asNumber = ( value: unknown, options?: { readonly parseString?: boolean }, diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts index 111479b752..883b03eeb4 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts @@ -495,6 +495,7 @@ describe("parseEditFilesArgs", () => { { path: "a.ts", edits: [{ search: "x", replace: "y" }] }, { path: 42, edits: [] }, // invalid: path not string null, // invalid: null + undefined, // invalid: undefined { path: "b.ts" }, // invalid: no edits array { path: "c.ts", edits: [{ search: "a", replace: "b" }] }, ], @@ -513,6 +514,131 @@ describe("parseEditFilesArgs", () => { expect(result).toHaveLength(1); expect(result[0].path).toBe("test.ts"); }); + + it("filters out edits with non-string search or replace", () => { + const args = { + files: [ + { + path: "a.ts", + edits: [ + { search: "x", replace: "y" }, + { search: "a" }, // missing replace + { replace: "b" }, // missing search + { search: 42, replace: "c" }, // non-string search + { search: "d", replace: null }, // non-string replace + null, // null edit + ], + }, + ], + }; + const result = parseEditFilesArgs(args); + expect(result).toHaveLength(1); + expect(result[0].edits).toHaveLength(1); + expect(result[0].edits[0]).toEqual({ search: "x", replace: "y" }); + }); + + // Yup.object() is optional by default, so undefined passes + // isValidSync in strict mode. Without .required() on the + // schemas, undefined entries survive the filter and crash + // the subsequent .map() accessing f.path. + it("rejects undefined file entries and edits", () => { + const args = { + files: [ + undefined, + { + path: "a.ts", + edits: [undefined, { search: "x", replace: "y" }], + }, + ], + }; + const result = parseEditFilesArgs(args); + expect(result).toHaveLength(1); + expect(result[0].path).toBe("a.ts"); + expect(result[0].edits).toHaveLength(1); + expect(result[0].edits[0]).toEqual({ search: "x", replace: "y" }); + }); + + // Regression: a partial edit with a missing replace field caused + // Diff.createPatch to crash inside its tokenize method with + // "Cannot read properties of undefined (reading 'split')". + // This reproduces the exact call path from Tool.tsx: + // parseEditFilesArgs(args) -> buildEditDiff(file.path, file.edits). + it("does not crash buildEditDiff when edits have missing replace", () => { + const args = { + files: [ + { + path: "src/app.ts", + edits: [ + { search: "const x = 1;", replace: "const x = 2;" }, + { search: "const y = 3;" }, // streamed edit, replace not yet present + ], + }, + ], + }; + const parsed = parseEditFilesArgs(args); + expect(parsed).toHaveLength(1); + // The incomplete edit should be filtered out, leaving only + // the valid one so buildEditDiff never sees undefined. + expect(parsed[0].edits).toHaveLength(1); + const diff = buildEditDiff(parsed[0].path, parsed[0].edits); + expect(diff).not.toBeNull(); + }); + + // search uses required() (rejects "") while replace uses + // defined() (allows ""). This asymmetry is intentional: + // empty search is meaningless, empty replace is a deletion. + it("rejects edits with empty-string search", () => { + const args = { + files: [ + { + path: "a.ts", + edits: [ + { search: "", replace: "new" }, + { search: "valid", replace: "also valid" }, + ], + }, + ], + }; + const result = parseEditFilesArgs(args); + expect(result).toHaveLength(1); + expect(result[0].edits).toHaveLength(1); + expect(result[0].edits[0].search).toBe("valid"); + }); + + it("preserves edits with empty-string replace (deletion)", () => { + const args = { + files: [ + { + path: "src/app.ts", + edits: [{ search: "const old = 1;", replace: "" }], + }, + ], + }; + const parsed = parseEditFilesArgs(args); + expect(parsed).toHaveLength(1); + expect(parsed[0].edits).toHaveLength(1); + expect(parsed[0].edits[0].replace).toBe(""); + }); + + // During streaming the model may emit a file entry before any + // edit is complete. Every edit has a missing replace, so all are + // filtered out. The file entry survives with an empty edits + // array and buildEditDiff returns null. + it("returns file entry with empty edits when all edits are invalid", () => { + const args = { + files: [ + { + path: "src/app.ts", + edits: [{ search: "const x = 1;" }, { search: "const y = 2;" }], + }, + ], + }; + const parsed = parseEditFilesArgs(args); + expect(parsed).toHaveLength(1); + expect(parsed[0].path).toBe("src/app.ts"); + expect(parsed[0].edits).toHaveLength(0); + expect(buildEditDiff(parsed[0].path, parsed[0].edits)).toBeNull(); + }); }); describe("buildEditDiff", () => { diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts index 804a1bd2ed..1fd11a3591 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts @@ -2,7 +2,8 @@ import type { FileDiffMetadata } from "@pierre/diffs"; import { parsePatchFiles } from "@pierre/diffs"; import * as Diff from "diff"; import type React from "react"; -import { asRecord, asString } from "../runtimeTypeUtils"; +import * as Yup from "yup"; +import { asRecord, asString, isValid } from "../runtimeTypeUtils"; export type ToolStatus = "completed" | "error" | "running"; @@ -11,6 +12,20 @@ export interface EditFilesFileEntry { edits: Array<{ search: string; replace: string }>; } +const searchReplaceSchema = Yup.object({ + search: Yup.string().required(), + replace: Yup.string().defined(), +}).required(); + +type SearchReplace = Yup.InferType; + +const fileEntrySchema = Yup.object({ + path: Yup.string().required(), + edits: Yup.array().defined(), +}).required(); + +type FileEntry = Yup.InferType; + export const toProviderLabel = ( providerDisplayName: string, providerID: string, @@ -515,13 +530,14 @@ export const parseEditFilesArgs = (args: unknown): EditFilesFileEntry[] => { if (!parsed) return []; const files = parsed.files; if (!Array.isArray(files)) return []; - return files.filter( - (f): f is EditFilesFileEntry => - f !== null && - typeof f === "object" && - typeof (f as Record).path === "string" && - Array.isArray((f as Record).edits), - ); + return files + .filter((f): f is FileEntry => isValid(fileEntrySchema, f)) + .map((f) => ({ + path: f.path, + edits: f.edits.filter((e): e is SearchReplace => + isValid(searchReplaceSchema, e), + ), + })); }; /**