diff --git a/.changeset/dynamic-workflow-plan-approval.md b/.changeset/dynamic-workflow-plan-approval.md new file mode 100644 index 00000000..d78e526d --- /dev/null +++ b/.changeset/dynamic-workflow-plan-approval.md @@ -0,0 +1,18 @@ +--- +'@pythoughts/pythinker-code-sdk': minor +'@pythoughts/pythinker-code': minor +--- + +Show the plan before a Dynamic Workflow runs, and let a good one be saved as a command + +Manual mode used to approve every `DynamicWorkflow` call outright. That approval +only ever fired in manual mode — auto and yolo approve earlier in the chain — so +the one mode whose purpose is to ask was the one mode that never saw what it was +agreeing to. A `DynamicWorkflow` call in manual mode now asks, and the approval +carries the fan-out: how many subagents, the task list, the prompt template, the +worker model, and the summed size of the prompts about to be sent. "Approve for +this session" is keyed to that workflow's description rather than granting every +future `DynamicWorkflow` call. + +`/workflow save ` writes the last run back out as a skill under +`.pythinker-code/skills/`, so a fan-out that worked can be re-run by name. diff --git a/apps/pythinker-code/src/tui/commands/dynamic-workflow.ts b/apps/pythinker-code/src/tui/commands/dynamic-workflow.ts index 4b16aa98..e2bb86f2 100644 --- a/apps/pythinker-code/src/tui/commands/dynamic-workflow.ts +++ b/apps/pythinker-code/src/tui/commands/dynamic-workflow.ts @@ -1,5 +1,10 @@ -import type { PermissionMode } from '@pythoughts/pythinker-code-sdk'; +import { + savedWorkflowSkillName, + writeSavedWorkflowSkill, + type PermissionMode, +} from '@pythoughts/pythinker-code-sdk'; +import { getDataDir } from '#/utils/paths'; import { DynamicWorkflowStartPermissionPromptComponent, type DynamicWorkflowStartPermissionChoice, @@ -25,6 +30,7 @@ export async function handleDynamicWorkflowCommand(host: SlashCommandHost, args: const prompt = args.trim(); if (handleModelSubcommand(host, prompt)) return; + if (await handleSaveSubcommand(host, prompt)) return; const mode = dynamicWorkflowModeSubcommand(prompt); if (mode !== undefined) { @@ -114,6 +120,69 @@ function withWorkerModelInstruction(prompt: string, model: string | undefined): : `${prompt}\n\nUse model "${model}" for the DynamicWorkflow subagents in this task.`; } +/** + * `/workflow save ` writes the last run back out as a skill, so a fan-out + * that worked can be re-run by name instead of re-described. + * + * Returns true when the input was a `save` subcommand and has been handled. + */ +async function handleSaveSubcommand(host: SlashCommandHost, input: string): Promise { + const match = /^save(?:\s+(.*))?$/iu.exec(input); + if (match === null) return false; + + const name = match[1]?.trim() ?? ''; + if (name.length === 0) { + host.showError('Usage: /workflow save '); + return true; + } + + const args = host.state.lastDynamicWorkflowArgs; + if (args === undefined) { + host.showError('No Dynamic Workflow has run in this session yet.'); + return true; + } + + const description = stringArg(args, 'description'); + if (description === undefined) { + host.showError('The last Dynamic Workflow has no description to save.'); + return true; + } + + try { + const dir = await writeSavedWorkflowSkill({ + scope: 'project', + projectRoot: host.state.appState.workDir, + brandHomeDir: getDataDir(), + workflow: { + name, + description, + subagentType: stringArg(args, 'subagent_type'), + promptTemplate: stringArg(args, 'prompt_template'), + model: stringArg(args, 'model'), + effort: stringArg(args, 'effort'), + outputSchema: recordArg(args, 'output_schema'), + }, + }); + host.refreshSlashCommandAutocomplete(); + host.showStatus(`Saved /${savedWorkflowSkillName(name)} to ${dir}.`); + } catch (error) { + host.showError(`Failed to save workflow: ${formatErrorMessage(error)}`); + } + return true; +} + +function stringArg(args: Record, key: string): string | undefined { + const value = args[key]; + return typeof value === 'string' && value.trim().length > 0 ? value : undefined; +} + +function recordArg(args: Record, key: string): Record | undefined { + const value = args[key]; + return typeof value === 'object' && value !== null && !Array.isArray(value) + ? (value as Record) + : undefined; +} + /** Returns true when the input was a `model` subcommand and has been handled. */ function handleModelSubcommand(host: SlashCommandHost, input: string): boolean { const match = /^model(?:\s+(.*))?$/iu.exec(input); diff --git a/apps/pythinker-code/src/tui/commands/registry.ts b/apps/pythinker-code/src/tui/commands/registry.ts index 0b7423b9..7cb77b50 100644 --- a/apps/pythinker-code/src/tui/commands/registry.ts +++ b/apps/pythinker-code/src/tui/commands/registry.ts @@ -21,6 +21,7 @@ const DYNAMIC_WORKFLOW_ARG_COMPLETIONS: readonly ArgCompletionSpec[] = [ { value: 'on', description: 'Turn Dynamic Workflow mode on' }, { value: 'off', description: 'Turn Dynamic Workflow mode off' }, { value: 'model', description: 'Set the model Dynamic Workflow subagents run on' }, + { value: 'save', description: 'Save the last Dynamic Workflow as a reusable command' }, ]; const FAST_ARG_COMPLETIONS: readonly ArgCompletionSpec[] = [ diff --git a/apps/pythinker-code/src/tui/components/dialogs/approval-panel.ts b/apps/pythinker-code/src/tui/components/dialogs/approval-panel.ts index b65342bc..fcf1b6ec 100644 --- a/apps/pythinker-code/src/tui/components/dialogs/approval-panel.ts +++ b/apps/pythinker-code/src/tui/components/dialogs/approval-panel.ts @@ -4,6 +4,8 @@ * Container-based component with keyboard navigation. */ +import { stripVTControlCharacters } from 'node:util'; + import { Container, Input, @@ -32,6 +34,7 @@ import type { DisplayBlock, FileContentDisplayBlock, PendingApproval, + WorkflowPlanDisplayBlock, } from '#/tui/reverse-rpc/types'; import { printableChar } from '#/tui/utils/printable-key'; @@ -167,6 +170,8 @@ function renderDisplayBlock( } return lines; } + case 'workflow_plan': + return renderWorkflowPlanDisplayBlock(block, s); case 'brief': return block.text ? block.text.split('\n').map((line) => (line.length > 0 ? s.strong(line) : '')) @@ -182,6 +187,57 @@ function renderDisplayBlock( } } +/** + * A workflow can carry up to 128 items. Listing all of them would push the + * buttons off the screen, so the panel shows enough to judge the shape of the + * fan-out and says how many it held back. + */ +const MAX_PREVIEW_ITEMS = 10; + +/** + * The plan is the thing being approved, and every field in it came from the + * model. Escape sequences would let that text repaint the panel it is being + * judged in — hide a line, redraw the buttons, or reverse the reading order — + * so they are removed rather than styled. `stripVTControlCharacters` takes the + * CSI and OSC sequences; the class escape then takes the bare control and + * format characters it leaves behind, which include the bidi overrides. + */ +function sanitizePlanText(text: string): string { + return stripVTControlCharacters(text).replaceAll(/[\p{Cc}\p{Cf}]/gu, ' '); +} + +function renderWorkflowPlanDisplayBlock( + block: WorkflowPlanDisplayBlock, + s: BlockStyles, +): string[] { + const plural = block.agent_count === 1 ? 'subagent' : 'subagents'; + const summary = [ + `${String(block.agent_count)} ${plural}`, + `~${String(block.prompt_tokens)} prompt tokens`, + ]; + if (block.model !== undefined && block.model.length > 0) { + summary.push(`model: ${sanitizePlanText(block.model)}`); + } + const lines = [s.strong(summary.join(' '))]; + + if (block.prompt_template !== undefined && block.prompt_template.length > 0) { + lines.push( + `${s.accent('prompt')} ${s.dim(truncateOneLine(sanitizePlanText(block.prompt_template), 200))}`, + ); + } + + for (const [index, item] of block.items.slice(0, MAX_PREVIEW_ITEMS).entries()) { + lines.push( + s.dim(`${String(index + 1).padStart(3)}. ${truncateOneLine(sanitizePlanText(item), 120)}`), + ); + } + const hidden = block.items.length - MAX_PREVIEW_ITEMS; + if (hidden > 0) { + lines.push(s.dim(` +${String(hidden)} more`)); + } + return lines; +} + function normalizeApprovalText(text: string): string { return text.replaceAll('\r\n', '\n').trim(); } @@ -209,6 +265,8 @@ function headerFor(toolName: string): string { return 'Stop this task?'; case 'ExitPlanMode': return 'Ready to build with this plan?'; + case 'DynamicWorkflow': + return 'Run this Dynamic Workflow?'; default: return `Approve ${toolName}?`; } diff --git a/apps/pythinker-code/src/tui/controllers/subagent-event-handler.ts b/apps/pythinker-code/src/tui/controllers/subagent-event-handler.ts index a8c69186..d20b3ad4 100644 --- a/apps/pythinker-code/src/tui/controllers/subagent-event-handler.ts +++ b/apps/pythinker-code/src/tui/controllers/subagent-event-handler.ts @@ -275,6 +275,10 @@ export class SubAgentEventHandler { if (this.isRetiredDynamicWorkflowToolCall(toolCallId)) return; const missionControl = this.ensureDynamicWorkflowMissionControl(toolCallId, args); missionControl.markInputComplete(); + // Captured here rather than in `ensure…`, which the delta path also calls: + // mid-stream arguments are half-parsed, and saving those would write a + // workflow missing most of its items. + this.host.state.lastDynamicWorkflowArgs = args; this.requestRender(); } diff --git a/apps/pythinker-code/src/tui/reverse-rpc/approval/adapter.ts b/apps/pythinker-code/src/tui/reverse-rpc/approval/adapter.ts index ee52cfe7..c54156f1 100644 --- a/apps/pythinker-code/src/tui/reverse-rpc/approval/adapter.ts +++ b/apps/pythinker-code/src/tui/reverse-rpc/approval/adapter.ts @@ -300,8 +300,8 @@ function adaptDisplay(display: ToolInputDisplay): DisplayBlock[] { scope: display.scope, }, ]; - case 'agent_call': - return [ + case 'agent_call': { + const blocks: DisplayBlock[] = [ { type: 'invocation', kind: 'agent', @@ -309,6 +309,18 @@ function adaptDisplay(display: ToolInputDisplay): DisplayBlock[] { description: display.prompt, }, ]; + if (display.workflow !== undefined) { + blocks.push({ + type: 'workflow_plan', + agent_count: display.workflow.agent_count, + items: [...display.workflow.items], + prompt_tokens: display.workflow.prompt_tokens, + prompt_template: display.workflow.prompt_template, + model: display.workflow.model, + }); + } + return blocks; + } case 'skill_call': return [ { diff --git a/apps/pythinker-code/src/tui/reverse-rpc/types.ts b/apps/pythinker-code/src/tui/reverse-rpc/types.ts index 072d5b1c..7dba9a7a 100644 --- a/apps/pythinker-code/src/tui/reverse-rpc/types.ts +++ b/apps/pythinker-code/src/tui/reverse-rpc/types.ts @@ -68,6 +68,19 @@ export interface InvocationDisplayBlock { description?: string | undefined; } +/** + * The fan-out a Dynamic Workflow is about to launch. Shown at approval time so + * the decision is made against the actual task list rather than a count. + */ +export interface WorkflowPlanDisplayBlock { + type: 'workflow_plan'; + agent_count: number; + items: string[]; + prompt_tokens: number; + prompt_template?: string; + model?: string; +} + export interface TodoDisplayItem { title: string; status: 'pending' | 'in_progress' | 'done'; @@ -95,6 +108,7 @@ export type DisplayBlock = | UrlFetchDisplayBlock | SearchDisplayBlock | InvocationDisplayBlock + | WorkflowPlanDisplayBlock | TodoDisplayBlock | BackgroundTaskDisplayBlock; diff --git a/apps/pythinker-code/src/tui/tui-state.ts b/apps/pythinker-code/src/tui/tui-state.ts index 65be7ecb..2e5c9618 100644 --- a/apps/pythinker-code/src/tui/tui-state.ts +++ b/apps/pythinker-code/src/tui/tui-state.ts @@ -63,6 +63,12 @@ export interface TUIState { externalEditorRunning: boolean; queuedMessages: QueuedMessage[]; dynamicWorkflowModeEntry: 'manual' | 'task' | undefined; + /** + * Arguments of the most recent DynamicWorkflow tool call, so `/workflow save` + * can turn a run that just worked into a reusable command. Overwritten as the + * call streams in; the last write is the complete one. + */ + lastDynamicWorkflowArgs: Record | undefined; } export function createTUIState(options: PythinkerTUIOptions): TUIState { @@ -143,5 +149,6 @@ export function createTUIState(options: PythinkerTUIOptions): TUIState { externalEditorRunning: false, queuedMessages: [], dynamicWorkflowModeEntry: undefined, + lastDynamicWorkflowArgs: undefined, }; } diff --git a/apps/pythinker-code/test/tui/commands/dynamic-workflow.test.ts b/apps/pythinker-code/test/tui/commands/dynamic-workflow.test.ts index 3e2a6424..61e779c9 100644 --- a/apps/pythinker-code/test/tui/commands/dynamic-workflow.test.ts +++ b/apps/pythinker-code/test/tui/commands/dynamic-workflow.test.ts @@ -1,3 +1,7 @@ +import { promises as fs } from 'node:fs'; +import { tmpdir } from 'node:os'; + +import { join } from 'pathe'; import { describe, expect, it, vi } from 'vitest'; import { handleDynamicWorkflowCommand } from '#/tui/commands/index'; @@ -24,6 +28,8 @@ function makeHost( permissionMode?: 'manual' | 'auto' | 'yolo'; dynamicWorkflowMode?: boolean; availableModels?: Record; + workDir?: string; + lastDynamicWorkflowArgs?: Record; } = {}, ) { const session = { @@ -40,10 +46,12 @@ function makeHost( availableModels: overrides.availableModels ?? { 'deepseek-v4': { provider: 'deepseek', model: 'deepseek-v4' }, }, + workDir: overrides.workDir ?? '/workspace', }, theme: currentTheme, transcriptContainer: { addChild: vi.fn() }, ui: { requestRender: vi.fn() }, + lastDynamicWorkflowArgs: overrides.lastDynamicWorkflowArgs, }, session: hasSession ? session : undefined, requireSession: () => session, @@ -54,6 +62,7 @@ function makeHost( restoreEditor: vi.fn(), restoreInputText: vi.fn(), sendNormalUserInput: vi.fn(), + refreshSlashCommandAutocomplete: vi.fn(), } as unknown as SlashCommandHost; return { host, session }; } @@ -410,3 +419,76 @@ describe('handleDynamicWorkflowCommand', () => { ); }); }); + +describe('/workflow save', () => { + it('writes the last run as a skill and refreshes the command list', async () => { + const workDir = await fs.mkdtemp(join(tmpdir(), 'workflow-save-')); + try { + const { host } = makeHost({ + permissionMode: 'auto', + workDir, + lastDynamicWorkflowArgs: { + description: 'Audit routes for missing auth', + subagent_type: 'reviewer', + prompt_template: 'Audit {{item}}', + model: 'deepseek-v4', + items: ['a.ts', 'b.ts'], + }, + }); + + await handleDynamicWorkflowCommand(host, 'save Audit Routes'); + + const saved = await fs.readFile( + join(workDir, '.pythinker-code', 'skills', 'audit-routes', 'SKILL.md'), + 'utf8', + ); + expect(saved).toContain('name: "audit-routes"'); + expect(saved).toContain('description: "Audit routes for missing auth"'); + expect(saved).toContain('subagent-type: "reviewer"'); + expect(saved).toContain('Audit {{item}}'); + expect(host.refreshSlashCommandAutocomplete).toHaveBeenCalled(); + expect(host.showError).not.toHaveBeenCalled(); + } finally { + await fs.rm(workDir, { recursive: true, force: true }); + } + }); + + it('refuses a name that would escape the project skills directory', async () => { + const workDir = await fs.mkdtemp(join(tmpdir(), 'workflow-save-')); + try { + const { host } = makeHost({ + permissionMode: 'auto', + workDir, + lastDynamicWorkflowArgs: { description: 'Audit routes' }, + }); + + await handleDynamicWorkflowCommand(host, 'save ../../../../tmp/pwned'); + + expect(host.showError).toHaveBeenCalledWith( + expect.stringContaining('not a valid skill name'), + ); + expect(host.refreshSlashCommandAutocomplete).not.toHaveBeenCalled(); + await expect(fs.stat(join(workDir, '.pythinker-code'))).rejects.toThrow(/ENOENT/u); + } finally { + await fs.rm(workDir, { recursive: true, force: true }); + } + }); + + it('explains itself when no workflow has run yet', async () => { + const { host } = makeHost({ permissionMode: 'auto' }); + + await handleDynamicWorkflowCommand(host, 'save nightly-audit'); + + expect(host.showError).toHaveBeenCalledWith( + 'No Dynamic Workflow has run in this session yet.', + ); + }); + + it('asks for a name when given none', async () => { + const { host } = makeHost({ permissionMode: 'auto' }); + + await handleDynamicWorkflowCommand(host, 'save'); + + expect(host.showError).toHaveBeenCalledWith('Usage: /workflow save '); + }); +}); diff --git a/apps/pythinker-code/test/tui/commands/registry.test.ts b/apps/pythinker-code/test/tui/commands/registry.test.ts index d83202b2..3083a5d4 100644 --- a/apps/pythinker-code/test/tui/commands/registry.test.ts +++ b/apps/pythinker-code/test/tui/commands/registry.test.ts @@ -124,9 +124,10 @@ describe('built-in slash command registry', () => { return items === null ? null : items.map((item) => item.value); }; - expect(values('')).toEqual(['on', 'off', 'model']); + expect(values('')).toEqual(['on', 'off', 'model', 'save']); expect(values('O')).toEqual(['on', 'off']); expect(values('mod')).toEqual(['model']); + expect(values('sa')).toEqual(['save']); expect(dynamicWorkflowArgumentCompletions('of')).toEqual([ { value: 'off', label: 'off', description: 'Turn Dynamic Workflow mode off' }, ]); diff --git a/apps/pythinker-code/test/tui/components/dialogs/approval-panel.test.ts b/apps/pythinker-code/test/tui/components/dialogs/approval-panel.test.ts index 2ca82063..8af9e9c7 100644 --- a/apps/pythinker-code/test/tui/components/dialogs/approval-panel.test.ts +++ b/apps/pythinker-code/test/tui/components/dialogs/approval-panel.test.ts @@ -89,6 +89,115 @@ describe('ApprovalPanelComponent', () => { expect(out).not.toContain('⚠'); }); + // The whole point of asking before a Dynamic Workflow is that the operator + // sees the fan-out. A block that reaches the panel and paints nothing would + // still typecheck, so assert the painted lines rather than the payload. + it('paints the Dynamic Workflow plan: counts, prompt size, template and items', () => { + const pending: PendingApproval = { + data: { + id: 'approval_workflow', + tool_call_id: 'tool_workflow', + tool_name: 'DynamicWorkflow', + action: 'run', + description: 'Review the diff', + display: [ + { + type: 'workflow_plan', + agent_count: 3, + items: ['src/a.ts', 'src/b.ts', 'src/c.ts'], + prompt_tokens: 128, + prompt_template: 'Review {{item}} for races', + model: 'claude-sonnet-4', + }, + ], + choices: [{ label: 'Approve once', response: 'approved' }], + }, + }; + + const out = strip(new ApprovalPanelComponent(pending, () => {}).render(80).join('\n')); + + expect(out).toContain('3 subagents'); + expect(out).toContain('~128 prompt tokens'); + expect(out).toContain('model: claude-sonnet-4'); + expect(out).toContain('Review {{item}} for races'); + expect(out).toContain('src/a.ts'); + expect(out).toContain('src/c.ts'); + }); + + // The plan is what the operator is judging, and every field in it came from + // the model. Escape sequences left intact could repaint the panel deciding + // their fate — blank a line, redraw the buttons, or reverse the text order. + it('strips terminal control sequences from the plan before painting it', () => { + const esc = String.fromCodePoint(0x1B); + const rtlOverride = String.fromCodePoint(0x202E); + const pending: PendingApproval = { + data: { + id: 'approval_workflow_ansi', + tool_call_id: 'tool_workflow_ansi', + tool_name: 'DynamicWorkflow', + action: 'run', + description: 'Sweep', + display: [ + { + type: 'workflow_plan', + agent_count: 2, + items: [ + `${esc}[2Kharmless-item`, + // OSC 8 hyperlink, and a right-to-left override. + `${esc}]8;;http://evil.example${esc}\\second-item${rtlOverride}`, + ], + prompt_tokens: 12, + prompt_template: `${esc}[31mReview {{item}}`, + model: `${esc}[1msonnet`, + }, + ], + choices: [{ label: 'Approve once', response: 'approved' }], + }, + }; + + const raw = new ApprovalPanelComponent(pending, () => {}).render(80).join('\n'); + const out = strip(raw); + + expect(out).toContain('harmless-item'); + expect(out).toContain('second-item'); + expect(out).toContain('Review {{item}}'); + expect(out).toContain('model: sonnet'); + // `strip` only removes SGR colour codes, so anything else the model smuggled + // in would still be sitting in `out`. + expect(out).not.toContain(`${esc}[2K`); + expect(out).not.toContain(']8;;'); + expect(out).not.toContain('http://evil.example'); + expect(out).not.toContain(rtlOverride); + }); + + it('caps the plan item list and says how many it held back', () => { + const pending: PendingApproval = { + data: { + id: 'approval_workflow_big', + tool_call_id: 'tool_workflow_big', + tool_name: 'DynamicWorkflow', + action: 'run', + description: 'Sweep', + display: [ + { + type: 'workflow_plan', + agent_count: 40, + items: Array.from({ length: 40 }, (_unused, index) => `item-${String(index)}`), + prompt_tokens: 4096, + }, + ], + choices: [{ label: 'Approve once', response: 'approved' }], + }, + }; + + const out = strip(new ApprovalPanelComponent(pending, () => {}).render(80).join('\n')); + + expect(out).toContain('40 subagents'); + expect(out).toContain('item-9'); + expect(out).not.toContain('item-10'); + expect(out).toContain('+30 more'); + }); + it('wraps a long single-line shell command instead of truncating it', () => { const head = 'approve-long-command-head'; const tail = 'approve-long-command-tail'; diff --git a/apps/pythinker-code/test/tui/reverse-rpc/approval-adapter.test.ts b/apps/pythinker-code/test/tui/reverse-rpc/approval-adapter.test.ts index 1cd084bd..7ef5e992 100644 --- a/apps/pythinker-code/test/tui/reverse-rpc/approval-adapter.test.ts +++ b/apps/pythinker-code/test/tui/reverse-rpc/approval-adapter.test.ts @@ -253,6 +253,63 @@ describe('approval adapter', () => { ]); }); + // A DynamicWorkflow approval is the one place the fan-out can still be + // refused, so the plan has to survive the trip into the panel rather than + // being flattened into the "N subagents" label. + it('carries a Dynamic Workflow plan through as its own display block', () => { + const adapted = adaptApprovalRequest({ + toolCallId: 'tc-workflow', + toolName: 'DynamicWorkflow', + action: 'run', + display: { + kind: 'agent_call', + agent_name: 'Dynamic Workflow (3 subagents)', + prompt: 'Review the diff', + workflow: { + agent_count: 3, + items: ['src/a.ts', 'src/b.ts', 'src/c.ts'], + prompt_tokens: 42, + prompt_template: 'Review {{item}}', + model: 'claude-sonnet-4', + }, + }, + }); + + expect(adapted.display).toEqual([ + { + type: 'invocation', + kind: 'agent', + name: 'Dynamic Workflow (3 subagents)', + description: 'Review the diff', + }, + { + type: 'workflow_plan', + agent_count: 3, + items: ['src/a.ts', 'src/b.ts', 'src/c.ts'], + prompt_tokens: 42, + prompt_template: 'Review {{item}}', + model: 'claude-sonnet-4', + }, + ]); + }); + + it('adds no plan block to a plain agent call', () => { + const adapted = adaptApprovalRequest({ + toolCallId: 'tc-agent', + toolName: 'Agent', + action: 'run', + display: { + kind: 'agent_call', + agent_name: 'coder', + prompt: 'Fix the build', + }, + }); + + expect(adapted.display).toEqual([ + { type: 'invocation', kind: 'agent', name: 'coder', description: 'Fix the build' }, + ]); + }); + it('maps approved-for-session responses into core approval payloads', () => { expect( adaptPanelResponse({ diff --git a/packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts b/packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts new file mode 100644 index 00000000..156cfbfb --- /dev/null +++ b/packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts @@ -0,0 +1,196 @@ +import { constants, promises as fs } from 'node:fs'; + +import path from 'pathe'; + +import { resolveSafePath } from '../../services/fs/fsPathSafety'; +import { normalizeSkillName } from '../../skill/types'; + +/** + * A saved workflow's name becomes both a directory name and a slash command, + * so it has to survive being neither. Lowercase alphanumeric words joined by + * single hyphens is the whole alphabet — no separators, no dots, nothing that + * `path.join` would resolve upwards. + */ +const SAFE_SKILL_NAME = /^[a-z0-9]+(?:-[a-z0-9]+)*$/u; + +/** + * Slug for a saved workflow, or a throw naming the offending input. + * + * `normalizeSkillName` only lowercases — it is not a sanitizer. Leaning on it + * alone would let `../../etc` through `path.join` and straight out of the + * project root, so the result is validated here rather than assumed safe. + */ +export function savedWorkflowSkillName(name: string): string { + const slug = normalizeSkillName(name.trim()).replaceAll(/\s+/gu, '-'); + if (!SAFE_SKILL_NAME.test(slug)) { + throw new Error( + `Cannot save workflow skill: name ${JSON.stringify(name)} is not a valid skill name. Use letters, digits and hyphens.`, + ); + } + return slug; +} + +export type SavedWorkflowScope = 'project' | 'personal'; + +export interface SavedWorkflow { + readonly name: string; + readonly description: string; + readonly subagentType?: string; + readonly promptTemplate?: string; + readonly model?: string; + readonly effort?: string; + readonly outputSchema?: Record; +} + +/** + * Directory where a saved dynamic-workflow skill lands. + * + * Project scope: `/.pythinker-code/skills/`. + * Personal scope: `/skills/`. brandHomeDir + * already IS the brand data dir (~/.pythinker-code or $PYTHINKER_CODE_HOME), + * so it must not gain another `.pythinker-code` segment — that would nest + * twice. The project path does need the `.pythinker-code` segment; the + * asymmetry is deliberate. + */ +export function savedWorkflowSkillDir(input: { + readonly scope: SavedWorkflowScope; + readonly name: string; + readonly projectRoot: string; + readonly brandHomeDir: string; +}): string { + const normalized = savedWorkflowSkillName(input.name); + if (input.scope === 'project') { + return path.join(input.projectRoot, '.pythinker-code', 'skills', normalized); + } + return path.join(input.brandHomeDir, 'skills', normalized); +} + +/** + * YAML's double-quoted style uses JSON's escapes, so `JSON.stringify` produces + * a valid scalar and handles what hand-rolled quote/backslash escaping misses: + * a newline or control character in a description would otherwise be emitted + * raw and split the frontmatter. + */ +function quoteYamlScalar(value: string): string { + return JSON.stringify(value); +} + +// A fence that cannot collide with the template body: start at ``` and grow +// until the sequence does not appear inside the content. +function renderFence(content: string): string { + let fence = '```'; + while (content.includes(fence)) { + fence += '`'; + } + return fence; +} + +export function renderSavedWorkflowSkill(workflow: SavedWorkflow): string { + const lines: string[] = [ + '---', + `name: ${quoteYamlScalar(workflow.name)}`, + `description: ${quoteYamlScalar(workflow.description)}`, + ]; + if (workflow.subagentType !== undefined) { + lines.push(`subagent-type: ${quoteYamlScalar(workflow.subagentType)}`); + } + if (workflow.model !== undefined) { + lines.push(`model: ${quoteYamlScalar(workflow.model)}`); + } + if (workflow.effort !== undefined) { + lines.push(`effort: ${quoteYamlScalar(workflow.effort)}`); + } + lines.push('---', '', `# ${workflow.description}`); + if (workflow.promptTemplate !== undefined) { + const fence = renderFence(workflow.promptTemplate); + lines.push('', '## Prompt template', '', fence, workflow.promptTemplate, fence); + } + if (workflow.outputSchema !== undefined) { + lines.push( + '', + '## Output schema', + '', + '```json', + JSON.stringify(workflow.outputSchema, null, 2), + '```', + ); + } + return `${lines.join('\n')}\n`; +} + +/** + * Write a saved workflow to disk and return the directory it landed in. + * + * Lives here rather than in the slash command so every surface that can run a + * workflow can also keep one. The name is validated before any directory is + * created, so a rejected name leaves nothing behind. + * + * A validated name is not enough on its own. Agents work in repositories they + * did not write, and a checked-out tree can already contain + * `.pythinker-code/skills//SKILL.md` as a symlink pointing anywhere on + * the machine — saving a workflow would then write through it. The resolved + * path is checked against the scope root before the write, and the write + * itself refuses to follow a final symlink, so neither a planted link nor one + * swapped in afterwards is followed. + */ +export async function writeSavedWorkflowSkill(input: { + readonly scope: SavedWorkflowScope; + readonly workflow: SavedWorkflow; + readonly projectRoot: string; + readonly brandHomeDir: string; +}): Promise { + const name = savedWorkflowSkillName(input.workflow.name); + const root = input.scope === 'project' ? input.projectRoot : input.brandHomeDir; + const dir = savedWorkflowSkillDir({ + scope: input.scope, + name: input.workflow.name, + projectRoot: input.projectRoot, + brandHomeDir: input.brandHomeDir, + }); + const content = renderSavedWorkflowSkill({ ...input.workflow, name }); + const skillMdPath = path.join(dir, 'SKILL.md'); + + // Before `mkdir`, not after: a symlinked `skills/` would otherwise have a + // directory created through it and left behind outside the project even + // though the write was refused. + await assertResolvesInsideRoot(root, dir); + await fs.mkdir(dir, { recursive: true }); + // Again once the directory exists, so a `SKILL.md` symlink planted inside it + // is resolved rather than treated as the not-yet-existing tail. + await assertResolvesInsideRoot(root, skillMdPath); + await writeFileNoFollow(skillMdPath, content); + return dir; +} + +/** Throws when `target` resolves outside `root` once every symlink is followed. */ +async function assertResolvesInsideRoot(root: string, target: string): Promise { + // `target` was built from `root` verbatim, so take the relative path against + // that same un-resolved root. Measuring it against the realpath instead makes + // every `/var` -> `/private/var` style link look like a `..` escape. + const relative = path.relative(root, target); + // `resolveSafePath` rejects an absolute or `..`-bearing input outright, and + // otherwise resolves the longest existing prefix through its symlinks before + // checking containment — which is exactly the planted-link case. + await resolveSafePath(await fs.realpath(root), relative); +} + +/** + * Write without following a symlink at the final path component. + * + * `O_NOFOLLOW` is POSIX; on a platform without it the constant is undefined and + * the containment check above is the guard, so the flag is added only when the + * runtime offers it rather than failing the save outright. + */ +async function writeFileNoFollow(target: string, content: string): Promise { + const noFollow = constants.O_NOFOLLOW ?? 0; + const handle = await fs.open( + target, + constants.O_WRONLY | constants.O_CREAT | constants.O_TRUNC | noFollow, + 0o600, + ); + try { + await handle.writeFile(content, 'utf8'); + } finally { + await handle.close(); + } +} diff --git a/packages/agent-core/src/agent/index.ts b/packages/agent-core/src/agent/index.ts index 92da6b3e..c90e03d5 100644 --- a/packages/agent-core/src/agent/index.ts +++ b/packages/agent-core/src/agent/index.ts @@ -66,6 +66,13 @@ export { assertAgentWireProtocolVersion, } from './records'; export type { DynamicWorkflowModeTrigger } from './dynamic-workflow'; +export { + renderSavedWorkflowSkill, + savedWorkflowSkillDir, + savedWorkflowSkillName, + writeSavedWorkflowSkill, +} from './dynamic-workflow/save-as-skill'; +export type { SavedWorkflow, SavedWorkflowScope } from './dynamic-workflow/save-as-skill'; export type { BuiltinTool, ToolInfo, ToolSource, UserToolRegistration } from './tool'; export * from './goal'; diff --git a/packages/agent-core/src/agent/permission/policies/dynamic-workflow-mode-approve.ts b/packages/agent-core/src/agent/permission/policies/dynamic-workflow-mode-approve.ts deleted file mode 100644 index f01376bc..00000000 --- a/packages/agent-core/src/agent/permission/policies/dynamic-workflow-mode-approve.ts +++ /dev/null @@ -1,16 +0,0 @@ -import type { Agent } from '../..'; -import type { PermissionPolicy, PermissionPolicyContext, PermissionPolicyResult } from '../types'; - -export class DynamicWorkflowModeApprovePermissionPolicy implements PermissionPolicy { - readonly name = 'dynamic-workflow-mode-approve'; - - constructor(private readonly agent: Agent) {} - - evaluate(context: PermissionPolicyContext): PermissionPolicyResult | undefined { - if (context.toolCall.name !== 'DynamicWorkflow') return; - if (!this.agent.dynamicWorkflowMode.isActive) return; - return { - kind: 'approve', - }; - } -} diff --git a/packages/agent-core/src/agent/permission/policies/index.ts b/packages/agent-core/src/agent/permission/policies/index.ts index 652f2039..f33000ec 100644 --- a/packages/agent-core/src/agent/permission/policies/index.ts +++ b/packages/agent-core/src/agent/permission/policies/index.ts @@ -15,7 +15,6 @@ import { PlanModeGuardDenyPermissionPolicy } from './plan-mode-guard-deny'; import { PlanModeToolApprovePermissionPolicy } from './plan-mode-tool-approve'; import { PreToolCallHookPermissionPolicy } from './pre-tool-call-hook'; import { SessionApprovalHistoryPermissionPolicy } from './session-approval-history'; -import { DynamicWorkflowModeApprovePermissionPolicy } from './dynamic-workflow-mode-approve'; import { UserConfiguredAllowPermissionPolicy, UserConfiguredAskPermissionPolicy, @@ -54,8 +53,12 @@ export function createPermissionDecisionPolicies(agent: Agent): PermissionPolicy new GitControlPathAccessAskPermissionPolicy(agent), // yolo mode → approve. new YoloModeApprovePermissionPolicy(agent), - // Dynamic Workflow mode keeps DynamicWorkflow available without making it a globally default-approved tool. - new DynamicWorkflowModeApprovePermissionPolicy(agent), + // No Dynamic Workflow policy sits here. Dynamic Workflow mode used to + // approve every DynamicWorkflow call outright, which only ever fired in + // manual mode (auto approves above, yolo just above) and so made the plan + // preview unreachable for the one mode that asks for it. A DynamicWorkflow + // call in manual mode now falls through to the ask below and renders its + // plan; `session-approval-history` remembers the answer per description. // Tool is in the default-approve list (read-only / UI helpers) → approve. new DefaultToolApprovePermissionPolicy(), // Write/Edit on POSIX paths inside cwd inside a git work tree → approve. diff --git a/packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts b/packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts index 190ec7a5..33fd8245 100644 --- a/packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts +++ b/packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts @@ -1,3 +1,5 @@ +import { createHash } from 'node:crypto'; + import { z } from 'zod'; import type { WorkflowWarningEvent } from '@pythoughts/protocol'; @@ -17,7 +19,9 @@ import { workflowSizeGuidelineTarget, } from '../../../agent/dynamic-workflow/size-guideline'; import { generateWorkflowRunId } from '../../../agent/dynamic-workflow/run-id'; +import { estimateTokens } from '../../../utils/tokens'; import { toInputJsonSchema } from '../../support/input-schema'; +import { literalRulePattern, matchesGlobRuleSubject } from '../../support/rule-match'; import DYNAMIC_WORKFLOW_DESCRIPTION from './dynamic-workflow.md?raw'; const DEFAULT_SUBAGENT_TYPE = 'coder'; @@ -160,20 +164,27 @@ export class DynamicWorkflowTool implements BuiltinTool matchesGlobRuleSubject(ruleArgs, approvalSubject), execute: (ctx) => this.execution(args, ctx), }; } @@ -312,9 +323,7 @@ function createDynamicWorkflowSpecs( } if (items.length > 0) { items.forEach((item, index) => { - const prompt = promptTemplate === undefined - ? item - : promptTemplate.split(PROMPT_TEMPLATE_PLACEHOLDER).join(item); + const prompt = renderItemPrompt(item, promptTemplate); const previousIndex = seenPrompts.get(prompt); if (previousIndex !== undefined) { throw new Error( @@ -337,6 +346,98 @@ function hasMinimumDynamicWorkflowInputs(itemCount: number, resumeCount: number) return resumeCount > 0 || itemCount >= 2; } +/** + * What the call is about to launch, for the approval panel — the counted + * subagents, the work each one gets, and how much prompt that adds up to. + * + * Built from the arguments alone and deliberately total: `resolveExecution` + * runs before `createDynamicWorkflowSpecs` validates anything, so a call this + * function threw on would never reach the panel that exists to refuse it. + * `prompt_tokens` is the summed prompt estimate, which is a real input size — + * not a guess at what the run will finally cost. + */ +function dynamicWorkflowPreview(args: DynamicWorkflowToolInput): { + agent_count: number; + items: string[]; + prompt_tokens: number; + prompt_template?: string; + model?: string; +} { + const { items } = normalizeWorkflowItems(args.items); + const promptTemplate = normalizeOptionalString(args.prompt_template); + const resumeIds = Object.keys(args.resume_agent_ids ?? {}); + const prompts = [ + ...Object.values(args.resume_agent_ids ?? {}), + ...items.map((item) => renderItemPrompt(item, promptTemplate)), + ]; + return { + agent_count: prompts.length, + // Resumed subagents carry an id rather than an item, so label them to keep + // the list the same length as the count it sits under. The prompt goes in + // too: it is the instruction that subagent will actually receive, and the + // approval digest is built from this list — an id alone would let a second + // call keep the same ids, change what it tells them to do, and match the + // earlier grant. + items: [ + ...resumeIds.map( + (agentId) => `resume ${agentId}: ${(args.resume_agent_ids ?? {})[agentId] ?? ''}`, + ), + ...items, + ], + prompt_tokens: prompts.reduce((total, prompt) => total + estimateTokens(prompt), 0), + prompt_template: promptTemplate, + model: normalizeOptionalString(args.model), + }; +} + +/** + * Characters a permission-rule subject can carry safely. The rule DSL parses + * `Tool(subject)` by splitting on the first paren, and the subject is then glob + * matched — so parens break parsing outright and `{}[]*?!+@|` survive neither + * escaping nor picomatch reliably. Everything else is dropped from the readable + * half of the subject; the digest carries the precision. + */ +const RULE_SUBJECT_UNSAFE = /[^a-zA-Z0-9 ._-]/gu; + +/** + * Subject a session approval is recorded against: a readable prefix plus a + * digest of the whole plan. + * + * Every field that changes what actually runs feeds the digest, the item list + * included. Keying on the description alone would let a later call keep the + * description, swap in a different 128-item list, and ride in on the earlier + * grant — the precise fan-out the preview exists to expose. Two plans share a + * grant only when they would launch the same work. + * + * The plan cannot be the subject verbatim: a JSON blob does not survive the + * rule DSL. Hence digest, with a trimmed description kept in front so a + * recorded rule is still recognisable. + */ +function dynamicWorkflowApprovalSubject( + args: DynamicWorkflowToolInput, + workflow: { readonly items: readonly string[]; readonly agent_count: number }, +): string { + const plan = JSON.stringify({ + description: args.description, + subagentType: normalizeOptionalString(args.subagent_type), + promptTemplate: normalizeOptionalString(args.prompt_template), + model: normalizeOptionalString(args.model), + effort: normalizeOptionalString(args.effort), + outputSchema: args.output_schema, + agentCount: workflow.agent_count, + items: workflow.items, + }); + const digest = createHash('sha256').update(plan).digest('hex').slice(0, 16); + const label = args.description.replace(RULE_SUBJECT_UNSAFE, ' ').trim(); + return label.length === 0 ? digest : `${label} ${digest}`; +} + +function renderItemPrompt(item: string, promptTemplate: string | undefined): string { + return promptTemplate === undefined + ? item + : promptTemplate.split(PROMPT_TEMPLATE_PLACEHOLDER).join(item); +} + /** * Trims items and drops the blank ones, reporting how many went. * diff --git a/packages/agent-core/test/agent/dynamic-workflow-save.test.ts b/packages/agent-core/test/agent/dynamic-workflow-save.test.ts new file mode 100644 index 00000000..205b8dcb --- /dev/null +++ b/packages/agent-core/test/agent/dynamic-workflow-save.test.ts @@ -0,0 +1,290 @@ +import { promises as fs } from 'node:fs'; +import { tmpdir } from 'node:os'; + +import { load as loadYaml } from 'js-yaml'; +import path from 'pathe'; +import { describe, expect, it } from 'vitest'; + +import { + renderSavedWorkflowSkill, + savedWorkflowSkillDir, + writeSavedWorkflowSkill, +} from '../../src/agent/dynamic-workflow/save-as-skill'; +import { parseSkillFromFile } from '../../src/skill/parser'; + +function frontmatterOf(rendered: string): string { + const lines = rendered.split('\n'); + expect(lines[0]).toBe('---'); + const end = lines.indexOf('---', 1); + expect(end).toBeGreaterThan(1); + return lines.slice(1, end).join('\n'); +} + +describe('savedWorkflowSkillDir', () => { + it('project scope path has exactly one .pythinker-code segment', () => { + const dir = savedWorkflowSkillDir({ + scope: 'project', + name: 'review', + projectRoot: '/repo', + brandHomeDir: '/home/user/.pythinker-code', + }); + expect(dir).toBe('/repo/.pythinker-code/skills/review'); + const segments = dir.split('/').filter((part) => part === '.pythinker-code'); + expect(segments).toHaveLength(1); + }); + + it('personal scope path has no added .pythinker-code segment', () => { + const dir = savedWorkflowSkillDir({ + scope: 'personal', + name: 'review', + projectRoot: '/repo', + brandHomeDir: '/home/user/.pythinker-code', + }); + expect(dir).toBe('/home/user/.pythinker-code/skills/review'); + const segments = dir.split('/').filter((part) => part === '.pythinker-code'); + expect(segments).toHaveLength(1); + }); + + it('slugifies a mixed-case name with spaces into the directory', () => { + const dir = savedWorkflowSkillDir({ + scope: 'project', + name: ' My Cool Workflow ', + projectRoot: '/repo', + brandHomeDir: '/home/user/.pythinker-code', + }); + expect(dir).toBe('/repo/.pythinker-code/skills/my-cool-workflow'); + }); + + // `normalizeSkillName` only lowercases. Trusting it as a sanitizer let + // `../../..` through `path.join` and resolved the saved skill clean out of + // the project root, to wherever the traversal pointed. + it.each([ + ['..', '..'], + ['traversal', '../../../../tmp/pwned'], + ['separator', 'a/b'], + ['backslash', 'a\\b'], + ['empty', ''], + ['blank', ' '], + ['dotted', 'a.b'], + ['leading hyphen', '-lead'], + ])('refuses to build a path from a %s name', (_label, name) => { + for (const scope of ['project', 'personal'] as const) { + expect(() => + savedWorkflowSkillDir({ + scope, + name, + projectRoot: '/repo', + brandHomeDir: '/home/user/.pythinker-code', + }), + ).toThrow(/not a valid skill name/); + } + }); +}); + +describe('renderSavedWorkflowSkill', () => { + it('omits absent optional keys and keeps the documented key order', () => { + const minimal = renderSavedWorkflowSkill({ + name: 'review', + description: 'Review the diff', + }); + const minimalFrontmatter = (loadYaml(frontmatterOf(minimal)) ?? {}) as Record< + string, + unknown + >; + expect(Object.keys(minimalFrontmatter)).toEqual(['name', 'description']); + + const full = renderSavedWorkflowSkill({ + name: 'review', + description: 'Review the diff', + subagentType: 'reviewer', + model: 'claude-sonnet-4', + effort: 'high', + }); + const fullFrontmatter = (loadYaml(frontmatterOf(full)) ?? {}) as Record< + string, + unknown + >; + expect(Object.keys(fullFrontmatter)).toEqual([ + 'name', + 'description', + 'subagent-type', + 'model', + 'effort', + ]); + expect(fullFrontmatter).toMatchObject({ + name: 'review', + description: 'Review the diff', + 'subagent-type': 'reviewer', + model: 'claude-sonnet-4', + effort: 'high', + }); + }); + + it('double-quotes and escapes a description with `: `, quotes, and backslashes', () => { + const rendered = renderSavedWorkflowSkill({ + name: 'triage', + description: 'Triage: the "urgent" queue #1 (C:\\work\\files)', + }); + const frontmatter = frontmatterOf(rendered); + expect(frontmatter).toContain( + 'description: "Triage: the \\"urgent\\" queue #1 (C:\\\\work\\\\files)"', + ); + const parsed = (loadYaml(frontmatter) ?? {}) as Record; + expect(parsed).toEqual({ + name: 'triage', + description: 'Triage: the "urgent" queue #1 (C:\\work\\files)', + }); + }); + + it('lengthens the fence when the template contains a ``` sequence', () => { + const template = 'Run this:\n```\nls -la\n```\nThen report.'; + const rendered = renderSavedWorkflowSkill({ + name: 'run', + description: 'Run a command', + promptTemplate: template, + }); + expect(rendered).toContain('## Prompt template'); + expect(rendered).toContain(`\`\`\`\`\n${template}\n\`\`\`\``); + expect(rendered.trimEnd().endsWith('````')).toBe(true); + }); + + it('renders the output schema block only when outputSchema is set', () => { + const withSchema = renderSavedWorkflowSkill({ + name: 'plan', + description: 'Make a plan', + outputSchema: { type: 'object', properties: { steps: { type: 'array' } } }, + }); + expect(withSchema).toContain('## Output schema'); + expect(withSchema).toContain('```json'); + expect(withSchema).toContain('"type": "object"'); + + const withoutSchema = renderSavedWorkflowSkill({ + name: 'plan', + description: 'Make a plan', + }); + expect(withoutSchema).not.toContain('## Output schema'); + expect(withoutSchema).not.toContain('```json'); + }); + + it('leads the body with a `# ` heading', () => { + const rendered = renderSavedWorkflowSkill({ + name: 'review', + description: 'Review the diff', + promptTemplate: 'Inspect the changes.', + }); + expect(rendered).toContain('\n---\n\n# Review the diff\n'); + }); +}); + +// Agents work in repositories they did not write. A checked-out tree can +// already contain the saved-skill path as a symlink pointing anywhere on the +// machine, and a validated name does nothing about that — the write would +// simply follow it. +describe('writeSavedWorkflowSkill refuses to write through a symlink', () => { + const workflow = { name: 'audit', description: 'Audit routes' }; + + it('refuses when SKILL.md is a symlink pointing outside the project', async () => { + const root = await fs.mkdtemp(path.join(tmpdir(), 'workflow-symlink-')); + try { + const outside = path.join(root, 'outside.txt'); + await fs.writeFile(outside, 'original', 'utf8'); + const skillDir = path.join(root, 'project', '.pythinker-code', 'skills', 'audit'); + await fs.mkdir(skillDir, { recursive: true }); + await fs.symlink(outside, path.join(skillDir, 'SKILL.md')); + + await expect( + writeSavedWorkflowSkill({ + scope: 'project', + workflow, + projectRoot: path.join(root, 'project'), + brandHomeDir: path.join(root, 'home'), + }), + ).rejects.toThrow(/rejected \(symlink_outside_cwd\)/u); + + // The planted target must be untouched, not merely "an error was thrown". + expect(await fs.readFile(outside, 'utf8')).toBe('original'); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + + it('refuses when a parent directory is a symlink pointing outside the project', async () => { + const root = await fs.mkdtemp(path.join(tmpdir(), 'workflow-symlink-dir-')); + try { + const projectRoot = path.join(root, 'project'); + const escape = path.join(root, 'escape'); + await fs.mkdir(escape, { recursive: true }); + await fs.mkdir(path.join(projectRoot, '.pythinker-code'), { recursive: true }); + await fs.symlink(escape, path.join(projectRoot, '.pythinker-code', 'skills')); + + await expect( + writeSavedWorkflowSkill({ + scope: 'project', + workflow, + projectRoot, + brandHomeDir: path.join(root, 'home'), + }), + ).rejects.toThrow(/rejected \(symlink_outside_cwd\)/u); + + expect(await fs.readdir(escape)).toEqual([]); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + + it('still writes a normal saved workflow', async () => { + const root = await fs.mkdtemp(path.join(tmpdir(), 'workflow-ok-')); + try { + const dir = await writeSavedWorkflowSkill({ + scope: 'project', + workflow, + projectRoot: root, + brandHomeDir: path.join(root, 'home'), + }); + expect(await fs.readFile(path.join(dir, 'SKILL.md'), 'utf8')).toContain('name: "audit"'); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); +}); + +// Rendering valid-looking Markdown is not the contract — being loadable as a +// skill is. This drives the real parser the discovery path uses, so a saved +// workflow that cannot be read back fails here rather than at the next launch. +describe('a saved workflow round-trips through the skill parser', () => { + it('parses back into a skill carrying the workflow fields', async () => { + const dir = await fs.mkdtemp(path.join(tmpdir(), 'workflow-skill-')); + try { + const skillMdPath = path.join(dir, 'SKILL.md'); + await fs.writeFile( + skillMdPath, + renderSavedWorkflowSkill({ + name: 'review-diff', + description: 'Triage: the "urgent" queue', + subagentType: 'reviewer', + model: 'claude-sonnet-4', + effort: 'high', + promptTemplate: 'Review {{item}} and report.', + outputSchema: { type: 'object' }, + }), + 'utf8', + ); + + const skill = await parseSkillFromFile({ + skillMdPath, + skillDirName: 'review-diff', + source: 'project', + }); + + expect(skill.name).toBe('review-diff'); + expect(skill.description).toBe('Triage: the "urgent" queue'); + expect(skill.content).toContain('Review {{item}} and report.'); + expect(skill.metadata).toMatchObject({ + model: 'claude-sonnet-4', + effort: 'high', + }); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/agent-core/test/agent/permission.test.ts b/packages/agent-core/test/agent/permission.test.ts index fe9c2fe5..e3c14cbe 100644 --- a/packages/agent-core/test/agent/permission.test.ts +++ b/packages/agent-core/test/agent/permission.test.ts @@ -22,7 +22,6 @@ import { AutoModeApprovePermissionPolicy } from '../../src/agent/permission/poli import { AutoModeAskUserQuestionDenyPermissionPolicy } from '../../src/agent/permission/policies/auto-mode-ask-user-question-deny'; import { FallbackAskPermissionPolicy } from '../../src/agent/permission/policies/fallback-ask'; import { createPermissionDecisionPolicies } from '../../src/agent/permission/policies'; -import { DynamicWorkflowModeApprovePermissionPolicy } from '../../src/agent/permission/policies/dynamic-workflow-mode-approve'; import { YoloModeApprovePermissionPolicy } from '../../src/agent/permission/policies/yolo-mode-approve'; import { ToolAccesses } from '../../src/loop'; import type { ToolInputDisplay } from '../../src/tools/display'; @@ -749,7 +748,6 @@ describe('Permission policy chain', () => { 'sensitive-file-access-ask', 'git-control-path-access-ask', 'yolo-mode-approve', - 'dynamic-workflow-mode-approve', 'default-tool-approve', 'git-cwd-write-approve', 'fallback-ask', @@ -792,6 +790,49 @@ describe('Permission policy chain', () => { }), ); }); + + // Dynamic Workflow mode once approved every DynamicWorkflow call on its own. + // That policy only ever fired in manual mode, so the one mode whose whole + // point is to ask was the one mode that never saw the plan. Manual mode must + // reach `fallback-ask` so the approval can carry a preview of the fan-out. + it('asks before a DynamicWorkflow call in manual mode even with workflow mode active', async () => { + const { manager, requestApproval, telemetryTrack } = makePermissionManager( + async () => ({ decision: 'approved' }), + { dynamicWorkflowModeActive: true }, + ); + manager.mode = 'manual'; + + await manager.beforeToolCall( + hookContext({ id: 'call_dynamic_workflow', toolName: 'DynamicWorkflow' }), + ); + + expect(requestApproval).toHaveBeenCalledTimes(1); + expect(telemetryTrack).toHaveBeenCalledWith( + 'permission_policy_decision', + expect.objectContaining({ + policy_name: 'fallback-ask', + tool_name: 'DynamicWorkflow', + permission_mode: 'manual', + decision: 'ask', + }), + ); + }); + + it('still approves a DynamicWorkflow call without asking in auto and yolo mode', async () => { + for (const mode of ['auto', 'yolo'] as const) { + const { manager, requestApproval } = makePermissionManager( + async () => ({ decision: 'approved' }), + { dynamicWorkflowModeActive: true }, + ); + manager.mode = mode; + + await manager.beforeToolCall( + hookContext({ id: `call_dynamic_workflow_${mode}`, toolName: 'DynamicWorkflow' }), + ); + + expect(requestApproval).not.toHaveBeenCalled(); + } + }); }); describe('Simple permission policy direct behavior', () => { @@ -847,23 +888,6 @@ describe('Simple permission policy direct behavior', () => { expect(policy.evaluate()).toEqual({ kind: 'approve' }); }); - it('approves only DynamicWorkflow when dynamic workflow mode is active', () => { - const dynamicWorkflowMode = { isActive: false }; - const agent = { dynamicWorkflowMode } as unknown as Agent; - const policy = new DynamicWorkflowModeApprovePermissionPolicy(agent); - - expect( - policy.evaluate(hookContext({ id: 'call_dynamic_workflow_inactive', toolName: 'DynamicWorkflow' })), - ).toBeUndefined(); - Object.assign(dynamicWorkflowMode, { isActive: true }); - expect( - policy.evaluate(hookContext({ id: 'call_dynamic_workflow_active', toolName: 'DynamicWorkflow' })), - ).toEqual({ kind: 'approve' }); - expect( - policy.evaluate(hookContext({ id: 'call_agent_active', toolName: 'Agent' })), - ).toBeUndefined(); - }); - it('denies DynamicWorkflow mixed with other tool calls in the same response', () => { const policy = new DynamicWorkflowExclusiveDenyPermissionPolicy(); const dynamicWorkflowCall = toolCall('call_dynamic_workflow', 'DynamicWorkflow', { diff --git a/packages/agent-core/test/hooks/runner.test.ts b/packages/agent-core/test/hooks/runner.test.ts index 7688dd08..3205589e 100644 --- a/packages/agent-core/test/hooks/runner.test.ts +++ b/packages/agent-core/test/hooks/runner.test.ts @@ -1,4 +1,12 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; + +// Delegates to the real `spawn` so every other test in this file keeps running +// actual processes; the wrapper exists only so the PowerShell test can assert +// which binary was launched instead of waiting on its output. +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, spawn: vi.fn(actual.spawn) }; +}); const RUNNER_MODULE = '../../src/session/hooks/runner' as string; @@ -140,16 +148,29 @@ describe('runHook process runner', () => { expect(result.stdout?.trim()).toBe('WriteFile'); }); - // PowerShell cold start can exceed several seconds on a loaded CI runner, so the - // hook budget and the surrounding test timeout both need headroom above it, and the - // test timeout must stay above the hook budget so the hook result is what gets asserted. + // Asserting on pwsh's own output would tie this test to PowerShell being installed + // and to its cold start fitting inside the hook budget -- on a loaded CI runner the + // budget expired, `runHook` returned allow with empty streams, and the assertion + // failed with a bare "expected false to be true". Widening it to accept the timeout + // would have made it vacuous: a runner that ignored `shell` entirely would pass. + // The contract worth asserting is the spawn itself, which is deterministic. it('uses a non-interactive PowerShell process when requested', async () => { + const { spawn } = await import('node:child_process'); + const spawnMock = vi.mocked(spawn); + spawnMock.mockClear(); + const runHook = await importRunHook(); - const result = await runHook(`Write-Output 'ok'`, {}, { timeout: 15, shell: 'powershell' }); + await runHook(`Write-Output 'ok'`, {}, { timeout: 5, shell: 'powershell' }); - expect(result.action).toBe('allow'); - expect( - result.stdout?.trim() === 'ok' || result.stderr?.includes('ENOENT') === true, - ).toBe(true); + expect(spawnMock).toHaveBeenCalledTimes(1); + expect(spawnMock.mock.calls[0]?.[0]).toBe( + process.platform === 'win32' ? 'powershell.exe' : 'pwsh', + ); + expect(spawnMock.mock.calls[0]?.[1]).toEqual([ + '-NoProfile', + '-NonInteractive', + '-Command', + `Write-Output 'ok'`, + ]); }, 30_000); }); diff --git a/packages/agent-core/test/tools/builtin-current.test.ts b/packages/agent-core/test/tools/builtin-current.test.ts index 13124c83..d4edb61b 100644 --- a/packages/agent-core/test/tools/builtin-current.test.ts +++ b/packages/agent-core/test/tools/builtin-current.test.ts @@ -19,6 +19,8 @@ import { } from '../../src/agent/dynamic-workflow/run-id'; import { resolveWorkflowSizeGuideline } from '../../src/agent/dynamic-workflow/size-guideline'; import { FLAG_DEFINITIONS, FlagResolver } from '../../src/flags'; +import { matchPermissionRule } from '../../src/agent/permission/matches-rule'; +import { estimateTokens } from '../../src/utils/tokens'; import type { QueuedSubagentRunResult, QueuedSubagentTask, @@ -48,6 +50,7 @@ import { DynamicWorkflowTool, DynamicWorkflowToolInputSchema, isDynamicWorkflowDisabled, + type DynamicWorkflowToolInput, } from '../../src/tools/builtin/collaboration/dynamic-workflow'; const signal = new AbortController().signal; @@ -89,6 +92,21 @@ function mockDynamicWorkflowMode(): DynamicWorkflowMode { return { enter: vi.fn() } as unknown as DynamicWorkflowMode; } +/** + * `resolveExecution` narrowed to its runnable branch. The narrowing lives here + * so the tests that need it stay free of the branch themselves. + */ +function runnableExecution( + tool: DynamicWorkflowTool, + args: DynamicWorkflowToolInput, +): Extract, { execute: unknown }> { + const execution = tool.resolveExecution(args); + if (execution.isError === true) { + throw new TypeError('DynamicWorkflow resolveExecution returned an error'); + } + return execution; +} + function processWithOutput(stdout: string, exitCode = 0): KaosProcess { const stdoutStream = Readable.from([stdout]); const stderrStream = Readable.from([]); @@ -851,17 +869,135 @@ describe('current builtin collaboration tools', () => { expect(result.isError).toBeUndefined(); }); - it('DynamicWorkflow does not expose permission rule argument matching', () => { + // "Approve for this session" is scoped to the workflow that was approved, so + // agreeing to one 3-file review does not silently pre-approve a later 128-agent + // fan-out. The matcher must ship with the keyed rule: an arg-bearing rule with + // no `matchesRule` never matches, which would record the grant and then ignore + // it on every later call. + it('DynamicWorkflow matches a session approval only for the identical plan', () => { + const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); + const subjectOf = (input: Parameters[0]): string => { + // `Tool(subject)` — recover the subject the approval was recorded against. + return runnableExecution(tool, input).approvalRule.slice('DynamicWorkflow('.length, -1); + }; + + const base = { + description: 'Review files', + prompt_template: 'Review {{item}}', + items: ['src/a.ts', 'src/b.ts'], + }; + const execution = runnableExecution(tool, base); + + expect(execution.matchesRule?.(subjectOf(base))).toBe(true); + + // The description is the obvious key and the wrong one: reusing it while + // swapping the item list is exactly how an unreviewed fan-out would ride in + // on an earlier approval. + expect( + execution.matchesRule?.( + subjectOf({ ...base, items: Array.from({ length: 128 }, (_, i) => `src/${String(i)}.ts`) }), + ), + ).toBe(false); + expect(execution.matchesRule?.(subjectOf({ ...base, model: 'other-model' }))).toBe(false); + expect(execution.matchesRule?.(subjectOf({ ...base, subagent_type: 'shell' }))).toBe(false); + expect( + execution.matchesRule?.(subjectOf({ ...base, prompt_template: 'Rewrite {{item}}' })), + ).toBe(false); + }); + + // `matchesRule` alone is not proof: the recorded rule is `Tool(subject)`, + // which is parsed by splitting on the first paren and then glob matched. A + // subject carrying JSON punctuation fails both, so an approval would be + // stored and then never match again. Drive the real matcher, not the callback. + it('DynamicWorkflow session approval survives the permission rule DSL', () => { + const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); + const args = { + // Punctuation the DSL cannot carry, plus a glob character. + description: 'Review (all) files: *.ts {urgent}', + prompt_template: 'Review {{item}}', + items: ['src/a.ts', 'src/b.ts'], + }; + const execution = runnableExecution(tool, args); + + const match = matchPermissionRule({ + rule: { + decision: 'allow', + scope: 'session-runtime', + pattern: execution.approvalRule, + reason: 'approve for session', + }, + toolName: 'DynamicWorkflow', + execution, + }); + + expect(match).toMatchObject({ strategy: 'matches_rule', hasRuleArgs: true }); + }); + + it('DynamicWorkflow previews the fan-out for the approval panel', () => { const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); - const execution = tool.resolveExecution({ + const execution = runnableExecution(tool, { description: 'Review files', prompt_template: 'Review {{item}}', items: ['src/a.ts', 'src/b.ts'], + model: 'deepseek-v4', + }); + + expect(execution.display).toMatchObject({ + kind: 'agent_call', + agent_name: 'Dynamic Workflow (2 subagents)', + workflow: { + agent_count: 2, + items: ['src/a.ts', 'src/b.ts'], + prompt_template: 'Review {{item}}', + model: 'deepseek-v4', + }, }); - if (execution.isError === true) throw new Error('DynamicWorkflow resolveExecution returned an error'); + // Both rendered prompts, not the template once. + const workflow = (execution.display as { workflow: { prompt_tokens: number } }).workflow; + expect(workflow.prompt_tokens).toBe( + estimateTokens('Review src/a.ts') + estimateTokens('Review src/b.ts'), + ); + }); - expect(execution.approvalRule).toBe('DynamicWorkflow'); - expect(execution.matchesRule).toBeUndefined(); + it('DynamicWorkflow counts resumed subagents in the preview', () => { + const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); + const execution = runnableExecution(tool, { + description: 'Finish the sweep', + items: ['src/a.ts', 'src/b.ts'], + resume_agent_ids: { 'agent-7': 'Carry on' }, + }); + + expect(execution.display).toMatchObject({ + agent_name: 'Dynamic Workflow (3 subagents)', + workflow: { + agent_count: 3, + // The resume prompt is shown, not just the agent id: it is the + // instruction that subagent receives, and it feeds the approval digest. + items: ['resume agent-7: Carry on', 'src/a.ts', 'src/b.ts'], + }, + }); + }); + + // A resumed subagent's prompt is what it will actually be told to do. If the + // digest ignored it, a second call could keep the same agent ids, change + // their instructions, and ride in on the first call's session approval. + it('DynamicWorkflow will not reuse an approval when a resume prompt changes', () => { + const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); + const base = { + description: 'Finish the sweep', + items: ['src/a.ts', 'src/b.ts'], + resume_agent_ids: { 'agent-7': 'Carry on' }, + }; + const execution = runnableExecution(tool, base); + const subjectOf = (input: DynamicWorkflowToolInput): string => + runnableExecution(tool, input).approvalRule.slice('DynamicWorkflow('.length, -1); + + expect(execution.matchesRule?.(subjectOf(base))).toBe(true); + expect( + execution.matchesRule?.( + subjectOf({ ...base, resume_agent_ids: { 'agent-7': 'Delete everything instead' } }), + ), + ).toBe(false); }); it('DynamicWorkflow accepts a full item list carrying a blank entry', async () => { diff --git a/packages/node-sdk/src/index.ts b/packages/node-sdk/src/index.ts index 02109975..ff42dfa9 100644 --- a/packages/node-sdk/src/index.ts +++ b/packages/node-sdk/src/index.ts @@ -42,6 +42,13 @@ export type { ImportCatalogProviderResult, } from '#/catalog'; +export { + renderSavedWorkflowSkill, + savedWorkflowSkillDir, + savedWorkflowSkillName, + writeSavedWorkflowSkill, +} from '@pythoughts/agent-core'; +export type { SavedWorkflow, SavedWorkflowScope } from '@pythoughts/agent-core'; export { buildSkillSlashCommands, isUserActivatableSkill } from '#/skill-commands'; export type { SkillSlashCommand, SkillSlashCommands } from '#/skill-commands'; diff --git a/packages/protocol/src/display.ts b/packages/protocol/src/display.ts index d1dd652a..562d36f2 100644 --- a/packages/protocol/src/display.ts +++ b/packages/protocol/src/display.ts @@ -41,6 +41,21 @@ export const ToolInputDisplaySchema = z.discriminatedUnion('kind', [ background: z.boolean().optional(), isolation: z.literal('worktree').optional(), cwd: z.string().optional(), + /** + * Fan-out plan for a Dynamic Workflow call, so an approval can show what + * is about to launch instead of only how many. Absent for a single agent + * call. `prompt_tokens` is the summed estimate of the rendered prompts — + * an input-size figure, not a projected total cost. + */ + workflow: z + .object({ + agent_count: z.number(), + items: z.array(z.string()), + prompt_tokens: z.number(), + prompt_template: z.string().optional(), + model: z.string().optional(), + }) + .optional(), }), z.object({ kind: z.literal('skill_call'),