From 41546154fe6835e094d057d520024e6b091823dc Mon Sep 17 00:00:00 2001 From: Muhammad Zakir Ramadhan <61570975+zakirkun@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:42:20 +0700 Subject: [PATCH] Add the gated worker subagent kind worker holds the write tools and routes every write and command through the parent's approval gate: same permission rules, same guard, same prompt, flagged as coming from a subagent. Without an approval channel, as in headless runs, the kind is not offered at all rather than silently downgraded to read-only. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- src/cli.tsx | 22 ++++- src/subagent.ts | 157 +++++++++++++++++++++++++++---- test/subagent-progress.test.ts | 90 ++++++++++++++++-- test/subagent.test.ts | 165 ++++++++++++++++++++++++++++++++- 4 files changed, 404 insertions(+), 30 deletions(-) diff --git a/src/cli.tsx b/src/cli.tsx index 8a650a0..49471fe 100644 --- a/src/cli.tsx +++ b/src/cli.tsx @@ -18,7 +18,7 @@ import * as registry from './registry'; import { Session } from './session'; import { loadSkills } from './skills'; import * as store from './store'; -import { createTaskTool } from './subagent'; +import { createTaskTool, type SubagentApproval } from './subagent'; import { VERSION, versionLine } from './version'; import { createAskBridge } from './ui/Ask'; import { App, createApprovalBridge, createNoticeBus, createSubagentBus, type AppHooks } from './ui/App'; @@ -245,6 +245,20 @@ async function persist(messages: ModelMessage[]): Promise { } const bridge = createApprovalBridge(); + +/** + * Late-bound so the task tool can be built before the Session that owns the gate. + * + * `createTaskTool` decides whether to offer the `worker` kind from whether an + * approval channel exists, so the callback has to be present at construction even + * though the Session it delegates to does not exist yet. + */ +let approveSubagent: SubagentApproval | undefined; +const subagentGate: SubagentApproval = (req) => { + if (!approveSubagent) throw new Error('the subagent approval channel is not wired yet'); + return approveSubagent(req); +}; + const session = new Session({ model: languageModel ?? unconfiguredModel, askApproval: bridge.ask, @@ -268,6 +282,10 @@ const session = new Session({ task: createTaskTool({ model: languageModel ?? unconfiguredModel, ...(headless ? {} : { report: subagents.emit }), + // A worker's writes go through the parent's rules and the parent's + // prompt. Headless has nobody to answer, so `worker` is withheld there + // rather than silently running unattended writes. + ...(headless ? {} : { approve: subagentGate }), }), }), }, @@ -280,6 +298,8 @@ const session = new Session({ }, }); +approveSubagent = session.approveForSubagent(); + async function shutdown(code: number): Promise { clearTimeout(saveTimer); if (session.messages.length > 0) await persist(session.messages); diff --git a/src/subagent.ts b/src/subagent.ts index 5b612ed..285d025 100644 --- a/src/subagent.ts +++ b/src/subagent.ts @@ -1,24 +1,73 @@ import { isStepCount, streamText, tool, type LanguageModel, type ToolSet } from 'ai'; import { z } from 'zod'; -import { globTool, grepTool, readFileTool } from './tools'; +import { + applyPatchTool, + bashTool, + editFileTool, + globTool, + grepTool, + listDirTool, + multiEditTool, + readFileTool, + readManyFilesTool, + writeFileTool, +} from './tools'; +import { gitTools } from './tools-git'; -export type SubagentKind = 'explore' | 'review'; +export type SubagentKind = 'explore' | 'review' | 'worker'; export type SubagentEvent = | { type: 'start'; id: string; kind: SubagentKind; description: string } | { type: 'step'; id: string; tool: string; summary: string } + | { type: 'result'; id: string; tool: string; summary: string; ok: boolean } | { type: 'end'; id: string; ok: boolean; steps: number } | { type: 'error'; id: string; message: string }; export type SubagentReporter = (event: SubagentEvent) => void; -const READ_ONLY: ToolSet = { read_file: readFileTool, glob: globTool, grep: grepTool }; +/** + * A subagent's approval callback, supplied by the parent. + * + * The parent owns the gate. A subagent that could approve its own writes would be + * a way to launder a tool call past the user, so `worker` routes every gated call + * back through the same rules and the same prompt as a direct call. + */ +export type SubagentApproval = (req: { toolName: string; input: unknown }) => Promise; + +const READ_TOOLS: ToolSet = { + read_file: readFileTool, + read_many_files: readManyFilesTool, + glob: globTool, + grep: grepTool, + list_dir: listDirTool, + ...gitTools, +}; + +/** + * `worker` adds the mutating tools. Every one of them is gated by the parent, so + * the extra capability is capability to *ask*, not capability to write unasked. + */ +const WRITE_TOOLS: ToolSet = { + write_file: writeFileTool, + edit_file: editFileTool, + multi_edit: multiEditTool, + apply_patch: applyPatchTool, + bash: bashTool, +}; + +const TOOLS: Record = { + explore: READ_TOOLS, + review: READ_TOOLS, + worker: { ...READ_TOOLS, ...WRITE_TOOLS }, +}; + +export const subagentToolNames = (kind: SubagentKind): string[] => Object.keys(TOOLS[kind]); const PROMPTS: Record string> = { explore: (cwd) => `You are a research subagent inside a coding agent. Workspace root: ${cwd} -Tools: read_file, glob, grep. You cannot write files, run commands, or ask questions. +You can read, search, and inspect git history. You cannot write files, run commands, or ask questions. Find what was asked and report once. Rules: - Give file paths with line numbers, plus a short quote where the quote is the answer. @@ -29,52 +78,107 @@ Find what was asked and report once. Rules: review: (cwd) => `You are a review subagent inside a coding agent. Workspace root: ${cwd} -Tools: read_file, glob, grep. You cannot write files, run commands, or ask questions. +You can read, search, and inspect git history. You cannot write files, run commands, or ask questions. Review what was asked and report once. Severity order: incorrect behaviour, missing validation at trust boundaries, security, resource handling, then clarity. For each finding give file, line, what breaks, and the fix. Say plainly when something is correct. Do not invent findings to look thorough.`, + + worker: (cwd) => `You are an implementation subagent inside a coding agent. + +Workspace root: ${cwd} +You can read, search, edit files, and run commands. Every write and every command is approved by the +user through the parent agent, so a denial is the user's decision: stop and report it, do not work +around it. + +You cannot ask questions. If the task is ambiguous, do the smaller reading of it and say in your +report which reading you took and what the alternative was. + +Rules: +- Do only what was asked. Do not tidy neighbouring code, rename things, or add abstraction. +- Read before you write. Match the file's existing style rather than inventing one. +- Verify: run the project's tests or build after changing code. "Should work" is not verification. + Report the actual command and its outcome. +- Report once, and make it a handover: every file you changed with its path, what you ran and what + it said, and anything you could not finish. The parent cannot see your transcript.`, }; +/** One line of detail for the panel: the argument that identifies the call. */ const summarize = (input: unknown): string => { if (input === null || typeof input !== 'object') return String(input); const o = input as Record; - const first = o['pattern'] ?? o['path'] ?? o['include']; - return typeof first === 'string' ? first : JSON.stringify(o).slice(0, 80); + const first = o['command'] ?? o['path'] ?? o['pattern'] ?? o['ref'] ?? o['include']; + if (typeof first === 'string') return first.length > 80 ? `${first.slice(0, 80)}...` : first; + + const files = o['files']; + if (Array.isArray(files)) return `${files.length} file${files.length === 1 ? '' : 's'}`; + const edits = o['edits']; + if (Array.isArray(edits)) return `${edits.length} edit${edits.length === 1 ? '' : 's'}`; + return JSON.stringify(o).slice(0, 80); +}; + +/** First line of a tool result, so the panel can show an outcome rather than just a call. */ +const outcome = (output: unknown): string => { + const text = typeof output === 'string' ? output : JSON.stringify(output ?? ''); + const line = text.split('\n').find((l) => l.trim().length > 0) ?? ''; + return line.length > 70 ? `${line.slice(0, 70)}...` : line; }; let counter = 0; /** - * Read-only child agent. + * Child agent with its own context window. * * It runs its own tool loop and returns one message, so the parent pays for the - * findings rather than the whole search transcript. No write, bash, or ask tool is - * passed in, which is also why a subagent can never trigger an approval prompt. + * findings rather than the whole transcript. + * + * `explore` and `review` hold no mutating tool, so they cannot reach the approval + * gate at all — that is structural, not policy. `worker` does hold them, and every + * one is routed back through the parent's `approve` callback. Without a callback, + * `worker` is refused rather than silently downgraded to read-only: a caller that + * asked for a worker and got an explorer would be told the task failed for the + * wrong reason. */ export function createTaskTool(opts: { model: LanguageModel; cwd?: string; maxSteps?: number; report?: SubagentReporter; + /** Parent-owned approval for a worker's gated calls. Omit to disable `worker`. */ + approve?: SubagentApproval; }) { + const canWrite = opts.approve !== undefined; + return tool({ description: - 'Delegate a read-only investigation to a subagent that can read, glob, and grep. Use it for questions ' + - 'spanning many files ("where is auth handled", "every caller of X") and to keep a long search out of your ' + - 'own context. The subagent sees none of this conversation, so its prompt must be self-contained. ' + - 'It returns one text report. Do not delegate something you can answer with a single grep.', + 'Delegate work to a subagent with its own context window. It sees none of this conversation, so its ' + + 'prompt must be self-contained, and it returns one text report.\n' + + 'explore: find and report, read-only. review: critique code for defects, read-only.' + + (canWrite + ? '\nworker: read, edit, and run commands to carry out a change. Its writes and commands are approved by ' + + 'the user exactly as yours are. Use it for a self-contained task whose intermediate steps you do not ' + + 'need to see; keep work you must supervise step by step in your own turn.' + : '') + + '\nDo not delegate something you can answer with a single grep.', inputSchema: z.object({ description: z.string().describe('Short label shown to the user, 3-6 words'), - prompt: z.string().describe('Self-contained instructions: what to find, where to look, what to return'), + prompt: z.string().describe('Self-contained instructions: what to do, where, and what to report'), kind: z - .enum(['explore', 'review']) + .enum(canWrite ? ['explore', 'review', 'worker'] : ['explore', 'review']) .optional() - .describe('explore: find and report. review: critique code for defects. Default explore.'), + .describe( + canWrite + ? 'explore: read-only research. review: read-only critique. worker: makes changes. Default explore.' + : 'explore: find and report. review: critique code for defects. Default explore.', + ), }), execute: async ({ description, prompt, kind }, { abortSignal }) => { - const id = `sub${++counter}`; const flavour: SubagentKind = kind ?? 'explore'; + if (flavour === 'worker' && !opts.approve) { + throw new Error('The worker kind needs an approval channel, which this session has not provided.'); + } + + const id = `sub${++counter}`; const report = opts.report; report?.({ type: 'start', id, kind: flavour, description }); @@ -86,8 +190,18 @@ export function createTaskTool(opts: { model: opts.model, system: PROMPTS[flavour](opts.cwd ?? process.cwd()), messages: [{ role: 'user', content: prompt }], - tools: READ_ONLY, + tools: TOOLS[flavour], stopWhen: isStepCount(opts.maxSteps ?? 20), + ...(opts.approve + ? { + toolApproval: async ({ toolCall }: { toolCall: { toolName: string; input: unknown } }) => { + const approved = await opts.approve!(toolCall); + return approved + ? undefined + : { type: 'denied' as const, reason: 'The user denied this call. Stop and report it.' }; + }, + } + : {}), ...(abortSignal ? { abortSignal } : {}), }); @@ -102,6 +216,11 @@ export function createTaskTool(opts: { if (part.type === 'tool-call') { steps++; report?.({ type: 'step', id, tool: part.toolName, summary: summarize(part.input) }); + } else if (part.type === 'tool-result') { + report?.({ type: 'result', id, tool: part.toolName, summary: outcome(part.output), ok: true }); + } else if (part.type === 'tool-error') { + const message = part.error instanceof Error ? part.error.message : String(part.error); + report?.({ type: 'result', id, tool: part.toolName, summary: outcome(message), ok: false }); } else if (part.type === 'text-delta') { text += part.text; } else if (part.type === 'error') { diff --git a/test/subagent-progress.test.ts b/test/subagent-progress.test.ts index 6b1a28c..a76e619 100644 --- a/test/subagent-progress.test.ts +++ b/test/subagent-progress.test.ts @@ -1,6 +1,7 @@ import { expect, test } from 'bun:test'; import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; import type { LanguageModelV4CallOptions, LanguageModelV4StreamPart } from '@ai-sdk/provider'; +import { z } from 'zod'; import { Session } from '../src/session'; import { createTaskTool, type SubagentEvent } from '../src/subagent'; @@ -30,7 +31,7 @@ const text = (body: string): LanguageModelV4StreamPart[] => [ const run = (tool: ReturnType, input: Record) => Promise.resolve(tool.execute!(input as never, { toolCallId: 'x', messages: [] } as never)) as Promise; -test('a subagent reports start, each step, and end', async () => { +test('a subagent reports start, each step, its outcome, and end', async () => { const seen: SubagentEvent[] = []; let n = 0; const model = new MockLanguageModelV4({ @@ -46,7 +47,7 @@ test('a subagent reports start, each step, and end', async () => { }); expect(out).toContain('src/auth.ts:12'); - expect(seen.map((e) => e.type)).toEqual(['start', 'step', 'end']); + expect(seen.map((e) => e.type)).toEqual(['start', 'step', 'result', 'end']); const start = seen[0]; if (start?.type !== 'start') throw new Error('expected start'); @@ -58,7 +59,14 @@ test('a subagent reports start, each step, and end', async () => { expect(step.tool).toBe('grep'); expect(step.summary).toBe('login'); - const end = seen[2]; + // The outcome, not just the call: a panel showing only calls cannot tell a + // search that found something from one that found nothing. + const result = seen[2]; + if (result?.type !== 'result') throw new Error('expected result'); + expect(result.tool).toBe('grep'); + expect(result.ok).toBe(true); + + const end = seen[3]; if (end?.type !== 'end') throw new Error('expected end'); expect(end).toMatchObject({ ok: true, steps: 1 }); }); @@ -87,7 +95,30 @@ test('explore is the default kind', async () => { expect(start.kind).toBe('explore'); }); -test('a subagent only ever gets read-only tools', async () => { +test('explore and review can only read, whatever else exists', async () => { + for (const kind of ['explore', 'review'] as const) { + const seen: LanguageModelV4CallOptions[] = []; + const model = new MockLanguageModelV4({ + doStream: async (o) => { + seen.push(o); + return stream(text('done')); + }, + }); + + // With an approval channel present, so this proves the read-only kinds are + // restricted by their tool set rather than by the absence of a gate. + await run(createTaskTool({ model, approve: async () => true }), { description: 'd', prompt: 'p', kind }); + + const names = (seen[0]?.tools ?? []).map((t) => t.name); + for (const write of ['write_file', 'edit_file', 'multi_edit', 'bash']) { + expect(names, `${kind} must not hold ${write}`).not.toContain(write); + } + expect(names).toContain('read_file'); + expect(names).toContain('grep'); + } +}); + +test('a worker holds the mutating tools as well', async () => { const seen: LanguageModelV4CallOptions[] = []; const model = new MockLanguageModelV4({ doStream: async (o) => { @@ -96,9 +127,54 @@ test('a subagent only ever gets read-only tools', async () => { }, }); - await run(createTaskTool({ model }), { description: 'd', prompt: 'p' }); - const names = (seen[0]?.tools ?? []).map((t) => t.name).sort(); - expect(names).toEqual(['glob', 'grep', 'read_file']); + await run(createTaskTool({ model, approve: async () => true }), { + description: 'd', + prompt: 'p', + kind: 'worker', + }); + + const names = (seen[0]?.tools ?? []).map((t) => t.name); + for (const write of ['write_file', 'edit_file', 'multi_edit', 'apply_patch', 'bash']) expect(names).toContain(write); +}); + +test('worker is not offered at all without an approval channel', async () => { + const model = new MockLanguageModelV4({ doStream: async () => stream(text('done')) }); + const readOnly = createTaskTool({ model }); + + const schema = JSON.stringify(z.toJSONSchema(readOnly.inputSchema as never)); + expect(schema).not.toContain('worker'); + expect(readOnly.description).not.toContain('worker'); + + // Asking for one anyway fails loudly. Silently downgrading to explore would + // report the task as complete having written nothing. + expect(run(readOnly, { description: 'd', prompt: 'p', kind: 'worker' })).rejects.toThrow( + /needs an approval channel/, + ); +}); + +test('a worker call the user denies is refused and the subagent is told', async () => { + const asked: string[] = []; + let n = 0; + const model = new MockLanguageModelV4({ + doStream: async () => + n++ === 0 + ? stream(toolCall('s1', 'write_file', { path: 'out.txt', content: 'x' })) + : stream(text('I was denied, so I stopped.')), + }); + + const out = await run( + createTaskTool({ + model, + approve: async ({ toolName }) => { + asked.push(toolName); + return false; + }, + }), + { description: 'write it', prompt: 'write out.txt', kind: 'worker' }, + ); + + expect(asked).toEqual(['write_file']); + expect(out).toContain('denied'); }); test('an empty report is stated rather than returned blank', async () => { diff --git a/test/subagent.test.ts b/test/subagent.test.ts index e6b8998..654e032 100644 --- a/test/subagent.test.ts +++ b/test/subagent.test.ts @@ -4,6 +4,8 @@ import type { LanguageModelV4CallOptions, LanguageModelV4StreamPart } from '@ai- import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { createHost } from '../src/plugins'; +import { guardPlugin } from '../src/plugins-builtin'; import { Session } from '../src/session'; import { createTaskTool } from '../src/subagent'; @@ -75,15 +77,172 @@ test('subagent greps the workspace and returns text to the parent', async () => expect(events).toEqual(['tool-start', 'tool-call', 'tool-result', 'text', 'done']); - // The subagent gets only read tools, so it can never trigger an approval prompt. - const subagentTools = (seen[1]?.tools ?? []).map((t) => t.name).sort(); - expect(subagentTools).toEqual(['glob', 'grep', 'read_file']); + // An explore subagent holds no mutating tool, so it cannot reach the approval + // gate at all. That is structural rather than policy. + const subagentTools = (seen[1]?.tools ?? []).map((t) => t.name); + for (const write of ['write_file', 'edit_file', 'multi_edit', 'bash']) { + expect(subagentTools).not.toContain(write); + } + expect(subagentTools).toContain('grep'); // Its findings reach the parent as a tool result, not as raw transcript. const toolMessage = session.messages.find((m) => m.role === 'tool'); expect(JSON.stringify(toolMessage)).toContain('src/auth.ts:1'); })); +test('a worker subagent edits a file, and the write goes through the parent gate', async () => + inTempDir(async () => { + await Bun.write('app.ts', 'const port = 8080;\n'); + + const asked: { toolName: string; subagent?: boolean }[] = []; + const seen: LanguageModelV4CallOptions[] = []; + const model = new MockLanguageModelV4({ + doStream: async (opts) => { + const call = seen.length; + seen.push(opts); + if (call === 0) { + return stream( + toolCall('c1', 'task', { description: 'bump the port', prompt: 'Set port to 9090 in app.ts.', kind: 'worker' }), + ); + } + if (call === 1) { + return stream( + toolCall('s1', 'edit_file', { path: 'app.ts', oldString: 'const port = 8080;', newString: 'const port = 9090;' }), + ); + } + if (call === 2) return stream(text('Changed app.ts: port is now 9090.')); + return stream(text('The worker bumped the port.')); + }, + }); + + const session: Session = new Session({ + model, + askApproval: async (req) => { + asked.push({ toolName: req.toolName, ...(req.subagent ? { subagent: true } : {}) }); + return 'once'; + }, + extraTools: { + task: createTaskTool({ model, approve: (r) => session.approveForSubagent()(r) }), + }, + autoApprove: ['task'], + }); + + for await (const _ of session.send('bump the port')) void _; + + // The write reached the user's prompt, flagged as coming from a subagent, and + // the file on disk actually changed. + expect(asked).toEqual([{ toolName: 'edit_file', subagent: true }]); + expect(await Bun.file('app.ts').text()).toBe('const port = 9090;\n'); + })); + +test('a denied worker write leaves the file alone and the worker reports it', async () => + inTempDir(async () => { + await Bun.write('app.ts', 'const port = 8080;\n'); + + const seen: LanguageModelV4CallOptions[] = []; + const model = new MockLanguageModelV4({ + doStream: async (opts) => { + const call = seen.length; + seen.push(opts); + if (call === 0) { + return stream(toolCall('c1', 'task', { description: 'bump it', prompt: 'set 9090', kind: 'worker' })); + } + if (call === 1) { + return stream( + toolCall('s1', 'edit_file', { path: 'app.ts', oldString: 'const port = 8080;', newString: 'const port = 9090;' }), + ); + } + if (call === 2) return stream(text('The user denied the edit, so I stopped.')); + return stream(text('The worker was denied.')); + }, + }); + + const session: Session = new Session({ + model, + askApproval: async () => 'deny', + extraTools: { + task: createTaskTool({ model, approve: (r) => session.approveForSubagent()(r) }), + }, + autoApprove: ['task'], + }); + + for await (const _ of session.send('bump the port')) void _; + + expect(await Bun.file('app.ts').text()).toBe('const port = 8080;\n'); + expect(JSON.stringify(session.messages)).toContain('denied'); + })); + +test('a permission rule denies a worker write without ever prompting', async () => + inTempDir(async () => { + await Bun.write('app.ts', 'const port = 8080;\n'); + + const seen: LanguageModelV4CallOptions[] = []; + const model = new MockLanguageModelV4({ + doStream: async (opts) => { + const call = seen.length; + seen.push(opts); + if (call === 0) { + return stream(toolCall('c1', 'task', { description: 'bump it', prompt: 'set 9090', kind: 'worker' })); + } + if (call === 1) { + return stream( + toolCall('s1', 'edit_file', { path: 'app.ts', oldString: 'const port = 8080;', newString: 'const port = 9090;' }), + ); + } + if (call === 2) return stream(text('That edit was refused.')); + return stream(text('Refused.')); + }, + }); + + const session: Session = new Session({ + model, + askApproval: async () => { + throw new Error('a rule that denies must not reach the prompt'); + }, + permissions: { edit_file: { '*': 'deny' } }, + extraTools: { + task: createTaskTool({ model, approve: (r) => session.approveForSubagent()(r) }), + }, + autoApprove: ['task'], + }); + + for await (const _ of session.send('bump the port')) void _; + expect(await Bun.file('app.ts').text()).toBe('const port = 8080;\n'); + })); + +test('the guard plugin refuses a worker command, as it does a direct one', async () => + inTempDir(async () => { + const seen: LanguageModelV4CallOptions[] = []; + const model = new MockLanguageModelV4({ + doStream: async (opts) => { + const call = seen.length; + seen.push(opts); + if (call === 0) { + return stream(toolCall('c1', 'task', { description: 'clean', prompt: 'clean the tree', kind: 'worker' })); + } + if (call === 1) return stream(toolCall('s1', 'bash', { command: 'rm -rf build' })); + if (call === 2) return stream(text('That command was refused.')); + return stream(text('Refused.')); + }, + }); + + const session: Session = new Session({ + yolo: true, + model, + askApproval: async () => { + throw new Error('the guard must refuse without asking'); + }, + plugins: createHost([guardPlugin]), + extraTools: { + task: createTaskTool({ model, approve: (r) => session.approveForSubagent()(r) }), + }, + autoApprove: ['task'], + }); + + for await (const _ of session.send('clean up')) void _; + expect(await Bun.file('build').exists()).toBe(false); + })); + test('subagent does not see the parent conversation', async () => inTempDir(async () => { const seen: LanguageModelV4CallOptions[] = [];