diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 84086bbd69..897824e00c 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -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 }); + }); }); }); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index c00ea4c0da..cbf151bfc7 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -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'; diff --git a/packages/core/src/tools/exit-plan-mode.test.ts b/packages/core/src/tools/exit-plan-mode.test.ts index 3501d9df56..4eaaca3b2b 100644 --- a/packages/core/src/tools/exit-plan-mode.test.ts +++ b/packages/core/src/tools/exit-plan-mode.test.ts @@ -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(); + }); }); }); diff --git a/packages/core/src/tools/exit-plan-mode.ts b/packages/core/src/tools/exit-plan-mode.ts index e88723a777..5318e56889 100644 --- a/packages/core/src/tools/exit-plan-mode.ts +++ b/packages/core/src/tools/exit-plan-mode.ts @@ -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)', diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index b35d26fe20..e276f1f792 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -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); + }); + }); }); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 9ec19b879f..080b3f1bf8 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -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'; diff --git a/packages/core/src/utils/planUtils.test.ts b/packages/core/src/utils/planUtils.test.ts index a8340d596a..5cf6ce0bc8 100644 --- a/packages/core/src/utils/planUtils.test.ts +++ b/packages/core/src/utils/planUtils.test.ts @@ -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', () => { diff --git a/packages/core/src/utils/planUtils.ts b/packages/core/src/utils/planUtils.ts index 8060fbfd51..a1a869edd0 100644 --- a/packages/core/src/utils/planUtils.ts +++ b/packages/core/src/utils/planUtils.ts @@ -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)); } /**