mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-07-10 10:00:53 -07:00
fix(core): support nested directory structure in Plan Mode write_file and edit
This commit is contained in:
@@ -1323,6 +1323,14 @@ function doIt() {
|
||||
});
|
||||
|
||||
describe('plan mode', () => {
|
||||
beforeEach(() => {
|
||||
vi.mocked(mockConfig.isPlanMode).mockReturnValue(true);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.mocked(mockConfig.isPlanMode).mockReturnValue(false);
|
||||
});
|
||||
|
||||
it('should allow edits to plans directory when isPlanMode is true', async () => {
|
||||
const mockProjectTempDir = path.join(tempDir, 'project');
|
||||
fs.mkdirSync(mockProjectTempDir);
|
||||
@@ -1332,8 +1340,6 @@ function doIt() {
|
||||
|
||||
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');
|
||||
@@ -1360,5 +1366,77 @@ function doIt() {
|
||||
|
||||
fs.rmSync(plansDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('should preserve nested directory structure within the plans directory in Plan Mode', 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.storage.getPlansDir).mockReturnValue(plansDir);
|
||||
|
||||
const nestedDir = path.join(plansDir, 'tracks', 'fibsqrt_20260519');
|
||||
fs.mkdirSync(nestedDir, { recursive: true });
|
||||
|
||||
const planFilePath = path.join(nestedDir, 'spec.md');
|
||||
const initialContent = 'some initial content';
|
||||
fs.writeFileSync(planFilePath, initialContent, 'utf8');
|
||||
|
||||
const params: EditToolParams = {
|
||||
file_path: 'tracks/fibsqrt_20260519/spec.md',
|
||||
instruction: 'Replace initial with new',
|
||||
old_string: 'initial',
|
||||
new_string: 'new',
|
||||
};
|
||||
|
||||
const invocation = tool.build(params);
|
||||
const result = await invocation.execute({
|
||||
abortSignal: new AbortController().signal,
|
||||
});
|
||||
|
||||
expect(result.llmContent).toMatch(/Successfully modified file/);
|
||||
expect(fs.readFileSync(planFilePath, 'utf8')).toBe('some new content');
|
||||
|
||||
fs.rmSync(plansDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('should strip the leading plansDir folder name segment if present in path', 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.storage.getPlansDir).mockReturnValue(plansDir);
|
||||
|
||||
const nestedDir = path.join(plansDir, 'tracks', 'fibsqrt_20260519');
|
||||
fs.mkdirSync(nestedDir, { recursive: true });
|
||||
|
||||
const planFilePath = path.join(nestedDir, 'spec.md');
|
||||
const initialContent = 'some initial content';
|
||||
fs.writeFileSync(planFilePath, initialContent, 'utf8');
|
||||
|
||||
const params: EditToolParams = {
|
||||
file_path: 'plans/tracks/fibsqrt_20260519/spec.md',
|
||||
instruction: 'Replace initial with new',
|
||||
old_string: 'initial',
|
||||
new_string: 'new',
|
||||
};
|
||||
|
||||
const invocation = tool.build(params);
|
||||
const result = await invocation.execute({
|
||||
abortSignal: new AbortController().signal,
|
||||
});
|
||||
|
||||
expect(result.llmContent).toMatch(/Successfully modified file/);
|
||||
expect(fs.readFileSync(planFilePath, 'utf8')).toBe('some new content');
|
||||
|
||||
fs.rmSync(plansDir, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -27,6 +27,7 @@ import { buildFilePathArgsPattern } from '../policy/utils.js';
|
||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
import { ToolErrorType } from './tool-error.js';
|
||||
import { makeRelative, shortenPath } from '../utils/paths.js';
|
||||
import { resolvePlanPath } from '../utils/planUtils.js';
|
||||
import { isNodeError } from '../utils/errors.js';
|
||||
import { correctPath } from '../utils/pathCorrector.js';
|
||||
import type { Config } from '../config/config.js';
|
||||
@@ -465,10 +466,10 @@ class EditToolInvocation
|
||||
() => this.config.getApprovalMode(),
|
||||
);
|
||||
if (this.config.isPlanMode()) {
|
||||
const safeFilename = path.basename(this.params.file_path);
|
||||
this.resolvedPath = path.join(
|
||||
this.resolvedPath = resolvePlanPath(
|
||||
this.params.file_path,
|
||||
this.config.storage.getPlansDir(),
|
||||
safeFilename,
|
||||
this.config.getTargetDir(),
|
||||
);
|
||||
} else if (!path.isAbsolute(this.params.file_path)) {
|
||||
const result = correctPath(this.params.file_path, this.config);
|
||||
@@ -1054,7 +1055,13 @@ export class EditTool
|
||||
}
|
||||
|
||||
let resolvedPath: string;
|
||||
if (!path.isAbsolute(params.file_path)) {
|
||||
if (this.config.isPlanMode()) {
|
||||
resolvedPath = resolvePlanPath(
|
||||
params.file_path,
|
||||
this.config.storage.getPlansDir(),
|
||||
this.config.getTargetDir(),
|
||||
);
|
||||
} else if (!path.isAbsolute(params.file_path)) {
|
||||
const result = correctPath(params.file_path, this.config);
|
||||
if (result.success) {
|
||||
resolvedPath = result.correctedPath;
|
||||
|
||||
@@ -508,5 +508,28 @@ Ask the user for specific feedback on how to improve the plan.`,
|
||||
});
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it('should accept nested valid path within plans directory', () => {
|
||||
const nestedDir = path.join(mockPlansDir, 'tracks', 'fibsqrt_20260519');
|
||||
fs.mkdirSync(nestedDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(nestedDir, 'spec.md'), '# Content');
|
||||
|
||||
const result = tool.validateToolParams({
|
||||
plan_filename: 'tracks/fibsqrt_20260519/spec.md',
|
||||
});
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it('should strip the leading plansDir folder name segment if present in path', () => {
|
||||
const plansDirName = path.basename(mockPlansDir);
|
||||
const nestedDir = path.join(mockPlansDir, 'tracks', 'fibsqrt_20260519');
|
||||
fs.mkdirSync(nestedDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(nestedDir, 'spec.md'), '# Content');
|
||||
|
||||
const result = tool.validateToolParams({
|
||||
plan_filename: `${plansDirName}/tracks/fibsqrt_20260519/spec.md`,
|
||||
});
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -19,7 +19,11 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
import path from 'node:path';
|
||||
import type { Config } from '../config/config.js';
|
||||
import { EXIT_PLAN_MODE_TOOL_NAME } from './tool-names.js';
|
||||
import { validatePlanPath, validatePlanContent } from '../utils/planUtils.js';
|
||||
import {
|
||||
validatePlanPath,
|
||||
validatePlanContent,
|
||||
resolvePlanPath,
|
||||
} from '../utils/planUtils.js';
|
||||
import { ApprovalMode } from '../policy/types.js';
|
||||
import { resolveToRealPath, isSubpath } from '../utils/paths.js';
|
||||
import { logPlanExecution } from '../telemetry/loggers.js';
|
||||
@@ -60,11 +64,11 @@ export class ExitPlanModeTool extends BaseDeclarativeTool<
|
||||
return 'plan_filename is required.';
|
||||
}
|
||||
|
||||
const safeFilename = path.basename(params.plan_filename);
|
||||
const plansDir = resolveToRealPath(this.config.storage.getPlansDir());
|
||||
const resolvedPath = path.join(
|
||||
const resolvedPath = resolvePlanPath(
|
||||
params.plan_filename,
|
||||
this.config.storage.getPlansDir(),
|
||||
safeFilename,
|
||||
this.config.getTargetDir(),
|
||||
);
|
||||
|
||||
const realPath = resolveToRealPath(resolvedPath);
|
||||
@@ -122,6 +126,7 @@ export class ExitPlanModeInvocation extends BaseToolInvocation<
|
||||
const pathError = await validatePlanPath(
|
||||
this.params.plan_filename,
|
||||
this.config.storage.getPlansDir(),
|
||||
this.config.getTargetDir(),
|
||||
);
|
||||
if (pathError) {
|
||||
this.planValidationError = pathError;
|
||||
@@ -179,8 +184,11 @@ export class ExitPlanModeInvocation extends BaseToolInvocation<
|
||||
* Note: Validation is done in validateToolParamValues, so this assumes the path is valid.
|
||||
*/
|
||||
private getResolvedPlanPath(): string {
|
||||
const safeFilename = path.basename(this.params.plan_filename);
|
||||
return path.join(this.config.storage.getPlansDir(), safeFilename);
|
||||
return resolvePlanPath(
|
||||
this.params.plan_filename,
|
||||
this.config.storage.getPlansDir(),
|
||||
this.config.getTargetDir(),
|
||||
);
|
||||
}
|
||||
|
||||
async execute({ abortSignal: _signal }: ExecuteOptions): Promise<ToolResult> {
|
||||
|
||||
@@ -109,6 +109,7 @@ const mockConfigInternal = {
|
||||
getActiveModel: () => 'test-model',
|
||||
storage: {
|
||||
getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'),
|
||||
getPlansDir: vi.fn().mockReturnValue('/tmp/plans'),
|
||||
},
|
||||
};
|
||||
|
||||
@@ -147,6 +148,7 @@ describe('WriteFileTool', () => {
|
||||
const workspaceContext = new WorkspaceContext(rootDir, [plansDir]);
|
||||
const mockStorage = {
|
||||
getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'),
|
||||
getPlansDir: vi.fn().mockReturnValue(plansDir),
|
||||
};
|
||||
|
||||
mockConfig = {
|
||||
@@ -1113,4 +1115,38 @@ describe('WriteFileTool', () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('Plan Mode path resolution', () => {
|
||||
beforeEach(() => {
|
||||
vi.mocked(mockConfigInternal.isPlanMode).mockReturnValue(true);
|
||||
vi.mocked(mockConfigInternal.storage.getPlansDir).mockReturnValue(
|
||||
plansDir,
|
||||
);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.mocked(mockConfigInternal.isPlanMode).mockReturnValue(false);
|
||||
});
|
||||
|
||||
it('should preserve nested directory structure within the plans directory', () => {
|
||||
const planFilePath = 'tracks/fibsqrt_20260519/spec.md';
|
||||
const params = { file_path: planFilePath, content: '# Spec' };
|
||||
const invocation = tool.build(params);
|
||||
|
||||
expect(
|
||||
(invocation as unknown as { resolvedPath: string }).resolvedPath,
|
||||
).toBe(path.resolve(plansDir, 'tracks/fibsqrt_20260519/spec.md'));
|
||||
});
|
||||
|
||||
it('should strip the leading plansDir folder name segment if present in path', () => {
|
||||
const plansDirName = path.basename(plansDir);
|
||||
const planFilePath = `${plansDirName}/tracks/fibsqrt_20260519/spec.md`;
|
||||
const params = { file_path: planFilePath, content: '# Spec' };
|
||||
const invocation = tool.build(params);
|
||||
|
||||
expect(
|
||||
(invocation as unknown as { resolvedPath: string }).resolvedPath,
|
||||
).toBe(path.resolve(plansDir, 'tracks/fibsqrt_20260519/spec.md'));
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -29,6 +29,7 @@ import {
|
||||
import { buildFilePathArgsPattern } from '../policy/utils.js';
|
||||
import { ToolErrorType } from './tool-error.js';
|
||||
import { makeRelative, shortenPath } from '../utils/paths.js';
|
||||
import { resolvePlanPath } from '../utils/planUtils.js';
|
||||
import { getErrorMessage, isNodeError } from '../utils/errors.js';
|
||||
import { ensureCorrectFileContent } from '../utils/editCorrector.js';
|
||||
import { detectLineEnding } from '../utils/textUtils.js';
|
||||
@@ -168,10 +169,10 @@ class WriteFileToolInvocation extends BaseToolInvocation<
|
||||
);
|
||||
|
||||
if (this.config.isPlanMode()) {
|
||||
const safeFilename = path.basename(this.params.file_path);
|
||||
this.resolvedPath = path.join(
|
||||
this.resolvedPath = resolvePlanPath(
|
||||
this.params.file_path,
|
||||
this.config.storage.getPlansDir(),
|
||||
safeFilename,
|
||||
this.config.getTargetDir(),
|
||||
);
|
||||
} else {
|
||||
this.resolvedPath = path.resolve(
|
||||
|
||||
@@ -8,7 +8,11 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import path from 'node:path';
|
||||
import * as fs from 'node:fs';
|
||||
import os from 'node:os';
|
||||
import { validatePlanPath, validatePlanContent } from './planUtils.js';
|
||||
import {
|
||||
validatePlanPath,
|
||||
validatePlanContent,
|
||||
resolvePlanPath,
|
||||
} from './planUtils.js';
|
||||
|
||||
describe('planUtils', () => {
|
||||
let tempRootDir: string;
|
||||
@@ -57,6 +61,55 @@ describe('planUtils', () => {
|
||||
const result = await validatePlanPath(maliciousPath, plansDir);
|
||||
expect(result).toContain('Access denied');
|
||||
});
|
||||
|
||||
it('should validate a nested path within the plans directory', async () => {
|
||||
const nestedDir = path.join(plansDir, 'tracks', 'fibsqrt_20260519');
|
||||
fs.mkdirSync(nestedDir, { recursive: true });
|
||||
const planPath = path.join('tracks', 'fibsqrt_20260519', 'spec.md');
|
||||
const fullPath = path.join(plansDir, planPath);
|
||||
fs.writeFileSync(fullPath, '# Nested Spec');
|
||||
|
||||
const result = await validatePlanPath(planPath, plansDir, tempRootDir);
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolvePlanPath', () => {
|
||||
it('should resolve simple filenames relative to plansDir', () => {
|
||||
const result = resolvePlanPath(
|
||||
'implementation_plan.md',
|
||||
plansDir,
|
||||
tempRootDir,
|
||||
);
|
||||
expect(result).toBe(path.join(plansDir, 'implementation_plan.md'));
|
||||
});
|
||||
|
||||
it('should preserve subdirectories if already inside plansDir', () => {
|
||||
const planPath = path.join(
|
||||
'plans',
|
||||
'tracks',
|
||||
'fibsqrt_20260519',
|
||||
'spec.md',
|
||||
);
|
||||
const result = resolvePlanPath(planPath, plansDir, tempRootDir);
|
||||
expect(result).toBe(
|
||||
path.join(plansDir, 'tracks', 'fibsqrt_20260519', 'spec.md'),
|
||||
);
|
||||
});
|
||||
|
||||
it('should resolve paths relative to plansDir if they contain subdirectories', () => {
|
||||
const planPath = path.join('tracks', 'fibsqrt_20260519', 'spec.md');
|
||||
const result = resolvePlanPath(planPath, plansDir, tempRootDir);
|
||||
expect(result).toBe(
|
||||
path.join(plansDir, 'tracks', 'fibsqrt_20260519', 'spec.md'),
|
||||
);
|
||||
});
|
||||
|
||||
it('should fallback to safe basename when escaping', () => {
|
||||
const planPath = '../../escaped.md';
|
||||
const result = resolvePlanPath(planPath, plansDir, tempRootDir);
|
||||
expect(result).toBe(path.join(plansDir, 'escaped.md'));
|
||||
});
|
||||
});
|
||||
|
||||
describe('validatePlanContent', () => {
|
||||
|
||||
@@ -22,6 +22,67 @@ export const PlanErrorMessages = {
|
||||
READ_FAILURE: (detail: string) => `Failed to read plan file: ${detail}`,
|
||||
} as const;
|
||||
|
||||
/**
|
||||
* Safely resolves a plan path within the plans directory, preserving subdirectories
|
||||
* if they are within the plans directory context.
|
||||
*
|
||||
* @param planPath The input file path for the plan.
|
||||
* @param plansDir The authorized project plans directory.
|
||||
* @param targetDir The project root directory.
|
||||
* @returns The resolved safe absolute path.
|
||||
*/
|
||||
export function resolvePlanPath(
|
||||
planPath: string,
|
||||
plansDir: string,
|
||||
targetDir: string = process.cwd(),
|
||||
): string {
|
||||
const realPlansDir = resolveToRealPath(plansDir);
|
||||
const plansDirName = path.basename(plansDir);
|
||||
|
||||
let normalizedPlanPath = planPath;
|
||||
if (!path.isAbsolute(planPath)) {
|
||||
const segments = planPath.split(/[\\/]+/);
|
||||
if (segments.length > 1 && segments[0] === plansDirName) {
|
||||
normalizedPlanPath = segments.slice(1).join(path.sep);
|
||||
}
|
||||
}
|
||||
|
||||
// 1. Try resolving relative to project root (targetDir)
|
||||
const resolved = path.isAbsolute(normalizedPlanPath)
|
||||
? normalizedPlanPath
|
||||
: path.resolve(targetDir, normalizedPlanPath);
|
||||
|
||||
try {
|
||||
const realResolved = resolveToRealPath(resolved);
|
||||
if (isSubpath(realPlansDir, realResolved)) {
|
||||
return resolved;
|
||||
}
|
||||
} catch {
|
||||
const directResolved = path.resolve(resolved);
|
||||
if (isSubpath(realPlansDir, directResolved)) {
|
||||
return resolved;
|
||||
}
|
||||
}
|
||||
|
||||
// 2. Try resolving relative to plansDir
|
||||
const nestedResolved = path.resolve(plansDir, normalizedPlanPath);
|
||||
try {
|
||||
const realNested = resolveToRealPath(nestedResolved);
|
||||
if (isSubpath(realPlansDir, realNested)) {
|
||||
return nestedResolved;
|
||||
}
|
||||
} catch {
|
||||
const directNested = path.resolve(nestedResolved);
|
||||
if (isSubpath(realPlansDir, directNested)) {
|
||||
return nestedResolved;
|
||||
}
|
||||
}
|
||||
|
||||
// 3. Fallback to standard safe behavior (basename) to avoid traversal
|
||||
const safeFilename = path.basename(planPath);
|
||||
return path.join(plansDir, safeFilename);
|
||||
}
|
||||
|
||||
/**
|
||||
* Validates a plan file path for safety (traversal) and existence.
|
||||
* @param planPath The untrusted path to the plan file.
|
||||
@@ -32,9 +93,9 @@ export const PlanErrorMessages = {
|
||||
export async function validatePlanPath(
|
||||
planPath: string,
|
||||
plansDir: string,
|
||||
targetDir?: string,
|
||||
): Promise<string | null> {
|
||||
const safeFilename = path.basename(planPath);
|
||||
const resolvedPath = path.join(plansDir, safeFilename);
|
||||
const resolvedPath = resolvePlanPath(planPath, plansDir, targetDir);
|
||||
const realPath = resolveToRealPath(resolvedPath);
|
||||
const realPlansDir = resolveToRealPath(plansDir);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user