Add move_file and delete_file, and flag a collapsed rewrite
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
co-authored by
Sisyphus
parent
6452299a45
commit
16d27e1611
+84
-2
@@ -249,6 +249,20 @@ export const applyPatchTool = tool({
|
|||||||
},
|
},
|
||||||
});
|
});
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A rewrite that collapses whitespace: similar character count, a fraction of the lines.
|
||||||
|
*
|
||||||
|
* A model under output pressure squeezes newlines and indentation before it cuts
|
||||||
|
* markup — the byte count stays close, the line count does not. That rewrite is
|
||||||
|
* rarely intended, so the result names it and the turn can fix it immediately.
|
||||||
|
*/
|
||||||
|
function collapsedRewrite(before: string, after: string): boolean {
|
||||||
|
if (before.length === 0) return false;
|
||||||
|
const ratio = after.length / before.length;
|
||||||
|
if (ratio < 0.5 || ratio > 1.5) return false;
|
||||||
|
return after.split('\n').length < before.split('\n').length / 2;
|
||||||
|
}
|
||||||
|
|
||||||
export const writeFileTool = tool({
|
export const writeFileTool = tool({
|
||||||
description: 'Create a file or overwrite it completely. Prefer edit_file for existing files.',
|
description: 'Create a file or overwrite it completely. Prefer edit_file for existing files.',
|
||||||
inputSchema: z.object({
|
inputSchema: z.object({
|
||||||
@@ -257,7 +271,16 @@ export const writeFileTool = tool({
|
|||||||
}),
|
}),
|
||||||
execute: async ({ path, content }) => {
|
execute: async ({ path, content }) => {
|
||||||
const abs = jail(path);
|
const abs = jail(path);
|
||||||
|
const before = await Bun.file(abs).exists() ? await Bun.file(abs).text() : undefined;
|
||||||
await Bun.write(abs, content);
|
await Bun.write(abs, content);
|
||||||
|
|
||||||
|
if (before !== undefined && collapsedRewrite(before, content)) {
|
||||||
|
const lines = content.split('\n').length;
|
||||||
|
return (
|
||||||
|
`Wrote ${content.length} chars to ${path}, but it collapsed ${before.split('\n').length} lines into ${lines}. ` +
|
||||||
|
'If that was not intended, re-send the content with its original newlines and indentation.'
|
||||||
|
);
|
||||||
|
}
|
||||||
return `Wrote ${content.length} chars to ${path}`;
|
return `Wrote ${content.length} chars to ${path}`;
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
@@ -662,6 +685,55 @@ export const bashTool = tool({
|
|||||||
},
|
},
|
||||||
});
|
});
|
||||||
|
|
||||||
|
export const moveFileTool = tool({
|
||||||
|
description:
|
||||||
|
'Move or rename one file. Creates the target directory. Refuses if the source is missing or the target ' +
|
||||||
|
'already exists, so a rename cannot silently overwrite work. For a rename plus its callers in one step, ' +
|
||||||
|
'use apply_patch.',
|
||||||
|
inputSchema: z.object({
|
||||||
|
from: z.string().describe('Existing file path'),
|
||||||
|
to: z.string().describe('New path, including the filename'),
|
||||||
|
}),
|
||||||
|
execute: async ({ from, to }) => {
|
||||||
|
const source = jail(from);
|
||||||
|
const target = jail(to);
|
||||||
|
if (source === target) throw new Error('from and to are the same path');
|
||||||
|
|
||||||
|
const file = Bun.file(source);
|
||||||
|
if (!(await file.exists())) throw new Error(`No such file: ${from}`);
|
||||||
|
if (await Bun.file(target).exists()) throw new Error(`${to} already exists. Delete it first or pick another name.`);
|
||||||
|
|
||||||
|
await Bun.write(target, file);
|
||||||
|
await file.delete();
|
||||||
|
return `Moved ${from} to ${to}`;
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
export const deleteFileTool = tool({
|
||||||
|
description:
|
||||||
|
'Delete one file. Refuses a directory: removing a tree is what the guard plugin blocks in bash, and it is ' +
|
||||||
|
'not something to do implicitly. Delete the files you mean, one call each.',
|
||||||
|
inputSchema: z.object({
|
||||||
|
path: z.string().describe('File to delete'),
|
||||||
|
}),
|
||||||
|
execute: async ({ path }) => {
|
||||||
|
const abs = jail(path);
|
||||||
|
|
||||||
|
// Bun.file on a directory reports exists() false, so the stat is what
|
||||||
|
// distinguishes "missing" from "a directory" and gives the right refusal.
|
||||||
|
let entry: Awaited<ReturnType<typeof stat>>;
|
||||||
|
try {
|
||||||
|
entry = await stat(abs);
|
||||||
|
} catch {
|
||||||
|
throw new Error(`No such file: ${path}`);
|
||||||
|
}
|
||||||
|
if (entry.isDirectory()) throw new Error(`${path} is a directory. Delete its files individually.`);
|
||||||
|
|
||||||
|
await Bun.file(abs).delete();
|
||||||
|
return `Deleted ${path} (${entry.size} bytes)`;
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
export const tools = {
|
export const tools = {
|
||||||
read_file: readFileTool,
|
read_file: readFileTool,
|
||||||
read_many_files: readManyFilesTool,
|
read_many_files: readManyFilesTool,
|
||||||
@@ -669,6 +741,8 @@ export const tools = {
|
|||||||
edit_file: editFileTool,
|
edit_file: editFileTool,
|
||||||
multi_edit: multiEditTool,
|
multi_edit: multiEditTool,
|
||||||
apply_patch: applyPatchTool,
|
apply_patch: applyPatchTool,
|
||||||
|
move_file: moveFileTool,
|
||||||
|
delete_file: deleteFileTool,
|
||||||
list_dir: listDirTool,
|
list_dir: listDirTool,
|
||||||
glob: globTool,
|
glob: globTool,
|
||||||
grep: grepTool,
|
grep: grepTool,
|
||||||
@@ -690,7 +764,7 @@ export const tools = {
|
|||||||
*/
|
*/
|
||||||
export const TOOL_SETS = {
|
export const TOOL_SETS = {
|
||||||
core: ['read_file', 'write_file', 'edit_file', 'glob', 'grep', 'bash'],
|
core: ['read_file', 'write_file', 'edit_file', 'glob', 'grep', 'bash'],
|
||||||
'edit-plus': ['multi_edit', 'list_dir', 'read_many_files', 'apply_patch'],
|
'edit-plus': ['multi_edit', 'list_dir', 'read_many_files', 'apply_patch', 'move_file', 'delete_file'],
|
||||||
git: GIT_TOOL_NAMES,
|
git: GIT_TOOL_NAMES,
|
||||||
net: NET_TOOL_NAMES,
|
net: NET_TOOL_NAMES,
|
||||||
} as const satisfies Record<string, readonly string[]>;
|
} as const satisfies Record<string, readonly string[]>;
|
||||||
@@ -722,6 +796,14 @@ export function disabledToolNames(enabled: readonly ToolSetName[] | undefined):
|
|||||||
}
|
}
|
||||||
|
|
||||||
/** Tools that mutate the workspace or run arbitrary code always ask the user first. */
|
/** Tools that mutate the workspace or run arbitrary code always ask the user first. */
|
||||||
export const MUTATING_TOOLS = ['write_file', 'edit_file', 'multi_edit', 'apply_patch', 'bash'] as const;
|
export const MUTATING_TOOLS = [
|
||||||
|
'write_file',
|
||||||
|
'edit_file',
|
||||||
|
'multi_edit',
|
||||||
|
'apply_patch',
|
||||||
|
'move_file',
|
||||||
|
'delete_file',
|
||||||
|
'bash',
|
||||||
|
] as const;
|
||||||
|
|
||||||
export { jail };
|
export { jail };
|
||||||
|
|||||||
@@ -5,17 +5,22 @@ import { join } from 'node:path';
|
|||||||
import {
|
import {
|
||||||
applyPatchTool,
|
applyPatchTool,
|
||||||
bashTool,
|
bashTool,
|
||||||
|
deleteFileTool,
|
||||||
editFileTool,
|
editFileTool,
|
||||||
globTool,
|
globTool,
|
||||||
grepTool,
|
grepTool,
|
||||||
interruptBash,
|
interruptBash,
|
||||||
jail,
|
jail,
|
||||||
listDirTool,
|
listDirTool,
|
||||||
|
moveFileTool,
|
||||||
multiEditTool,
|
multiEditTool,
|
||||||
|
MUTATING_TOOLS,
|
||||||
onBashOutput,
|
onBashOutput,
|
||||||
parsePatch,
|
parsePatch,
|
||||||
readFileTool,
|
readFileTool,
|
||||||
readManyFilesTool,
|
readManyFilesTool,
|
||||||
|
tools,
|
||||||
|
toolSetOf,
|
||||||
writeFileTool,
|
writeFileTool,
|
||||||
} from '../src/tools';
|
} from '../src/tools';
|
||||||
|
|
||||||
@@ -137,6 +142,99 @@ test('write_file then glob and grep find the content', async () => {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('overwriting a file with collapsed whitespace is flagged in the result', async () => {
|
||||||
|
const before = [
|
||||||
|
"@extends('layouts.app')",
|
||||||
|
"@section('content')",
|
||||||
|
'<section class="page-hero">',
|
||||||
|
' <div class="container">',
|
||||||
|
' <p>Explore homes</p>',
|
||||||
|
' </div>',
|
||||||
|
'</section>',
|
||||||
|
'@endsection',
|
||||||
|
'',
|
||||||
|
].join('\n');
|
||||||
|
await Bun.write(join(dir, 'index.blade.php'), before);
|
||||||
|
|
||||||
|
// What a compressed rewrite looks like: the same markup, most newlines gone.
|
||||||
|
const collapsed = before.replace(/\n\s*/g, '');
|
||||||
|
const out = await run(writeFileTool, { path: 'index.blade.php', content: collapsed });
|
||||||
|
|
||||||
|
expect(out).toContain('Wrote');
|
||||||
|
expect(out).toContain('newline');
|
||||||
|
expect(await Bun.file(join(dir, 'index.blade.php')).text()).toBe(collapsed);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('a normal rewrite is not flagged', async () => {
|
||||||
|
await Bun.write(join(dir, 'a.ts'), 'const a = 1;\nconst b = 2;\n');
|
||||||
|
const out = await run(writeFileTool, { path: 'a.ts', content: 'const a = 10;\nconst b = 20;\n' });
|
||||||
|
expect(out).toBe('Wrote 28 chars to a.ts');
|
||||||
|
|
||||||
|
// And a genuine deletion is not either: fewer lines is fine when the content
|
||||||
|
// is also much shorter — the flag is for whitespace collapse, not truncation.
|
||||||
|
await Bun.write(join(dir, 'b.ts'), 'line 1\nline 2\nline 3\n');
|
||||||
|
const short = await run(writeFileTool, { path: 'b.ts', content: 'line 1\n' });
|
||||||
|
expect(short).toBe('Wrote 7 chars to b.ts');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('move_file renames a file and creates the parent directory', async () => {
|
||||||
|
await Bun.write(join(dir, 'old.ts'), 'export const a = 1;\n');
|
||||||
|
|
||||||
|
const out = await run(moveFileTool, { from: 'old.ts', to: 'src/new.ts' });
|
||||||
|
|
||||||
|
expect(out).toContain('old.ts');
|
||||||
|
expect(out).toContain('src/new.ts');
|
||||||
|
expect(await Bun.file(join(dir, 'src/new.ts')).text()).toBe('export const a = 1;\n');
|
||||||
|
expect(await Bun.file(join(dir, 'old.ts')).exists()).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('move_file refuses a missing source and an occupied target', async () => {
|
||||||
|
await Bun.write(join(dir, 'one.ts'), 'a\n');
|
||||||
|
await Bun.write(join(dir, 'two.ts'), 'b\n');
|
||||||
|
|
||||||
|
expect(run(moveFileTool, { from: 'gone.ts', to: 'x.ts' })).rejects.toThrow(/no such file/i);
|
||||||
|
expect(run(moveFileTool, { from: 'one.ts', to: 'two.ts' })).rejects.toThrow(/already exists/i);
|
||||||
|
|
||||||
|
// Neither refusal may have touched anything.
|
||||||
|
expect(await Bun.file(join(dir, 'one.ts')).text()).toBe('a\n');
|
||||||
|
expect(await Bun.file(join(dir, 'two.ts')).text()).toBe('b\n');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('move_file refuses either path outside the workspace', async () => {
|
||||||
|
await Bun.write(join(dir, 'in.ts'), 'x\n');
|
||||||
|
expect(run(moveFileTool, { from: 'in.ts', to: '../escaped.ts' })).rejects.toThrow(/escapes workspace/);
|
||||||
|
expect(run(moveFileTool, { from: '../../etc/passwd', to: 'here.ts' })).rejects.toThrow(/escapes workspace/);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('delete_file removes one file and reports it', async () => {
|
||||||
|
await Bun.write(join(dir, 'gone.ts'), 'x\n');
|
||||||
|
|
||||||
|
const out = await run(deleteFileTool, { path: 'gone.ts' });
|
||||||
|
|
||||||
|
expect(out).toContain('gone.ts');
|
||||||
|
expect(await Bun.file(join(dir, 'gone.ts')).exists()).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('delete_file refuses a missing file, a directory, and an escaping path', async () => {
|
||||||
|
await Bun.write(join(dir, 'sub/keep.ts'), 'x\n');
|
||||||
|
|
||||||
|
expect(run(deleteFileTool, { path: 'nope.ts' })).rejects.toThrow(/no such file/i);
|
||||||
|
// A directory delete is recursive by nature, which is the one thing this must
|
||||||
|
// not do quietly: that is the guard plugin's `rm -rf` case.
|
||||||
|
expect(run(deleteFileTool, { path: 'sub' })).rejects.toThrow(/directory/i);
|
||||||
|
expect(run(deleteFileTool, { path: '../outside.ts' })).rejects.toThrow(/escapes workspace/);
|
||||||
|
|
||||||
|
expect(await Bun.file(join(dir, 'sub/keep.ts')).exists()).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('both new write tools are gated and belong to a set', () => {
|
||||||
|
for (const name of ['move_file', 'delete_file']) {
|
||||||
|
expect(MUTATING_TOOLS as readonly string[]).toContain(name);
|
||||||
|
expect(toolSetOf(name)).toBe('edit-plus');
|
||||||
|
expect(Object.keys(tools)).toContain(name);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
test('multi_edit applies every edit in order, each seeing the last', async () => {
|
test('multi_edit applies every edit in order, each seeing the last', async () => {
|
||||||
await Bun.write(join(dir, 'm.ts'), 'const a = 1;\nconst b = 2;\n');
|
await Bun.write(join(dir, 'm.ts'), 'const a = 1;\nconst b = 2;\n');
|
||||||
const out = await run(multiEditTool, {
|
const out = await run(multiEditTool, {
|
||||||
|
|||||||
Reference in New Issue
Block a user