🩹 fix: Coerce Stringified edit_file Edits (JSON-in-JSON) (#14056)

Models sometimes pass edit_file's `edits` as a JSON-encoded string
(or stringify individual edit entries) instead of a real array. That
failed validation with "Provide old_text and new_text, or a non-empty
edits array" and forced a full retry round-trip.

normalizeEditArgs now JSON-parses a stringified `edits` value (and
stringified entries) before validating. Non-strings and unparseable
strings are left untouched, so the existing explicit errors still fire.
This commit is contained in:
Danny Avila 2026-07-01 12:41:26 -04:00 committed by GitHub
parent 89931baf22
commit 53ee82fe5d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 140 additions and 2 deletions

View file

@ -1544,6 +1544,127 @@ describe('createToolExecuteHandler', () => {
);
});
it('coerces a stringified edits array (JSON-in-JSON) so the edit still applies', async () => {
const saveSkillFileContent = jest.fn();
const handler = makeAuthoringHandler({
getSkillByName: jest.fn(async () => ({
_id: SKILL_ID,
name: 'edit-skill',
body: '# Existing',
fileCount: 1,
version: 1,
})),
getSkillFileByPath: jest.fn(async () => ({
content: 'hello old\n',
isBinary: false,
mimeType: 'text/markdown',
bytes: 10,
filepath: '/tmp/a.md',
source: 'local',
relativePath: 'references/a.md',
})),
saveSkillFileContent,
});
const [result] = await invokeHandler(handler, [
{
id: 'call_edit_file_stringified_array',
name: 'edit_file',
args: {
path: 'skills/edit-skill/references/a.md',
edits: JSON.stringify([{ old_text: 'hello old', new_text: 'hello new' }]),
},
},
]);
expect(result.status).toBe('success');
expect(result.artifact).toMatchObject({
path: 'skills/edit-skill/references/a.md',
edits: 1,
strategies: ['exact'],
});
expect(saveSkillFileContent).toHaveBeenCalledWith(
expect.objectContaining({ relativePath: 'references/a.md', content: 'hello new\n' }),
);
});
it('coerces stringified entries inside an edits array', async () => {
const saveSkillFileContent = jest.fn();
const handler = makeAuthoringHandler({
getSkillByName: jest.fn(async () => ({
_id: SKILL_ID,
name: 'edit-skill',
body: '# Existing',
fileCount: 1,
version: 1,
})),
getSkillFileByPath: jest.fn(async () => ({
content: 'hello old\n',
isBinary: false,
mimeType: 'text/markdown',
bytes: 10,
filepath: '/tmp/a.md',
source: 'local',
relativePath: 'references/a.md',
})),
saveSkillFileContent,
});
const [result] = await invokeHandler(handler, [
{
id: 'call_edit_file_stringified_entry',
name: 'edit_file',
args: {
path: 'skills/edit-skill/references/a.md',
edits: [JSON.stringify({ old_text: 'hello old', new_text: 'hello new' })],
},
},
]);
expect(result.status).toBe('success');
expect(saveSkillFileContent).toHaveBeenCalledWith(
expect.objectContaining({ relativePath: 'references/a.md', content: 'hello new\n' }),
);
});
it('still rejects an unparseable edits string with the explicit error', async () => {
const saveSkillFileContent = jest.fn();
const handler = makeAuthoringHandler({
getSkillByName: jest.fn(async () => ({
_id: SKILL_ID,
name: 'edit-skill',
body: '# Existing',
fileCount: 1,
version: 1,
})),
getSkillFileByPath: jest.fn(async () => ({
content: 'hello old\n',
isBinary: false,
mimeType: 'text/markdown',
bytes: 10,
filepath: '/tmp/a.md',
source: 'local',
relativePath: 'references/a.md',
})),
saveSkillFileContent,
});
const [result] = await invokeHandler(handler, [
{
id: 'call_edit_file_bad_edits',
name: 'edit_file',
args: {
path: 'skills/edit-skill/references/a.md',
edits: 'not valid json',
},
},
]);
expect(result.status).toBe('error');
expect(result.errorMessage).toContain('non-empty edits array');
expect(saveSkillFileContent).not.toHaveBeenCalled();
});
it('rejects bundled skill file writes when the skill version changed after reading', async () => {
const getSkillByName = jest
.fn()

View file

@ -793,14 +793,31 @@ function getAuthorInfo(req: ServerRequest): {
};
}
/* Models often stringify nested JSON (JSON-in-JSON) instead of passing a
real array/object, which would otherwise fail validation and cost a retry
round-trip. Parse a JSON string back to its value; leave non-strings and
unparseable strings untouched so the explicit errors below still fire. */
function coerceJsonValue(value: unknown): unknown {
if (typeof value !== 'string') {
return value;
}
try {
return JSON.parse(value);
} catch {
return value;
}
}
function normalizeEditArgs(args: {
old_text?: unknown;
new_text?: unknown;
edits?: unknown;
}): TextEdit[] | string {
if (Array.isArray(args.edits) && args.edits.length > 0) {
const coercedEdits = coerceJsonValue(args.edits);
if (Array.isArray(coercedEdits) && coercedEdits.length > 0) {
const edits: TextEdit[] = [];
for (const edit of args.edits) {
for (const rawEdit of coercedEdits) {
const edit = coerceJsonValue(rawEdit);
if (!edit || typeof edit !== 'object') {
return 'Each edit must be an object with old_text and new_text.';
}