From 16d27e16117c573ecf1069ac35987734ae2d9c9b Mon Sep 17 00:00:00 2001 From: Muhammad Zakir Ramadhan <61570975+zakirkun@users.noreply.github.com> Date: Sat, 5 Sep 2026 17:26:49 +0700 Subject: [PATCH] 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 --- src/tools.ts | 86 +++++++++++++++++++++++++++++++++++++++- test/tools.test.ts | 98 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 2 deletions(-) diff --git a/src/tools.ts b/src/tools.ts index 91e01ec..121b746 100644 --- a/src/tools.ts +++ b/src/tools.ts @@ -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({ description: 'Create a file or overwrite it completely. Prefer edit_file for existing files.', inputSchema: z.object({ @@ -257,7 +271,16 @@ export const writeFileTool = tool({ }), execute: async ({ path, content }) => { const abs = jail(path); + const before = await Bun.file(abs).exists() ? await Bun.file(abs).text() : undefined; 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}`; }, }); @@ -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>; + 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 = { read_file: readFileTool, read_many_files: readManyFilesTool, @@ -669,6 +741,8 @@ export const tools = { edit_file: editFileTool, multi_edit: multiEditTool, apply_patch: applyPatchTool, + move_file: moveFileTool, + delete_file: deleteFileTool, list_dir: listDirTool, glob: globTool, grep: grepTool, @@ -690,7 +764,7 @@ export const tools = { */ export const TOOL_SETS = { 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, net: NET_TOOL_NAMES, } as const satisfies Record; @@ -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. */ -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 }; diff --git a/test/tools.test.ts b/test/tools.test.ts index 006a77b..8fa63a9 100644 --- a/test/tools.test.ts +++ b/test/tools.test.ts @@ -5,17 +5,22 @@ import { join } from 'node:path'; import { applyPatchTool, bashTool, + deleteFileTool, editFileTool, globTool, grepTool, interruptBash, jail, listDirTool, + moveFileTool, multiEditTool, + MUTATING_TOOLS, onBashOutput, parsePatch, readFileTool, readManyFilesTool, + tools, + toolSetOf, writeFileTool, } 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')", + '
', + '
', + '

Explore homes

', + '
', + '
', + '@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 () => { await Bun.write(join(dir, 'm.ts'), 'const a = 1;\nconst b = 2;\n'); const out = await run(multiEditTool, {