mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-07-23 00:01:24 -07:00
Compare commits
13 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 8df1206999 | |||
| 2609f52178 | |||
| e5ffe7e3d6 | |||
| 8bfa3fe92a | |||
| 0f34012730 | |||
| bd6980b174 | |||
| 4a5ad95cb9 | |||
| 4516a3f0ab | |||
| d5e3a40c96 | |||
| e2168036a6 | |||
| c1859cbe5c | |||
| 214a44ab89 | |||
| 1668e0e1ce |
@@ -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
|
||||
@@ -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) {
|
||||
|
||||
@@ -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],
|
||||
|
||||
+6
-6
@@ -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),
|
||||
};
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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 });
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -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'),
|
||||
|
||||
@@ -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[] {
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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))) {
|
||||
|
||||
Reference in New Issue
Block a user