diff --git a/src/plugins-builtin.ts b/src/plugins-builtin.ts index 672ce7a..63c0d58 100644 --- a/src/plugins-builtin.ts +++ b/src/plugins-builtin.ts @@ -1,5 +1,6 @@ import { tool } from 'ai'; import { z } from 'zod'; +import { posix } from './ignore'; import type { Plugin } from './plugins'; /** @@ -87,7 +88,7 @@ const SECRET_PATHS: { re: RegExp; why: string }[] = [ { re: /(^|[\\/])\.gnupg[\\/]/i, why: 'a GPG directory' }, ]; -/** Every path a write tool might carry, including a patch's markers. */ +/** Every path a write tool might carry, including a patch's markers and a move's ends. */ function writtenPaths(toolName: string, input: unknown): string[] { const o = (input ?? {}) as Record; @@ -99,9 +100,16 @@ function writtenPaths(toolName: string, input: unknown): string[] { ]; } + if (toolName === 'move_file') { + return [o['from'], o['to']].filter((p): p is string => typeof p === 'string'); + } + return typeof o['path'] === 'string' ? [o['path']] : []; } +/** Write tools a path-based guard has to cover. Missing one is a silent bypass. */ +const WRITE_TOOLS = ['write_file', 'edit_file', 'multi_edit', 'apply_patch', 'move_file', 'delete_file']; + export const secretsPlugin: Plugin = { name: 'secrets', description: 'refuses to write credential files', @@ -110,7 +118,7 @@ export const secretsPlugin: Plugin = { 'user which file and which key, and let them write it themselves. Do not work around the refusal by writing ' + 'the same content somewhere else.', beforeToolCall: ({ toolName, input }) => { - if (!['write_file', 'edit_file', 'multi_edit', 'apply_patch'].includes(toolName)) return undefined; + if (!WRITE_TOOLS.includes(toolName)) return undefined; for (const path of writtenPaths(toolName, input)) { for (const { re, why } of SECRET_PATHS) { @@ -123,6 +131,49 @@ export const secretsPlugin: Plugin = { }, }; +/** + * Paths a write must not touch, for reasons other than secrecy. + * + * These are not credentials, so the secrets plugin has nothing to say about them. + * They are files whose contents are owned by a tool rather than by anyone editing + * them by hand: git's own object store, a resolver's lockfile, an installed + * dependency tree, a build directory. A model editing one of these produces a + * repository that looks fine and behaves wrongly, and the failure surfaces + * somewhere else entirely. + */ +const PROTECTED_PATHS: { re: RegExp; why: string }[] = [ + { re: /(^|[\\/])\.git[\\/]/i, why: "git's own object store" }, + { + re: /(^|[\\/])(bun\.lock|bun\.lockb|package-lock\.json|pnpm-lock\.yaml|yarn\.lock|Cargo\.lock|poetry\.lock|uv\.lock|composer\.lock|go\.sum|Gemfile\.lock)$/i, + why: 'a lockfile the package manager owns', + }, + { re: /(^|[\\/])node_modules[\\/]/i, why: 'an installed dependency' }, + { re: /(^|[\\/])(vendor|target[\\/]debug|target[\\/]release)[\\/]/i, why: 'a vendored or build directory' }, + { re: /(^|[\\/])(dist|build|out|\.next|\.nuxt|\.svelte-kit|coverage)[\\/]/i, why: 'generated build output' }, + { re: /(^|[\\/])\.(venv|tox|mypy_cache|pytest_cache|ruff_cache|turbo|parcel-cache)[\\/]/i, why: 'a tool cache' }, +]; + +export const protectPlugin: Plugin = { + name: 'protect', + description: 'refuses writes to lockfiles, .git, dependencies, and build output', + appendix: + 'The protect plugin refuses writes to .git, lockfiles, node_modules, vendored code, and build output. A ' + + 'lockfile is regenerated by its package manager: run the install or update command through bash instead of ' + + 'editing the file. Generated output is regenerated by its build. Do not route around the refusal.', + beforeToolCall: ({ toolName, input }) => { + if (!WRITE_TOOLS.includes(toolName)) return undefined; + + for (const path of writtenPaths(toolName, input)) { + for (const { re, why } of PROTECTED_PATHS) { + if (re.test(posix(path))) { + return `refusing to write ${path} (${why}). Regenerate it with the tool that owns it rather than editing it.`; + } + } + } + return undefined; + }, +}; + const FORMATTERS: { file: string; script: string; command: string[] }[] = [ { file: 'package.json', script: 'format', command: ['bun', 'run', 'format'] }, { file: 'Cargo.toml', script: '', command: ['cargo', 'fmt'] }, @@ -167,15 +218,22 @@ export const formatPlugin: Plugin = { }, }; -export const BUILTIN_PLUGINS: Plugin[] = [guardPlugin, secretsPlugin, bellPlugin, timePlugin, formatPlugin]; +export const BUILTIN_PLUGINS: Plugin[] = [ + guardPlugin, + secretsPlugin, + protectPlugin, + bellPlugin, + timePlugin, + formatPlugin, +]; /** * Enabled unless the config turns them off. * - * `guard` and `secrets` are refusals, so they are on: a user who has to opt into a - * safety check does not have it. `bell` and `format` both act on their own — one - * makes noise, the other writes files — so they are opt-in. + * `guard`, `secrets`, and `protect` are refusals, so they are on: a user who has to + * opt into a safety check does not have it. `bell` and `format` both act on their + * own — one makes noise, the other writes files — so they are opt-in. */ -export const DEFAULT_ENABLED = ['guard', 'secrets', 'time']; +export const DEFAULT_ENABLED = ['guard', 'secrets', 'protect', 'time']; -export { DESTRUCTIVE, SECRET_PATHS }; +export { DESTRUCTIVE, SECRET_PATHS, PROTECTED_PATHS }; diff --git a/test/plugins-builtin.test.ts b/test/plugins-builtin.test.ts index 8f69399..59d61ab 100644 --- a/test/plugins-builtin.test.ts +++ b/test/plugins-builtin.test.ts @@ -3,14 +3,16 @@ 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'; +import { BUILTIN_PLUGINS, DEFAULT_ENABLED, formatPlugin, protectPlugin, secretsPlugin } from '../src/plugins-builtin'; const cwd = process.cwd(); const check = (toolName: string, input: unknown) => secretsPlugin.beforeToolCall!({ toolName, input, cwd }); +const guarded = (toolName: string, input: unknown) => protectPlugin.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'); + expect(DEFAULT_ENABLED).toContain('protect'); // 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'); @@ -41,7 +43,7 @@ test('writing a registry or credential file is refused', async () => { }); test('every write tool is covered, including apply_patch', async () => { - for (const tool of ['write_file', 'edit_file', 'multi_edit']) { + for (const tool of ['write_file', 'edit_file', 'multi_edit', 'delete_file']) { expect(await check(tool, { path: '.env' }), tool).toContain('refusing to write'); } @@ -50,6 +52,59 @@ test('every write tool is covered, including apply_patch', async () => { 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'); + + // A move carries two paths and neither is called `path`. + expect(await check('move_file', { from: 'src/app.ts', to: '.env' })).toContain('refusing to write'); + expect(await check('move_file', { from: '.env', to: 'src/app.ts' })).toContain('refusing to write'); +}); + +test('protect refuses git internals, lockfiles, dependencies, and build output', async () => { + for (const path of [ + '.git/config', + '.git/hooks/pre-commit', + 'bun.lock', + 'package-lock.json', + 'Cargo.lock', + 'go.sum', + 'node_modules/react/index.js', + 'vendor/github.com/pkg/errors.go', + 'dist/bundle.js', + 'coverage/lcov.info', + '.next/build-manifest.json', + ]) { + expect(await guarded('write_file', { path }), path).toContain('refusing to write'); + } +}); + +test('protect covers every write tool and both ends of a move', async () => { + for (const tool of ['write_file', 'edit_file', 'multi_edit', 'delete_file']) { + expect(await guarded(tool, { path: 'bun.lock' }), tool).toContain('refusing to write'); + } + expect(await guarded('apply_patch', { patch: '*** Update File: bun.lock\n-a\n+b' })).toContain('refusing to write'); + expect(await guarded('move_file', { from: 'src/a.ts', to: 'node_modules/a.ts' })).toContain('refusing to write'); +}); + +test('protect matches a Windows path separator as well', async () => { + expect(await guarded('write_file', { path: 'node_modules\\react\\index.js' })).toContain('refusing to write'); + expect(await guarded('write_file', { path: '.git\\config' })).toContain('refusing to write'); +}); + +test('protect leaves ordinary files and lookalike names alone', async () => { + for (const path of [ + 'src/app.ts', + 'docs/dist-layout.md', + 'src/gitignore-parser.ts', + 'test/node_modules-resolution.test.ts', + 'package.json', + 'distributed/queue.ts', + ]) { + expect(await guarded('write_file', { path }), path).toBeUndefined(); + } +}); + +test('protect says what to do instead of editing the file', async () => { + expect(protectPlugin.appendix).toContain('package manager'); + expect(String(await guarded('write_file', { path: 'bun.lock' }))).toContain('Regenerate it'); }); test('ordinary source files are untouched', async () => {