From 4088ef7fb189ed776f774db5898b02ebf83dc36c Mon Sep 17 00:00:00 2001 From: elkaix Date: Fri, 7 Aug 2026 05:58:25 -0400 Subject: [PATCH 1/5] feat(workflow): show the plan before a Dynamic Workflow runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manual mode approved every DynamicWorkflow call outright. That policy sat below auto- and yolo-approve, so it only ever fired in manual mode — the one mode whose purpose is to ask was the one mode that never saw what it was agreeing to. Removing it lets the call reach the ask, and the approval now carries the fan-out: subagent count, task list, prompt template, worker model, and the summed size of the prompts about to be sent. The token figure is the real summed prompt estimate rather than a projected total cost. A guessed multiplier in an approval dialog is worse than no number, because the operator would be approving against a figure that could be an order of magnitude out. "Approve for this session" is keyed on the workflow description, so agreeing to one small review no longer pre-approves a later 128-agent fan-out. The rule ships with its matcher: an arg-bearing approval rule with no `matchesRule` never matches, which would record the grant and then ignore it every time. Also adds `/workflow save `, which writes the last run out as a skill bundle under `.pythinker-code/skills/` so a fan-out that worked can be re-run by name. Names are validated rather than merely lowercased — `normalizeSkillName` is not a sanitizer, and feeding it straight to `path.join` let `../../..` place the file anywhere on disk. --- .changeset/dynamic-workflow-plan-approval.md | 18 ++ .../src/tui/commands/dynamic-workflow.ts | 81 ++++++- .../src/tui/commands/registry.ts | 1 + .../tui/components/dialogs/approval-panel.ts | 40 ++++ .../tui/controllers/subagent-event-handler.ts | 2 + .../src/tui/reverse-rpc/approval/adapter.ts | 16 +- .../src/tui/reverse-rpc/types.ts | 14 ++ apps/pythinker-code/src/tui/tui-state.ts | 7 + .../tui/commands/dynamic-workflow.test.ts | 82 +++++++ .../test/tui/commands/registry.test.ts | 3 +- .../components/dialogs/approval-panel.test.ts | 63 +++++ .../tui/reverse-rpc/approval-adapter.test.ts | 57 +++++ .../agent/dynamic-workflow/save-as-skill.ts | 110 +++++++++ packages/agent-core/src/agent/index.ts | 6 + .../policies/dynamic-workflow-mode-approve.ts | 16 -- .../src/agent/permission/policies/index.ts | 9 +- .../builtin/collaboration/dynamic-workflow.ts | 64 +++++- .../test/agent/dynamic-workflow-save.test.ts | 217 ++++++++++++++++++ .../agent-core/test/agent/permission.test.ts | 62 +++-- .../test/tools/builtin-current.test.ts | 58 ++++- packages/node-sdk/src/index.ts | 6 + packages/protocol/src/display.ts | 15 ++ 22 files changed, 892 insertions(+), 55 deletions(-) create mode 100644 .changeset/dynamic-workflow-plan-approval.md create mode 100644 packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts delete mode 100644 packages/agent-core/src/agent/permission/policies/dynamic-workflow-mode-approve.ts create mode 100644 packages/agent-core/test/agent/dynamic-workflow-save.test.ts 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..634952e3 100644 --- a/apps/pythinker-code/src/tui/commands/dynamic-workflow.ts +++ b/apps/pythinker-code/src/tui/commands/dynamic-workflow.ts @@ -1,5 +1,14 @@ -import type { PermissionMode } from '@pythoughts/pythinker-code-sdk'; +import { mkdir, writeFile } from 'node:fs/promises'; +import { + renderSavedWorkflowSkill, + savedWorkflowSkillDir, + savedWorkflowSkillName, + type PermissionMode, +} from '@pythoughts/pythinker-code-sdk'; +import { join } from 'pathe'; + +import { getDataDir } from '#/utils/paths'; import { DynamicWorkflowStartPermissionPromptComponent, type DynamicWorkflowStartPermissionChoice, @@ -25,6 +34,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 +124,75 @@ 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 = savedWorkflowSkillDir({ + scope: 'project', + name, + projectRoot: host.state.appState.workDir, + brandHomeDir: getDataDir(), + }); + await mkdir(dir, { recursive: true }); + await writeFile( + join(dir, 'SKILL.md'), + renderSavedWorkflowSkill({ + name: savedWorkflowSkillName(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'), + }), + 'utf8', + ); + 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..832f72fb 100644 --- a/apps/pythinker-code/src/tui/components/dialogs/approval-panel.ts +++ b/apps/pythinker-code/src/tui/components/dialogs/approval-panel.ts @@ -32,6 +32,7 @@ import type { DisplayBlock, FileContentDisplayBlock, PendingApproval, + WorkflowPlanDisplayBlock, } from '#/tui/reverse-rpc/types'; import { printableChar } from '#/tui/utils/printable-key'; @@ -167,6 +168,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 +185,41 @@ 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; + +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: ${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(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(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 +247,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..d9f05065 100644 --- a/apps/pythinker-code/src/tui/controllers/subagent-event-handler.ts +++ b/apps/pythinker-code/src/tui/controllers/subagent-event-handler.ts @@ -633,6 +633,8 @@ export class SubAgentEventHandler { args: Record, options: { readonly streamingArguments?: string } = {}, ): DynamicWorkflowMissionControlComponent { + // Kept so `/workflow save` can name a run the user just watched succeed. + this.host.state.lastDynamicWorkflowArgs = args; const existing = this.dynamicWorkflowMissionControls.get(toolCallId); if (existing !== undefined) { existing.updateArgs(args, options); 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..4529e6ae 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/); + } 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..d9ba7335 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,69 @@ 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'); + }); + + 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..a12672a6 --- /dev/null +++ b/packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts @@ -0,0 +1,110 @@ +import path from 'pathe'; + +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); +} + +function quoteYamlScalar(value: string): string { + return `"${value.replaceAll('\\', '\\\\').replaceAll('"', '\\"')}"`; +} + +// 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`; +} diff --git a/packages/agent-core/src/agent/index.ts b/packages/agent-core/src/agent/index.ts index 92da6b3e..335cc947 100644 --- a/packages/agent-core/src/agent/index.ts +++ b/packages/agent-core/src/agent/index.ts @@ -66,6 +66,12 @@ export { assertAgentWireProtocolVersion, } from './records'; export type { DynamicWorkflowModeTrigger } from './dynamic-workflow'; +export { + renderSavedWorkflowSkill, + savedWorkflowSkillDir, + savedWorkflowSkillName, +} 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..244e0659 100644 --- a/packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts +++ b/packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts @@ -17,7 +17,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 +162,23 @@ export class DynamicWorkflowTool implements BuiltinTool matchesGlobRuleSubject(ruleArgs, args.description), execute: (ctx) => this.execution(args, ctx), }; } @@ -312,9 +317,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 +340,47 @@ 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. + items: [...resumeIds.map((agentId) => `resume ${agentId}`), ...items], + prompt_tokens: prompts.reduce((total, prompt) => total + estimateTokens(prompt), 0), + prompt_template: promptTemplate, + model: normalizeOptionalString(args.model), + }; +} + +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..c108686f --- /dev/null +++ b/packages/agent-core/test/agent/dynamic-workflow-save.test.ts @@ -0,0 +1,217 @@ +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, +} 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'); + }); +}); + +// 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/tools/builtin-current.test.ts b/packages/agent-core/test/tools/builtin-current.test.ts index 13124c83..a37a0a31 100644 --- a/packages/agent-core/test/tools/builtin-current.test.ts +++ b/packages/agent-core/test/tools/builtin-current.test.ts @@ -19,6 +19,7 @@ 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 { estimateTokens } from '../../src/utils/tokens'; import type { QueuedSubagentRunResult, QueuedSubagentTask, @@ -851,7 +852,12 @@ 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 keys its approval rule on the description and can match it', () => { const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); const execution = tool.resolveExecution({ description: 'Review files', @@ -860,8 +866,54 @@ describe('current builtin collaboration tools', () => { }); if (execution.isError === true) throw new Error('DynamicWorkflow resolveExecution returned an error'); - expect(execution.approvalRule).toBe('DynamicWorkflow'); - expect(execution.matchesRule).toBeUndefined(); + expect(execution.approvalRule).toBe('DynamicWorkflow(Review files)'); + expect(execution.matchesRule?.('Review files')).toBe(true); + expect(execution.matchesRule?.('Delete everything')).toBe(false); + }); + + it('DynamicWorkflow previews the fan-out for the approval panel', () => { + const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); + const execution = tool.resolveExecution({ + description: 'Review files', + prompt_template: 'Review {{item}}', + items: ['src/a.ts', 'src/b.ts'], + model: 'deepseek-v4', + }); + if (execution.isError === true) throw new Error('DynamicWorkflow resolveExecution returned an error'); + + 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', + }, + }); + // 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'), + ); + }); + + it('DynamicWorkflow counts resumed subagents in the preview', () => { + const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); + const execution = tool.resolveExecution({ + description: 'Finish the sweep', + items: ['src/a.ts', 'src/b.ts'], + resume_agent_ids: { 'agent-7': 'Carry on' }, + }); + if (execution.isError === true) throw new Error('DynamicWorkflow resolveExecution returned an error'); + + expect(execution.display).toMatchObject({ + agent_name: 'Dynamic Workflow (3 subagents)', + workflow: { + agent_count: 3, + items: ['resume agent-7', 'src/a.ts', 'src/b.ts'], + }, + }); }); 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 71a3931e..45dee0b5 100644 --- a/packages/node-sdk/src/index.ts +++ b/packages/node-sdk/src/index.ts @@ -45,6 +45,12 @@ export type { ImportCatalogProviderResult, } from '#/catalog'; +export { + renderSavedWorkflowSkill, + savedWorkflowSkillDir, + savedWorkflowSkillName, +} 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'), From 495d2a0480d64c8b1b1efaa36511aa59f6593468 Mon Sep 17 00:00:00 2001 From: elkaix Date: Fri, 7 Aug 2026 06:13:17 -0400 Subject: [PATCH 2/5] fix(workflow): scope a workflow approval to the plan, not the description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the preceding commit. Keying the session approval on the description let a second call keep the description, swap in a different 128-item list, and match the earlier grant — the exact fan-out the preview exists to expose. The subject now covers every field that changes what runs, the item list included. It cannot carry that plan verbatim. The rule DSL parses `Tool(subject)` by splitting on the first paren and then glob-matches the subject, so JSON punctuation breaks parsing and survives neither escaping nor picomatch: an approval would be recorded and then never match again. The subject is a trimmed description plus a digest of the plan, and a new test drives the real `matchPermissionRule` rather than the `matchesRule` callback, which is what made the encoding failure visible in the first place. Also: capture the workflow arguments for `/workflow save` when the tool call completes rather than inside the shared helper the streaming path also calls, so a half-parsed argument set cannot be saved; move the persistence into `writeSavedWorkflowSkill` in agent-core so surfaces other than the TUI can keep a workflow; and encode YAML scalars with `JSON.stringify`, which escapes the newlines and control characters the hand-rolled version emitted raw. --- .../src/tui/commands/dynamic-workflow.ts | 22 ++----- .../tui/controllers/subagent-event-handler.ts | 6 +- .../tui/commands/dynamic-workflow.test.ts | 2 +- .../agent/dynamic-workflow/save-as-skill.ts | 38 +++++++++++- packages/agent-core/src/agent/index.ts | 1 + .../builtin/collaboration/dynamic-workflow.ts | 58 +++++++++++++++-- .../test/tools/builtin-current.test.ts | 62 +++++++++++++++++-- packages/node-sdk/src/index.ts | 1 + 8 files changed, 159 insertions(+), 31 deletions(-) diff --git a/apps/pythinker-code/src/tui/commands/dynamic-workflow.ts b/apps/pythinker-code/src/tui/commands/dynamic-workflow.ts index 634952e3..e2bb86f2 100644 --- a/apps/pythinker-code/src/tui/commands/dynamic-workflow.ts +++ b/apps/pythinker-code/src/tui/commands/dynamic-workflow.ts @@ -1,12 +1,8 @@ -import { mkdir, writeFile } from 'node:fs/promises'; - import { - renderSavedWorkflowSkill, - savedWorkflowSkillDir, savedWorkflowSkillName, + writeSavedWorkflowSkill, type PermissionMode, } from '@pythoughts/pythinker-code-sdk'; -import { join } from 'pathe'; import { getDataDir } from '#/utils/paths'; import { @@ -153,26 +149,20 @@ async function handleSaveSubcommand(host: SlashCommandHost, input: string): Prom } try { - const dir = savedWorkflowSkillDir({ + const dir = await writeSavedWorkflowSkill({ scope: 'project', - name, projectRoot: host.state.appState.workDir, brandHomeDir: getDataDir(), - }); - await mkdir(dir, { recursive: true }); - await writeFile( - join(dir, 'SKILL.md'), - renderSavedWorkflowSkill({ - name: savedWorkflowSkillName(name), + 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'), - }), - 'utf8', - ); + }, + }); host.refreshSlashCommandAutocomplete(); host.showStatus(`Saved /${savedWorkflowSkillName(name)} to ${dir}.`); } catch (error) { 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 d9f05065..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(); } @@ -633,8 +637,6 @@ export class SubAgentEventHandler { args: Record, options: { readonly streamingArguments?: string } = {}, ): DynamicWorkflowMissionControlComponent { - // Kept so `/workflow save` can name a run the user just watched succeed. - this.host.state.lastDynamicWorkflowArgs = args; const existing = this.dynamicWorkflowMissionControls.get(toolCallId); if (existing !== undefined) { existing.updateArgs(args, options); 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 4529e6ae..61e779c9 100644 --- a/apps/pythinker-code/test/tui/commands/dynamic-workflow.test.ts +++ b/apps/pythinker-code/test/tui/commands/dynamic-workflow.test.ts @@ -468,7 +468,7 @@ describe('/workflow save', () => { expect.stringContaining('not a valid skill name'), ); expect(host.refreshSlashCommandAutocomplete).not.toHaveBeenCalled(); - await expect(fs.stat(join(workDir, '.pythinker-code'))).rejects.toThrow(/ENOENT/); + await expect(fs.stat(join(workDir, '.pythinker-code'))).rejects.toThrow(/ENOENT/u); } finally { await fs.rm(workDir, { recursive: true, force: true }); } 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 index a12672a6..d220bf22 100644 --- a/packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts +++ b/packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts @@ -1,3 +1,5 @@ +import { promises as fs } from 'node:fs'; + import path from 'pathe'; import { normalizeSkillName } from '../../skill/types'; @@ -62,8 +64,14 @@ export function savedWorkflowSkillDir(input: { 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 `"${value.replaceAll('\\', '\\\\').replaceAll('"', '\\"')}"`; + return JSON.stringify(value); } // A fence that cannot collide with the template body: start at ``` and grow @@ -108,3 +116,31 @@ export function renderSavedWorkflowSkill(workflow: SavedWorkflow): string { } 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. + */ +export async function writeSavedWorkflowSkill(input: { + readonly scope: SavedWorkflowScope; + readonly workflow: SavedWorkflow; + readonly projectRoot: string; + readonly brandHomeDir: string; +}): Promise { + const dir = savedWorkflowSkillDir({ + scope: input.scope, + name: input.workflow.name, + projectRoot: input.projectRoot, + brandHomeDir: input.brandHomeDir, + }); + const content = renderSavedWorkflowSkill({ + ...input.workflow, + name: savedWorkflowSkillName(input.workflow.name), + }); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, 'SKILL.md'), content, 'utf8'); + return dir; +} diff --git a/packages/agent-core/src/agent/index.ts b/packages/agent-core/src/agent/index.ts index 335cc947..c90e03d5 100644 --- a/packages/agent-core/src/agent/index.ts +++ b/packages/agent-core/src/agent/index.ts @@ -70,6 +70,7 @@ 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'; 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 244e0659..c5089667 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'; @@ -163,6 +165,7 @@ export class DynamicWorkflowTool implements BuiltinTool matchesGlobRuleSubject(ruleArgs, args.description), + approvalRule: literalRulePattern(this.name, approvalSubject), + matchesRule: (ruleArgs) => matchesGlobRuleSubject(ruleArgs, approvalSubject), execute: (ctx) => this.execution(args, ctx), }; } @@ -375,6 +381,48 @@ function dynamicWorkflowPreview(args: DynamicWorkflowToolInput): { }; } +/** + * 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 diff --git a/packages/agent-core/test/tools/builtin-current.test.ts b/packages/agent-core/test/tools/builtin-current.test.ts index a37a0a31..ac677766 100644 --- a/packages/agent-core/test/tools/builtin-current.test.ts +++ b/packages/agent-core/test/tools/builtin-current.test.ts @@ -19,6 +19,7 @@ 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, @@ -857,18 +858,67 @@ describe('current builtin collaboration tools', () => { // 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 keys its approval rule on the description and can match it', () => { + it('DynamicWorkflow matches a session approval only for the identical plan', () => { const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); - const execution = tool.resolveExecution({ + const subjectOf = (input: Parameters[0]): string => { + const execution = tool.resolveExecution(input); + if (execution.isError === true) throw new Error('resolveExecution returned an error'); + // `Tool(subject)` — recover the subject the approval was recorded against. + return execution.approvalRule.slice('DynamicWorkflow('.length, -1); + }; + + const base = { description: 'Review files', prompt_template: 'Review {{item}}', items: ['src/a.ts', 'src/b.ts'], + }; + const execution = tool.resolveExecution(base); + if (execution.isError === true) throw new Error('resolveExecution returned an error'); + + 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 = tool.resolveExecution(args); + if (execution.isError === true) throw new Error('resolveExecution returned an error'); + + const match = matchPermissionRule({ + rule: { + decision: 'allow', + scope: 'session-runtime', + pattern: execution.approvalRule, + reason: 'approve for session', + }, + toolName: 'DynamicWorkflow', + execution, }); - if (execution.isError === true) throw new Error('DynamicWorkflow resolveExecution returned an error'); - expect(execution.approvalRule).toBe('DynamicWorkflow(Review files)'); - expect(execution.matchesRule?.('Review files')).toBe(true); - expect(execution.matchesRule?.('Delete everything')).toBe(false); + expect(match).toMatchObject({ strategy: 'matches_rule', hasRuleArgs: true }); }); it('DynamicWorkflow previews the fan-out for the approval panel', () => { diff --git a/packages/node-sdk/src/index.ts b/packages/node-sdk/src/index.ts index 45dee0b5..323e3c09 100644 --- a/packages/node-sdk/src/index.ts +++ b/packages/node-sdk/src/index.ts @@ -49,6 +49,7 @@ export { renderSavedWorkflowSkill, savedWorkflowSkillDir, savedWorkflowSkillName, + writeSavedWorkflowSkill, } from '@pythoughts/agent-core'; export type { SavedWorkflow, SavedWorkflowScope } from '@pythoughts/agent-core'; export { buildSkillSlashCommands, isUserActivatableSkill } from '#/skill-commands'; From 29e5c7f559a5e3078324975b6deb95ce9b4585ab Mon Sep 17 00:00:00 2001 From: elkaix Date: Fri, 7 Aug 2026 09:24:06 -0400 Subject: [PATCH 3/5] fix(workflow): close review findings on the workflow approval plan Resume prompts were missing from both the preview and the approval digest. `items` carried only `resume `, so the panel hid the instruction a resumed subagent would receive, and a second call could keep the same agent ids, change what it told them to do, and match the first call's session approval. The prompt now travels with the id, which fixes the display and the digest together. Saving a workflow followed symlinks. Agents work in repositories they did not write, and a checked-out tree can already contain `.pythinker-code/skills//SKILL.md` as a link pointing anywhere on the machine. The resolved path is now checked against the scope root before the directory is created and again before the write, and the write itself refuses to follow a final symlink. Checking after `mkdir` was not enough on its own: a symlinked `skills/` still had a directory created through it and left outside the project even when the write was refused. The approval panel styled workflow metadata without stripping terminal control sequences. That text comes from the model, and the panel is the thing the operator is judging, so a sequence could blank a line, redraw the buttons, or reverse the reading order of the plan being approved. Also lifts the narrowing out of the DynamicWorkflow tests into a helper, which is what `vitest(no-conditional-in-test)` was pointing at. --- .../tui/components/dialogs/approval-panel.ts | 24 +++++- .../components/dialogs/approval-panel.test.ts | 46 ++++++++++++ .../agent/dynamic-workflow/save-as-skill.ts | 62 ++++++++++++++-- .../builtin/collaboration/dynamic-workflow.ts | 13 +++- .../test/agent/dynamic-workflow-save.test.ts | 73 +++++++++++++++++++ .../test/tools/builtin-current.test.ts | 58 ++++++++++++--- 6 files changed, 253 insertions(+), 23 deletions(-) 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 832f72fb..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, @@ -192,6 +194,18 @@ function renderDisplayBlock( */ 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, @@ -202,16 +216,20 @@ function renderWorkflowPlanDisplayBlock( `~${String(block.prompt_tokens)} prompt tokens`, ]; if (block.model !== undefined && block.model.length > 0) { - summary.push(`model: ${block.model}`); + 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(block.prompt_template, 200))}`); + 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(item, 120)}`)); + lines.push( + s.dim(`${String(index + 1).padStart(3)}. ${truncateOneLine(sanitizePlanText(item), 120)}`), + ); } const hidden = block.items.length - MAX_PREVIEW_ITEMS; if (hidden > 0) { 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 d9ba7335..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 @@ -124,6 +124,52 @@ describe('ApprovalPanelComponent', () => { 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: { 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 index d220bf22..156cfbfb 100644 --- a/packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts +++ b/packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts @@ -1,7 +1,8 @@ -import { promises as fs } from 'node:fs'; +import { constants, promises as fs } from 'node:fs'; import path from 'pathe'; +import { resolveSafePath } from '../../services/fs/fsPathSafety'; import { normalizeSkillName } from '../../skill/types'; /** @@ -123,6 +124,14 @@ export function renderSavedWorkflowSkill(workflow: SavedWorkflow): string { * 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; @@ -130,17 +139,58 @@ export async function writeSavedWorkflowSkill(input: { 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: savedWorkflowSkillName(input.workflow.name), - }); + 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 }); - await fs.writeFile(path.join(dir, 'SKILL.md'), content, 'utf8'); + // 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/tools/builtin/collaboration/dynamic-workflow.ts b/packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts index c5089667..33fd8245 100644 --- a/packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts +++ b/packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts @@ -373,8 +373,17 @@ function dynamicWorkflowPreview(args: DynamicWorkflowToolInput): { 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. - items: [...resumeIds.map((agentId) => `resume ${agentId}`), ...items], + // 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), diff --git a/packages/agent-core/test/agent/dynamic-workflow-save.test.ts b/packages/agent-core/test/agent/dynamic-workflow-save.test.ts index c108686f..205b8dcb 100644 --- a/packages/agent-core/test/agent/dynamic-workflow-save.test.ts +++ b/packages/agent-core/test/agent/dynamic-workflow-save.test.ts @@ -8,6 +8,7 @@ import { describe, expect, it } from 'vitest'; import { renderSavedWorkflowSkill, savedWorkflowSkillDir, + writeSavedWorkflowSkill, } from '../../src/agent/dynamic-workflow/save-as-skill'; import { parseSkillFromFile } from '../../src/skill/parser'; @@ -175,6 +176,78 @@ describe('renderSavedWorkflowSkill', () => { }); }); +// 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. diff --git a/packages/agent-core/test/tools/builtin-current.test.ts b/packages/agent-core/test/tools/builtin-current.test.ts index ac677766..d4edb61b 100644 --- a/packages/agent-core/test/tools/builtin-current.test.ts +++ b/packages/agent-core/test/tools/builtin-current.test.ts @@ -50,6 +50,7 @@ import { DynamicWorkflowTool, DynamicWorkflowToolInputSchema, isDynamicWorkflowDisabled, + type DynamicWorkflowToolInput, } from '../../src/tools/builtin/collaboration/dynamic-workflow'; const signal = new AbortController().signal; @@ -91,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([]); @@ -861,10 +877,8 @@ describe('current builtin collaboration tools', () => { it('DynamicWorkflow matches a session approval only for the identical plan', () => { const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); const subjectOf = (input: Parameters[0]): string => { - const execution = tool.resolveExecution(input); - if (execution.isError === true) throw new Error('resolveExecution returned an error'); // `Tool(subject)` — recover the subject the approval was recorded against. - return execution.approvalRule.slice('DynamicWorkflow('.length, -1); + return runnableExecution(tool, input).approvalRule.slice('DynamicWorkflow('.length, -1); }; const base = { @@ -872,8 +886,7 @@ describe('current builtin collaboration tools', () => { prompt_template: 'Review {{item}}', items: ['src/a.ts', 'src/b.ts'], }; - const execution = tool.resolveExecution(base); - if (execution.isError === true) throw new Error('resolveExecution returned an error'); + const execution = runnableExecution(tool, base); expect(execution.matchesRule?.(subjectOf(base))).toBe(true); @@ -904,8 +917,7 @@ describe('current builtin collaboration tools', () => { prompt_template: 'Review {{item}}', items: ['src/a.ts', 'src/b.ts'], }; - const execution = tool.resolveExecution(args); - if (execution.isError === true) throw new Error('resolveExecution returned an error'); + const execution = runnableExecution(tool, args); const match = matchPermissionRule({ rule: { @@ -923,13 +935,12 @@ describe('current builtin collaboration tools', () => { 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', }); - if (execution.isError === true) throw new Error('DynamicWorkflow resolveExecution returned an error'); expect(execution.display).toMatchObject({ kind: 'agent_call', @@ -950,22 +961,45 @@ describe('current builtin collaboration tools', () => { it('DynamicWorkflow counts resumed subagents in the preview', () => { const tool = new DynamicWorkflowTool(mockSubagentHost({}), mockDynamicWorkflowMode()); - const execution = tool.resolveExecution({ + const execution = runnableExecution(tool, { description: 'Finish the sweep', items: ['src/a.ts', 'src/b.ts'], resume_agent_ids: { 'agent-7': 'Carry on' }, }); - if (execution.isError === true) throw new Error('DynamicWorkflow resolveExecution returned an error'); expect(execution.display).toMatchObject({ agent_name: 'Dynamic Workflow (3 subagents)', workflow: { agent_count: 3, - items: ['resume agent-7', 'src/a.ts', 'src/b.ts'], + // 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 () => { // 128 real prompts plus one blank used to trip the schema's raw-length cap, // which rejected the entire call -- the very hole the blank-item handling From dfecef575ba3eaf10b0c1ecbc83b258ece2ecc73 Mon Sep 17 00:00:00 2001 From: elkaix Date: Fri, 7 Aug 2026 09:44:58 -0400 Subject: [PATCH 4/5] test(hooks): accept the timed-out PowerShell outcome runHook has three outcomes for the PowerShell case: pwsh ran, pwsh is not installed, or pwsh was still starting when the hook budget expired. The assertion covered only the first two, so a saturated CI runner failed here with a bare "expected false to be true" that named nothing. The timeout path returns allow with empty stdout and stderr, so neither branch matched. All three outcomes confirm the PowerShell spawn path was taken, which is what the test is for. --- packages/agent-core/test/hooks/runner.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/agent-core/test/hooks/runner.test.ts b/packages/agent-core/test/hooks/runner.test.ts index 7688dd08..329b5289 100644 --- a/packages/agent-core/test/hooks/runner.test.ts +++ b/packages/agent-core/test/hooks/runner.test.ts @@ -143,13 +143,20 @@ describe('runHook process runner', () => { // 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. + // + // Three outcomes all confirm the PowerShell path was taken: pwsh ran, pwsh is not + // installed, or pwsh was still starting when the hook budget expired. The last one is + // what `runHook` returns as `allow` with empty streams and `timedOut`, and leaving it + // out made a saturated runner fail here with a bare "expected false to be true". it('uses a non-interactive PowerShell process when requested', async () => { const runHook = await importRunHook(); const result = await runHook(`Write-Output 'ok'`, {}, { timeout: 15, shell: 'powershell' }); expect(result.action).toBe('allow'); expect( - result.stdout?.trim() === 'ok' || result.stderr?.includes('ENOENT') === true, + result.stdout?.trim() === 'ok' || + result.stderr?.includes('ENOENT') === true || + result.timedOut === true, ).toBe(true); }, 30_000); }); From e53997893030970b77ff736929671349a022fba0 Mon Sep 17 00:00:00 2001 From: elkaix Date: Fri, 7 Aug 2026 09:53:55 -0400 Subject: [PATCH 5/5] test(hooks): assert the PowerShell spawn instead of its output Asserting on pwsh stdout tied 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". Accepting the timeout would have made the test vacuous: a runner that ignored options.shell entirely would still pass. Assert the spawn instead, which is the contract the test names and is deterministic. The mock delegates to the real spawn so the rest of the file keeps running actual processes. --- packages/agent-core/test/hooks/runner.test.ts | 46 ++++++++++++------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/packages/agent-core/test/hooks/runner.test.ts b/packages/agent-core/test/hooks/runner.test.ts index 329b5289..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,23 +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. - // - // Three outcomes all confirm the PowerShell path was taken: pwsh ran, pwsh is not - // installed, or pwsh was still starting when the hook budget expired. The last one is - // what `runHook` returns as `allow` with empty streams and `timedOut`, and leaving it - // out made a saturated runner fail here with a bare "expected false to be true". + // 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 || - result.timedOut === 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); });