fix(workflow): scope a workflow approval to the plan, not the description

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.

Author: elkaix

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) {

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<string, unknown>,
     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);

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 });
     }

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<string> {
+  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;
+}

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';

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<DynamicWorkflowToolInput
 
   resolveExecution(args: DynamicWorkflowToolInput): ToolExecution {
     const workflow = dynamicWorkflowPreview(args);
+    const approvalSubject = dynamicWorkflowApprovalSubject(args, workflow);
     return {
       accesses: ToolAccesses.all(),
       description: `Launching Dynamic Workflow: ${args.description}`,
@@ -172,13 +175,16 @@ export class DynamicWorkflowTool implements BuiltinTool<DynamicWorkflowToolInput
         prompt: args.description,
         workflow,
       },
-      // Keyed on the description so "approve for this session" grants this one
-      // workflow, not every DynamicWorkflow call for the rest of the session.
-      // The matcher has to come with it: an arg-bearing rule with no
+      // Keyed on the whole plan, not the tool name and not the description.
+      // The bare name granted every future DynamicWorkflow call; the
+      // description alone would let a second call reuse it and swap in 128
+      // different items, which is exactly the fan-out the preview exists to
+      // show. "Approve for this session" now grants the plan that was shown.
+      // The matcher has to ship with it: an arg-bearing rule with no
       // `matchesRule` never matches, so the grant would be recorded and then
       // silently ignored on every later call.
-      approvalRule: literalRulePattern(this.name, args.description),
-      matchesRule: (ruleArgs) => 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

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<DynamicWorkflowTool['resolveExecution']>[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', () => {

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';