Skip to content

Commit 29e5c7f

Browse files
committed
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 <agentId>`, 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/<name>/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.
1 parent 495d2a0 commit 29e5c7f

6 files changed

Lines changed: 253 additions & 23 deletions

File tree

‎apps/pythinker-code/src/tui/components/dialogs/approval-panel.ts‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
* Container-based component with keyboard navigation.
55
*/
66

7+
import { stripVTControlCharacters } from 'node:util';
8+
79
import {
810
Container,
911
Input,
@@ -192,6 +194,18 @@ function renderDisplayBlock(
192194
*/
193195
const MAX_PREVIEW_ITEMS = 10;
194196

197+
/**
198+
* The plan is the thing being approved, and every field in it came from the
199+
* model. Escape sequences would let that text repaint the panel it is being
200+
* judged in — hide a line, redraw the buttons, or reverse the reading order —
201+
* so they are removed rather than styled. `stripVTControlCharacters` takes the
202+
* CSI and OSC sequences; the class escape then takes the bare control and
203+
* format characters it leaves behind, which include the bidi overrides.
204+
*/
205+
function sanitizePlanText(text: string): string {
206+
return stripVTControlCharacters(text).replaceAll(/[\p{Cc}\p{Cf}]/gu, ' ');
207+
}
208+
195209
function renderWorkflowPlanDisplayBlock(
196210
block: WorkflowPlanDisplayBlock,
197211
s: BlockStyles,
@@ -202,16 +216,20 @@ function renderWorkflowPlanDisplayBlock(
202216
`~${String(block.prompt_tokens)} prompt tokens`,
203217
];
204218
if (block.model !== undefined && block.model.length > 0) {
205-
summary.push(`model: ${block.model}`);
219+
summary.push(`model: ${sanitizePlanText(block.model)}`);
206220
}
207221
const lines = [s.strong(summary.join(' '))];
208222

209223
if (block.prompt_template !== undefined && block.prompt_template.length > 0) {
210-
lines.push(`${s.accent('prompt')} ${s.dim(truncateOneLine(block.prompt_template, 200))}`);
224+
lines.push(
225+
`${s.accent('prompt')} ${s.dim(truncateOneLine(sanitizePlanText(block.prompt_template), 200))}`,
226+
);
211227
}
212228

213229
for (const [index, item] of block.items.slice(0, MAX_PREVIEW_ITEMS).entries()) {
214-
lines.push(s.dim(`${String(index + 1).padStart(3)}. ${truncateOneLine(item, 120)}`));
230+
lines.push(
231+
s.dim(`${String(index + 1).padStart(3)}. ${truncateOneLine(sanitizePlanText(item), 120)}`),
232+
);
215233
}
216234
const hidden = block.items.length - MAX_PREVIEW_ITEMS;
217235
if (hidden > 0) {

‎apps/pythinker-code/test/tui/components/dialogs/approval-panel.test.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,52 @@ describe('ApprovalPanelComponent', () => {
124124
expect(out).toContain('src/c.ts');
125125
});
126126

127+
// The plan is what the operator is judging, and every field in it came from
128+
// the model. Escape sequences left intact could repaint the panel deciding
129+
// their fate — blank a line, redraw the buttons, or reverse the text order.
130+
it('strips terminal control sequences from the plan before painting it', () => {
131+
const esc = String.fromCodePoint(0x1B);
132+
const rtlOverride = String.fromCodePoint(0x202E);
133+
const pending: PendingApproval = {
134+
data: {
135+
id: 'approval_workflow_ansi',
136+
tool_call_id: 'tool_workflow_ansi',
137+
tool_name: 'DynamicWorkflow',
138+
action: 'run',
139+
description: 'Sweep',
140+
display: [
141+
{
142+
type: 'workflow_plan',
143+
agent_count: 2,
144+
items: [
145+
`${esc}[2Kharmless-item`,
146+
// OSC 8 hyperlink, and a right-to-left override.
147+
`${esc}]8;;http://evil.example${esc}\\second-item${rtlOverride}`,
148+
],
149+
prompt_tokens: 12,
150+
prompt_template: `${esc}[31mReview {{item}}`,
151+
model: `${esc}[1msonnet`,
152+
},
153+
],
154+
choices: [{ label: 'Approve once', response: 'approved' }],
155+
},
156+
};
157+
158+
const raw = new ApprovalPanelComponent(pending, () => {}).render(80).join('\n');
159+
const out = strip(raw);
160+
161+
expect(out).toContain('harmless-item');
162+
expect(out).toContain('second-item');
163+
expect(out).toContain('Review {{item}}');
164+
expect(out).toContain('model: sonnet');
165+
// `strip` only removes SGR colour codes, so anything else the model smuggled
166+
// in would still be sitting in `out`.
167+
expect(out).not.toContain(`${esc}[2K`);
168+
expect(out).not.toContain(']8;;');
169+
expect(out).not.toContain('http://evil.example');
170+
expect(out).not.toContain(rtlOverride);
171+
});
172+
127173
it('caps the plan item list and says how many it held back', () => {
128174
const pending: PendingApproval = {
129175
data: {

‎packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts‎

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1-
import { promises as fs } from 'node:fs';
1+
import { constants, promises as fs } from 'node:fs';
22

33
import path from 'pathe';
44

5+
import { resolveSafePath } from '../../services/fs/fsPathSafety';
56
import { normalizeSkillName } from '../../skill/types';
67

78
/**
@@ -123,24 +124,73 @@ export function renderSavedWorkflowSkill(workflow: SavedWorkflow): string {
123124
* Lives here rather than in the slash command so every surface that can run a
124125
* workflow can also keep one. The name is validated before any directory is
125126
* created, so a rejected name leaves nothing behind.
127+
*
128+
* A validated name is not enough on its own. Agents work in repositories they
129+
* did not write, and a checked-out tree can already contain
130+
* `.pythinker-code/skills/<name>/SKILL.md` as a symlink pointing anywhere on
131+
* the machine — saving a workflow would then write through it. The resolved
132+
* path is checked against the scope root before the write, and the write
133+
* itself refuses to follow a final symlink, so neither a planted link nor one
134+
* swapped in afterwards is followed.
126135
*/
127136
export async function writeSavedWorkflowSkill(input: {
128137
readonly scope: SavedWorkflowScope;
129138
readonly workflow: SavedWorkflow;
130139
readonly projectRoot: string;
131140
readonly brandHomeDir: string;
132141
}): Promise<string> {
142+
const name = savedWorkflowSkillName(input.workflow.name);
143+
const root = input.scope === 'project' ? input.projectRoot : input.brandHomeDir;
133144
const dir = savedWorkflowSkillDir({
134145
scope: input.scope,
135146
name: input.workflow.name,
136147
projectRoot: input.projectRoot,
137148
brandHomeDir: input.brandHomeDir,
138149
});
139-
const content = renderSavedWorkflowSkill({
140-
...input.workflow,
141-
name: savedWorkflowSkillName(input.workflow.name),
142-
});
150+
const content = renderSavedWorkflowSkill({ ...input.workflow, name });
151+
const skillMdPath = path.join(dir, 'SKILL.md');
152+
153+
// Before `mkdir`, not after: a symlinked `skills/` would otherwise have a
154+
// directory created through it and left behind outside the project even
155+
// though the write was refused.
156+
await assertResolvesInsideRoot(root, dir);
143157
await fs.mkdir(dir, { recursive: true });
144-
await fs.writeFile(path.join(dir, 'SKILL.md'), content, 'utf8');
158+
// Again once the directory exists, so a `SKILL.md` symlink planted inside it
159+
// is resolved rather than treated as the not-yet-existing tail.
160+
await assertResolvesInsideRoot(root, skillMdPath);
161+
await writeFileNoFollow(skillMdPath, content);
145162
return dir;
146163
}
164+
165+
/** Throws when `target` resolves outside `root` once every symlink is followed. */
166+
async function assertResolvesInsideRoot(root: string, target: string): Promise<void> {
167+
// `target` was built from `root` verbatim, so take the relative path against
168+
// that same un-resolved root. Measuring it against the realpath instead makes
169+
// every `/var` -> `/private/var` style link look like a `..` escape.
170+
const relative = path.relative(root, target);
171+
// `resolveSafePath` rejects an absolute or `..`-bearing input outright, and
172+
// otherwise resolves the longest existing prefix through its symlinks before
173+
// checking containment — which is exactly the planted-link case.
174+
await resolveSafePath(await fs.realpath(root), relative);
175+
}
176+
177+
/**
178+
* Write without following a symlink at the final path component.
179+
*
180+
* `O_NOFOLLOW` is POSIX; on a platform without it the constant is undefined and
181+
* the containment check above is the guard, so the flag is added only when the
182+
* runtime offers it rather than failing the save outright.
183+
*/
184+
async function writeFileNoFollow(target: string, content: string): Promise<void> {
185+
const noFollow = constants.O_NOFOLLOW ?? 0;
186+
const handle = await fs.open(
187+
target,
188+
constants.O_WRONLY | constants.O_CREAT | constants.O_TRUNC | noFollow,
189+
0o600,
190+
);
191+
try {
192+
await handle.writeFile(content, 'utf8');
193+
} finally {
194+
await handle.close();
195+
}
196+
}

‎packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -373,8 +373,17 @@ function dynamicWorkflowPreview(args: DynamicWorkflowToolInput): {
373373
return {
374374
agent_count: prompts.length,
375375
// Resumed subagents carry an id rather than an item, so label them to keep
376-
// the list the same length as the count it sits under.
377-
items: [...resumeIds.map((agentId) => `resume ${agentId}`), ...items],
376+
// the list the same length as the count it sits under. The prompt goes in
377+
// too: it is the instruction that subagent will actually receive, and the
378+
// approval digest is built from this list — an id alone would let a second
379+
// call keep the same ids, change what it tells them to do, and match the
380+
// earlier grant.
381+
items: [
382+
...resumeIds.map(
383+
(agentId) => `resume ${agentId}: ${(args.resume_agent_ids ?? {})[agentId] ?? ''}`,
384+
),
385+
...items,
386+
],
378387
prompt_tokens: prompts.reduce((total, prompt) => total + estimateTokens(prompt), 0),
379388
prompt_template: promptTemplate,
380389
model: normalizeOptionalString(args.model),

‎packages/agent-core/test/agent/dynamic-workflow-save.test.ts‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { describe, expect, it } from 'vitest';
88
import {
99
renderSavedWorkflowSkill,
1010
savedWorkflowSkillDir,
11+
writeSavedWorkflowSkill,
1112
} from '../../src/agent/dynamic-workflow/save-as-skill';
1213
import { parseSkillFromFile } from '../../src/skill/parser';
1314

@@ -175,6 +176,78 @@ describe('renderSavedWorkflowSkill', () => {
175176
});
176177
});
177178

179+
// Agents work in repositories they did not write. A checked-out tree can
180+
// already contain the saved-skill path as a symlink pointing anywhere on the
181+
// machine, and a validated name does nothing about that — the write would
182+
// simply follow it.
183+
describe('writeSavedWorkflowSkill refuses to write through a symlink', () => {
184+
const workflow = { name: 'audit', description: 'Audit routes' };
185+
186+
it('refuses when SKILL.md is a symlink pointing outside the project', async () => {
187+
const root = await fs.mkdtemp(path.join(tmpdir(), 'workflow-symlink-'));
188+
try {
189+
const outside = path.join(root, 'outside.txt');
190+
await fs.writeFile(outside, 'original', 'utf8');
191+
const skillDir = path.join(root, 'project', '.pythinker-code', 'skills', 'audit');
192+
await fs.mkdir(skillDir, { recursive: true });
193+
await fs.symlink(outside, path.join(skillDir, 'SKILL.md'));
194+
195+
await expect(
196+
writeSavedWorkflowSkill({
197+
scope: 'project',
198+
workflow,
199+
projectRoot: path.join(root, 'project'),
200+
brandHomeDir: path.join(root, 'home'),
201+
}),
202+
).rejects.toThrow(/rejected \(symlink_outside_cwd\)/u);
203+
204+
// The planted target must be untouched, not merely "an error was thrown".
205+
expect(await fs.readFile(outside, 'utf8')).toBe('original');
206+
} finally {
207+
await fs.rm(root, { recursive: true, force: true });
208+
}
209+
});
210+
211+
it('refuses when a parent directory is a symlink pointing outside the project', async () => {
212+
const root = await fs.mkdtemp(path.join(tmpdir(), 'workflow-symlink-dir-'));
213+
try {
214+
const projectRoot = path.join(root, 'project');
215+
const escape = path.join(root, 'escape');
216+
await fs.mkdir(escape, { recursive: true });
217+
await fs.mkdir(path.join(projectRoot, '.pythinker-code'), { recursive: true });
218+
await fs.symlink(escape, path.join(projectRoot, '.pythinker-code', 'skills'));
219+
220+
await expect(
221+
writeSavedWorkflowSkill({
222+
scope: 'project',
223+
workflow,
224+
projectRoot,
225+
brandHomeDir: path.join(root, 'home'),
226+
}),
227+
).rejects.toThrow(/rejected \(symlink_outside_cwd\)/u);
228+
229+
expect(await fs.readdir(escape)).toEqual([]);
230+
} finally {
231+
await fs.rm(root, { recursive: true, force: true });
232+
}
233+
});
234+
235+
it('still writes a normal saved workflow', async () => {
236+
const root = await fs.mkdtemp(path.join(tmpdir(), 'workflow-ok-'));
237+
try {
238+
const dir = await writeSavedWorkflowSkill({
239+
scope: 'project',
240+
workflow,
241+
projectRoot: root,
242+
brandHomeDir: path.join(root, 'home'),
243+
});
244+
expect(await fs.readFile(path.join(dir, 'SKILL.md'), 'utf8')).toContain('name: "audit"');
245+
} finally {
246+
await fs.rm(root, { recursive: true, force: true });
247+
}
248+
});
249+
});
250+
178251
// Rendering valid-looking Markdown is not the contract — being loadable as a
179252
// skill is. This drives the real parser the discovery path uses, so a saved
180253
// workflow that cannot be read back fails here rather than at the next launch.

0 commit comments

Comments
 (0)