Add apply_patch for atomic multi-file edits
One envelope carrying Add, Update, Move, and Delete file markers, validated in full before anything is written: a failure on the fourth file leaves the first three untouched. Permission rules match every path a patch touches, and the write guard and secret checks cover it alongside the other write tools. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
Muhammad Zakir Ramadhan
co-authored by
Sisyphus
Sisyphus
parent
87219350f9
commit
9e03512cb0
@@ -0,0 +1,142 @@
|
||||
import { expect, test } from 'bun:test';
|
||||
import { mkdtempSync, rmSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import { createHost } from '../src/plugins';
|
||||
import { BUILTIN_PLUGINS, DEFAULT_ENABLED, formatPlugin, secretsPlugin } from '../src/plugins-builtin';
|
||||
|
||||
const cwd = process.cwd();
|
||||
const check = (toolName: string, input: unknown) => secretsPlugin.beforeToolCall!({ toolName, input, cwd });
|
||||
|
||||
test('secrets is on by default, because an opt-in safety check is not one', () => {
|
||||
expect(DEFAULT_ENABLED).toContain('secrets');
|
||||
expect(DEFAULT_ENABLED).toContain('guard');
|
||||
// Both of these act on their own rather than refusing, so they are opt-in.
|
||||
expect(DEFAULT_ENABLED).not.toContain('bell');
|
||||
expect(DEFAULT_ENABLED).not.toContain('format');
|
||||
});
|
||||
|
||||
test('every builtin plugin has a unique name and a description', () => {
|
||||
const names = BUILTIN_PLUGINS.map((p) => p.name);
|
||||
expect(new Set(names).size).toBe(names.length);
|
||||
for (const p of BUILTIN_PLUGINS) expect(p.description.length, p.name).toBeGreaterThan(5);
|
||||
});
|
||||
|
||||
test('writing an env file is refused', async () => {
|
||||
for (const path of ['.env', 'config/.env', 'app/.env.production', '.env.local']) {
|
||||
expect(await check('write_file', { path }), path).toContain('refusing to write');
|
||||
}
|
||||
});
|
||||
|
||||
test('writing a key or certificate is refused', async () => {
|
||||
for (const path of ['certs/server.pem', 'app.key', 'store.p12', 'keys/id_rsa', '.ssh/id_ed25519']) {
|
||||
expect(await check('write_file', { path }), path).toContain('refusing to write');
|
||||
}
|
||||
});
|
||||
|
||||
test('writing a registry or credential file is refused', async () => {
|
||||
for (const path of ['.npmrc', '.netrc', 'credentials.json', 'secrets.yaml', '.aws/config']) {
|
||||
expect(await check('write_file', { path }), path).toContain('refusing to write');
|
||||
}
|
||||
});
|
||||
|
||||
test('every write tool is covered, including apply_patch', async () => {
|
||||
for (const tool of ['write_file', 'edit_file', 'multi_edit']) {
|
||||
expect(await check(tool, { path: '.env' }), tool).toContain('refusing to write');
|
||||
}
|
||||
|
||||
// A patch carries its paths in markers, so a guard that only reads `path` is
|
||||
// bypassed by the one tool that can touch several files at once.
|
||||
const patch = '*** Update File: src/app.ts\n-a\n+b\n*** Add File: .env\n+SECRET=x';
|
||||
expect(await check('apply_patch', { patch })).toContain('refusing to write');
|
||||
expect(await check('apply_patch', { patch: '*** Move to: .env.production\n' })).toContain('refusing to write');
|
||||
});
|
||||
|
||||
test('ordinary source files are untouched', async () => {
|
||||
for (const path of ['src/app.ts', 'README.md', 'docs/env-vars.md', 'test/environment.test.ts', 'keychain.ts']) {
|
||||
expect(await check('write_file', { path }), path).toBeUndefined();
|
||||
}
|
||||
});
|
||||
|
||||
test('an example env file may be written, since it holds no secret', async () => {
|
||||
// The permission defaults allow *reading* .env.example for the same reason.
|
||||
expect(await check('write_file', { path: '.env.example' })).toBeUndefined();
|
||||
});
|
||||
|
||||
test('reading is not the secrets plugin′s business', async () => {
|
||||
// Reads are refused by the permission defaults instead, so this must not also
|
||||
// block them: two mechanisms refusing the same thing means one is dead code.
|
||||
expect(await check('read_file', { path: '.env' })).toBeUndefined();
|
||||
expect(await check('bash', { command: 'cat .env' })).toBeUndefined();
|
||||
});
|
||||
|
||||
test('the appendix tells the model what to do instead', () => {
|
||||
expect(secretsPlugin.appendix).toContain('let them write it');
|
||||
});
|
||||
|
||||
test('the guard and secrets both run, and the first refusal wins', async () => {
|
||||
const host = createHost(BUILTIN_PLUGINS.filter((p) => DEFAULT_ENABLED.includes(p.name)));
|
||||
|
||||
expect(await host.guard({ toolName: 'bash', input: { command: 'rm -rf /' }, cwd })).toContain('guard plugin');
|
||||
expect(await host.guard({ toolName: 'write_file', input: { path: '.env' }, cwd })).toContain('secrets plugin');
|
||||
expect(await host.guard({ toolName: 'write_file', input: { path: 'src/app.ts' }, cwd })).toBeUndefined();
|
||||
});
|
||||
|
||||
function inTempDir<T>(fn: (dir: string) => Promise<T>): Promise<T> {
|
||||
const orig = process.cwd();
|
||||
const dir = mkdtempSync(join(tmpdir(), 'shiro-fmt-'));
|
||||
process.chdir(dir);
|
||||
return fn(dir).finally(() => {
|
||||
process.chdir(orig);
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
}
|
||||
|
||||
test('format runs the script the project already defines', async () =>
|
||||
inTempDir(async (dir) => {
|
||||
// A script that touches a file, so the assertion is that the formatter ran
|
||||
// rather than that some formatter binary happens to be installed.
|
||||
await Bun.write(join(dir, 'marker.js'), 'require("fs").writeFileSync("formatted.txt", "yes")');
|
||||
await Bun.write(join(dir, 'package.json'), JSON.stringify({ scripts: { format: 'node marker.js' } }));
|
||||
|
||||
await formatPlugin.afterTurn!();
|
||||
expect(await Bun.file(join(dir, 'formatted.txt')).text()).toBe('yes');
|
||||
}), 30_000);
|
||||
|
||||
test('format does nothing when the project defines no formatter', async () =>
|
||||
inTempDir(async (dir) => {
|
||||
await Bun.write(join(dir, 'package.json'), JSON.stringify({ scripts: { test: 'bun test' } }));
|
||||
// No throw and no side effect: a project without a format script gets nothing.
|
||||
await formatPlugin.afterTurn!();
|
||||
expect(await Bun.file(join(dir, 'formatted.txt')).exists()).toBe(false);
|
||||
}), 30_000);
|
||||
|
||||
test('format survives a manifest it cannot parse', async () =>
|
||||
inTempDir(async (dir) => {
|
||||
await Bun.write(join(dir, 'package.json'), '{ not json');
|
||||
await formatPlugin.afterTurn!();
|
||||
expect(true).toBe(true);
|
||||
}), 30_000);
|
||||
|
||||
test('format does nothing in a directory with no manifest at all', async () =>
|
||||
inTempDir(async () => {
|
||||
await formatPlugin.afterTurn!();
|
||||
expect(true).toBe(true);
|
||||
}), 30_000);
|
||||
|
||||
test('a throwing afterTurn does not stop the other plugins', async () => {
|
||||
let ran = 0;
|
||||
const host = createHost([
|
||||
{
|
||||
name: 'broken',
|
||||
description: 'throws',
|
||||
afterTurn: () => {
|
||||
throw new Error('boom');
|
||||
},
|
||||
},
|
||||
{ name: 'counter', description: 'counts', afterTurn: () => void ran++ },
|
||||
]);
|
||||
|
||||
await host.afterTurn();
|
||||
expect(ran).toBe(1);
|
||||
});
|
||||
+133
-1
@@ -3,6 +3,7 @@ import { mkdtempSync, rmSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import {
|
||||
applyPatchTool,
|
||||
bashTool,
|
||||
editFileTool,
|
||||
globTool,
|
||||
@@ -12,6 +13,7 @@ import {
|
||||
listDirTool,
|
||||
multiEditTool,
|
||||
onBashOutput,
|
||||
parsePatch,
|
||||
readFileTool,
|
||||
readManyFilesTool,
|
||||
writeFileTool,
|
||||
@@ -229,6 +231,134 @@ test('list_dir scopes to a subdirectory and refuses a file', async () => {
|
||||
expect(run(listDirTool, { path: 'other.ts' })).rejects.toThrow(/Not a directory/);
|
||||
});
|
||||
|
||||
const PATCH_ADD = `*** Add File: src/new.ts
|
||||
+export const a = 1;
|
||||
+export const b = 2;`;
|
||||
|
||||
test('parsePatch reads add, update, move, and delete markers', () => {
|
||||
const ops = parsePatch(`${PATCH_ADD}
|
||||
*** Update File: src/old.ts
|
||||
*** Move to: src/renamed.ts
|
||||
-const port = 8080;
|
||||
+const port = 9090;
|
||||
*** Delete File: src/gone.ts`);
|
||||
|
||||
expect(ops).toEqual([
|
||||
{ kind: 'add', path: 'src/new.ts', content: 'export const a = 1;\nexport const b = 2;' },
|
||||
{
|
||||
kind: 'update',
|
||||
path: 'src/old.ts',
|
||||
moveTo: 'src/renamed.ts',
|
||||
oldString: 'const port = 8080;',
|
||||
newString: 'const port = 9090;',
|
||||
},
|
||||
{ kind: 'delete', path: 'src/gone.ts' },
|
||||
]);
|
||||
});
|
||||
|
||||
test('parsePatch rejects a malformed envelope rather than guessing', () => {
|
||||
expect(() => parsePatch('')).toThrow(/empty/);
|
||||
expect(() => parsePatch('just some text')).toThrow(/not a marker/);
|
||||
expect(() => parsePatch('*** Update File: a.ts\n+only additions')).toThrow(/at least one - line/);
|
||||
expect(() => parsePatch('*** Add File: a.ts\n-removing')).toThrow(/cannot remove lines/);
|
||||
expect(() => parsePatch('*** Add File: a.ts\n*** Move to: b.ts\n+x')).toThrow(/only valid on an Update/);
|
||||
expect(() => parsePatch('*** Update File: a.ts\nno prefix here')).toThrow(/neither \+ nor -/);
|
||||
});
|
||||
|
||||
test('apply_patch adds, updates, moves, and deletes in one call', async () => {
|
||||
await Bun.write(join(dir, 'old.ts'), 'const port = 8080;\n');
|
||||
await Bun.write(join(dir, 'gone.ts'), 'obsolete\n');
|
||||
await Bun.write(join(dir, 'moving.ts'), 'const name = "a";\n');
|
||||
|
||||
const out = await run(applyPatchTool, {
|
||||
patch: `*** Add File: fresh.ts
|
||||
+export const fresh = true;
|
||||
*** Update File: old.ts
|
||||
-const port = 8080;
|
||||
+const port = 9090;
|
||||
*** Update File: moving.ts
|
||||
*** Move to: moved.ts
|
||||
-const name = "a";
|
||||
+const name = "b";
|
||||
*** Delete File: gone.ts`,
|
||||
});
|
||||
|
||||
expect(out).toContain('4 changes');
|
||||
expect(await Bun.file(join(dir, 'fresh.ts')).text()).toBe('export const fresh = true;\n');
|
||||
expect(await Bun.file(join(dir, 'old.ts')).text()).toBe('const port = 9090;\n');
|
||||
expect(await Bun.file(join(dir, 'moved.ts')).text()).toBe('const name = "b";\n');
|
||||
expect(await Bun.file(join(dir, 'moving.ts')).exists()).toBe(false);
|
||||
expect(await Bun.file(join(dir, 'gone.ts')).exists()).toBe(false);
|
||||
});
|
||||
|
||||
test('a patch that fails on its third file writes nothing at all', async () => {
|
||||
await Bun.write(join(dir, 'one.ts'), 'const a = 1;\n');
|
||||
await Bun.write(join(dir, 'two.ts'), 'const b = 2;\n');
|
||||
|
||||
expect(
|
||||
run(applyPatchTool, {
|
||||
patch: `*** Update File: one.ts
|
||||
-const a = 1;
|
||||
+const a = 10;
|
||||
*** Update File: two.ts
|
||||
-const b = 2;
|
||||
+const b = 20;
|
||||
*** Update File: three.ts
|
||||
-const c = 3;
|
||||
+const c = 30;`,
|
||||
}),
|
||||
).rejects.toThrow(/no such file/);
|
||||
|
||||
await Bun.sleep(20);
|
||||
// Atomicity is the whole reason this tool exists rather than three edit_file
|
||||
// calls, so the first two files must be untouched.
|
||||
expect(await Bun.file(join(dir, 'one.ts')).text()).toBe('const a = 1;\n');
|
||||
expect(await Bun.file(join(dir, 'two.ts')).text()).toBe('const b = 2;\n');
|
||||
});
|
||||
|
||||
test('apply_patch refuses an ambiguous match without writing', async () => {
|
||||
await Bun.write(join(dir, 'dup.ts'), 'x\nx\n');
|
||||
expect(run(applyPatchTool, { patch: '*** Update File: dup.ts\n-x\n+y' })).rejects.toThrow(/appear 2 times/);
|
||||
await Bun.sleep(20);
|
||||
expect(await Bun.file(join(dir, 'dup.ts')).text()).toBe('x\nx\n');
|
||||
});
|
||||
|
||||
test('apply_patch refuses to add over an existing file', async () => {
|
||||
await Bun.write(join(dir, 'here.ts'), 'original\n');
|
||||
expect(run(applyPatchTool, { patch: '*** Add File: here.ts\n+replacement' })).rejects.toThrow(/already exists/);
|
||||
await Bun.sleep(20);
|
||||
expect(await Bun.file(join(dir, 'here.ts')).text()).toBe('original\n');
|
||||
});
|
||||
|
||||
test('apply_patch refuses to move onto an existing file', async () => {
|
||||
await Bun.write(join(dir, 'from.ts'), 'const a = 1;\n');
|
||||
await Bun.write(join(dir, 'to.ts'), 'occupied\n');
|
||||
|
||||
expect(
|
||||
run(applyPatchTool, { patch: '*** Update File: from.ts\n*** Move to: to.ts\n-const a = 1;\n+const a = 2;' }),
|
||||
).rejects.toThrow(/already exists/);
|
||||
await Bun.sleep(20);
|
||||
expect(await Bun.file(join(dir, 'from.ts')).text()).toBe('const a = 1;\n');
|
||||
expect(await Bun.file(join(dir, 'to.ts')).text()).toBe('occupied\n');
|
||||
});
|
||||
|
||||
test('apply_patch refuses the same file twice in one patch', async () => {
|
||||
await Bun.write(join(dir, 'twice.ts'), 'a\nb\n');
|
||||
expect(
|
||||
run(applyPatchTool, { patch: '*** Update File: twice.ts\n-a\n+A\n*** Update File: twice.ts\n-b\n+B' }),
|
||||
).rejects.toThrow(/appears twice/);
|
||||
});
|
||||
|
||||
test('apply_patch refuses a path outside the workspace', async () => {
|
||||
expect(run(applyPatchTool, { patch: '*** Add File: ../escape.ts\n+x' })).rejects.toThrow(/escapes workspace/);
|
||||
});
|
||||
|
||||
test('an update that removes lines and adds none deletes them', async () => {
|
||||
await Bun.write(join(dir, 'trim.ts'), 'keep\nremove me\nkeep too\n');
|
||||
await run(applyPatchTool, { patch: '*** Update File: trim.ts\n-remove me\n' });
|
||||
expect(await Bun.file(join(dir, 'trim.ts')).text()).toBe('keep\n\nkeep too\n');
|
||||
});
|
||||
|
||||
test('glob skips gitignored paths and honours includeIgnored', async () => {
|
||||
await Bun.write(join(dir, '.gitignore'), 'dist/\n');
|
||||
await Bun.write(join(dir, 'dist/app.js'), 'x');
|
||||
@@ -293,7 +423,9 @@ test('bash streams output to the listener before the command exits', async () =>
|
||||
|
||||
const script =
|
||||
process.platform === 'win32'
|
||||
? 'echo first && ping -n 2 127.0.0.1 > nul && echo second'
|
||||
? // Unconditional sequencing: a blocked loopback makes `ping` exit 1, and `&&`
|
||||
// would then skip `echo second` on machines where ICMP is filtered.
|
||||
'echo first & ping -n 2 127.0.0.1 > nul & echo second'
|
||||
: 'echo first; sleep 0.4; echo second';
|
||||
await run(bashTool, { command: script });
|
||||
|
||||
|
||||
Reference in New Issue
Block a user