From fc03891a113da762175011f4e8e93acd0da49b0d Mon Sep 17 00:00:00 2001 From: Emily Hedlund Date: Fri, 20 Mar 2026 19:36:52 -0400 Subject: [PATCH] fix(extensions): revert broken extension removal behavior (#23317) --- .../cli/src/config/extension-manager.test.ts | 60 ------------------- packages/cli/src/config/extension-manager.ts | 15 ++--- packages/cli/src/config/extension.test.ts | 28 +++++---- 3 files changed, 22 insertions(+), 81 deletions(-) diff --git a/packages/cli/src/config/extension-manager.test.ts b/packages/cli/src/config/extension-manager.test.ts index 67636d922e..6c20737be9 100644 --- a/packages/cli/src/config/extension-manager.test.ts +++ b/packages/cli/src/config/extension-manager.test.ts @@ -637,64 +637,4 @@ describe('ExtensionManager', () => { ); }); }); - - describe('orphaned extension cleanup', () => { - it('should remove broken extension metadata on startup to allow re-installation', async () => { - const extName = 'orphaned-ext'; - const sourceDir = path.join(tempHomeDir, 'valid-source'); - fs.mkdirSync(sourceDir, { recursive: true }); - fs.writeFileSync( - path.join(sourceDir, 'gemini-extension.json'), - JSON.stringify({ name: extName, version: '1.0.0' }), - ); - - // Link an extension successfully. - await extensionManager.loadExtensions(); - await extensionManager.installOrUpdateExtension({ - source: sourceDir, - type: 'link', - }); - - const destinationPath = path.join(userExtensionsDir, extName); - const metadataPath = path.join( - destinationPath, - '.gemini-extension-install.json', - ); - expect(fs.existsSync(metadataPath)).toBe(true); - - // Simulate metadata corruption (e.g., pointing to a non-existent source). - fs.writeFileSync( - metadataPath, - JSON.stringify({ source: '/NON_EXISTENT_PATH', type: 'link' }), - ); - - // Simulate CLI startup. The manager should detect the broken link - // and proactively delete the orphaned metadata directory. - const newManager = new ExtensionManager({ - settings: createTestMergedSettings(), - workspaceDir: tempWorkspaceDir, - requestConsent: vi.fn().mockResolvedValue(true), - requestSetting: null, - integrityManager: mockIntegrityManager, - }); - - await newManager.loadExtensions(); - - // Verify the extension failed to load and was proactively cleaned up. - expect(newManager.getExtensions().some((e) => e.name === extName)).toBe( - false, - ); - expect(fs.existsSync(destinationPath)).toBe(false); - - // Verify the system is self-healed and allows re-linking to the valid source. - await newManager.installOrUpdateExtension({ - source: sourceDir, - type: 'link', - }); - - expect(newManager.getExtensions().some((e) => e.name === extName)).toBe( - true, - ); - }); - }); }); diff --git a/packages/cli/src/config/extension-manager.ts b/packages/cli/src/config/extension-manager.ts index dd37d0ea1b..04487bc5f8 100644 --- a/packages/cli/src/config/extension-manager.ts +++ b/packages/cli/src/config/extension-manager.ts @@ -982,18 +982,11 @@ Would you like to attempt to install via "git clone" instead?`, plan: config.plan, }; } catch (e) { - const extName = path.basename(extensionDir); - debugLogger.warn( - `Warning: Removing broken extension ${extName}: ${getErrorMessage(e)}`, + debugLogger.error( + `Warning: Skipping extension in ${effectiveExtensionPath}: ${getErrorMessage( + e, + )}`, ); - try { - await fs.promises.rm(extensionDir, { recursive: true, force: true }); - } catch (rmError) { - debugLogger.error( - `Failed to remove broken extension directory ${extensionDir}:`, - rmError, - ); - } return null; } } diff --git a/packages/cli/src/config/extension.test.ts b/packages/cli/src/config/extension.test.ts index fa957d8f7f..ef7e61cf25 100644 --- a/packages/cli/src/config/extension.test.ts +++ b/packages/cli/src/config/extension.test.ts @@ -249,8 +249,10 @@ describe('extension tests', () => { expect(extensions[0].name).toBe('test-extension'); }); - it('should log a warning and remove the extension if a context file path is outside the extension directory', async () => { - const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + it('should skip the extension if a context file path is outside the extension directory and log an error', async () => { + const consoleSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}); createExtension({ extensionsDir: userExtensionsDir, name: 'traversal-extension', @@ -660,8 +662,10 @@ name = "yolo-checker" expect(serverConfig.env!['MISSING_VAR_BRACES']).toBe('${ALSO_UNDEFINED}'); }); - it('should remove an extension with invalid JSON config and log a warning', async () => { - const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + it('should skip an extension with invalid JSON config and log an error', async () => { + const consoleSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}); // Good extension createExtension({ @@ -682,15 +686,17 @@ name = "yolo-checker" expect(extensions[0].name).toBe('good-ext'); expect(consoleSpy).toHaveBeenCalledWith( expect.stringContaining( - `Warning: Removing broken extension bad-ext: Failed to load extension config from ${badConfigPath}`, + `Warning: Skipping extension in ${badExtDir}: Failed to load extension config from ${badConfigPath}`, ), ); consoleSpy.mockRestore(); }); - it('should remove an extension with missing "name" in config and log a warning', async () => { - const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + it('should skip an extension with missing "name" in config and log an error', async () => { + const consoleSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}); // Good extension createExtension({ @@ -711,7 +717,7 @@ name = "yolo-checker" expect(extensions[0].name).toBe('good-ext'); expect(consoleSpy).toHaveBeenCalledWith( expect.stringContaining( - `Warning: Removing broken extension bad-ext-no-name: Failed to load extension config from ${badConfigPath}: Invalid configuration in ${badConfigPath}: missing "name"`, + `Warning: Skipping extension in ${badExtDir}: Failed to load extension config from ${badConfigPath}: Invalid configuration in ${badConfigPath}: missing "name"`, ), ); @@ -737,8 +743,10 @@ name = "yolo-checker" expect(extensions[0].mcpServers?.['test-server'].trust).toBeUndefined(); }); - it('should log a warning for invalid extension names during loading', async () => { - const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + it('should log an error for invalid extension names during loading', async () => { + const consoleSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}); createExtension({ extensionsDir: userExtensionsDir, name: 'bad_name',