From 15aa924932edd32147dc4205dc3f964b7b30065c Mon Sep 17 00:00:00 2001 From: Mahima Shanware Date: Mon, 6 Apr 2026 16:19:33 +0000 Subject: [PATCH] fix(core): migrate consumers to lazily-evaluated getPlansDir Updates prompts and tool implementations (edit, write-file, enter/exit plan mode) to route through Config.getPlansDir() instead of Storage.getPlansDir(). This ensures the plan directory is lazily created exactly when these features attempt to use it, preventing ENOENT failures. --- .../src/core/__snapshots__/prompts.test.ts.snap | 8 ++++---- packages/core/src/core/prompts.test.ts | 4 +++- packages/core/src/prompts/promptProvider.test.ts | 1 + packages/core/src/prompts/promptProvider.ts | 2 +- packages/core/src/tools/edit.test.ts | 4 +++- packages/core/src/tools/edit.ts | 5 +---- packages/core/src/tools/enter-plan-mode.test.ts | 15 +-------------- packages/core/src/tools/enter-plan-mode.ts | 15 --------------- packages/core/src/tools/exit-plan-mode.test.ts | 1 + packages/core/src/tools/exit-plan-mode.ts | 13 +++++-------- packages/core/src/tools/tool-registry.ts | 2 +- packages/core/src/tools/write-file.test.ts | 1 + packages/core/src/tools/write-file.ts | 5 +---- 13 files changed, 23 insertions(+), 53 deletions(-) diff --git a/packages/core/src/core/__snapshots__/prompts.test.ts.snap b/packages/core/src/core/__snapshots__/prompts.test.ts.snap index 86c5151721..3474d164a7 100644 --- a/packages/core/src/core/__snapshots__/prompts.test.ts.snap +++ b/packages/core/src/core/__snapshots__/prompts.test.ts.snap @@ -572,7 +572,7 @@ For example: # Active Approval Mode: Plan -You are operating in **Plan Mode**. Your goal is to produce an implementation plan in \`/tmp/project-temp/plans/\` and get user approval before editing source code. +You are operating in **Plan Mode**. Your goal is to produce an implementation plan in \`/tmp/plans/\` and get user approval before editing source code. ## Available Tools The following tools are available in Plan Mode: @@ -588,8 +588,8 @@ The following tools are available in Plan Mode: ## Rules -1. **Read-Only:** You cannot modify source code. You may ONLY use read-only tools to explore, and you can only write to \`/tmp/project-temp/plans/\`. If the user asks you to modify source code directly, you MUST explain that you are in Plan Mode and must first create a plan and get approval. -2. **Write Constraint:** \`write_file\` and \`replace\` may ONLY be used to write .md plan files to \`/tmp/project-temp/plans/\`. They cannot modify source code. +1. **Read-Only:** You cannot modify source code. You may ONLY use read-only tools to explore, and you can only write to \`/tmp/plans/\`. If the user asks you to modify source code directly, you MUST explain that you are in Plan Mode and must first create a plan and get approval. +2. **Write Constraint:** \`write_file\` and \`replace\` may ONLY be used to write .md plan files to \`/tmp/plans/\`. They cannot modify source code. 3. **Efficiency:** Autonomously combine discovery and drafting phases to minimize conversational turns. If the request is ambiguous, use \`ask_user\` to clarify. Use multi-select to offer flexibility and include detailed descriptions for each option to help the user understand the implications of their choice. 4. **Inquiries and Directives:** Distinguish between Inquiries and Directives to minimize unnecessary planning. - **Inquiries:** If the request is an **Inquiry** (e.g., "How does X work?"), answer directly. DO NOT create a plan. @@ -612,7 +612,7 @@ The depth of your consultation should be proportional to the task's complexity. **CRITICAL:** You MUST NOT proceed to Step 3 (Draft) or Step 4 (Review & Approval) in the same turn as your initial strategy proposal. You MUST wait for user feedback and reach a clear agreement before drafting or submitting the plan. ### 3. Draft -Write the implementation plan to \`/tmp/project-temp/plans/\`. The plan's structure adapts to the task: +Write the implementation plan to \`/tmp/plans/\`. The plan's structure adapts to the task: - **Simple Tasks:** Include a bulleted list of specific **Changes** and **Verification** steps. - **Standard Tasks:** Include an **Objective**, **Key Files & Context**, **Implementation Steps**, and **Verification & Testing**. - **Complex Tasks:** Include **Background & Motivation**, **Scope & Impact**, **Proposed Solution**, **Alternatives Considered**, a phased **Implementation Plan**, **Verification**, and **Migration & Rollback** strategies. diff --git a/packages/core/src/core/prompts.test.ts b/packages/core/src/core/prompts.test.ts index c8f5fe6cc7..13dc95cad1 100644 --- a/packages/core/src/core/prompts.test.ts +++ b/packages/core/src/core/prompts.test.ts @@ -90,9 +90,10 @@ describe('Core System Prompt (prompts.ts)', () => { getToolRegistry: vi.fn().mockReturnValue(mockRegistry), getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true), getSandboxEnabled: vi.fn().mockReturnValue(false), + getPlansDir: vi.fn().mockReturnValue('/tmp/plans'), storage: { getProjectTempDir: vi.fn().mockReturnValue('/tmp/project-temp'), - getPlansDir: vi.fn().mockReturnValue('/tmp/project-temp/plans'), + getPlansDir: vi.fn().mockReturnValue('/tmp/plans'), getProjectTempTrackerDir: vi .fn() .mockReturnValue('/mock/.gemini/tmp/session/tracker'), @@ -423,6 +424,7 @@ describe('Core System Prompt (prompts.ts)', () => { getToolRegistry: vi.fn().mockReturnValue(mockToolRegistry), getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true), getSandboxEnabled: vi.fn().mockReturnValue(false), + getPlansDir: vi.fn().mockReturnValue('/tmp/plans'), storage: { getProjectTempDir: vi.fn().mockReturnValue('/tmp/project-temp'), }, diff --git a/packages/core/src/prompts/promptProvider.test.ts b/packages/core/src/prompts/promptProvider.test.ts index 2f82ae56a4..73c7b1eb43 100644 --- a/packages/core/src/prompts/promptProvider.test.ts +++ b/packages/core/src/prompts/promptProvider.test.ts @@ -61,6 +61,7 @@ describe('PromptProvider', () => { topicState: new TopicState(), getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true), getSandboxEnabled: vi.fn().mockReturnValue(false), + getPlansDir: vi.fn().mockReturnValue('/tmp/project-temp/plans'), storage: { getProjectTempDir: vi.fn().mockReturnValue('/tmp/project-temp'), getPlansDir: vi.fn().mockReturnValue('/tmp/project-temp/plans'), diff --git a/packages/core/src/prompts/promptProvider.ts b/packages/core/src/prompts/promptProvider.ts index 0036dae560..d43a88009a 100644 --- a/packages/core/src/prompts/promptProvider.ts +++ b/packages/core/src/prompts/promptProvider.ts @@ -188,7 +188,7 @@ export class PromptProvider { () => ({ interactive: interactiveMode, planModeToolsList, - plansDir: context.config.storage.getPlansDir(), + plansDir: context.config.getPlansDir(), approvedPlanPath: context.config.getApprovedPlanPath(), }), isPlanMode, diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 66111aed9d..1df12952ab 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -132,9 +132,10 @@ describe('EditTool', () => { getDisableLLMCorrection: vi.fn(() => true), getExperiments: () => {}, isPlanMode: vi.fn(() => false), + getPlansDir: vi.fn().mockReturnValue('/tmp/project/plans'), storage: { getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), - getPlansDir: vi.fn().mockReturnValue('/tmp/plans'), + getPlansDir: vi.fn().mockReturnValue('/tmp/project/plans'), }, isPathAllowed(this: Config, absolutePath: string): boolean { const workspaceContext = this.getWorkspaceContext(); @@ -1314,6 +1315,7 @@ function doIt() { fs.mkdirSync(plansDir); vi.mocked(mockConfig.isPlanMode).mockReturnValue(true); + vi.mocked(mockConfig.getPlansDir).mockReturnValue(plansDir); vi.mocked(mockConfig.storage.getPlansDir).mockReturnValue(plansDir); const filePath = path.join(rootDir, 'test-file.txt'); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 55c7f2f9ab..9e6e299fd2 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -465,10 +465,7 @@ class EditToolInvocation ); if (this.config.isPlanMode()) { const safeFilename = path.basename(this.params.file_path); - this.resolvedPath = path.join( - this.config.storage.getPlansDir(), - safeFilename, - ); + this.resolvedPath = path.join(this.config.getPlansDir(), safeFilename); } else if (!path.isAbsolute(this.params.file_path)) { const result = correctPath(this.params.file_path, this.config); if (result.success) { diff --git a/packages/core/src/tools/enter-plan-mode.test.ts b/packages/core/src/tools/enter-plan-mode.test.ts index ed88a4b49b..7b5218d08d 100644 --- a/packages/core/src/tools/enter-plan-mode.test.ts +++ b/packages/core/src/tools/enter-plan-mode.test.ts @@ -11,7 +11,6 @@ import type { Config } from '../config/config.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { ToolConfirmationOutcome } from './tools.js'; import { ApprovalMode } from '../policy/types.js'; -import fs from 'node:fs'; vi.mock('node:fs', async () => { const actual = await vi.importActual('node:fs'); @@ -39,6 +38,7 @@ describe('EnterPlanModeTool', () => { mockConfig = { setApprovalMode: vi.fn(), + getPlansDir: vi.fn().mockReturnValue('/mock/plans/dir'), storage: { getPlansDir: vi.fn().mockReturnValue('/mock/plans/dir'), } as unknown as Config['storage'], @@ -119,7 +119,6 @@ describe('EnterPlanModeTool', () => { describe('execute', () => { it('should set approval mode to PLAN and return message', async () => { const invocation = tool.build({}); - vi.mocked(fs.existsSync).mockReturnValue(true); const result = await invocation.execute(new AbortController().signal); @@ -130,21 +129,9 @@ describe('EnterPlanModeTool', () => { expect(result.returnDisplay).toBe('Switching to Plan mode'); }); - it('should create plans directory if it does not exist', async () => { - const invocation = tool.build({}); - vi.mocked(fs.existsSync).mockReturnValue(false); - - await invocation.execute(new AbortController().signal); - - expect(fs.mkdirSync).toHaveBeenCalledWith('/mock/plans/dir', { - recursive: true, - }); - }); - it('should include optional reason in output display but not in llmContent', async () => { const reason = 'Design new database schema'; const invocation = tool.build({ reason }); - vi.mocked(fs.existsSync).mockReturnValue(true); const result = await invocation.execute(new AbortController().signal); diff --git a/packages/core/src/tools/enter-plan-mode.ts b/packages/core/src/tools/enter-plan-mode.ts index 7e4ce658ba..dee8569669 100644 --- a/packages/core/src/tools/enter-plan-mode.ts +++ b/packages/core/src/tools/enter-plan-mode.ts @@ -4,7 +4,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import fs from 'node:fs'; import { BaseDeclarativeTool, BaseToolInvocation, @@ -19,7 +18,6 @@ import { ENTER_PLAN_MODE_TOOL_NAME } from './tool-names.js'; import { ApprovalMode } from '../policy/types.js'; import { ENTER_PLAN_MODE_DEFINITION } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; -import { debugLogger } from '../utils/debugLogger.js'; export interface EnterPlanModeParams { reason?: string; @@ -124,19 +122,6 @@ export class EnterPlanModeInvocation extends BaseToolInvocation< this.config.setApprovalMode(ApprovalMode.PLAN); - // Ensure plans directory exists so that the agent can write the plan file. - // In sandboxed environments, the plans directory must exist on the host - // before it can be bound/allowed in the sandbox. - const plansDir = this.config.storage.getPlansDir(); - if (!fs.existsSync(plansDir)) { - try { - fs.mkdirSync(plansDir, { recursive: true }); - } catch (e) { - // Log error but don't fail; write_file will try again later - debugLogger.error(`Failed to create plans directory: ${plansDir}`, e); - } - } - return { llmContent: 'Switching to Plan mode.', returnDisplay: this.params.reason diff --git a/packages/core/src/tools/exit-plan-mode.test.ts b/packages/core/src/tools/exit-plan-mode.test.ts index ad643c6cb2..768a86ca0e 100644 --- a/packages/core/src/tools/exit-plan-mode.test.ts +++ b/packages/core/src/tools/exit-plan-mode.test.ts @@ -44,6 +44,7 @@ describe('ExitPlanModeTool', () => { getTargetDir: vi.fn().mockReturnValue(tempRootDir), setApprovalMode: vi.fn(), setApprovedPlanPath: vi.fn(), + getPlansDir: vi.fn().mockReturnValue(mockPlansDir), storage: { getPlansDir: vi.fn().mockReturnValue(mockPlansDir), } as unknown as Config['storage'], diff --git a/packages/core/src/tools/exit-plan-mode.ts b/packages/core/src/tools/exit-plan-mode.ts index 483b1e5f3d..d0d25e309b 100644 --- a/packages/core/src/tools/exit-plan-mode.ts +++ b/packages/core/src/tools/exit-plan-mode.ts @@ -60,11 +60,8 @@ export class ExitPlanModeTool extends BaseDeclarativeTool< } const safeFilename = path.basename(params.plan_filename); - const plansDir = resolveToRealPath(this.config.storage.getPlansDir()); - const resolvedPath = path.join( - this.config.storage.getPlansDir(), - safeFilename, - ); + const plansDir = resolveToRealPath(this.config.getPlansDir()); + const resolvedPath = path.join(this.config.getPlansDir(), safeFilename); const realPath = resolveToRealPath(resolvedPath); @@ -120,7 +117,7 @@ export class ExitPlanModeInvocation extends BaseToolInvocation< const pathError = await validatePlanPath( this.params.plan_filename, - this.config.storage.getPlansDir(), + this.config.getPlansDir(), ); if (pathError) { this.planValidationError = pathError; @@ -170,7 +167,7 @@ export class ExitPlanModeInvocation extends BaseToolInvocation< } getDescription(): string { - return `Requesting plan approval for: ${path.join(this.config.storage.getPlansDir(), this.params.plan_filename)}`; + return `Requesting plan approval for: ${path.join(this.config.getPlansDir(), this.params.plan_filename)}`; } /** @@ -179,7 +176,7 @@ export class ExitPlanModeInvocation extends BaseToolInvocation< */ private getResolvedPlanPath(): string { const safeFilename = path.basename(this.params.plan_filename); - return path.join(this.config.storage.getPlansDir(), safeFilename); + return path.join(this.config.getPlansDir(), safeFilename); } async execute(_signal: AbortSignal): Promise { diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index f9551d75da..bebff7be6c 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -617,7 +617,7 @@ export class ToolRegistry { */ getFunctionDeclarations(modelId?: string): FunctionDeclaration[] { const isPlanMode = this.config.getApprovalMode() === ApprovalMode.PLAN; - const plansDir = this.config.storage.getPlansDir(); + const plansDir = this.config.getPlansDir(); const declarations: FunctionDeclaration[] = []; const seenNames = new Set(); diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index aa8ff623ea..28c0672839 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -107,6 +107,7 @@ const mockConfigInternal = { getDisableLLMCorrection: vi.fn(() => true), isPlanMode: vi.fn(() => false), getActiveModel: () => 'test-model', + getPlansDir: vi.fn().mockReturnValue('/tmp/plans'), storage: { getProjectTempDir: vi.fn().mockReturnValue('/tmp/project'), }, diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 1d36909dd4..3eeb4eab00 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -168,10 +168,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< if (this.config.isPlanMode()) { const safeFilename = path.basename(this.params.file_path); - this.resolvedPath = path.join( - this.config.storage.getPlansDir(), - safeFilename, - ); + this.resolvedPath = path.join(this.config.getPlansDir(), safeFilename); } else { this.resolvedPath = path.resolve( this.config.getTargetDir(),