fix(core): support nested directory structure in Plan Mode write_file and edit

This commit is contained in:
Mahima Shanware
2026-05-19 20:28:50 +00:00
parent d43af03598
commit b9d3ede51d
8 changed files with 240 additions and 39 deletions
+80 -2
View File
@@ -1371,6 +1371,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);
@@ -1380,8 +1388,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 = 'test-file.txt';
@@ -1408,5 +1414,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 });
});
});
});
+1
View File
@@ -28,6 +28,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';
@@ -515,5 +515,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();
});
});
});
+3 -3
View File
@@ -227,7 +227,7 @@ export class ExitPlanModeInvocation extends BaseToolInvocation<
return {
llmContent: `${exitMessage}
The approved implementation plan is stored at: ${resolvedPlanPath}
Read and follow the plan strictly during implementation.`,
returnDisplay: `Plan approved: ${resolvedPlanPath}`,
@@ -237,7 +237,7 @@ Read and follow the plan strictly during implementation.`,
if (feedback) {
return {
llmContent: `Plan rejected. User feedback: ${feedback}
The plan is stored at: ${resolvedPlanPath}
Revise the plan based on the feedback.`,
returnDisplay: `Feedback: ${feedback}`,
@@ -245,7 +245,7 @@ Revise the plan based on the feedback.`,
} else {
return {
llmContent: `Plan rejected. No feedback provided.
The plan is stored at: ${resolvedPlanPath}
Ask the user for specific feedback on how to improve the plan.`,
returnDisplay: 'Rejected (no feedback)',
@@ -110,6 +110,7 @@ const mockConfigInternal = {
getActiveModel: () => 'test-model',
storage: {
getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'),
getPlansDir: vi.fn().mockReturnValue('/tmp/plans'),
},
};
@@ -148,6 +149,7 @@ describe('WriteFileTool', () => {
const workspaceContext = new WorkspaceContext(rootDir, [plansDir]);
const mockStorage = {
getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'),
getPlansDir: vi.fn().mockReturnValue(plansDir),
};
mockConfig = {
@@ -1146,4 +1148,34 @@ describe('WriteFileTool', () => {
expect(fs.readFileSync(expectedWritePath, 'utf8')).toBe('nested content');
});
});
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);
});
});
});
+1
View File
@@ -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';
+54 -1
View File
@@ -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;
@@ -63,6 +67,55 @@ describe('planUtils', () => {
);
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', () => {
+46 -33
View File
@@ -22,14 +22,6 @@ export const PlanErrorMessages = {
READ_FAILURE: (detail: string) => `Failed to read plan file: ${detail}`,
} as const;
/**
* Resolves a plan file path and strictly validates it against the plans directory boundary.
* Useful for tools that need to write or read plans.
* @param planPath The untrusted file path provided by the model.
* @param plansDir The authorized project plans directory.
* @returns The safely resolved path string.
* @throws Error if the path is empty, malicious, or escapes boundaries.
*/
export function resolveAndValidatePlanPath(
planPath: string,
plansDir: string,
@@ -40,38 +32,59 @@ export function resolveAndValidatePlanPath(
throw new Error('Plan file path must be non-empty.');
}
// 1. Handle case where agent provided an absolute path
if (path.isAbsolute(trimmedPath)) {
if (
isSubpath(resolveToRealPath(plansDir), resolveToRealPath(trimmedPath))
) {
return trimmedPath;
const realPlansDir = resolveToRealPath(plansDir);
const plansDirName = path.basename(plansDir);
let normalizedPlanPath = trimmedPath;
if (!path.isAbsolute(trimmedPath)) {
const segments = trimmedPath.split(/[\\/]+/);
if (segments.length > 1 && segments[0] === plansDirName) {
normalizedPlanPath = segments.slice(1).join(path.sep);
}
}
// 2. Handle case where agent provided a path relative to the project root
const resolvedFromProjectRoot = path.resolve(projectRoot, trimmedPath);
if (
isSubpath(
resolveToRealPath(plansDir),
resolveToRealPath(resolvedFromProjectRoot),
)
) {
return resolvedFromProjectRoot;
// 1. Handle case where agent provided an absolute path
if (path.isAbsolute(normalizedPlanPath)) {
try {
const realResolved = resolveToRealPath(normalizedPlanPath);
if (isSubpath(realPlansDir, realResolved)) {
return normalizedPlanPath;
}
} catch {
// Fall through if resolveToRealPath fails
}
}
// 3. Handle default case where agent provided a path relative to the plans directory
const resolvedPath = path.resolve(plansDir, trimmedPath);
const realPath = resolveToRealPath(resolvedPath);
const realPlansDir = resolveToRealPath(plansDir);
if (!isSubpath(realPlansDir, realPath)) {
throw new Error(
PlanErrorMessages.PATH_ACCESS_DENIED(trimmedPath, plansDir),
);
// 2. Try resolving relative to project root
const resolvedFromProjectRoot = path.resolve(projectRoot, normalizedPlanPath);
try {
const realResolved = resolveToRealPath(resolvedFromProjectRoot);
if (isSubpath(realPlansDir, realResolved)) {
return resolvedFromProjectRoot;
}
} catch {
const directResolved = path.resolve(resolvedFromProjectRoot);
if (isSubpath(realPlansDir, directResolved)) {
return resolvedFromProjectRoot;
}
}
return resolvedPath;
// 3. Try resolving relative to plansDir
const resolvedFromPlansDir = path.resolve(plansDir, normalizedPlanPath);
try {
const realResolved = resolveToRealPath(resolvedFromPlansDir);
if (isSubpath(realPlansDir, realResolved)) {
return resolvedFromPlansDir;
}
} catch {
const directResolved = path.resolve(resolvedFromPlansDir);
if (isSubpath(realPlansDir, directResolved)) {
return resolvedFromPlansDir;
}
}
// Fallback boundary check: if still not a subpath, throw PATH_ACCESS_DENIED
throw new Error(PlanErrorMessages.PATH_ACCESS_DENIED(trimmedPath, plansDir));
}
/**