diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index 64af14f1cd..ccee6d3715 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -9,6 +9,7 @@ import * as path from 'node:path'; import * as crypto from 'node:crypto'; import { fileURLToPath } from 'node:url'; import { Storage } from '../config/storage.js'; +import { debugLogger } from '../utils/debugLogger.js'; import { ApprovalMode, type PolicyEngineConfig, @@ -28,7 +29,6 @@ import { } from '../confirmation-bus/types.js'; import { type MessageBus } from '../confirmation-bus/message-bus.js'; import { coreEvents } from '../utils/events.js'; -import { debugLogger } from '../utils/debugLogger.js'; import { SHELL_TOOL_NAMES } from '../utils/shell-utils.js'; import { SHELL_TOOL_NAME, @@ -794,6 +794,7 @@ export function createPolicyUpdater( if (message.persist) { persistenceQueue = persistenceQueue.then(async () => { + let tmpFile: string | undefined; try { const policyFile = message.persistScope === 'workspace' @@ -814,11 +815,27 @@ export function createPolicyUpdater( existingData = parsed as { rule?: TomlRule[] }; } } catch (error) { - if (!isNodeError(error) || error.code !== 'ENOENT') { - debugLogger.warn( - `Failed to parse ${policyFile}, overwriting with new policy.`, - error, + if (isNodeError(error) && error.code === 'ENOENT') { + // File doesn't exist yet, start fresh + } else if (!isNodeError(error)) { + // TOML parse error — back up corrupted file and recover + coreEvents.emitFeedback( + 'warning', + `Syntax error found in policy file. Backing up corrupted file to ${policyFile}.bak and starting fresh.`, ); + if ( + !( + await fs.lstat(policyFile).catch(() => null) + )?.isSymbolicLink() + ) { + await fs + .copyFile(policyFile, `${policyFile}.bak`) + .catch(() => {}); + } + existingData = {}; + } else { + // Real filesystem error (e.g. EACCES) — throw to prevent silent failure + throw error; } } @@ -866,7 +883,7 @@ export function createPolicyUpdater( // Using a unique suffix avoids race conditions where concurrent processes // overwrite each other's temporary files, leading to ENOENT errors on rename. const tmpSuffix = crypto.randomBytes(8).toString('hex'); - const tmpFile = `${policyFile}.${tmpSuffix}.tmp`; + tmpFile = `${policyFile}.${tmpSuffix}.tmp`; let handle: fs.FileHandle | undefined; try { @@ -876,11 +893,37 @@ export function createPolicyUpdater( } finally { await handle?.close(); } - await fs.rename(tmpFile, policyFile); + try { + await fs.rename(tmpFile, policyFile); + } catch (renameError) { + // Cross-device rename fails with EXDEV on some Linux mount configurations. + // Fall back to copy + unlink which works across filesystems. + if ( + isNodeError(renameError) && + (renameError.code === 'EXDEV' || renameError.code === 'EBUSY') + ) { + if ( + ( + await fs.lstat(policyFile).catch(() => null) + )?.isSymbolicLink() + ) + throw renameError; + await fs.copyFile(tmpFile, policyFile); + await fs.unlink(tmpFile).catch(() => {}); + } else { + throw renameError; + } + } } catch (error) { + // Clean up orphaned tmp file if it was created + if (tmpFile) { + await fs.unlink(tmpFile).catch(() => {}); + } + const reason = + error instanceof Error ? error.message : String(error); coreEvents.emitFeedback( 'error', - `Failed to persist policy for ${toolName}`, + `Failed to persist policy for ${toolName}: ${reason}`, error, ); } diff --git a/packages/core/src/policy/persistence.test.ts b/packages/core/src/policy/persistence.test.ts index 067ac41e4a..70c7cb1fb0 100644 --- a/packages/core/src/policy/persistence.test.ts +++ b/packages/core/src/policy/persistence.test.ts @@ -5,6 +5,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import * as fs from 'node:fs/promises'; import * as path from 'node:path'; import { createPolicyUpdater, @@ -16,10 +17,20 @@ import { MessageBusType } from '../confirmation-bus/types.js'; import { Storage, AUTO_SAVED_POLICY_FILENAME } from '../config/storage.js'; import { ApprovalMode } from './types.js'; import { vol, fs as memfs } from 'memfs'; +import { coreEvents } from '../utils/events.js'; // Use memfs for all fs operations in this test vi.mock('node:fs/promises', () => import('memfs').then((m) => m.fs.promises)); +/** + * Creates a Node.js-style error with a `code` property. + */ +function makeNodeError(message: string, code: string): NodeJS.ErrnoException { + const err = new Error(message) as NodeJS.ErrnoException; + err.code = code; + return err; +} + vi.mock('../config/storage.js'); describe('createPolicyUpdater', () => { @@ -57,8 +68,6 @@ describe('createPolicyUpdater', () => { persist: true, }); - // Policy updater handles persistence asynchronously in a promise queue. - // We use advanceTimersByTimeAsync to yield to the microtask queue. await vi.advanceTimersByTimeAsync(100); const fileExists = memfs.existsSync(policyFile); @@ -243,6 +252,147 @@ decision = "deny" expect(content).toContain('toolName = "test_tool"'); }); + it('should include error details in feedback message on persistence failure', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + + const workspacePoliciesDir = '/mock/project/.gemini/policies'; + const policyFile = path.join( + workspacePoliciesDir, + AUTO_SAVED_POLICY_FILENAME, + ); + vi.spyOn(mockStorage, 'getWorkspacePoliciesDir').mockReturnValue( + workspacePoliciesDir, + ); + vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile); + vi.spyOn(fs, 'mkdir').mockRejectedValue(new Error('Permission denied')); + + const feedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'test_tool', + persist: true, + }); + + await vi.runAllTimersAsync(); + expect(feedbackSpy).toHaveBeenCalledWith( + 'error', + expect.stringContaining('Permission denied'), + expect.any(Error), + ); + }); + + it('should clean up tmp file on write failure', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + + const workspacePoliciesDir = '/mock/project/.gemini/policies'; + const policyFile = path.join( + workspacePoliciesDir, + AUTO_SAVED_POLICY_FILENAME, + ); + vi.spyOn(mockStorage, 'getWorkspacePoliciesDir').mockReturnValue( + workspacePoliciesDir, + ); + vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile); + vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined as never); + vi.spyOn(fs, 'readFile').mockRejectedValue( + makeNodeError('ENOENT: no such file or directory', 'ENOENT'), + ); + + const mockFileHandle = { + writeFile: vi.fn().mockRejectedValue(new Error('Disk full')), + close: vi.fn().mockResolvedValue(undefined), + }; + vi.spyOn(fs, 'open').mockResolvedValue(mockFileHandle as never); + vi.spyOn(fs, 'unlink').mockResolvedValue(undefined as never); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'test_tool', + persist: true, + }); + + await vi.runAllTimersAsync(); + expect(fs.unlink).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/)); + }); + + it('should abort persistence on non-ENOENT read errors', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + + const workspacePoliciesDir = '/mock/project/.gemini/policies'; + const policyFile = path.join( + workspacePoliciesDir, + AUTO_SAVED_POLICY_FILENAME, + ); + vi.spyOn(mockStorage, 'getWorkspacePoliciesDir').mockReturnValue( + workspacePoliciesDir, + ); + vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile); + vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined as never); + vi.spyOn(fs, 'readFile').mockRejectedValue( + makeNodeError('Permission denied', 'EACCES'), + ); + const openSpy = vi.spyOn(fs, 'open'); + + const feedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'test_tool', + persist: true, + }); + + await vi.runAllTimersAsync(); + expect(openSpy).not.toHaveBeenCalled(); + expect(feedbackSpy).toHaveBeenCalledWith( + 'error', + expect.stringContaining('Permission denied'), + expect.any(Error), + ); + }); + + it('should fall back to copy+unlink when rename fails with EXDEV', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + + const workspacePoliciesDir = '/mock/project/.gemini/policies'; + const policyFile = path.join( + workspacePoliciesDir, + AUTO_SAVED_POLICY_FILENAME, + ); + vi.spyOn(mockStorage, 'getWorkspacePoliciesDir').mockReturnValue( + workspacePoliciesDir, + ); + vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile); + vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined as never); + vi.spyOn(fs, 'readFile').mockRejectedValue( + makeNodeError('ENOENT: no such file or directory', 'ENOENT'), + ); + + const mockFileHandle = { + writeFile: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; + vi.spyOn(fs, 'open').mockResolvedValue(mockFileHandle as never); + vi.spyOn(fs, 'rename').mockRejectedValue( + makeNodeError('EXDEV: cross-device link not permitted', 'EXDEV'), + ); + vi.spyOn(fs, 'copyFile').mockResolvedValue(undefined as never); + vi.spyOn(fs, 'unlink').mockResolvedValue(undefined as never); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'test_tool', + persist: true, + }); + + await vi.runAllTimersAsync(); + expect(fs.copyFile).toHaveBeenCalledWith( + expect.stringMatching(/\.tmp$/), + policyFile, + ); + expect(fs.unlink).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/)); + }); + it('should include modes if provided', async () => { createPolicyUpdater(policyEngine, messageBus, mockStorage); @@ -295,4 +445,78 @@ modes = [ "autoEdit", "yolo" ] expect(ruleCount).toBe(1); expect(content).toContain('modes = [ "default", "autoEdit", "yolo" ]'); }); + + it('should fall back to copy+unlink when rename fails with EBUSY', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + + const workspacePoliciesDir = '/mock/project/.gemini/policies'; + const policyFile = path.join( + workspacePoliciesDir, + AUTO_SAVED_POLICY_FILENAME, + ); + vi.spyOn(mockStorage, 'getWorkspacePoliciesDir').mockReturnValue( + workspacePoliciesDir, + ); + vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile); + vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined); + vi.spyOn(fs, 'readFile').mockRejectedValue( + makeNodeError('ENOENT: no such file or directory', 'ENOENT'), + ); + + const mockFileHandle = { + writeFile: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; + vi.spyOn(fs, 'open').mockResolvedValue( + mockFileHandle as unknown as fs.FileHandle, + ); + vi.spyOn(fs, 'rename').mockRejectedValue( + makeNodeError('EBUSY: resource busy or locked', 'EBUSY'), + ); + vi.spyOn(fs, 'copyFile').mockResolvedValue(undefined); + vi.spyOn(fs, 'unlink').mockResolvedValue(undefined); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'test_tool', + persist: true, + }); + + await vi.runAllTimersAsync(); + expect(fs.copyFile).toHaveBeenCalledWith( + expect.stringMatching(/\.tmp$/), + policyFile, + ); + expect(fs.unlink).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/)); + }); + + it('should back up corrupted TOML file and recover', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + + const policyFile = '/mock/user/.gemini/policies/auto-saved.toml'; + vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile); + + const dir = path.dirname(policyFile); + memfs.mkdirSync(dir, { recursive: true }); + memfs.writeFileSync(policyFile, 'this is not valid toml ][[['); + + const feedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'test_tool', + persist: true, + }); + + await vi.advanceTimersByTimeAsync(100); + + expect(feedbackSpy).toHaveBeenCalledWith( + 'warning', + expect.stringContaining('.bak'), + ); + + expect(memfs.existsSync(policyFile)).toBe(true); + const content = memfs.readFileSync(policyFile, 'utf-8') as string; + expect(content).toContain('toolName = "test_tool"'); + }); }); diff --git a/packages/core/src/policy/policy-updater.test.ts b/packages/core/src/policy/policy-updater.test.ts index 5ee9d65df4..1e374ae3ea 100644 --- a/packages/core/src/policy/policy-updater.test.ts +++ b/packages/core/src/policy/policy-updater.test.ts @@ -121,7 +121,11 @@ describe('createPolicyUpdater', () => { it('should persist mcpName to TOML', async () => { createPolicyUpdater(policyEngine, messageBus, mockStorage); - vi.mocked(fs.readFile).mockRejectedValue({ code: 'ENOENT' }); + vi.mocked(fs.readFile).mockRejectedValue( + Object.assign(new Error('ENOENT: no such file or directory'), { + code: 'ENOENT', + }), + ); vi.mocked(fs.mkdir).mockResolvedValue(undefined); const mockFileHandle = { @@ -142,9 +146,9 @@ describe('createPolicyUpdater', () => { }); // Wait for the async listener to complete - await new Promise((resolve) => setTimeout(resolve, 0)); - - expect(fs.open).toHaveBeenCalled(); + await vi.waitFor(() => { + expect(fs.open).toHaveBeenCalled(); + }); const [content] = mockFileHandle.writeFile.mock.calls[0] as [ string, string, @@ -199,7 +203,11 @@ describe('createPolicyUpdater', () => { it('should persist multiple rules correctly to TOML', async () => { createPolicyUpdater(policyEngine, messageBus, mockStorage); - vi.mocked(fs.readFile).mockRejectedValue({ code: 'ENOENT' }); + const enoentError = Object.assign( + new Error('ENOENT: no such file or directory'), + { code: 'ENOENT' }, + ); + vi.mocked(fs.readFile).mockRejectedValue(enoentError); vi.mocked(fs.mkdir).mockResolvedValue(undefined); const mockFileHandle = { @@ -219,17 +227,17 @@ describe('createPolicyUpdater', () => { }); // Wait for the async listener to complete - await new Promise((resolve) => setTimeout(resolve, 0)); + await vi.waitFor(() => { + expect(fs.open).toHaveBeenCalled(); + const [content] = mockFileHandle.writeFile.mock.calls[0] as [ + string, + string, + ]; + const parsed = toml.parse(content) as unknown as ParsedPolicy; - expect(fs.open).toHaveBeenCalled(); - const [content] = mockFileHandle.writeFile.mock.calls[0] as [ - string, - string, - ]; - const parsed = toml.parse(content) as unknown as ParsedPolicy; - - expect(parsed.rule).toHaveLength(1); - expect(parsed.rule![0].commandPrefix).toEqual(['echo', 'ls']); + expect(parsed.rule).toHaveLength(1); + expect(parsed.rule![0].commandPrefix).toEqual(['echo', 'ls']); + }); }); it('should reject unsafe regex patterns', async () => {