From fd8cfad6f9c3faa93f4da82fd51c7aff9b686cef Mon Sep 17 00:00:00 2001 From: Mahima Shanware Date: Tue, 7 Apr 2026 21:00:43 +0000 Subject: [PATCH] fix(core): ensure JIT provisioning of plan directory only occurs in active plan mode Address review bot feedback to prevent EACCES errors during startup by restricting filesystem mutation and workspace context registration to when the user is actively in an interactive plan session. Non-plan tool registration now uses the safely resolved path without attempting to create the directory. --- packages/core/src/config/config.test.ts | 50 +++++++++++++++++++++---- packages/core/src/config/config.ts | 35 +++++++++-------- 2 files changed, 62 insertions(+), 23 deletions(-) diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index a2b340658b..68278bc705 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -3367,7 +3367,7 @@ describe('Plans Directory Initialization', () => { expect(context.getDirectories()).not.toContain(plansDir); }); - it('should create plans directory and add it to workspace context when getPlansDir is called', async () => { + it('should create plans directory and add it to workspace context when getPlansDir is called and ApprovalMode.PLAN is active', async () => { vi.spyOn(fs, 'mkdirSync').mockReturnValue(undefined); const config = new Config({ ...baseParams, @@ -3375,6 +3375,7 @@ describe('Plans Directory Initialization', () => { }); await config.initialize(); + config.setApprovalMode(ApprovalMode.PLAN); const plansDir = config.getPlansDir(); expect(fs.mkdirSync).toHaveBeenCalledWith(plansDir, { @@ -3385,7 +3386,7 @@ describe('Plans Directory Initialization', () => { expect(context.getDirectories()).toContain(plansDir); }); - it('should gracefully handle existing directories by relying on mkdirSync recursive: true', async () => { + it('should NOT create plans directory if ApprovalMode is not PLAN even if plan is enabled', async () => { vi.spyOn(fs, 'mkdirSync').mockReturnValue(undefined); const config = new Config({ ...baseParams, @@ -3393,6 +3394,23 @@ describe('Plans Directory Initialization', () => { }); await config.initialize(); + // Default mode is DEFAULT, not PLAN + const plansDir = config.getPlansDir(); + + expect(fs.mkdirSync).not.toHaveBeenCalled(); + const context = config.getWorkspaceContext(); + expect(context.getDirectories()).not.toContain(plansDir); + }); + + it('should gracefully handle existing directories by relying on mkdirSync recursive: true when in PLAN mode', async () => { + vi.spyOn(fs, 'mkdirSync').mockReturnValue(undefined); + const config = new Config({ + ...baseParams, + plan: true, + }); + + await config.initialize(); + config.setApprovalMode(ApprovalMode.PLAN); const plansDir = config.getPlansDir(); // mkdirSync should be called unconditionally @@ -3403,7 +3421,10 @@ describe('Plans Directory Initialization', () => { expect(context.getDirectories()).toContain(plansDir); }); - it('should throw an error if the plan directory path is blocked by an existing file (EEXIST)', async () => { + it('should log a warning if the plan directory path is blocked by an existing file (EEXIST) in PLAN mode', async () => { + const writeSpy = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); vi.spyOn(fs, 'mkdirSync').mockImplementation(() => { const err = new Error('File exists') as NodeJS.ErrnoException; err.code = 'EEXIST'; @@ -3415,13 +3436,21 @@ describe('Plans Directory Initialization', () => { }); await config.initialize(); + config.setApprovalMode(ApprovalMode.PLAN); + config.getPlansDir(); - expect(() => config.getPlansDir()).toThrow( - /Failed to initialize active plan directory.*File exists/, + expect(writeSpy).toHaveBeenCalledWith( + expect.stringMatching( + /Failed to initialize active plan directory.*File exists/, + ), ); + writeSpy.mockRestore(); }); - it('should throw an error if mkdirSync fails during getPlansDir (e.g. EACCES)', async () => { + it('should log a warning if mkdirSync fails during getPlansDir (e.g. EACCES) in PLAN mode', async () => { + const writeSpy = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); vi.spyOn(fs, 'mkdirSync').mockImplementation(() => { const err = new Error('Permission denied') as NodeJS.ErrnoException; err.code = 'EACCES'; @@ -3433,10 +3462,15 @@ describe('Plans Directory Initialization', () => { }); await config.initialize(); + config.setApprovalMode(ApprovalMode.PLAN); + config.getPlansDir(); - expect(() => config.getPlansDir()).toThrow( - /Failed to initialize active plan directory.*Permission denied/, + expect(writeSpy).toHaveBeenCalledWith( + expect.stringMatching( + /Failed to initialize active plan directory.*Permission denied/, + ), ); + writeSpy.mockRestore(); }); it('should NOT create plans directory or add it to workspace context when plan is disabled', async () => { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 4fa6f4396e..9679a7ccc0 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -2269,26 +2269,31 @@ export class Config implements McpContext, AgentLoopContext { return plansDir; } - try { - fs.mkdirSync(plansDir, { recursive: true }); - - let realPlansDir = plansDir; + if (this.planEnabled && this.isPlanMode()) { try { - const resolved = resolveToRealPath(plansDir); - if (resolved) { - realPlansDir = resolved; + fs.mkdirSync(plansDir, { recursive: true }); + + let realPlansDir = plansDir; + try { + const resolved = resolveToRealPath(plansDir); + if (resolved) { + realPlansDir = resolved; + } + } catch { + // Ignore failures in mock environments } - } catch { - // Ignore failures in mock environments + this.workspaceContext.addDirectory(realPlansDir); + this.initializedPlanDirs.add(plansDir); + } catch (e: unknown) { + const errorMessage = e instanceof Error ? e.message : String(e); + process.stderr.write( + `Failed to initialize active plan directory at '${plansDir}': ${errorMessage}\n`, + ); } - this.workspaceContext.addDirectory(realPlansDir); + } else if (!this.planEnabled) { this.initializedPlanDirs.add(plansDir); - } catch (e: unknown) { - const errorMessage = e instanceof Error ? e.message : String(e); - throw new Error( - `Failed to initialize active plan directory at '${plansDir}': ${errorMessage}`, - ); } + return plansDir; }