Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/cli/src/ui/components/ExitPlanModeDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ function usePlanContent(planPath: string, config: Config): PlanContentState {
const pathError = await validatePlanPath(
planPath,
config.storage.getPlansDir(),
config.getTargetDir(),
);
if (ignore) return;
if (pathError) {
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2149,6 +2149,10 @@ export class Config implements McpContext, AgentLoopContext {
return this.policyEngine.getApprovalMode();
}

isPlanMode(): boolean {
return this.getApprovalMode() === ApprovalMode.PLAN;
}

getPolicyUpdateConfirmationRequest():
| PolicyUpdateConfirmationRequest
| undefined {
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tools/confirmation-policy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ describe('Tool Confirmation Policy Updates', () => {
getDisableLLMCorrection: () => true,
getIdeMode: () => false,
getActiveModel: () => 'test-model',
isPlanMode: () => false,
getWorkspaceContext: () => ({
isPathWithinWorkspace: () => true,
getDirectories: () => [rootDir],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,13 @@ exports[`coreTools snapshots for specific models > Model: gemini-2.5-pro > snaps
"name": "exit_plan_mode",
"parametersJsonSchema": {
"properties": {
"plan_path": {
"description": "The file path to the finalized plan (e.g., "/mock/plans/feature-x.md"). This path MUST be within the designated plans directory: /mock/plans/",
"plan_filename": {
"description": "The filename of the finalized plan (e.g., "feature-x.md"). Do not provide an absolute path.",
"type": "string",
},
},
"required": [
"plan_path",
"plan_filename",
],
"type": "object",
},
Expand Down Expand Up @@ -956,13 +956,13 @@ exports[`coreTools snapshots for specific models > Model: gemini-3-pro-preview >
"name": "exit_plan_mode",
"parametersJsonSchema": {
"properties": {
"plan_path": {
"description": "The file path to the finalized plan (e.g., "/mock/plans/feature-x.md"). This path MUST be within the designated plans directory: /mock/plans/",
"plan_filename": {
"description": "The filename of the finalized plan (e.g., "feature-x.md"). Do not provide an absolute path.",
"type": "string",
},
},
"required": [
"plan_path",
"plan_filename",
],
"type": "object",
},
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/tools/definitions/base-declarations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export const ASK_USER_OPTION_PARAM_DESCRIPTION = 'description';

// -- exit_plan_mode --
export const EXIT_PLAN_MODE_TOOL_NAME = 'exit_plan_mode';
export const EXIT_PLAN_PARAM_PLAN_PATH = 'plan_path';
export const EXIT_PLAN_PARAM_PLAN_FILENAME = 'plan_filename';

// -- enter_plan_mode --
export const ENTER_PLAN_MODE_TOOL_NAME = 'enter_plan_mode';
Expand Down
8 changes: 4 additions & 4 deletions packages/core/src/tools/definitions/coreTools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export {
ASK_USER_OPTION_PARAM_LABEL,
ASK_USER_OPTION_PARAM_DESCRIPTION,
PLAN_MODE_PARAM_REASON,
EXIT_PLAN_PARAM_PLAN_PATH,
EXIT_PLAN_PARAM_PLAN_FILENAME,
SKILL_PARAM_NAME,
} from './base-declarations.js';

Expand Down Expand Up @@ -244,10 +244,10 @@ export function getShellDefinition(
};
}

export function getExitPlanModeDefinition(plansDir: string): ToolDefinition {
export function getExitPlanModeDefinition(): ToolDefinition {
return {
base: getExitPlanModeDeclaration(plansDir),
overrides: (modelId) => getToolSet(modelId).exit_plan_mode(plansDir),
base: getExitPlanModeDeclaration(),
overrides: (modelId) => getToolSet(modelId).exit_plan_mode(),
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ describe('coreTools snapshots for specific models', () => {
{ name: 'enter_plan_mode', definition: ENTER_PLAN_MODE_DEFINITION },
{
name: 'exit_plan_mode',
definition: getExitPlanModeDefinition('/mock/plans'),
definition: getExitPlanModeDefinition(),
},
{
name: 'activate_skill',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
PARAM_DESCRIPTION,
PARAM_DIR_PATH,
SHELL_PARAM_IS_BACKGROUND,
EXIT_PLAN_PARAM_PLAN_PATH,
EXIT_PLAN_PARAM_PLAN_FILENAME,
SKILL_PARAM_NAME,
} from './base-declarations.js';

Expand Down Expand Up @@ -118,20 +118,18 @@ export function getShellDeclaration(
/**
* Returns the FunctionDeclaration for exiting plan mode.
*/
export function getExitPlanModeDeclaration(
plansDir: string,
): FunctionDeclaration {
export function getExitPlanModeDeclaration(): FunctionDeclaration {
return {
name: EXIT_PLAN_MODE_TOOL_NAME,
description:
'Finalizes the planning phase and transitions to implementation by presenting the plan for user approval. This tool MUST be used to exit Plan Mode before any source code edits can be performed. Call this whenever a plan is ready or the user requests implementation.',
parametersJsonSchema: {
type: 'object',
required: [EXIT_PLAN_PARAM_PLAN_PATH],
required: [EXIT_PLAN_PARAM_PLAN_FILENAME],
properties: {
[EXIT_PLAN_PARAM_PLAN_PATH]: {
[EXIT_PLAN_PARAM_PLAN_FILENAME]: {
type: 'string',
description: `The file path to the finalized plan (e.g., "${plansDir}/feature-x.md"). This path MUST be within the designated plans directory: ${plansDir}/`,
description: `The filename of the finalized plan (e.g., "feature-x.md"). Do not provide an absolute path.`,
},
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -732,6 +732,6 @@ The agent did not use the todo list because this task could be completed by a ti
},
},

exit_plan_mode: (plansDir) => getExitPlanModeDeclaration(plansDir),
exit_plan_mode: () => getExitPlanModeDeclaration(),
activate_skill: (skillNames) => getActivateSkillDeclaration(skillNames),
};
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,6 @@ The agent did not use the todo list because this task could be completed by a ti
},
},

exit_plan_mode: (plansDir) => getExitPlanModeDeclaration(plansDir),
exit_plan_mode: () => getExitPlanModeDeclaration(),
activate_skill: (skillNames) => getActivateSkillDeclaration(skillNames),
};
2 changes: 1 addition & 1 deletion packages/core/src/tools/definitions/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,6 @@ export interface CoreToolSet {
get_internal_docs: FunctionDeclaration;
ask_user: FunctionDeclaration;
enter_plan_mode: FunctionDeclaration;
exit_plan_mode: (plansDir: string) => FunctionDeclaration;
exit_plan_mode: () => FunctionDeclaration;
activate_skill: (skillNames: string[]) => FunctionDeclaration;
}
1 change: 1 addition & 0 deletions packages/core/src/tools/edit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ describe('EditTool', () => {
isInteractive: () => false,
getDisableLLMCorrection: vi.fn(() => true),
getExperiments: () => {},
isPlanMode: () => false,
Comment thread
Adib234 marked this conversation as resolved.
Outdated
storage: {
getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'),
},
Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/tools/edit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,13 @@ class EditToolInvocation
displayName?: string,
) {
super(params, messageBus, toolName, displayName);
if (!path.isAbsolute(this.params.file_path)) {
if (this.config.isPlanMode()) {
const safeFilename = path.basename(this.params.file_path);
this.resolvedPath = path.join(
this.config.storage.getPlansDir(),
safeFilename,
);
} else if (!path.isAbsolute(this.params.file_path)) {
const result = correctPath(this.params.file_path, this.config);
if (result.success) {
this.resolvedPath = result.correctedPath;
Expand Down
67 changes: 25 additions & 42 deletions packages/core/src/tools/exit-plan-mode.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ describe('ExitPlanModeTool', () => {
describe('shouldConfirmExecute', () => {
it('should return plan approval confirmation details when plan has content', async () => {
const planRelativePath = createPlanFile('test-plan.md', '# My Plan');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const result = await invocation.shouldConfirmExecute(
new AbortController().signal,
Expand All @@ -97,7 +97,7 @@ describe('ExitPlanModeTool', () => {

it('should return false when plan file is empty', async () => {
const planRelativePath = createPlanFile('empty.md', ' ');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const result = await invocation.shouldConfirmExecute(
new AbortController().signal,
Expand All @@ -108,7 +108,7 @@ describe('ExitPlanModeTool', () => {

it('should return false when plan file cannot be read', async () => {
const planRelativePath = path.join('plans', 'non-existent.md');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const result = await invocation.shouldConfirmExecute(
new AbortController().signal,
Expand All @@ -119,7 +119,7 @@ describe('ExitPlanModeTool', () => {

it('should auto-approve when policy decision is ALLOW', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

vi.spyOn(
invocation as unknown as {
Expand All @@ -142,7 +142,7 @@ describe('ExitPlanModeTool', () => {

it('should throw error when policy decision is DENY', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

vi.spyOn(
invocation as unknown as {
Expand All @@ -160,7 +160,7 @@ describe('ExitPlanModeTool', () => {
describe('execute with invalid plan', () => {
it('should return error when plan file is empty', async () => {
const planRelativePath = createPlanFile('empty.md', '');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

await invocation.shouldConfirmExecute(new AbortController().signal);
const result = await invocation.execute(new AbortController().signal);
Expand All @@ -170,8 +170,8 @@ describe('ExitPlanModeTool', () => {
});

it('should return error when plan file cannot be read', async () => {
const planRelativePath = 'plans/ghost.md';
const invocation = tool.build({ plan_path: planRelativePath });
const planRelativePath = 'ghost.md';
const invocation = tool.build({ plan_filename: planRelativePath });

await invocation.shouldConfirmExecute(new AbortController().signal);
const result = await invocation.execute(new AbortController().signal);
Expand All @@ -183,7 +183,7 @@ describe('ExitPlanModeTool', () => {
describe('execute', () => {
it('should return approval message when plan is approved with DEFAULT mode', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const confirmDetails = await invocation.shouldConfirmExecute(
new AbortController().signal,
Expand Down Expand Up @@ -211,7 +211,7 @@ Read and follow the plan strictly during implementation.`,

it('should return approval message when plan is approved with AUTO_EDIT mode', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const confirmDetails = await invocation.shouldConfirmExecute(
new AbortController().signal,
Expand Down Expand Up @@ -242,7 +242,7 @@ Read and follow the plan strictly during implementation.`,

it('should return feedback message when plan is rejected with feedback', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const confirmDetails = await invocation.shouldConfirmExecute(
new AbortController().signal,
Expand All @@ -269,7 +269,7 @@ Revise the plan based on the feedback.`,

it('should handle rejection without feedback gracefully', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const confirmDetails = await invocation.shouldConfirmExecute(
new AbortController().signal,
Expand All @@ -295,7 +295,7 @@ Ask the user for specific feedback on how to improve the plan.`,

it('should log plan execution event when plan is approved', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const confirmDetails = await invocation.shouldConfirmExecute(
new AbortController().signal,
Expand All @@ -319,7 +319,7 @@ Ask the user for specific feedback on how to improve the plan.`,

it('should return cancellation message when cancelled', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const confirmDetails = await invocation.shouldConfirmExecute(
new AbortController().signal,
Expand All @@ -342,7 +342,7 @@ Ask the user for specific feedback on how to improve the plan.`,
describe('execute when shouldConfirmExecute is never called', () => {
it('should approve with DEFAULT mode when approvalPayload is null (policy ALLOW skips confirmation)', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

// Simulate the scheduler's policy ALLOW path: execute() is called
// directly without ever calling shouldConfirmExecute(), leaving
Expand All @@ -362,7 +362,7 @@ Ask the user for specific feedback on how to improve the plan.`,
describe('getApprovalModeDescription (internal)', () => {
it('should handle all valid approval modes', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const testMode = async (mode: ApprovalMode, expected: string) => {
const confirmDetails = await invocation.shouldConfirmExecute(
Expand Down Expand Up @@ -391,7 +391,7 @@ Ask the user for specific feedback on how to improve the plan.`,

it('should throw for invalid post-planning modes', async () => {
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });

const testInvalidMode = async (mode: ApprovalMode) => {
const confirmDetails = await invocation.shouldConfirmExecute(
Expand All @@ -414,36 +414,19 @@ Ask the user for specific feedback on how to improve the plan.`,
});
});

it('should throw error during build if plan path is outside plans directory', () => {
expect(() => tool.build({ plan_path: '../../../etc/passwd' })).toThrow(
/Access denied/,
);
});

describe('validateToolParams', () => {
it('should reject empty plan_path', () => {
const result = tool.validateToolParams({ plan_path: '' });
expect(result).toBe('plan_path is required.');
const result = tool.validateToolParams({ plan_filename: '' });
expect(result).toBe('plan_filename is required.');
});

it('should reject whitespace-only plan_path', () => {
const result = tool.validateToolParams({ plan_path: ' ' });
expect(result).toBe('plan_path is required.');
});

it('should reject path outside plans directory', () => {
const result = tool.validateToolParams({
plan_path: '../../../etc/passwd',
});
expect(result).toContain('Access denied');
const result = tool.validateToolParams({ plan_filename: ' ' });
expect(result).toBe('plan_filename is required.');
});

it('should reject non-existent plan file', async () => {
const result = await validatePlanPath(
'plans/ghost.md',
mockPlansDir,
tempRootDir,
);
const result = await validatePlanPath('ghost.md', mockPlansDir);
expect(result).toContain('Plan file does not exist');
});

Expand All @@ -454,18 +437,18 @@ Ask the user for specific feedback on how to improve the plan.`,
fs.symlinkSync(outsideFile, maliciousPath);

const result = tool.validateToolParams({
plan_path: 'plans/malicious.md',
plan_filename: 'malicious.md',
});

expect(result).toBe(
'Access denied: plan path must be within the designated plans directory.',
`Access denied: plan path (${path.join(mockPlansDir, 'malicious.md')}) must be within the designated plans directory (${mockPlansDir}).`,
);
});

it('should accept valid path within plans directory', () => {
createPlanFile('valid.md', '# Content');
const result = tool.validateToolParams({
plan_path: 'plans/valid.md',
plan_filename: 'valid.md',
});
expect(result).toBeNull();
});
Expand Down
Loading
Loading