Compare commits

...

13 Commits

Author SHA1 Message Date
A.K.M. Adib 8df1206999 test 2026-03-23 11:10:50 -04:00
Adib234 2609f52178 Merge branch 'main' into fix/plan-mode-paths 2026-03-23 09:51:51 -04:00
A.K.M. Adib e5ffe7e3d6 revert 2026-03-22 22:38:39 -04:00
A.K.M. Adib 8bfa3fe92a test 2026-03-22 08:52:20 -04:00
Adib234 0f34012730 Merge branch 'main' into fix/plan-mode-paths 2026-03-20 15:52:22 -04:00
A.K.M. Adib bd6980b174 fix build 2026-03-19 11:15:55 -04:00
Adib234 4a5ad95cb9 Merge branch 'main' into fix/plan-mode-paths 2026-03-19 11:07:18 -04:00
A.K.M. Adib 4516a3f0ab update test comments 2026-03-19 11:04:12 -04:00
A.K.M. Adib d5e3a40c96 add test to test isPlanMode true behavior 2026-03-19 10:34:07 -04:00
A.K.M. Adib e2168036a6 update file path shown in exit plan mode dialog 2026-03-17 15:50:48 -04:00
Adib234 c1859cbe5c Merge branch 'main' into fix/plan-mode-paths 2026-03-17 13:48:49 -04:00
Adib234 214a44ab89 Update packages/core/src/utils/planUtils.ts
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
2026-03-17 12:58:22 -04:00
A.K.M. Adib 1668e0e1ce fix(core): sandbox path resolution in Plan Mode to prevent hallucinations and traversal 2026-03-16 16:56:09 -04:00
23 changed files with 188 additions and 161 deletions
+37 -19
View File
@@ -4,14 +4,14 @@ on:
workflow_dispatch:
inputs:
branch_ref:
description: 'Branch to run on'
required: true
default: 'main'
type: 'string'
test_name_pattern:
description: 'The test name pattern to use'
description: 'Branch to run on (defaults to selected branch)'
required: false
type: 'string'
test_filter:
description: 'Test name pattern to run (passed to vitest -t)'
required: false
default: 'should allow write_file only in the plans directory in plan mode'
type: 'string'
runs:
description: 'The number of runs'
required: false
@@ -27,7 +27,6 @@ jobs:
deflake_e2e_linux:
name: 'E2E Test (Linux) - ${{ matrix.sandbox }}'
runs-on: 'gemini-cli-ubuntu-16-core'
if: "github.repository == 'google-gemini/gemini-cli'"
strategy:
fail-fast: false
matrix:
@@ -41,7 +40,7 @@ jobs:
- name: 'Checkout'
uses: 'actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955' # ratchet:actions/checkout@v5
with:
ref: '${{ github.event.pull_request.head.sha }}'
ref: '${{ github.event.inputs.branch_ref || github.ref }}'
repository: '${{ github.repository }}'
- name: 'Set up Node.js ${{ matrix.node-version }}'
@@ -62,28 +61,37 @@ jobs:
- name: 'Run E2E tests'
env:
GEMINI_API_KEY: '${{ secrets.GEMINI_API_KEY }}'
DEBUG_SCHEDULER: 'true'
IS_DOCKER: "${{ matrix.sandbox == 'sandbox:docker' }}"
KEEP_OUTPUT: 'true'
RUNS: '${{ github.event.inputs.runs }}'
TEST_NAME_PATTERN: '${{ github.event.inputs.test_name_pattern }}'
TEST_FILTER: '${{ github.event.inputs.test_filter }}'
VERBOSE: 'true'
shell: 'bash'
run: |
# Construct the inner command
if [[ "${IS_DOCKER}" == "true" ]]; then
npm run deflake:test:integration:sandbox:docker -- --runs="${RUNS}" -- --testNamePattern "'${TEST_NAME_PATTERN}'"
INNER_CMD="npm run test:integration:sandbox:docker -- --retry=0"
else
npm run deflake:test:integration:sandbox:none -- --runs="${RUNS}" -- --testNamePattern "'${TEST_NAME_PATTERN}'"
INNER_CMD="npm run test:integration:sandbox:none -- --retry=0"
fi
# Append test filter if provided
if [[ -n "${TEST_FILTER}" ]]; then
INNER_CMD="${INNER_CMD} -t \"${TEST_FILTER}\""
fi
# Run deflake script directly
node scripts/deflake.js --command="${INNER_CMD}" --runs="${RUNS}"
deflake_e2e_mac:
name: 'E2E Test (macOS)'
runs-on: 'macos-latest'
if: "github.repository == 'google-gemini/gemini-cli'"
steps:
- name: 'Checkout'
uses: 'actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955' # ratchet:actions/checkout@v5
with:
ref: '${{ github.event.pull_request.head.sha }}'
ref: '${{ github.event.inputs.branch_ref || github.ref }}'
repository: '${{ github.repository }}'
- name: 'Set up Node.js 20.x'
@@ -105,23 +113,28 @@ jobs:
if: "runner.os != 'Windows'"
env:
GEMINI_API_KEY: '${{ secrets.GEMINI_API_KEY }}'
DEBUG_SCHEDULER: 'true'
KEEP_OUTPUT: 'true'
RUNS: '${{ github.event.inputs.runs }}'
SANDBOX: 'sandbox:none'
TEST_NAME_PATTERN: '${{ github.event.inputs.test_name_pattern }}'
TEST_FILTER: '${{ github.event.inputs.test_filter }}'
VERBOSE: 'true'
run: |
npm run deflake:test:integration:sandbox:none -- --runs="${RUNS}" -- --testNamePattern "'${TEST_NAME_PATTERN}'"
INNER_CMD="npm run test:integration:sandbox:none -- --retry=0"
if [[ -n "${TEST_FILTER}" ]]; then
INNER_CMD="${INNER_CMD} -t \"${TEST_FILTER}\""
fi
node scripts/deflake.js --command="${INNER_CMD}" --runs="${RUNS}"
deflake_e2e_windows:
name: 'Slow E2E - Win'
runs-on: 'gemini-cli-windows-16-core'
if: "github.repository == 'google-gemini/gemini-cli'"
steps:
- name: 'Checkout'
uses: 'actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955' # ratchet:actions/checkout@v5
with:
ref: '${{ github.event.pull_request.head.sha }}'
ref: '${{ github.event.inputs.branch_ref || github.ref }}'
repository: '${{ github.repository }}'
- name: 'Set up Node.js 20.x'
@@ -159,6 +172,7 @@ jobs:
- name: 'Run E2E tests'
env:
GEMINI_API_KEY: '${{ secrets.GEMINI_API_KEY }}'
DEBUG_SCHEDULER: 'true'
KEEP_OUTPUT: 'true'
SANDBOX: 'sandbox:none'
VERBOSE: 'true'
@@ -166,7 +180,11 @@ jobs:
UV_THREADPOOL_SIZE: '32'
NODE_ENV: 'test'
RUNS: '${{ github.event.inputs.runs }}'
TEST_NAME_PATTERN: '${{ github.event.inputs.test_name_pattern }}'
TEST_FILTER: '${{ github.event.inputs.test_filter }}'
shell: 'pwsh'
run: |
npm run deflake:test:integration:sandbox:none -- --runs="$env:RUNS" -- --testNamePattern "'$env:TEST_NAME_PATTERN'"
$InnerCmd = "npm run test:integration:sandbox:none -- --retry=0"
if ($env:TEST_FILTER) {
$InnerCmd = "$InnerCmd -t `"$env:TEST_FILTER`""
}
node scripts/deflake.js --command "$InnerCmd" --runs $env:RUNS
+8 -31
View File
@@ -4,10 +4,8 @@
* SPDX-License-Identifier: Apache-2.0
*/
import { writeFileSync } from 'node:fs';
import { join } from 'node:path';
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
import { TestRig, checkModelOutputContent, GEMINI_DIR } from './test-helper.js';
import { TestRig, checkModelOutputContent } from './test-helper.js';
describe('Plan Mode', () => {
let rig: TestRig;
@@ -84,21 +82,14 @@ describe('Plan Mode', () => {
},
});
// Disable the interactive terminal setup prompt in tests
writeFileSync(
join(rig.homeDir!, GEMINI_DIR, 'state.json'),
JSON.stringify({ terminalSetupPromptShown: true }, null, 2),
);
const run = await rig.runInteractive({
await rig.run({
approvalMode: 'plan',
stdin: 'Create a file called plan.md in the plans directory.',
});
await run.type('Create a file called plan.md in the plans directory.');
await run.type('\r');
await rig.expectToolCallSuccess(['write_file'], 30000, (args) =>
args.includes('plan.md'),
const writeFileCallFound = await rig.waitForToolCall('write_file_command');
expect(writeFileCallFound, 'Expected write_file_command to fail').toBe(
false,
);
const toolLogs = rig.readToolLogs();
@@ -131,19 +122,11 @@ describe('Plan Mode', () => {
},
});
// Disable the interactive terminal setup prompt in tests
writeFileSync(
join(rig.homeDir!, GEMINI_DIR, 'state.json'),
JSON.stringify({ terminalSetupPromptShown: true }, null, 2),
);
const run = await rig.runInteractive({
await rig.run({
approvalMode: 'plan',
stdin: 'Create a file called hello.txt in the current directory.',
});
await run.type('Create a file called hello.txt in the current directory.');
await run.type('\r');
const toolLogs = rig.readToolLogs();
const writeLog = toolLogs.find(
(l) =>
@@ -169,12 +152,6 @@ describe('Plan Mode', () => {
},
});
// Disable the interactive terminal setup prompt in tests
writeFileSync(
join(rig.homeDir!, GEMINI_DIR, 'state.json'),
JSON.stringify({ terminalSetupPromptShown: true }, null, 2),
);
// Start in default mode and ask to enter plan mode.
await rig.run({
approvalMode: 'default',
@@ -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) {
+4
View File
@@ -2288,6 +2288,10 @@ export class Config implements McpContext, AgentLoopContext {
return this.policyEngine.getApprovalMode();
}
isPlanMode(): boolean {
return this.getApprovalMode() === ApprovalMode.PLAN;
}
getPolicyUpdateConfirmationRequest():
| PolicyUpdateConfirmationRequest
| undefined {
@@ -71,6 +71,7 @@ describe('Tool Confirmation Policy Updates', () => {
getDisableLLMCorrection: () => true,
getIdeMode: () => false,
getActiveModel: () => 'test-model',
isPlanMode: () => false,
getWorkspaceContext: () => ({
isPathWithinWorkspace: () => true,
getDirectories: () => [rootDir],
@@ -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",
},
@@ -958,13 +958,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",
},
@@ -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';
@@ -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';
@@ -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(),
};
}
@@ -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',
@@ -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';
@@ -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.`,
},
},
},
@@ -739,6 +739,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),
};
@@ -714,6 +714,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),
};
+1 -1
View File
@@ -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;
}
+40
View File
@@ -131,8 +131,10 @@ describe('EditTool', () => {
isInteractive: () => false,
getDisableLLMCorrection: vi.fn(() => true),
getExperiments: () => {},
isPlanMode: vi.fn(() => false),
storage: {
getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'),
getPlansDir: vi.fn().mockReturnValue('/tmp/plans'),
},
isPathAllowed(this: Config, absolutePath: string): boolean {
const workspaceContext = this.getWorkspaceContext();
@@ -1299,4 +1301,42 @@ function doIt() {
);
});
});
describe('plan mode', () => {
it('should allow edits to plans directory when isPlanMode is true', async () => {
const mockProjectTempDir = path.join(tempDir, 'project');
fs.mkdirSync(mockProjectTempDir);
vi.mocked(mockConfig.storage.getProjectTempDir).mockReturnValue(
mockProjectTempDir,
);
const plansDir = path.join(mockProjectTempDir, 'plans');
fs.mkdirSync(plansDir);
vi.mocked(mockConfig.isPlanMode).mockReturnValue(true);
vi.mocked(mockConfig.storage.getPlansDir).mockReturnValue(plansDir);
const filePath = path.join(rootDir, 'test-file.txt');
const planFilePath = path.join(plansDir, 'test-file.txt');
const initialContent = 'some initial content';
fs.writeFileSync(planFilePath, initialContent, 'utf8');
const params: EditToolParams = {
file_path: filePath,
instruction: 'Replace initial with new',
old_string: 'initial',
new_string: 'new',
};
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(/Successfully modified file/);
// Verify plan file is written with new content
expect(fs.readFileSync(planFilePath, 'utf8')).toBe('some new content');
fs.rmSync(plansDir, { recursive: true, force: true });
});
});
});
+7 -1
View File
@@ -463,7 +463,13 @@ class EditToolInvocation
true,
() => this.config.getApprovalMode(),
);
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;
+29 -46
View File
@@ -79,7 +79,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,
@@ -98,7 +98,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,
@@ -109,7 +109,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,
@@ -120,7 +120,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 {
@@ -143,7 +143,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 {
@@ -161,7 +161,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);
@@ -171,8 +171,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);
@@ -184,7 +184,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,
@@ -212,7 +212,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,
@@ -243,7 +243,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,
@@ -270,7 +270,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,
@@ -296,7 +296,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,
@@ -320,7 +320,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,
@@ -343,7 +343,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
@@ -364,7 +364,7 @@ Ask the user for specific feedback on how to improve the plan.`,
it('should return YOLO when config.isInteractive() is false', async () => {
mockConfig.isInteractive = vi.fn().mockReturnValue(false);
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });
// Directly call execute to trigger the internal getAllowApprovalMode
const result = await invocation.execute(new AbortController().signal);
@@ -378,7 +378,7 @@ Ask the user for specific feedback on how to improve the plan.`,
it('should return DEFAULT when config.isInteractive() is true', async () => {
mockConfig.isInteractive = vi.fn().mockReturnValue(true);
const planRelativePath = createPlanFile('test.md', '# Content');
const invocation = tool.build({ plan_path: planRelativePath });
const invocation = tool.build({ plan_filename: planRelativePath });
// Directly call execute to trigger the internal getAllowApprovalMode
const result = await invocation.execute(new AbortController().signal);
@@ -393,7 +393,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(
@@ -426,7 +426,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(
@@ -448,36 +448,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.');
it('should reject empty plan_filename', () => {
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');
it('should reject whitespace-only plan_filename', () => {
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');
});
@@ -488,18 +471,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();
});
+14 -17
View File
@@ -28,7 +28,7 @@ import { resolveToolDeclaration } from './definitions/resolver.js';
import { getPlanModeExitMessage } from '../utils/approvalModeUtils.js';
export interface ExitPlanModeParams {
plan_path: string;
plan_filename: string;
}
export class ExitPlanModeTool extends BaseDeclarativeTool<
@@ -41,8 +41,7 @@ export class ExitPlanModeTool extends BaseDeclarativeTool<
private config: Config,
messageBus: MessageBus,
) {
const plansDir = config.storage.getPlansDir();
const definition = getExitPlanModeDefinition(plansDir);
const definition = getExitPlanModeDefinition();
super(
ExitPlanModeTool.Name,
'Exit Plan Mode',
@@ -56,22 +55,21 @@ export class ExitPlanModeTool extends BaseDeclarativeTool<
protected override validateToolParamValues(
params: ExitPlanModeParams,
): string | null {
if (!params.plan_path || params.plan_path.trim() === '') {
return 'plan_path is required.';
if (!params.plan_filename || params.plan_filename.trim() === '') {
return 'plan_filename is required.';
}
// Since validateToolParamValues is synchronous, we use a basic synchronous check
// for path traversal safety. High-level async validation is deferred to shouldConfirmExecute.
const safeFilename = path.basename(params.plan_filename);
const plansDir = resolveToRealPath(this.config.storage.getPlansDir());
const resolvedPath = path.resolve(
this.config.getTargetDir(),
params.plan_path,
const resolvedPath = path.join(
this.config.storage.getPlansDir(),
safeFilename,
);
const realPath = resolveToRealPath(resolvedPath);
if (!isSubpath(plansDir, realPath)) {
return `Access denied: plan path must be within the designated plans directory.`;
return `Access denied: plan path (${resolvedPath}) must be within the designated plans directory (${plansDir}).`;
}
return null;
@@ -93,8 +91,7 @@ export class ExitPlanModeTool extends BaseDeclarativeTool<
}
override getSchema(modelId?: string) {
const plansDir = this.config.storage.getPlansDir();
return resolveToolDeclaration(getExitPlanModeDefinition(plansDir), modelId);
return resolveToolDeclaration(getExitPlanModeDefinition(), modelId);
}
}
@@ -122,9 +119,8 @@ export class ExitPlanModeInvocation extends BaseToolInvocation<
const resolvedPlanPath = this.getResolvedPlanPath();
const pathError = await validatePlanPath(
this.params.plan_path,
this.params.plan_filename,
this.config.storage.getPlansDir(),
this.config.getTargetDir(),
);
if (pathError) {
this.planValidationError = pathError;
@@ -174,7 +170,7 @@ export class ExitPlanModeInvocation extends BaseToolInvocation<
}
getDescription(): string {
return `Requesting plan approval for: ${this.params.plan_path}`;
return `Requesting plan approval for: ${path.join(this.config.storage.getPlansDir(), this.params.plan_filename)}`;
}
/**
@@ -182,7 +178,8 @@ export class ExitPlanModeInvocation extends BaseToolInvocation<
* Note: Validation is done in validateToolParamValues, so this assumes the path is valid.
*/
private getResolvedPlanPath(): string {
return path.resolve(this.config.getTargetDir(), this.params.plan_path);
const safeFilename = path.basename(this.params.plan_filename);
return path.join(this.config.storage.getPlansDir(), safeFilename);
}
async execute(_signal: AbortSignal): Promise<ToolResult> {
@@ -85,6 +85,10 @@ const mockConfigInternal = {
discoverTools: vi.fn(),
}) as unknown as ToolRegistry,
isInteractive: () => false,
isPlanMode: () => false,
storage: {
getPlansDir: () => '/tmp/plans',
},
};
const mockConfig = mockConfigInternal as unknown as Config;
+2 -2
View File
@@ -73,7 +73,7 @@ import {
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 './definitions/coreTools.js';
@@ -146,7 +146,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,
};
@@ -105,6 +105,7 @@ const mockConfigInternal = {
}) as unknown as ToolRegistry,
isInteractive: () => false,
getDisableLLMCorrection: vi.fn(() => true),
isPlanMode: vi.fn(() => false),
getActiveModel: () => 'test-model',
storage: {
getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'),
+13 -4
View File
@@ -165,10 +165,19 @@ class WriteFileToolInvocation extends BaseToolInvocation<
true,
() => this.config.getApprovalMode(),
);
this.resolvedPath = path.resolve(
this.config.getTargetDir(),
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 {
this.resolvedPath = path.resolve(
this.config.getTargetDir(),
this.params.file_path,
);
}
}
override toolLocations(): ToolLocation[] {
+3 -13
View File
@@ -35,19 +35,13 @@ describe('planUtils', () => {
const fullPath = path.join(tempRootDir, planPath);
fs.writeFileSync(fullPath, '# My Plan');
const result = await validatePlanPath(planPath, plansDir, tempRootDir);
const result = await validatePlanPath(planPath, plansDir);
expect(result).toBeNull();
});
it('should return error for path traversal', async () => {
const planPath = path.join('..', 'secret.txt');
const result = await validatePlanPath(planPath, plansDir, tempRootDir);
expect(result).toContain('Access denied');
});
it('should return error for non-existent file', async () => {
const planPath = path.join('plans', 'ghost.md');
const result = await validatePlanPath(planPath, plansDir, tempRootDir);
const result = await validatePlanPath(planPath, plansDir);
expect(result).toContain('Plan file does not exist');
});
@@ -60,11 +54,7 @@ describe('planUtils', () => {
// Create a symbolic link pointing outside the plans directory
fs.symlinkSync(outsideFile, fullMaliciousPath);
const result = await validatePlanPath(
maliciousPath,
plansDir,
tempRootDir,
);
const result = await validatePlanPath(maliciousPath, plansDir);
expect(result).toContain('Access denied');
});
});
+5 -5
View File
@@ -13,8 +13,8 @@ import { isSubpath, resolveToRealPath } from './paths.js';
* Shared between backend tools and CLI UI for consistency.
*/
export const PlanErrorMessages = {
PATH_ACCESS_DENIED:
'Access denied: plan path must be within the designated plans directory.',
PATH_ACCESS_DENIED: (planPath: string, plansDir: string) =>
`Access denied: plan path (${planPath}) must be within the designated plans directory (${plansDir}).`,
FILE_NOT_FOUND: (path: string) =>
`Plan file does not exist: ${path}. You must create the plan file before requesting approval.`,
FILE_EMPTY:
@@ -32,14 +32,14 @@ export const PlanErrorMessages = {
export async function validatePlanPath(
planPath: string,
plansDir: string,
targetDir: string,
): Promise<string | null> {
const resolvedPath = path.resolve(targetDir, planPath);
const safeFilename = path.basename(planPath);
const resolvedPath = path.join(plansDir, safeFilename);
const realPath = resolveToRealPath(resolvedPath);
const realPlansDir = resolveToRealPath(plansDir);
if (!isSubpath(realPlansDir, realPath)) {
return PlanErrorMessages.PATH_ACCESS_DENIED;
return PlanErrorMessages.PATH_ACCESS_DENIED(planPath, realPlansDir);
}
if (!(await fileExists(resolvedPath))) {