fix(cli): return proper errors from loadExtensionConfig (#8909)

Co-authored-by: Taneja Hriday <hridayt@google.com>

packages/cli/src/config/extension.test.ts

@@ -373,6 +373,70 @@ describe('extension tests', () => {
       expect(serverConfig.env!.MISSING_VAR_BRACES).toBe('${ALSO_UNDEFINED}');
     });
 
+    it('should skip extensions with invalid JSON and log a warning', () => {
+      const consoleSpy = vi
+        .spyOn(console, 'error')
+        .mockImplementation(() => {});
+
+      // Good extension
+      createExtension({
+        extensionsDir: userExtensionsDir,
+        name: 'good-ext',
+        version: '1.0.0',
+      });
+
+      // Bad extension
+      const badExtDir = path.join(userExtensionsDir, 'bad-ext');
+      fs.mkdirSync(badExtDir);
+      const badConfigPath = path.join(badExtDir, EXTENSIONS_CONFIG_FILENAME);
+      fs.writeFileSync(badConfigPath, '{ "name": "bad-ext"'); // Malformed
+
+      const extensions = loadExtensions();
+
+      expect(extensions).toHaveLength(1);
+      expect(extensions[0].config.name).toBe('good-ext');
+      expect(consoleSpy).toHaveBeenCalledOnce();
+      expect(consoleSpy).toHaveBeenCalledWith(
+        expect.stringContaining(
+          `Warning: Skipping extension in ${badExtDir}: Failed to load extension config from ${badConfigPath}`,
+        ),
+      );
+
+      consoleSpy.mockRestore();
+    });
+
+    it('should skip extensions with missing name and log a warning', () => {
+      const consoleSpy = vi
+        .spyOn(console, 'error')
+        .mockImplementation(() => {});
+
+      // Good extension
+      createExtension({
+        extensionsDir: userExtensionsDir,
+        name: 'good-ext',
+        version: '1.0.0',
+      });
+
+      // Bad extension
+      const badExtDir = path.join(userExtensionsDir, 'bad-ext-no-name');
+      fs.mkdirSync(badExtDir);
+      const badConfigPath = path.join(badExtDir, EXTENSIONS_CONFIG_FILENAME);
+      fs.writeFileSync(badConfigPath, JSON.stringify({ version: '1.0.0' }));
+
+      const extensions = loadExtensions();
+
+      expect(extensions).toHaveLength(1);
+      expect(extensions[0].config.name).toBe('good-ext');
+      expect(consoleSpy).toHaveBeenCalledOnce();
+      expect(consoleSpy).toHaveBeenCalledWith(
+        expect.stringContaining(
+          `Warning: Skipping extension in ${badExtDir}: Failed to load extension config from ${badConfigPath}: Invalid configuration in ${badConfigPath}: missing "name"`,
+        ),
+      );
+
+      consoleSpy.mockRestore();
+    });
+
     it('should filter trust out of mcp servers', () => {
       createExtension({
         extensionsDir: userExtensionsDir,
@@ -591,15 +655,49 @@ describe('extension tests', () => {
     it('should throw an error and cleanup if gemini-extension.json is missing', async () => {
       const sourceExtDir = path.join(tempHomeDir, 'bad-extension');
       fs.mkdirSync(sourceExtDir, { recursive: true });
+      const configPath = path.join(sourceExtDir, EXTENSIONS_CONFIG_FILENAME);
+
+      await expect(
+        installExtension({ source: sourceExtDir, type: 'local' }),
+      ).rejects.toThrow(`Configuration file not found at ${configPath}`);
+
+      const targetExtDir = path.join(userExtensionsDir, 'bad-extension');
+      expect(fs.existsSync(targetExtDir)).toBe(false);
+    });
+
+    it('should throw an error for invalid JSON in gemini-extension.json', async () => {
+      const sourceExtDir = path.join(tempHomeDir, 'bad-json-ext');
+      fs.mkdirSync(sourceExtDir, { recursive: true });
+      const configPath = path.join(sourceExtDir, EXTENSIONS_CONFIG_FILENAME);
+      fs.writeFileSync(configPath, '{ "name": "bad-json", "version": "1.0.0"'); // Malformed JSON
 
       await expect(
         installExtension({ source: sourceExtDir, type: 'local' }),
       ).rejects.toThrow(
-        `Invalid extension at ${sourceExtDir}. Please make sure it has a valid gemini-extension.json file.`,
+        new RegExp(
+          `^Failed to load extension config from ${configPath.replace(
+            /\\/g,
+            '\\\\',
+          )}`,
+        ),
       );
+    });
 
-      const targetExtDir = path.join(userExtensionsDir, 'bad-extension');
-      expect(fs.existsSync(targetExtDir)).toBe(false);
+    it('should throw an error for missing name in gemini-extension.json', async () => {
+      const sourceExtDir = createExtension({
+        extensionsDir: tempHomeDir,
+        name: 'missing-name-ext',
+        version: '1.0.0',
+      });
+      const configPath = path.join(sourceExtDir, EXTENSIONS_CONFIG_FILENAME);
+      // Overwrite with invalid config
+      fs.writeFileSync(configPath, JSON.stringify({ version: '1.0.0' }));
+
+      await expect(
+        installExtension({ source: sourceExtDir, type: 'local' }),
+      ).rejects.toThrow(
+        `Invalid configuration in ${configPath}: missing "name"`,
+      );
     });
 
     it('should install an extension from a git URL', async () => {

packages/cli/src/config/extension.ts

@@ -211,31 +211,11 @@ export function loadExtension(context: LoadExtensionContext): Extension | null {
     effectiveExtensionPath = installMetadata.source;
   }
 
-  const configFilePath = path.join(
-    effectiveExtensionPath,
-    EXTENSIONS_CONFIG_FILENAME,
-  );
-  if (!fs.existsSync(configFilePath)) {
-    console.error(
-      `Warning: extension directory ${effectiveExtensionPath} does not contain a config file ${configFilePath}.`,
-    );
-    return null;
-  }
-
   try {
-    const configContent = fs.readFileSync(configFilePath, 'utf-8');
-    let config = recursivelyHydrateStrings(JSON.parse(configContent), {
-      extensionPath: extensionDir,
-      workspacePath: workspaceDir,
-      '/': path.sep,
-      pathSeparator: path.sep,
-    }) as unknown as ExtensionConfig;
-    if (!config.name || !config.version) {
-      console.error(
-        `Invalid extension config in ${configFilePath}: missing name or version.`,
-      );
-      return null;
-    }
+    let config = loadExtensionConfig({
+      extensionDir: effectiveExtensionPath,
+      workspaceDir,
+    });
 
     config = resolveEnvVarsInObject(config);
 
@@ -262,7 +242,7 @@ export function loadExtension(context: LoadExtensionContext): Extension | null {
     };
   } catch (e) {
     console.error(
-      `Warning: error parsing extension config in ${configFilePath}: ${getErrorMessage(
+      `Warning: Skipping extension in ${effectiveExtensionPath}: ${getErrorMessage(
         e,
       )}`,
     );
@@ -443,15 +423,10 @@ export async function installExtension(
     }
 
     try {
-      newExtensionConfig = await loadExtensionConfig({
+      newExtensionConfig = loadExtensionConfig({
         extensionDir: localSourcePath,
         workspaceDir: cwd,
       });
-      if (!newExtensionConfig) {
-        throw new Error(
-          `Invalid extension at ${installMetadata.source}. Please make sure it has a valid gemini-extension.json file.`,
-        );
-      }
 
       const newExtensionName = newExtensionConfig.name;
       const extensionStorage = new ExtensionStorage(newExtensionName);
@@ -507,10 +482,14 @@ export async function installExtension(
     // Attempt to load config from the source path even if installation fails
     // to get the name and version for logging.
     if (!newExtensionConfig && localSourcePath) {
-      newExtensionConfig = await loadExtensionConfig({
-        extensionDir: localSourcePath,
-        workspaceDir: cwd,
-      });
+      try {
+        newExtensionConfig = loadExtensionConfig({
+          extensionDir: localSourcePath,
+          workspaceDir: cwd,
+        });
+      } catch {
+        // Ignore error, this is just for logging.
+      }
     }
     logger?.logExtensionInstallEvent(
       new ExtensionInstallEvent(
@@ -560,13 +539,13 @@ async function requestConsent(extensionConfig: ExtensionConfig) {
   }
 }
 
-export async function loadExtensionConfig(
+export function loadExtensionConfig(
   context: LoadExtensionContext,
-): Promise<ExtensionConfig | null> {
+): ExtensionConfig {
   const { extensionDir, workspaceDir } = context;
   const configFilePath = path.join(extensionDir, EXTENSIONS_CONFIG_FILENAME);
   if (!fs.existsSync(configFilePath)) {
-    return null;
+    throw new Error(`Configuration file not found at ${configFilePath}`);
   }
   try {
     const configContent = fs.readFileSync(configFilePath, 'utf-8');
@@ -577,11 +556,17 @@ export async function loadExtensionConfig(
       pathSeparator: path.sep,
     }) as unknown as ExtensionConfig;
     if (!config.name || !config.version) {
-      return null;
+      throw new Error(
+        `Invalid configuration in ${configFilePath}: missing ${!config.name ? '"name"' : '"version"'}`,
+      );
     }
     return config;
-  } catch (_) {
-    return null;
+  } catch (e) {
+    throw new Error(
+      `Failed to load extension config from ${configFilePath}: ${getErrorMessage(
+        e,
+      )}`,
+    );
   }
 }